2021-06-04 13:53:35

by Maximilian Luz

[permalink] [raw]
Subject: [PATCH v2 0/7] platform/surface: aggregator: Extend user-space interface for events

Extend the user-space debug interface so that it can be used to receive
SSAM events in user-space.

Version 1 and rationale:
https://lore.kernel.org/platform-driver-x86/[email protected]/

Changes in version 2:
- PATCH 2/7: Avoid code duplication, remove unused variable
- PATCH 4/7: Add missing mutex_destroy() calls

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 | 332 +++++++++--
.../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 | 534 +++++++++++++++++-
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, 1018 insertions(+), 109 deletions(-)

--
2.31.1


2021-06-04 13:53:47

by Maximilian Luz

[permalink] [raw]
Subject: [PATCH v2 6/7] platform/surface: aggregator_cdev: Add lockdep support

Mark functions with locking requirements via the corresponding lockdep
calls for debugging and documentary purposes.

Signed-off-by: Maximilian Luz <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
---

Changes in v2:
- None

---
.../platform/surface/surface_aggregator_cdev.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/drivers/platform/surface/surface_aggregator_cdev.c b/drivers/platform/surface/surface_aggregator_cdev.c
index 7b86b36eaaa0..30fb50fde450 100644
--- a/drivers/platform/surface/surface_aggregator_cdev.c
+++ b/drivers/platform/surface/surface_aggregator_cdev.c
@@ -139,6 +139,8 @@ static int ssam_cdev_notifier_register(struct ssam_cdev_client *client, u8 tc, i
struct ssam_cdev_notifier *nf;
int status;

+ lockdep_assert_held_read(&client->cdev->lock);
+
/* Validate notifier target category. */
if (!ssh_rqid_is_event(rqid))
return -EINVAL;
@@ -188,6 +190,8 @@ static int ssam_cdev_notifier_unregister(struct ssam_cdev_client *client, u8 tc)
const u16 event = ssh_rqid_to_event(rqid);
int status;

+ lockdep_assert_held_read(&client->cdev->lock);
+
/* Validate notifier target category. */
if (!ssh_rqid_is_event(rqid))
return -EINVAL;
@@ -257,6 +261,8 @@ static long ssam_cdev_request(struct ssam_cdev_client *client, struct ssam_cdev_
void __user *rspdata;
int status = 0, ret = 0, tmp;

+ lockdep_assert_held_read(&client->cdev->lock);
+
ret = copy_struct_from_user(&rqst, sizeof(rqst), r, sizeof(*r));
if (ret)
goto out;
@@ -367,6 +373,8 @@ static long ssam_cdev_notif_register(struct ssam_cdev_client *client,
struct ssam_cdev_notifier_desc desc;
long ret;

+ lockdep_assert_held_read(&client->cdev->lock);
+
ret = copy_struct_from_user(&desc, sizeof(desc), d, sizeof(*d));
if (ret)
return ret;
@@ -380,6 +388,8 @@ static long ssam_cdev_notif_unregister(struct ssam_cdev_client *client,
struct ssam_cdev_notifier_desc desc;
long ret;

+ lockdep_assert_held_read(&client->cdev->lock);
+
ret = copy_struct_from_user(&desc, sizeof(desc), d, sizeof(*d));
if (ret)
return ret;
@@ -395,6 +405,8 @@ static long ssam_cdev_event_enable(struct ssam_cdev_client *client,
struct ssam_event_id id;
long ret;

+ lockdep_assert_held_read(&client->cdev->lock);
+
/* Read descriptor from user-space. */
ret = copy_struct_from_user(&desc, sizeof(desc), d, sizeof(*d));
if (ret)
@@ -421,6 +433,8 @@ static long ssam_cdev_event_disable(struct ssam_cdev_client *client,
struct ssam_event_id id;
long ret;

+ lockdep_assert_held_read(&client->cdev->lock);
+
/* Read descriptor from user-space. */
ret = copy_struct_from_user(&desc, sizeof(desc), d, sizeof(*d));
if (ret)
@@ -513,6 +527,8 @@ static int ssam_cdev_device_release(struct inode *inode, struct file *filp)
static long __ssam_cdev_device_ioctl(struct ssam_cdev_client *client, unsigned int cmd,
unsigned long arg)
{
+ lockdep_assert_held_read(&client->cdev->lock);
+
switch (cmd) {
case SSAM_CDEV_REQUEST:
return ssam_cdev_request(client, (struct ssam_cdev_request __user *)arg);
--
2.31.1

2021-06-04 20:20:31

by Hans de Goede

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

Hi,

On 6/4/21 3:47 PM, Maximilian Luz wrote:
> Extend the user-space debug interface so that it can be used to receive
> SSAM events in user-space.
>
> Version 1 and rationale:
> https://lore.kernel.org/platform-driver-x86/[email protected]/
>
> Changes in version 2:
> - PATCH 2/7: Avoid code duplication, remove unused variable
> - PATCH 4/7: Add missing mutex_destroy() calls
>
> 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

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

I've done one small fixup to patch 2/7, see my reply to that patch.

Once the builders have had some time to test this branch the patches there
will be added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



>
> .../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 | 332 +++++++++--
> .../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 | 534 +++++++++++++++++-
> 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, 1018 insertions(+), 109 deletions(-)
>