Re: [PATCH 0/7] platform/surface: aggregator: Extend user-space interface for events

Hans de Goede
2021-06-04T11:40:12Z

Hi Maxime,

On 6/4/21 1:45 AM, Maximilian Luz wrote:
> Extend the user-space debug interface so that it can be used to receive
> SSAM events in user-space.
>
> Currently, inspecting SSAM events requires writing a custom client
> device and corresponding driver. This is not particularly user-friendly
> for quick testing and comes with higher iteration times. Since we
> already have a user-space interface, we can extend this to forward
> events from SSAM via the controller device file to user-space. With this
> we can then essentially write user-space SSAM clients for testing and
> reverse-engineering, providing us with all the essential functionality
> that previously only a kernel driver would have access to. Note that
> this is still only intended to be an interface for debugging and
> reverse-engineering purposes.
>
> To achieve this, we need to extend the core to decouple events from
> notifiers. Right now, enabling an event group requires registering a
> notifier for that group. This notifier provides a callback that is
> called when the event occurs. For user-space forwarding, we need to run
> all events through the same file. In the current implementation, this
> presents a problem as, when we don't know the exact events or can't
> filter for them, multiple notifiers for the same target category will
> lead to duplicate events to be sent through the file, one per notifier.
>
> Decoupling notifier registration from event enable-/disablement (and the
> corresponding reference counting) allows us to avoid this issue. We can
> then register one notifier for a whole target category and enable or
> disable events independently of this notifier. Since events are strictly
> separated by their target category, this will not lead to duplicate
> events.
>
> With this, we can then provide user-space with two new IOCTLs for
> registering notifiers for a specific target category of events they are
> interested in. This allows us to forward all events received by those
> notifiers to the internal buffer of the device file, from which they can
> be read by user-space. In other words, user-space can, via those two
> IOCTLs, select which event target categories they are interested in.
>
> Furthermore, we add another two IOCTLs for enabling and disabling events
> via the controller. While events can already be enabled and disabled via
> generic requests, this does not respect the controller-internal
> reference counting mechanism. Due to that, this can lead to an event
> group being disabled even though a kernel-driver has requested it to be
> enabled. Or in other words: Without this, a user-space client cannot
> safely reset the state as it has only two options, keeping the event
> group enabled and not attempt cleanup at all, or disable the event group
> for all clients and potentially stop them from working properly.
>
> Also update the copyright lines since we're already doing some work on
> the core.

Overall this series looks good to me. I've found one small issue with
PATCH 4/7 (see my reply to that patch) and as the kernel test robot
pointed out there is an used "struct ssam_nf_head *nf_head;" in
PATCH 2/7.

With these 2 small issues fixed you can add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

to v2 of the series.

Regards,

Hans

>
> Maximilian Luz (7):
> platform/surface: aggregator: Allow registering notifiers without
> enabling events
> platform/surface: aggregator: Allow enabling of events without
> notifiers
> platform/surface: aggregator: Update copyright
> platform/surface: aggregator_cdev: Add support for forwarding events
> to user-space
> platform/surface: aggregator_cdev: Allow enabling of events from
> user-space
> platform/surface: aggregator_cdev: Add lockdep support
> docs: driver-api: Update Surface Aggregator user-space interface
> documentation
>
> .../surface_aggregator/clients/cdev.rst | 127 ++++-
> .../userspace-api/ioctl/ioctl-number.rst | 2 +-
> drivers/platform/surface/aggregator/Kconfig | 2 +-
> drivers/platform/surface/aggregator/Makefile | 2 +-
> drivers/platform/surface/aggregator/bus.c | 2 +-
> drivers/platform/surface/aggregator/bus.h | 2 +-
> .../platform/surface/aggregator/controller.c | 206 ++++++-
> .../platform/surface/aggregator/controller.h | 2 +-
> drivers/platform/surface/aggregator/core.c | 2 +-
> .../platform/surface/aggregator/ssh_msgb.h | 2 +-
> .../surface/aggregator/ssh_packet_layer.c | 2 +-
> .../surface/aggregator/ssh_packet_layer.h | 2 +-
> .../platform/surface/aggregator/ssh_parser.c | 2 +-
> .../platform/surface/aggregator/ssh_parser.h | 2 +-
> .../surface/aggregator/ssh_request_layer.c | 2 +-
> .../surface/aggregator/ssh_request_layer.h | 2 +-
> drivers/platform/surface/aggregator/trace.h | 2 +-
> .../surface/surface_aggregator_cdev.c | 531 +++++++++++++++++-
> include/linux/surface_aggregator/controller.h | 27 +-
> include/linux/surface_aggregator/device.h | 2 +-
> include/linux/surface_aggregator/serial_hub.h | 2 +-
> include/uapi/linux/surface_aggregator/cdev.h | 73 ++-
> 22 files changed, 921 insertions(+), 77 deletions(-)
>