| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
|
|
list_del/list_add_tail in ssh_packet_layer.c
Using list_move_tail() instead of list_del() + list_add_tail() in ssh_packet_layer.c.
Reported-by: Hulk Robot <[email protected]>
Signed-off-by: Baokun Li <[email protected]>
Reviewed-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
list_del/list_add_tail in ssh_request_layer.c
Using list_move_tail() instead of list_del() + list_add_tail() in ssh_request_layer.c.
Reported-by: Hulk Robot <[email protected]>
Signed-off-by: Baokun Li <[email protected]>
Reviewed-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
The status variable in ssam_controller_event_disable() is always set, no
need to initialize it.
Signed-off-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
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 <[email protected]>
Fixes: 640ee17199e4 ("platform/surface: aggregator: Allow enabling of events without notifiers")
Signed-off-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
It's 2021, update the copyright accordingly.
Signed-off-by: Maximilian Luz <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
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 <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Reviewed-by: Hans de Goede <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
|
|
events
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 <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
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: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
Signed-off-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
The acpi_walk_dep_device_list() function is not as generic as its
name implies, serving only to decrement the dependency count for each
dependent device of the input.
Extend it to accept a callback which can be applied to all the
dependencies in acpi_dep_list.
Replace all existing calls to the function with calls to a wrapper,
passing a callback that applies the same dependency reduction.
Reviewed-by: Andy Shevchenko <[email protected]>
Acked-by: Maximilian Luz <[email protected]> # for platform/surface parts
Signed-off-by: Daniel Scally <[email protected]>
[ rjw: Changelog edits ]
Signed-off-by: Rafael J. Wysocki <[email protected]>
|
|
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: 507cf5a2f1e2 ("platform/surface: aggregator: move to use request_irq by IRQF_NO_AUTOEN flag")
Signed-off-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
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: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Dan Carpenter <[email protected]>
Reviewed-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/YH6UUhJhGk3mk13b@mwanda
Signed-off-by: Hans de Goede <[email protected]>
|
|
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 <[email protected]>
Reviewed-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
functions
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 <[email protected]>
Fixes: b78b4982d763 ("platform/surface: Add platform profile driver")
Signed-off-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
The raw message frame length is unaligned and explicitly marked as
little endian. It should not be accessed without the appropriate
accessor functions. Fix this.
Note that payload.len already contains the correct length after parsing
via sshp_parse_frame(), so we can simply use that instead.
Reported-by: kernel-test-robot <[email protected]>
Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
Signed-off-by: Maximilian Luz <[email protected]>
Acked-by: Mark Gross <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
The braces of the unlikely() macro inside the if condition only cover
the subtraction part, not the whole statement. This causes the result of
the subtraction to be converted to zero or one. While that still works
in this context, it causes static analysis tools to complain (and is
just plain wrong).
Fix the bracket placement and, while at it, simplify the if-condition.
Also add a comment to the if-condition explaining what we expect the
result to be and what happens on the failure path, as it seems to have
caused a bit of confusion.
This commit should not cause any difference in behavior or generated
code.
Reported-by: Dan Carpenter <[email protected]>
Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
Signed-off-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
Both, ssh_rtl_rx_start() and ssh_rtl_tx_start() functions, do not exist
and have been consolidated into ssh_rtl_start(). Nevertheless,
kernel-doc references the former functions. Replace those references
with references to ssh_rtl_start().
Signed-off-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
A function has a different name between their prototype
and its kernel-doc markup:
../drivers/platform/surface/aggregator/ssh_request_layer.c:1065: warning: expecting prototype for ssh_rtl_tx_start(). Prototype was for ssh_rtl_start() instead
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Reviewed-by: Maximilian Luz <[email protected]>
Link: https://lore.kernel.org/r/4a6bf33cfbd06654d78294127f2b6d354d073089.1610610937.git.mchehab+huawei@kernel.org
Signed-off-by: Hans de Goede <[email protected]>
|
|
The Surface Aggregator EC provides varying functionality, depending on
the Surface device. To manage this functionality, we use dedicated
client devices for each subsystem or virtual device of the EC. While
some of these clients are described as standard devices in ACPI and the
corresponding client drivers can be implemented as platform drivers in
the kernel (making use of the controller API already present), many
devices, especially on newer Surface models, cannot be found there.
To simplify management of these devices, we introduce a new bus and
client device type for the Surface Aggregator subsystem. The new device
type takes care of managing the controller reference, essentially
guaranteeing its validity for as long as the client device exists, thus
alleviating the need to manually establish device links for that purpose
in the client driver (as has to be done with the platform devices).
Signed-off-by: Maximilian Luz <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
This commit adds error injection hooks to the Surface Serial Hub
communication protocol implementation, to:
- simulate simple serial transmission errors,
- drop packets, requests, and responses, simulating communication
failures and potentially trigger retransmission timeouts, as well as
- inject invalid data into submitted and received packets.
Together with the trace points introduced in the previous commit, these
facilities are intended to aid in testing, validation, and debugging of
the Surface Aggregator communication layer.
Signed-off-by: Maximilian Luz <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Acked-by: Steven Rostedt (VMware) <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
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 <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Acked-by: Steven Rostedt (VMware) <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
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 <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
Surface Serial Hub communication is, in its core, packet based. Each
sequenced packet requires to be acknowledged, via an ACK-type control
packet. In case invalid data has been received by the driver, a NAK-type
(not-acknowledge/negative acknowledge) control packet is sent,
triggering retransmission.
Control packets are therefore a core communication primitive and used
frequently enough (with every sequenced packet transmission sent by the
embedded controller, including events and request responses) that it may
warrant caching their allocations to reduce possible memory
fragmentation.
Signed-off-by: Maximilian Luz <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
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 <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|