| Message ID | 20210603234526.2503590-1-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | platform/surface: aggregator: Extend user-space interface for events | expand |
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(-) >
On 6/4/21 1:40 PM, Hans de Goede wrote: > 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. >> [...] > > 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. Thank you for your review. Based on the issue reported by the kernel test robot I've been restructuring PATCH 2/7 to remove some code-duplication. I'll add your R-b to all except that one. Thanks, Max
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. 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(-)