From: Hans de Goede <hdegoede@redhat.com>
To: Maximilian Luz <luzmaximilian@gmail.com>
Cc: Mark Gross <mgross@linux.intel.com>,
Jonathan Corbet <corbet@lwn.net>,
platform-driver-x86@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/7] platform/surface: aggregator: Extend user-space interface for events
Date: Fri, 4 Jun 2021 13:40:06 +0200 [thread overview]
Message-ID: <c86a976e-64cb-ea10-486e-fa5d4482ad5b@redhat.com> (raw)
In-Reply-To: <20210603234526.2503590-1-luzmaximilian@gmail.com>
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(-)
>
next prev parent reply other threads:[~2021-06-04 11:40 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-03 23:45 [PATCH 0/7] platform/surface: aggregator: Extend user-space interface for events Maximilian Luz
2021-06-03 23:45 ` [PATCH 1/7] platform/surface: aggregator: Allow registering notifiers without enabling events Maximilian Luz
2021-06-03 23:45 ` [PATCH 2/7] platform/surface: aggregator: Allow enabling of events without notifiers Maximilian Luz
2021-06-04 5:55 ` kernel test robot
2021-06-03 23:45 ` [PATCH 3/7] platform/surface: aggregator: Update copyright Maximilian Luz
2021-06-03 23:45 ` [PATCH 4/7] platform/surface: aggregator_cdev: Add support for forwarding events to user-space Maximilian Luz
2021-06-04 11:32 ` Hans de Goede
2021-06-04 12:47 ` Maximilian Luz
2021-06-03 23:45 ` [PATCH 5/7] platform/surface: aggregator_cdev: Allow enabling of events from user-space Maximilian Luz
2021-06-03 23:45 ` [PATCH 6/7] platform/surface: aggregator_cdev: Add lockdep support Maximilian Luz
2021-06-03 23:45 ` [PATCH 7/7] docs: driver-api: Update Surface Aggregator user-space interface documentation Maximilian Luz
2021-06-04 11:40 ` Hans de Goede [this message]
2021-06-04 12:58 ` [PATCH 0/7] platform/surface: aggregator: Extend user-space interface for events Maximilian Luz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c86a976e-64cb-ea10-486e-fa5d4482ad5b@redhat.com \
--to=hdegoede@redhat.com \
--cc=corbet@lwn.net \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luzmaximilian@gmail.com \
--cc=mgross@linux.intel.com \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).