The status variable in ssam_nf_refcount_disable_free() is only set when
the reference count equals zero. Otherwise, it is returned
uninitialized. Fix this by always initializing status to zero.
Reported-by: kernel test robot <lkp@intel.com>
Fixes: 640ee17199e4 ("platform/surface: aggregator: Allow enabling of events without notifiers")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210604210907.25738-2-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
We can already enable and disable SAM events via one of two ways: either
via a (non-observer) notifier tied to a specific event group, or a
generic event enable/disable request. In some instances, however,
neither method may be desirable.
The first method will tie the event enable request to a specific
notifier, however, when we want to receive notifications for multiple
event groups of the same target category and forward this to the same
notifier callback, we may receive duplicate events, i.e. one event per
registered notifier. The second method will bypass the internal
reference counting mechanism, meaning that a disable request will
disable the event regardless of any other client driver using it, which
may break the functionality of that driver.
To address this problem, add new functions that allow enabling and
disabling of events via the event reference counting mechanism built
into the controller, without needing to register a notifier.
This can then be used in combination with observer notifiers to process
multiple events of the same target category without duplication in the
same callback function.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210604134755.535590-3-luzmaximilian@gmail.com
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Currently, each SSAM event notifier is directly tied to one group of
events. This makes sense as registering a notifier will automatically
take care of enabling the corresponding event group and normally drivers
only need notifications for a very limited number of events, associated
with different callbacks for each group.
However, there are rare cases, especially for debugging, when we want to
get notifications for a whole event target category instead of just a
single group of events in that category. Registering multiple notifiers,
i.e. one per group, may be infeasible due to two issues: a) we might not
know every event enable/disable specification as some events are
auto-enabled by the EC and b) forwarding this to the same callback will
lead to duplicate events as we might not know the full event
specification to perform the appropriate filtering.
This commit introduces observer-notifiers, which are notifiers that are
not tied to a specific event group and do not attempt to manage any
events. In other words, they can be registered without enabling any
event group or incrementing the corresponding reference count and just
act as silent observers, listening to all currently/previously enabled
events based on their match-specification.
Essentially, this allows us to register one single notifier for a full
event target category, meaning that we can process all events of that
target category in a single callback without duplication. Specifically,
this will be used in the cdev debug interface to forward events to
user-space via a device file from which the events can be read.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20210604134755.535590-2-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Disabling events silently fails due to the wrong command ID being used.
Instead of the command ID for the disable call, the command ID for the
enable call was being used. This causes the disable call to enable the
event instead. As the event is already enabled when we call this
function, the EC silently drops this command and does nothing.
Use the correct command ID for disabling the event to fix this.
Fixes: c167b9c7e3 ("platform/surface: Add Surface Aggregator subsystem")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210603000636.568846-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Having both IRQF_NO_AUTOEN and IRQF_SHARED set causes
request_threaded_irq() to return with -EINVAL (see comment in flag
validation in that function). As the interrupt is currently not shared
between multiple devices, drop the IRQF_SHARED flag.
Fixes: 507cf5a2f1 ("platform/surface: aggregator: move to use request_irq by IRQF_NO_AUTOEN flag")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210505133635.1499703-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The "funcs" variable is a u64. If "func" is more than 31 then the
BIT() shift will wrap instead of testing the high bits.
Fixes: c167b9c7e3 ("platform/surface: Add Surface Aggregator subsystem")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/YH6UUhJhGk3mk13b@mwanda
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
disable_irq() after request_irq() still has a time gap in which
interrupts can come. request_irq() with IRQF_NO_AUTOEN flag will
disable IRQ auto-enable because of requesting.
this patch is made base on "add IRQF_NO_AUTOEN for request_irq" which
is being merged: https://lore.kernel.org/patchwork/patch/1388765/
Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/1617778852-26492-1-git-send-email-tiantao6@hisilicon.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
The SSAM_DEFINE_SYNC_REQUEST_x() macros are intended to reduce
boiler-plate code for SSAM request definitions by defining a wrapper
function for the specified request. The client device variants of those
macros, i.e. SSAM_DEFINE_SYNC_REQUEST_CL_x() in particular rely on the
multi-device (MD) variants, e.g.:
#define SSAM_DEFINE_SYNC_REQUEST_CL_R(name, rtype, spec...) \
SSAM_DEFINE_SYNC_REQUEST_MD_R(__raw_##name, rtype, spec) \
int name(struct ssam_device *sdev, rtype *ret) \
{ \
return __raw_##name(sdev->ctrl, sdev->uid.target, \
sdev->uid.instance, ret); \
}
This now creates the problem that it is not possible to declare the
generated functions static via
static SSAM_DEFINE_SYNC_REQUEST_CL_R(...)
as this will only apply to the function defined by the multi-device
macro, i.e. SSAM_DEFINE_SYNC_REQUEST_MD_R(). Thus compiling with
`-Wmissing-prototypes' rightfully complains that there is a 'static'
keyword missing.
To solve this, make all SSAM_DEFINE_SYNC_REQUEST_x() macros define
static functions. Non-client-device macros are also changed for
consistency. In general, we expect those functions to be only used
locally in the respective drivers for the corresponding interfaces, so
having to define a wrapper function to be able to export this should be
the odd case out.
Reported-by: kernel test robot <lkp@intel.com>
Fixes: b78b4982d7 ("platform/surface: Add platform profile driver")
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20210304190524.1172197-1-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Add trace points to the Surface Aggregator subsystem core. These trace
points can be used to track packets, requests, and allocations. They are
further intended for debugging and testing/validation, specifically in
combination with the error injection capabilities introduced in the
subsequent commit.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Link: https://lore.kernel.org/r/20201221183959.1186143-5-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Event items are used for completing Surface Aggregator EC events, i.e.
placing event command data and payload on a workqueue for later
processing to avoid doing said processing directly on the receiver
thread. This means that event items are allocated for each incoming
event, regardless of that event being transmitted via sequenced or
unsequenced packets.
On the Surface Book 3 and Surface Laptop 3, touchpad HID input events
(unsequenced), can constitute a larger amount of traffic, and therefore
allocation of event items. This warrants caching event items to reduce
memory fragmentation. The size of the cached objects is specifically
tuned to accommodate keyboard and touchpad input events and their
payloads on those devices. As a result, this effectively also covers
most other event types. In case of a larger event payload, event item
allocation will fall back to kzalloc().
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20201221183959.1186143-4-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Add Surface System Aggregator Module core and Surface Serial Hub driver,
required for the embedded controller found on Microsoft Surface devices.
The Surface System Aggregator Module (SSAM, SAM or Surface Aggregator)
is an embedded controller (EC) found on 4th and later generation
Microsoft Surface devices, with the exception of the Surface Go series.
This EC provides various functionality, depending on the device in
question. This can include battery status and thermal reporting (5th and
later generations), but also HID keyboard (6th+) and touchpad input
(7th+) on Surface Laptop and Surface Book 3 series devices.
This patch provides the basic necessities for communication with the SAM
EC on 5th and later generation devices. On these devices, the EC
provides an interface that acts as serial device, called the Surface
Serial Hub (SSH). 4th generation devices, on which the EC interface is
provided via an HID-over-I2C device, are not supported by this patch.
Specifically, this patch adds a driver for the SSH device (device HID
MSHW0084 in ACPI), as well as a controller structure and associated API.
This represents the functional core of the Surface Aggregator kernel
subsystem, introduced with this patch, and will be expanded upon in
subsequent commits.
The SSH driver acts as the main attachment point for this subsystem and
sets-up and manages the controller structure. The controller in turn
provides a basic communication interface, allowing to send requests from
host to EC and receiving the corresponding responses, as well as
managing and receiving events, sent from EC to host. It is structured
into multiple layers, with the top layer presenting the API used by
other kernel drivers and the lower layers modeled after the serial
protocol used for communication.
Said other drivers are then responsible for providing the (Surface model
specific) functionality accessible through the EC (e.g. battery status
reporting, thermal information, ...) via said controller structure and
API, and will be added in future commits.
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Link: https://lore.kernel.org/r/20201221183959.1186143-2-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
