linux-stable.git/drivers/platform/surface/aggregator/controller.c, branch linux-6.3.y

platform/surface: aggregator: Allow completion work-items to be executed in parallel

2023-06-14T09:16:43+00:00

[ Upstream commit 539e0a7f9105d19c00629c3f4da00330488e8c60 ]

Currently, event completion work-items are restricted to be run strictly
in non-parallel fashion by the respective workqueue. However, this has
lead to some problems:

In some instances, the event notifier function called inside this
completion workqueue takes a non-negligible amount of time to execute.
One such example is the battery event handling code (surface_battery.c),
which can result in a full battery information refresh, involving
further synchronous communication with the EC inside the event handler.
This is made worse if the communication fails spuriously, generally
incurring a multi-second timeout.

Since the event completions are run strictly non-parallel, this blocks
other events from being propagated to the respective subsystems. This
becomes especially noticeable for keyboard and touchpad input, which
also funnel their events through this system. Here, users have reported
occasional multi-second "freezes".

Note, however, that the event handling system was never intended to run
purely sequentially. Instead, we have one work struct per EC/SAM
subsystem, processing the event queue for that subsystem. These work
structs were intended to run in parallel, allowing sequential processing
of work items for each subsystem but parallel processing of work items
across subsystems.

The only restriction to this is the way the workqueue is created.
Therefore, replace create_workqueue() with alloc_workqueue() and do not
restrict the maximum number of parallel work items to be executed on
that queue, resolving any cross-subsystem blockage.

Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
Link: https://github.com/linux-surface/linux-surface/issues/1026
Signed-off-by: Maximilian Luz 
Link: https://lore.kernel.org/r/20230525210110.2785470-1-luzmaximilian@gmail.com
Reviewed-by: Hans de Goede 
Signed-off-by: Hans de Goede 
Signed-off-by: Sasha Levin

platform/surface: aggregator: Rename top-level request functions to avoid ambiguities

2023-02-02T21:48:20+00:00

We currently have a struct ssam_request_sync and a function
ssam_request_sync(). While this is valid C, there are some downsides to
it.

One of these is that current Sphinx versions (>= 3.0) cannot
disambiguate between the two (see disucssion and pull request linked
below). It instead emits a "WARNING: Duplicate C declaration" and links
for the struct and function in the resulting documentation link to the
same entry (i.e. both to either function or struct documentation)
instead of their respective own entries.

While we could just ignore that and wait for a fix, there's also a point
to be made that the current naming can be somewhat confusing when
searching (e.g. via grep) or trying to understand the levels of
abstraction at play:

We currently have struct ssam_request_sync and associated functions
ssam_request_sync_[alloc|free|init|wait|...]() operating on this struct.
However, function ssam_request_sync() is one abstraction level above
this. Similarly, ssam_request_sync_with_buffer() is not a function
operating on struct ssam_request_sync, but rather a sibling to
ssam_request_sync(), both using the struct under the hood.

Therefore, rename the top level request functions:

  ssam_request_sync() -> ssam_request_do_sync()
  ssam_request_sync_with_buffer() -> ssam_request_do_sync_with_buffer()
  ssam_request_sync_onstack() -> ssam_request_do_sync_onstack()

Link: https://lore.kernel.org/all/085e0ada65c11da9303d07e70c510dc45f21315b.1656756450.git.mchehab@kernel.org/
Link: https://github.com/sphinx-doc/sphinx/pull/8313
Signed-off-by: Maximilian Luz 
Link: https://lore.kernel.org/r/20221220175608.1436273-2-luzmaximilian@gmail.com
Reviewed-by: Hans de Goede 
Signed-off-by: Hans de Goede

platform/surface: aggregator: Improve documentation and handling of message target and source IDs

2023-02-02T21:48:20+00:00

The `tid_in` and `tid_out` fields of the serial hub protocol command
struct (struct ssh_command) are actually source and target IDs,
indicating the peer from which the message originated and the peer for
which it is intended.

Change the naming of those fields accordingly and improve the protocol
documentation. Additionally, introduce an enum containing all currently
known peers, i.e. targets and sources.

Signed-off-by: Maximilian Luz 
Link: https://lore.kernel.org/r/20221202223327.690880-3-luzmaximilian@gmail.com
Reviewed-by: Hans de Goede 
Signed-off-by: Hans de Goede

platform/surface: aggregator: Add missing call to ssam_request_sync_free()

2023-01-12T18:12:02+00:00

Although rare, ssam_request_sync_init() can fail. In that case, the
request should be freed via ssam_request_sync_free(). Currently it is
leaked instead. Fix this.

Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
Signed-off-by: Maximilian Luz 
Link: https://lore.kernel.org/r/20221220175608.1436273-1-luzmaximilian@gmail.com
Reviewed-by: Hans de Goede 
Signed-off-by: Hans de Goede

platform/surface: Update copyright year of various drivers

2022-07-02T09:23:30+00:00

Update the copyright of various Surface drivers to the current year.

Signed-off-by: Maximilian Luz 
Link: https://lore.kernel.org/r/20220624205800.1355621-4-luzmaximilian@gmail.com
Reviewed-by: Hans de Goede 
Signed-off-by: Hans de Goede

platform/surface: aggregator: Allow notifiers to avoid communication on unregistering

2022-06-13T15:25:07+00:00

When SSAM client devices have been (physically) hot-removed,
communication attempts with those devices may fail and time out. This
can even extend to event notifiers, due to which timeouts may occur
during device removal, slowing down that process.

Add a parameter to the notifier unregister function that allows skipping
communication with the EC to prevent this. Furthermore, add wrappers for
registering and unregistering notifiers belonging to SSAM client devices
that automatically check if the device has been marked as hot-removed
and communication should be avoided.

Note that non-SSAM client devices can generally not be hot-removed, so
also add a convenience wrapper for those, defaulting to allow
communication.

Signed-off-by: Maximilian Luz 
Link: https://lore.kernel.org/r/20220527023447.2460025-4-luzmaximilian@gmail.com
Reviewed-by: Hans de Goede 
Signed-off-by: Hans de Goede

platform/surface: aggregator: Drop unnecessary variable initialization

2021-06-16T15:47:54+00:00

The status variable in ssam_controller_event_disable() is always set, no
need to initialize it.

Signed-off-by: Maximilian Luz 
Link: https://lore.kernel.org/r/20210604210907.25738-3-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede

platform/surface: aggregator: Do not return uninitialized value

2021-06-16T15:47:54+00:00

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 
Fixes: 640ee17199e4 ("platform/surface: aggregator: Allow enabling of events without notifiers")
Signed-off-by: Maximilian Luz 
Link: https://lore.kernel.org/r/20210604210907.25738-2-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede

platform/surface: aggregator: Update copyright

2021-06-16T15:47:53+00:00

It's 2021, update the copyright accordingly.

Signed-off-by: Maximilian Luz 
Reviewed-by: Hans de Goede 
Link: https://lore.kernel.org/r/20210604134755.535590-4-luzmaximilian@gmail.com
Signed-off-by: Hans de Goede

platform/surface: aggregator: Allow enabling of events without notifiers

2021-06-16T15:47:53+00:00

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 
Link: https://lore.kernel.org/r/20210604134755.535590-3-luzmaximilian@gmail.com
Reviewed-by: Hans de Goede 
Signed-off-by: Hans de Goede