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

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

[Maximilian Luz]

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(-)

-- 
2.31.1

[PATCH 4/7] platform/surface: aggregator_cdev: Add support for forwarding events to user-space

[Maximilian Luz]

Currently, debugging unknown events requires writing a custom driver.
This is somewhat difficult, slow to adapt, and not entirely
user-friendly for quickly trying to figure out things on devices of some
third-party user. We can do better. We already have a user-space
interface intended for debugging SAM EC requests, so let's add support
for receiving events to that.

This commit provides support for receiving events by reading from the
controller file. It additionally introduces two new IOCTLs to control
which event categories will be forwarded. Specifically, a user-space
client can specify which target categories it wants to receive events
from by registering the corresponding notifier(s) via the IOCTLs and
after that, read the received events by reading from the controller
device.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 .../userspace-api/ioctl/ioctl-number.rst      |   2 +-
 .../surface/surface_aggregator_cdev.c         | 457 +++++++++++++++++-
 include/uapi/linux/surface_aggregator/cdev.h  |  41 +-
 3 files changed, 474 insertions(+), 26 deletions(-)

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 9bfc2b510c64..1409e40e6345 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -325,7 +325,7 @@ Code  Seq#    Include File                                           Comments
 0xA3  90-9F  linux/dtlk.h
 0xA4  00-1F  uapi/linux/tee.h                                        Generic TEE subsystem
 0xA4  00-1F  uapi/asm/sgx.h                                          <mailto:linux-sgx@vger.kernel.org>
-0xA5  01     linux/surface_aggregator/cdev.h                         Microsoft Surface Platform System Aggregator
+0xA5  01-05  linux/surface_aggregator/cdev.h                         Microsoft Surface Platform System Aggregator
                                                                      <mailto:luzmaximilian@gmail.com>
 0xA5  20-2F  linux/surface_aggregator/dtx.h                          Microsoft Surface DTX driver
                                                                      <mailto:luzmaximilian@gmail.com>
diff --git a/drivers/platform/surface/surface_aggregator_cdev.c b/drivers/platform/surface/surface_aggregator_cdev.c
index 79e28fab7e40..807930144039 100644
--- a/drivers/platform/surface/surface_aggregator_cdev.c
+++ b/drivers/platform/surface/surface_aggregator_cdev.c
@@ -3,29 +3,69 @@
  * Provides user-space access to the SSAM EC via the /dev/surface/aggregator
  * misc device. Intended for debugging and development.
  *
- * Copyright (C) 2020 Maximilian Luz <luzmaximilian@gmail.com>
+ * Copyright (C) 2020-2021 Maximilian Luz <luzmaximilian@gmail.com>
  */
 
 #include <linux/fs.h>
+#include <linux/ioctl.h>
 #include <linux/kernel.h>
+#include <linux/kfifo.h>
 #include <linux/kref.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/poll.h>
 #include <linux/rwsem.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include <linux/vmalloc.h>
 
 #include <linux/surface_aggregator/cdev.h>
 #include <linux/surface_aggregator/controller.h>
+#include <linux/surface_aggregator/serial_hub.h>
 
 #define SSAM_CDEV_DEVICE_NAME	"surface_aggregator_cdev"
 
+
+/* -- Main structures. ------------------------------------------------------ */
+
+enum ssam_cdev_device_state {
+	SSAM_CDEV_DEVICE_SHUTDOWN_BIT = BIT(0),
+};
+
 struct ssam_cdev {
 	struct kref kref;
 	struct rw_semaphore lock;
+
+	struct device *dev;
 	struct ssam_controller *ctrl;
 	struct miscdevice mdev;
+	unsigned long flags;
+
+	struct rw_semaphore client_lock;  /* Guards client list. */
+	struct list_head client_list;
+};
+
+struct ssam_cdev_client;
+
+struct ssam_cdev_notifier {
+	struct ssam_cdev_client *client;
+	struct ssam_event_notifier nf;
+};
+
+struct ssam_cdev_client {
+	struct ssam_cdev *cdev;
+	struct list_head node;
+
+	struct mutex notifier_lock;	/* Guards notifier access for registration */
+	struct ssam_cdev_notifier *notifier[SSH_NUM_EVENTS];
+
+	struct mutex read_lock;		/* Guards FIFO buffer read access */
+	struct mutex write_lock;	/* Guards FIFO buffer write access */
+	DECLARE_KFIFO(buffer, u8, 4096);
+
+	wait_queue_head_t waitq;
+	struct fasync_struct *fasync;
 };
 
 static void __ssam_cdev_release(struct kref *kref)
@@ -47,24 +87,169 @@ static void ssam_cdev_put(struct ssam_cdev *cdev)
 		kref_put(&cdev->kref, __ssam_cdev_release);
 }
 
-static int ssam_cdev_device_open(struct inode *inode, struct file *filp)
+
+/* -- Notifier handling. ---------------------------------------------------- */
+
+static u32 ssam_cdev_notifier(struct ssam_event_notifier *nf, const struct ssam_event *in)
 {
-	struct miscdevice *mdev = filp->private_data;
-	struct ssam_cdev *cdev = container_of(mdev, struct ssam_cdev, mdev);
+	struct ssam_cdev_notifier *cdev_nf = container_of(nf, struct ssam_cdev_notifier, nf);
+	struct ssam_cdev_client *client = cdev_nf->client;
+	struct ssam_cdev_event event;
+	size_t n = struct_size(&event, data, in->length);
+
+	/* Translate event. */
+	event.target_category = in->target_category;
+	event.target_id = in->target_id;
+	event.command_id = in->command_id;
+	event.instance_id = in->instance_id;
+	event.length = in->length;
+
+	mutex_lock(&client->write_lock);
+
+	/* Make sure we have enough space. */
+	if (kfifo_avail(&client->buffer) < n) {
+		dev_warn(client->cdev->dev,
+			 "buffer full, dropping event (tc: %#04x, tid: %#04x, cid: %#04x, iid: %#04x)\n",
+			 in->target_category, in->target_id, in->command_id, in->instance_id);
+		mutex_unlock(&client->write_lock);
+		return 0;
+	}
 
-	filp->private_data = ssam_cdev_get(cdev);
-	return stream_open(inode, filp);
+	/* Copy event header and payload. */
+	kfifo_in(&client->buffer, (const u8 *)&event, struct_size(&event, data, 0));
+	kfifo_in(&client->buffer, &in->data[0], in->length);
+
+	mutex_unlock(&client->write_lock);
+
+	/* Notify waiting readers. */
+	kill_fasync(&client->fasync, SIGIO, POLL_IN);
+	wake_up_interruptible(&client->waitq);
+
+	/*
+	 * Don't mark events as handled, this is the job of a proper driver and
+	 * not the debugging interface.
+	 */
+	return 0;
 }
 
-static int ssam_cdev_device_release(struct inode *inode, struct file *filp)
+static int ssam_cdev_notifier_register(struct ssam_cdev_client *client, u8 tc, int priority)
 {
-	ssam_cdev_put(filp->private_data);
-	return 0;
+	const u16 rqid = ssh_tc_to_rqid(tc);
+	const u16 event = ssh_rqid_to_event(rqid);
+	struct ssam_cdev_notifier *nf;
+	int status;
+
+	/* Validate notifier target category. */
+	if (!ssh_rqid_is_event(rqid))
+		return -EINVAL;
+
+	mutex_lock(&client->notifier_lock);
+
+	/* Check if the notifier has already been registered. */
+	if (client->notifier[event]) {
+		mutex_unlock(&client->notifier_lock);
+		return -EEXIST;
+	}
+
+	/* Allocate new notifier. */
+	nf = kzalloc(sizeof(*nf), GFP_KERNEL);
+	if (!nf) {
+		mutex_unlock(&client->notifier_lock);
+		return -ENOMEM;
+	}
+
+	/*
+	 * Create a dummy notifier with the minimal required fields for
+	 * observer registration. Note that we can skip fully specifying event
+	 * and registry here as we do not need any matching and use silent
+	 * registration, which does not enable the corresponding event.
+	 */
+	nf->client = client;
+	nf->nf.base.fn = ssam_cdev_notifier;
+	nf->nf.base.priority = priority;
+	nf->nf.event.id.target_category = tc;
+	nf->nf.event.mask = 0;	/* Do not do any matching. */
+	nf->nf.flags = SSAM_EVENT_NOTIFIER_OBSERVER;
+
+	/* Register notifier. */
+	status = ssam_notifier_register(client->cdev->ctrl, &nf->nf);
+	if (status)
+		kfree(nf);
+	else
+		client->notifier[event] = nf;
+
+	mutex_unlock(&client->notifier_lock);
+	return status;
 }
 
-static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
+static int ssam_cdev_notifier_unregister(struct ssam_cdev_client *client, u8 tc)
+{
+	const u16 rqid = ssh_tc_to_rqid(tc);
+	const u16 event = ssh_rqid_to_event(rqid);
+	int status;
+
+	/* Validate notifier target category. */
+	if (!ssh_rqid_is_event(rqid))
+		return -EINVAL;
+
+	mutex_lock(&client->notifier_lock);
+
+	/* Check if the notifier is currently registered. */
+	if (!client->notifier[event]) {
+		mutex_unlock(&client->notifier_lock);
+		return -ENOENT;
+	}
+
+	/* Unregister and free notifier. */
+	status = ssam_notifier_unregister(client->cdev->ctrl, &client->notifier[event]->nf);
+	kfree(client->notifier[event]);
+	client->notifier[event] = NULL;
+
+	mutex_unlock(&client->notifier_lock);
+	return status;
+}
+
+static void ssam_cdev_notifier_unregister_all(struct ssam_cdev_client *client)
+{
+	int i;
+
+	down_read(&client->cdev->lock);
+
+	/*
+	 * This function may be used during shutdown, thus we need to test for
+	 * cdev->ctrl instead of the SSAM_CDEV_DEVICE_SHUTDOWN_BIT bit.
+	 */
+	if (client->cdev->ctrl) {
+		for (i = 0; i < SSH_NUM_EVENTS; i++)
+			ssam_cdev_notifier_unregister(client, i + 1);
+
+	} else {
+		int count = 0;
+
+		/*
+		 * Device has been shut down. Any notifier remaining is a bug,
+		 * so warn about that as this would otherwise hardly be
+		 * noticeable. Nevertheless, free them as well.
+		 */
+		mutex_lock(&client->notifier_lock);
+		for (i = 0; i < SSH_NUM_EVENTS; i++) {
+			count += !!(client->notifier[i]);
+			kfree(client->notifier[i]);
+			client->notifier[i] = NULL;
+		}
+		mutex_unlock(&client->notifier_lock);
+
+		WARN_ON(count > 0);
+	}
+
+	up_read(&client->cdev->lock);
+}
+
+
+/* -- IOCTL functions. ------------------------------------------------------ */
+
+static long ssam_cdev_request(struct ssam_cdev_client *client, struct ssam_cdev_request __user *r)
 {
-	struct ssam_cdev_request __user *r;
 	struct ssam_cdev_request rqst;
 	struct ssam_request spec = {};
 	struct ssam_response rsp = {};
@@ -72,7 +257,6 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
 	void __user *rspdata;
 	int status = 0, ret = 0, tmp;
 
-	r = (struct ssam_cdev_request __user *)arg;
 	ret = copy_struct_from_user(&rqst, sizeof(rqst), r, sizeof(*r));
 	if (ret)
 		goto out;
@@ -152,7 +336,7 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
 	}
 
 	/* Perform request. */
-	status = ssam_request_sync(cdev->ctrl, &spec, &rsp);
+	status = ssam_request_sync(client->cdev->ctrl, &spec, &rsp);
 	if (status)
 		goto out;
 
@@ -177,48 +361,244 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
 	return ret;
 }
 
-static long __ssam_cdev_device_ioctl(struct ssam_cdev *cdev, unsigned int cmd,
+static long ssam_cdev_notif_register(struct ssam_cdev_client *client,
+				     const struct ssam_cdev_notifier_desc __user *d)
+{
+	struct ssam_cdev_notifier_desc desc;
+	long ret;
+
+	ret = copy_struct_from_user(&desc, sizeof(desc), d, sizeof(*d));
+	if (ret)
+		return ret;
+
+	return ssam_cdev_notifier_register(client, desc.target_category, desc.priority);
+}
+
+static long ssam_cdev_notif_unregister(struct ssam_cdev_client *client,
+				       const struct ssam_cdev_notifier_desc __user *d)
+{
+	struct ssam_cdev_notifier_desc desc;
+	long ret;
+
+	ret = copy_struct_from_user(&desc, sizeof(desc), d, sizeof(*d));
+	if (ret)
+		return ret;
+
+	return ssam_cdev_notifier_unregister(client, desc.target_category);
+}
+
+
+/* -- File operations. ------------------------------------------------------ */
+
+static int ssam_cdev_device_open(struct inode *inode, struct file *filp)
+{
+	struct miscdevice *mdev = filp->private_data;
+	struct ssam_cdev_client *client;
+	struct ssam_cdev *cdev = container_of(mdev, struct ssam_cdev, mdev);
+
+	/* Initialize client */
+	client = vzalloc(sizeof(*client));
+	if (!client)
+		return -ENOMEM;
+
+	client->cdev = ssam_cdev_get(cdev);
+
+	INIT_LIST_HEAD(&client->node);
+
+	mutex_init(&client->notifier_lock);
+
+	mutex_init(&client->read_lock);
+	mutex_init(&client->write_lock);
+	INIT_KFIFO(client->buffer);
+	init_waitqueue_head(&client->waitq);
+
+	filp->private_data = client;
+
+	/* Attach client. */
+	down_write(&cdev->client_lock);
+
+	if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &cdev->flags)) {
+		up_write(&cdev->client_lock);
+		ssam_cdev_put(client->cdev);
+		vfree(client);
+		return -ENODEV;
+	}
+	list_add_tail(&client->node, &cdev->client_list);
+
+	up_write(&cdev->client_lock);
+
+	stream_open(inode, filp);
+	return 0;
+}
+
+static int ssam_cdev_device_release(struct inode *inode, struct file *filp)
+{
+	struct ssam_cdev_client *client = filp->private_data;
+
+	/* Force-unregister all remaining notifiers of this client. */
+	ssam_cdev_notifier_unregister_all(client);
+
+	/* Detach client. */
+	down_write(&client->cdev->client_lock);
+	list_del(&client->node);
+	up_write(&client->cdev->client_lock);
+
+	/* Free client. */
+	mutex_destroy(&client->write_lock);
+	mutex_destroy(&client->read_lock);
+
+	mutex_destroy(&client->notifier_lock);
+
+	ssam_cdev_put(client->cdev);
+	vfree(client);
+
+	return 0;
+}
+
+static long __ssam_cdev_device_ioctl(struct ssam_cdev_client *client, unsigned int cmd,
 				     unsigned long arg)
 {
 	switch (cmd) {
 	case SSAM_CDEV_REQUEST:
-		return ssam_cdev_request(cdev, arg);
+		return ssam_cdev_request(client, (struct ssam_cdev_request __user *)arg);
+
+	case SSAM_CDEV_NOTIF_REGISTER:
+		return ssam_cdev_notif_register(client,
+						(struct ssam_cdev_notifier_desc __user *)arg);
+
+	case SSAM_CDEV_NOTIF_UNREGISTER:
+		return ssam_cdev_notif_unregister(client,
+						  (struct ssam_cdev_notifier_desc __user *)arg);
 
 	default:
 		return -ENOTTY;
 	}
 }
 
-static long ssam_cdev_device_ioctl(struct file *file, unsigned int cmd,
-				   unsigned long arg)
+static long ssam_cdev_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	struct ssam_cdev *cdev = file->private_data;
+	struct ssam_cdev_client *client = file->private_data;
 	long status;
 
 	/* Ensure that controller is valid for as long as we need it. */
+	if (down_read_killable(&client->cdev->lock))
+		return -ERESTARTSYS;
+
+	if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &client->cdev->flags)) {
+		up_read(&client->cdev->lock);
+		return -ENODEV;
+	}
+
+	status = __ssam_cdev_device_ioctl(client, cmd, arg);
+
+	up_read(&client->cdev->lock);
+	return status;
+}
+
+static ssize_t ssam_cdev_read(struct file *file, char __user *buf, size_t count, loff_t *offs)
+{
+	struct ssam_cdev_client *client = file->private_data;
+	struct ssam_cdev *cdev = client->cdev;
+	unsigned int copied;
+	int status = 0;
+
 	if (down_read_killable(&cdev->lock))
 		return -ERESTARTSYS;
 
-	if (!cdev->ctrl) {
+	/* Make sure we're not shut down. */
+	if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &cdev->flags)) {
 		up_read(&cdev->lock);
 		return -ENODEV;
 	}
 
-	status = __ssam_cdev_device_ioctl(cdev, cmd, arg);
+	do {
+		/* Check availability, wait if necessary. */
+		if (kfifo_is_empty(&client->buffer)) {
+			up_read(&cdev->lock);
+
+			if (file->f_flags & O_NONBLOCK)
+				return -EAGAIN;
+
+			status = wait_event_interruptible(client->waitq,
+							  !kfifo_is_empty(&client->buffer) ||
+							  test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT,
+								   &cdev->flags));
+			if (status < 0)
+				return status;
+
+			if (down_read_killable(&cdev->lock))
+				return -ERESTARTSYS;
+
+			/* Need to check that we're not shut down again. */
+			if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &cdev->flags)) {
+				up_read(&cdev->lock);
+				return -ENODEV;
+			}
+		}
+
+		/* Try to read from FIFO. */
+		if (mutex_lock_interruptible(&client->read_lock)) {
+			up_read(&cdev->lock);
+			return -ERESTARTSYS;
+		}
+
+		status = kfifo_to_user(&client->buffer, buf, count, &copied);
+		mutex_unlock(&client->read_lock);
+
+		if (status < 0) {
+			up_read(&cdev->lock);
+			return status;
+		}
+
+		/* We might not have gotten anything, check this here. */
+		if (copied == 0 && (file->f_flags & O_NONBLOCK)) {
+			up_read(&cdev->lock);
+			return -EAGAIN;
+		}
+	} while (copied == 0);
 
 	up_read(&cdev->lock);
-	return status;
+	return copied;
+}
+
+static __poll_t ssam_cdev_poll(struct file *file, struct poll_table_struct *pt)
+{
+	struct ssam_cdev_client *client = file->private_data;
+	__poll_t events = 0;
+
+	if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &client->cdev->flags))
+		return EPOLLHUP | EPOLLERR;
+
+	poll_wait(file, &client->waitq, pt);
+
+	if (!kfifo_is_empty(&client->buffer))
+		events |= EPOLLIN | EPOLLRDNORM;
+
+	return events;
+}
+
+static int ssam_cdev_fasync(int fd, struct file *file, int on)
+{
+	struct ssam_cdev_client *client = file->private_data;
+
+	return fasync_helper(fd, file, on, &client->fasync);
 }
 
 static const struct file_operations ssam_controller_fops = {
 	.owner          = THIS_MODULE,
 	.open           = ssam_cdev_device_open,
 	.release        = ssam_cdev_device_release,
+	.read           = ssam_cdev_read,
+	.poll           = ssam_cdev_poll,
+	.fasync         = ssam_cdev_fasync,
 	.unlocked_ioctl = ssam_cdev_device_ioctl,
 	.compat_ioctl   = ssam_cdev_device_ioctl,
-	.llseek         = noop_llseek,
+	.llseek         = no_llseek,
 };
 
+
+/* -- Device and driver setup ----------------------------------------------- */
+
 static int ssam_dbg_device_probe(struct platform_device *pdev)
 {
 	struct ssam_controller *ctrl;
@@ -236,6 +616,7 @@ static int ssam_dbg_device_probe(struct platform_device *pdev)
 	kref_init(&cdev->kref);
 	init_rwsem(&cdev->lock);
 	cdev->ctrl = ctrl;
+	cdev->dev = &pdev->dev;
 
 	cdev->mdev.parent   = &pdev->dev;
 	cdev->mdev.minor    = MISC_DYNAMIC_MINOR;
@@ -243,6 +624,9 @@ static int ssam_dbg_device_probe(struct platform_device *pdev)
 	cdev->mdev.nodename = "surface/aggregator";
 	cdev->mdev.fops     = &ssam_controller_fops;
 
+	init_rwsem(&cdev->client_lock);
+	INIT_LIST_HEAD(&cdev->client_list);
+
 	status = misc_register(&cdev->mdev);
 	if (status) {
 		kfree(cdev);
@@ -256,8 +640,32 @@ static int ssam_dbg_device_probe(struct platform_device *pdev)
 static int ssam_dbg_device_remove(struct platform_device *pdev)
 {
 	struct ssam_cdev *cdev = platform_get_drvdata(pdev);
+	struct ssam_cdev_client *client;
 
-	misc_deregister(&cdev->mdev);
+	/*
+	 * Mark device as shut-down. Prevent new clients from being added and
+	 * new operations from being executed.
+	 */
+	set_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &cdev->flags);
+
+	down_write(&cdev->client_lock);
+
+	/* Remove all notifiers registered by us. */
+	list_for_each_entry(client, &cdev->client_list, node) {
+		ssam_cdev_notifier_unregister_all(client);
+	}
+
+	/* Wake up async clients. */
+	list_for_each_entry(client, &cdev->client_list, node) {
+		kill_fasync(&client->fasync, SIGIO, POLL_HUP);
+	}
+
+	/* Wake up blocking clients. */
+	list_for_each_entry(client, &cdev->client_list, node) {
+		wake_up_interruptible(&client->waitq);
+	}
+
+	up_write(&cdev->client_lock);
 
 	/*
 	 * The controller is only guaranteed to be valid for as long as the
@@ -266,8 +674,11 @@ static int ssam_dbg_device_remove(struct platform_device *pdev)
 	 */
 	down_write(&cdev->lock);
 	cdev->ctrl = NULL;
+	cdev->dev = NULL;
 	up_write(&cdev->lock);
 
+	misc_deregister(&cdev->mdev);
+
 	ssam_cdev_put(cdev);
 	return 0;
 }
diff --git a/include/uapi/linux/surface_aggregator/cdev.h b/include/uapi/linux/surface_aggregator/cdev.h
index fbcce04abfe9..4f393fafc235 100644
--- a/include/uapi/linux/surface_aggregator/cdev.h
+++ b/include/uapi/linux/surface_aggregator/cdev.h
@@ -6,7 +6,7 @@
  * device. This device provides direct user-space access to the SSAM EC.
  * Intended for debugging and development.
  *
- * Copyright (C) 2020 Maximilian Luz <luzmaximilian@gmail.com>
+ * Copyright (C) 2020-2021 Maximilian Luz <luzmaximilian@gmail.com>
  */
 
 #ifndef _UAPI_LINUX_SURFACE_AGGREGATOR_CDEV_H
@@ -73,6 +73,43 @@ struct ssam_cdev_request {
 	} response;
 } __attribute__((__packed__));
 
-#define SSAM_CDEV_REQUEST	_IOWR(0xA5, 1, struct ssam_cdev_request)
+/**
+ * struct ssam_cdev_notifier_desc - Notifier descriptor.
+ * @priority:        Priority value determining the order in which notifier
+ *                   callbacks will be called. A higher value means higher
+ *                   priority, i.e. the associated callback will be executed
+ *                   earlier than other (lower priority) callbacks.
+ * @target_category: The event target category for which this notifier should
+ *                   receive events.
+ *
+ * Specifies the notifier that should be registered or unregistered,
+ * specifically with which priority and for which target category of events.
+ */
+struct ssam_cdev_notifier_desc {
+	__s32 priority;
+	__u8 target_category;
+} __attribute__((__packed__));
+
+/**
+ * struct ssam_cdev_event - SSAM event sent by the EC.
+ * @target_category: Target category of the event source. See &enum ssam_ssh_tc.
+ * @target_id:       Target ID of the event source.
+ * @command_id:      Command ID of the event.
+ * @instance_id:     Instance ID of the event source.
+ * @length:          Length of the event payload in bytes.
+ * @data:            Event payload data.
+ */
+struct ssam_cdev_event {
+	__u8 target_category;
+	__u8 target_id;
+	__u8 command_id;
+	__u8 instance_id;
+	__u16 length;
+	__u8 data[];
+} __attribute__((__packed__));
+
+#define SSAM_CDEV_REQUEST		_IOWR(0xA5, 1, struct ssam_cdev_request)
+#define SSAM_CDEV_NOTIF_REGISTER	_IOW(0xA5, 2, struct ssam_cdev_notifier_desc)
+#define SSAM_CDEV_NOTIF_UNREGISTER	_IOW(0xA5, 3, struct ssam_cdev_notifier_desc)
 
 #endif /* _UAPI_LINUX_SURFACE_AGGREGATOR_CDEV_H */
-- 
2.31.1

[PATCH 7/7] docs: driver-api: Update Surface Aggregator user-space interface documentation

[Maximilian Luz]

Update the controller-device user-space interface (cdev) documentation
for the newly introduced IOCTLs and event interface.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 .../surface_aggregator/clients/cdev.rst       | 127 +++++++++++++++++-
 1 file changed, 122 insertions(+), 5 deletions(-)

diff --git a/Documentation/driver-api/surface_aggregator/clients/cdev.rst b/Documentation/driver-api/surface_aggregator/clients/cdev.rst
index 248c1372d879..0134a841a079 100644
--- a/Documentation/driver-api/surface_aggregator/clients/cdev.rst
+++ b/Documentation/driver-api/surface_aggregator/clients/cdev.rst
@@ -1,9 +1,8 @@
 .. SPDX-License-Identifier: GPL-2.0+
 
-.. |u8| replace:: :c:type:`u8 <u8>`
-.. |u16| replace:: :c:type:`u16 <u16>`
 .. |ssam_cdev_request| replace:: :c:type:`struct ssam_cdev_request <ssam_cdev_request>`
 .. |ssam_cdev_request_flags| replace:: :c:type:`enum ssam_cdev_request_flags <ssam_cdev_request_flags>`
+.. |ssam_cdev_event| replace:: :c:type:`struct ssam_cdev_event <ssam_cdev_event>`
 
 ==============================
 User-Space EC Interface (cdev)
@@ -23,6 +22,40 @@ These IOCTLs and their respective input/output parameter structs are defined in
 A small python library and scripts for accessing this interface can be found
 at https://github.com/linux-surface/surface-aggregator-module/tree/master/scripts/ssam.
 
+.. contents::
+
+
+Receiving Events
+================
+
+Events can be received by reading from the device-file. The are represented by
+the |ssam_cdev_event| datatype.
+
+Before events are available to be read, however, the desired notifiers must be
+registered via the ``SSAM_CDEV_NOTIF_REGISTER`` IOCTL. Notifiers are, in
+essence, callbacks, called when the EC sends an event. They are, in this
+interface, associated with a specific target category and device-file-instance.
+They forward any event of this category to the buffer of the corresponding
+instance, from which it can then be read.
+
+Notifiers themselves do not enable events on the EC. Thus, it may additionally
+be necessary to enable events via the ``SSAM_CDEV_EVENT_ENABLE`` IOCTL. While
+notifiers work per-client (i.e. per-device-file-instance), events are enabled
+globally, for the EC and all of its clients (regardless of userspace or
+non-userspace). The ``SSAM_CDEV_EVENT_ENABLE`` and ``SSAM_CDEV_EVENT_DISABLE``
+IOCTLs take care of reference counting the events, such that an event is
+enabled as long as there is a client that has requested it.
+
+Note that enabled events are not automatically disabled once the client
+instance is closed. Therefore any client process (or group of processes) should
+balance their event enable calls with the corresponding event disable calls. It
+is, however, perfectly valid to enable and disable events on different client
+instances. For example, it is valid to set up notifiers and read events on
+client instance ``A``, enable those events on instance ``B`` (note that these
+will also be received by A since events are enabled/disabled globally), and
+after no more events are desired, disable the previously enabled events via
+instance ``C``.
+
 
 Controller IOCTLs
 =================
@@ -45,9 +78,33 @@ The following IOCTLs are provided:
      - ``REQUEST``
      - Perform synchronous SAM request.
 
+   * - ``0xA5``
+     - ``2``
+     - ``W``
+     - ``NOTIF_REGISTER``
+     - Register event notifier.
 
-``REQUEST``
------------
+   * - ``0xA5``
+     - ``3``
+     - ``W``
+     - ``NOTIF_UNREGISTER``
+     - Unregister event notifier.
+
+   * - ``0xA5``
+     - ``4``
+     - ``W``
+     - ``EVENT_ENABLE``
+     - Enable event source.
+
+   * - ``0xA5``
+     - ``5``
+     - ``W``
+     - ``EVENT_DISABLE``
+     - Disable event source.
+
+
+``SSAM_CDEV_REQUEST``
+---------------------
 
 Defined as ``_IOWR(0xA5, 1, struct ssam_cdev_request)``.
 
@@ -82,6 +139,66 @@ submitted, and completed (i.e. handed back to user-space) successfully from
 inside the IOCTL, but the request ``status`` member may still be negative in
 case the actual execution of the request failed after it has been submitted.
 
-A full definition of the argument struct is provided below:
+A full definition of the argument struct is provided below.
+
+``SSAM_CDEV_NOTIF_REGISTER``
+----------------------------
+
+Defined as ``_IOW(0xA5, 2, struct ssam_cdev_notifier_desc)``.
+
+Register a notifier for the event target category specified in the given
+notifier description with the specified priority. Notifiers registration is
+required to receive events, but does not enable events themselves. After a
+notifier for a specific target category has been registered, all events of that
+category will be forwarded to the userspace client and can then be read from
+the device file instance. Note that events may have to be enabled, e.g. via the
+``SSAM_CDEV_EVENT_ENABLE`` IOCTL, before the EC will send them.
+
+Only one notifier can be registered per target category and client instance. If
+a notifier has already been registered, this IOCTL will fail with ``-EEXIST``.
+
+Notifiers will automatically be removed when the device file instance is
+closed.
+
+``SSAM_CDEV_NOTIF_UNREGISTER``
+------------------------------
+
+Defined as ``_IOW(0xA5, 3, struct ssam_cdev_notifier_desc)``.
+
+Unregisters the notifier associated with the specified target category. The
+priority field will be ignored by this IOCTL. If no notifier has been
+registered for this client instance and the given category, this IOCTL will
+fail with ``-ENOENT``.
+
+``SSAM_CDEV_EVENT_ENABLE``
+--------------------------
+
+Defined as ``_IOW(0xA5, 4, struct ssam_cdev_event_desc)``.
+
+Enable the event associated with the given event descriptor.
+
+Note that this call will not register a notifier itself, it will only enable
+events on the controller. If you want to receive events by reading from the
+device file, you will need to register the corresponding notifier(s) on that
+instance.
+
+Events are not automatically disabled when the device file is closed. This must
+be done manually, via a call to the ``SSAM_CDEV_EVENT_DISABLE`` IOCTL.
+
+``SSAM_CDEV_EVENT_DISABLE``
+---------------------------
+
+Defined as ``_IOW(0xA5, 5, struct ssam_cdev_event_desc)``.
+
+Disable the event associated with the given event descriptor.
+
+Note that this will not unregister any notifiers. Events may still be received
+and forwarded to user-space after this call. The only safe way of stopping
+events from being received is unregistering all previously registered
+notifiers.
+
+
+Structures and Enums
+====================
 
 .. kernel-doc:: include/uapi/linux/surface_aggregator/cdev.h
-- 
2.31.1

Re: [PATCH 4/7] platform/surface: aggregator_cdev: Add support for forwarding events to user-space

[Hans de Goede]

Hi,

I've one review remark inline below.

On 6/4/21 1:45 AM, Maximilian Luz wrote:
> Currently, debugging unknown events requires writing a custom driver.
> This is somewhat difficult, slow to adapt, and not entirely
> user-friendly for quickly trying to figure out things on devices of some
> third-party user. We can do better. We already have a user-space
> interface intended for debugging SAM EC requests, so let's add support
> for receiving events to that.
> 
> This commit provides support for receiving events by reading from the
> controller file. It additionally introduces two new IOCTLs to control
> which event categories will be forwarded. Specifically, a user-space
> client can specify which target categories it wants to receive events
> from by registering the corresponding notifier(s) via the IOCTLs and
> after that, read the received events by reading from the controller
> device.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
>  .../userspace-api/ioctl/ioctl-number.rst      |   2 +-
>  .../surface/surface_aggregator_cdev.c         | 457 +++++++++++++++++-
>  include/uapi/linux/surface_aggregator/cdev.h  |  41 +-
>  3 files changed, 474 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 9bfc2b510c64..1409e40e6345 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -325,7 +325,7 @@ Code  Seq#    Include File                                           Comments
>  0xA3  90-9F  linux/dtlk.h
>  0xA4  00-1F  uapi/linux/tee.h                                        Generic TEE subsystem
>  0xA4  00-1F  uapi/asm/sgx.h                                          <mailto:linux-sgx@vger.kernel.org>
> -0xA5  01     linux/surface_aggregator/cdev.h                         Microsoft Surface Platform System Aggregator
> +0xA5  01-05  linux/surface_aggregator/cdev.h                         Microsoft Surface Platform System Aggregator
>                                                                       <mailto:luzmaximilian@gmail.com>
>  0xA5  20-2F  linux/surface_aggregator/dtx.h                          Microsoft Surface DTX driver
>                                                                       <mailto:luzmaximilian@gmail.com>
> diff --git a/drivers/platform/surface/surface_aggregator_cdev.c b/drivers/platform/surface/surface_aggregator_cdev.c
> index 79e28fab7e40..807930144039 100644
> --- a/drivers/platform/surface/surface_aggregator_cdev.c
> +++ b/drivers/platform/surface/surface_aggregator_cdev.c
> @@ -3,29 +3,69 @@
>   * Provides user-space access to the SSAM EC via the /dev/surface/aggregator
>   * misc device. Intended for debugging and development.
>   *
> - * Copyright (C) 2020 Maximilian Luz <luzmaximilian@gmail.com>
> + * Copyright (C) 2020-2021 Maximilian Luz <luzmaximilian@gmail.com>
>   */
>  
>  #include <linux/fs.h>
> +#include <linux/ioctl.h>
>  #include <linux/kernel.h>
> +#include <linux/kfifo.h>
>  #include <linux/kref.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/poll.h>
>  #include <linux/rwsem.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
> +#include <linux/vmalloc.h>
>  
>  #include <linux/surface_aggregator/cdev.h>
>  #include <linux/surface_aggregator/controller.h>
> +#include <linux/surface_aggregator/serial_hub.h>
>  
>  #define SSAM_CDEV_DEVICE_NAME	"surface_aggregator_cdev"
>  
> +
> +/* -- Main structures. ------------------------------------------------------ */
> +
> +enum ssam_cdev_device_state {
> +	SSAM_CDEV_DEVICE_SHUTDOWN_BIT = BIT(0),
> +};
> +
>  struct ssam_cdev {
>  	struct kref kref;
>  	struct rw_semaphore lock;
> +
> +	struct device *dev;
>  	struct ssam_controller *ctrl;
>  	struct miscdevice mdev;
> +	unsigned long flags;
> +
> +	struct rw_semaphore client_lock;  /* Guards client list. */
> +	struct list_head client_list;
> +};
> +
> +struct ssam_cdev_client;
> +
> +struct ssam_cdev_notifier {
> +	struct ssam_cdev_client *client;
> +	struct ssam_event_notifier nf;
> +};
> +
> +struct ssam_cdev_client {
> +	struct ssam_cdev *cdev;
> +	struct list_head node;
> +
> +	struct mutex notifier_lock;	/* Guards notifier access for registration */
> +	struct ssam_cdev_notifier *notifier[SSH_NUM_EVENTS];
> +
> +	struct mutex read_lock;		/* Guards FIFO buffer read access */
> +	struct mutex write_lock;	/* Guards FIFO buffer write access */
> +	DECLARE_KFIFO(buffer, u8, 4096);
> +
> +	wait_queue_head_t waitq;
> +	struct fasync_struct *fasync;
>  };
>  
>  static void __ssam_cdev_release(struct kref *kref)
> @@ -47,24 +87,169 @@ static void ssam_cdev_put(struct ssam_cdev *cdev)
>  		kref_put(&cdev->kref, __ssam_cdev_release);
>  }
>  
> -static int ssam_cdev_device_open(struct inode *inode, struct file *filp)
> +
> +/* -- Notifier handling. ---------------------------------------------------- */
> +
> +static u32 ssam_cdev_notifier(struct ssam_event_notifier *nf, const struct ssam_event *in)
>  {
> -	struct miscdevice *mdev = filp->private_data;
> -	struct ssam_cdev *cdev = container_of(mdev, struct ssam_cdev, mdev);
> +	struct ssam_cdev_notifier *cdev_nf = container_of(nf, struct ssam_cdev_notifier, nf);
> +	struct ssam_cdev_client *client = cdev_nf->client;
> +	struct ssam_cdev_event event;
> +	size_t n = struct_size(&event, data, in->length);
> +
> +	/* Translate event. */
> +	event.target_category = in->target_category;
> +	event.target_id = in->target_id;
> +	event.command_id = in->command_id;
> +	event.instance_id = in->instance_id;
> +	event.length = in->length;
> +
> +	mutex_lock(&client->write_lock);
> +
> +	/* Make sure we have enough space. */
> +	if (kfifo_avail(&client->buffer) < n) {
> +		dev_warn(client->cdev->dev,
> +			 "buffer full, dropping event (tc: %#04x, tid: %#04x, cid: %#04x, iid: %#04x)\n",
> +			 in->target_category, in->target_id, in->command_id, in->instance_id);
> +		mutex_unlock(&client->write_lock);
> +		return 0;
> +	}
>  
> -	filp->private_data = ssam_cdev_get(cdev);
> -	return stream_open(inode, filp);
> +	/* Copy event header and payload. */
> +	kfifo_in(&client->buffer, (const u8 *)&event, struct_size(&event, data, 0));
> +	kfifo_in(&client->buffer, &in->data[0], in->length);
> +
> +	mutex_unlock(&client->write_lock);
> +
> +	/* Notify waiting readers. */
> +	kill_fasync(&client->fasync, SIGIO, POLL_IN);
> +	wake_up_interruptible(&client->waitq);
> +
> +	/*
> +	 * Don't mark events as handled, this is the job of a proper driver and
> +	 * not the debugging interface.
> +	 */
> +	return 0;
>  }
>  
> -static int ssam_cdev_device_release(struct inode *inode, struct file *filp)
> +static int ssam_cdev_notifier_register(struct ssam_cdev_client *client, u8 tc, int priority)
>  {
> -	ssam_cdev_put(filp->private_data);
> -	return 0;
> +	const u16 rqid = ssh_tc_to_rqid(tc);
> +	const u16 event = ssh_rqid_to_event(rqid);
> +	struct ssam_cdev_notifier *nf;
> +	int status;
> +
> +	/* Validate notifier target category. */
> +	if (!ssh_rqid_is_event(rqid))
> +		return -EINVAL;
> +
> +	mutex_lock(&client->notifier_lock);
> +
> +	/* Check if the notifier has already been registered. */
> +	if (client->notifier[event]) {
> +		mutex_unlock(&client->notifier_lock);
> +		return -EEXIST;
> +	}
> +
> +	/* Allocate new notifier. */
> +	nf = kzalloc(sizeof(*nf), GFP_KERNEL);
> +	if (!nf) {
> +		mutex_unlock(&client->notifier_lock);
> +		return -ENOMEM;
> +	}
> +
> +	/*
> +	 * Create a dummy notifier with the minimal required fields for
> +	 * observer registration. Note that we can skip fully specifying event
> +	 * and registry here as we do not need any matching and use silent
> +	 * registration, which does not enable the corresponding event.
> +	 */
> +	nf->client = client;
> +	nf->nf.base.fn = ssam_cdev_notifier;
> +	nf->nf.base.priority = priority;
> +	nf->nf.event.id.target_category = tc;
> +	nf->nf.event.mask = 0;	/* Do not do any matching. */
> +	nf->nf.flags = SSAM_EVENT_NOTIFIER_OBSERVER;
> +
> +	/* Register notifier. */
> +	status = ssam_notifier_register(client->cdev->ctrl, &nf->nf);
> +	if (status)
> +		kfree(nf);
> +	else
> +		client->notifier[event] = nf;
> +
> +	mutex_unlock(&client->notifier_lock);
> +	return status;
>  }
>  
> -static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
> +static int ssam_cdev_notifier_unregister(struct ssam_cdev_client *client, u8 tc)
> +{
> +	const u16 rqid = ssh_tc_to_rqid(tc);
> +	const u16 event = ssh_rqid_to_event(rqid);
> +	int status;
> +
> +	/* Validate notifier target category. */
> +	if (!ssh_rqid_is_event(rqid))
> +		return -EINVAL;
> +
> +	mutex_lock(&client->notifier_lock);
> +
> +	/* Check if the notifier is currently registered. */
> +	if (!client->notifier[event]) {
> +		mutex_unlock(&client->notifier_lock);
> +		return -ENOENT;
> +	}
> +
> +	/* Unregister and free notifier. */
> +	status = ssam_notifier_unregister(client->cdev->ctrl, &client->notifier[event]->nf);
> +	kfree(client->notifier[event]);
> +	client->notifier[event] = NULL;
> +
> +	mutex_unlock(&client->notifier_lock);
> +	return status;
> +}
> +
> +static void ssam_cdev_notifier_unregister_all(struct ssam_cdev_client *client)
> +{
> +	int i;
> +
> +	down_read(&client->cdev->lock);
> +
> +	/*
> +	 * This function may be used during shutdown, thus we need to test for
> +	 * cdev->ctrl instead of the SSAM_CDEV_DEVICE_SHUTDOWN_BIT bit.
> +	 */
> +	if (client->cdev->ctrl) {
> +		for (i = 0; i < SSH_NUM_EVENTS; i++)
> +			ssam_cdev_notifier_unregister(client, i + 1);
> +
> +	} else {
> +		int count = 0;
> +
> +		/*
> +		 * Device has been shut down. Any notifier remaining is a bug,
> +		 * so warn about that as this would otherwise hardly be
> +		 * noticeable. Nevertheless, free them as well.
> +		 */
> +		mutex_lock(&client->notifier_lock);
> +		for (i = 0; i < SSH_NUM_EVENTS; i++) {
> +			count += !!(client->notifier[i]);
> +			kfree(client->notifier[i]);
> +			client->notifier[i] = NULL;
> +		}
> +		mutex_unlock(&client->notifier_lock);
> +
> +		WARN_ON(count > 0);
> +	}
> +
> +	up_read(&client->cdev->lock);
> +}
> +
> +
> +/* -- IOCTL functions. ------------------------------------------------------ */
> +
> +static long ssam_cdev_request(struct ssam_cdev_client *client, struct ssam_cdev_request __user *r)
>  {
> -	struct ssam_cdev_request __user *r;
>  	struct ssam_cdev_request rqst;
>  	struct ssam_request spec = {};
>  	struct ssam_response rsp = {};
> @@ -72,7 +257,6 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
>  	void __user *rspdata;
>  	int status = 0, ret = 0, tmp;
>  
> -	r = (struct ssam_cdev_request __user *)arg;
>  	ret = copy_struct_from_user(&rqst, sizeof(rqst), r, sizeof(*r));
>  	if (ret)
>  		goto out;
> @@ -152,7 +336,7 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
>  	}
>  
>  	/* Perform request. */
> -	status = ssam_request_sync(cdev->ctrl, &spec, &rsp);
> +	status = ssam_request_sync(client->cdev->ctrl, &spec, &rsp);
>  	if (status)
>  		goto out;
>  
> @@ -177,48 +361,244 @@ static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
>  	return ret;
>  }
>  
> -static long __ssam_cdev_device_ioctl(struct ssam_cdev *cdev, unsigned int cmd,
> +static long ssam_cdev_notif_register(struct ssam_cdev_client *client,
> +				     const struct ssam_cdev_notifier_desc __user *d)
> +{
> +	struct ssam_cdev_notifier_desc desc;
> +	long ret;
> +
> +	ret = copy_struct_from_user(&desc, sizeof(desc), d, sizeof(*d));
> +	if (ret)
> +		return ret;
> +
> +	return ssam_cdev_notifier_register(client, desc.target_category, desc.priority);
> +}
> +
> +static long ssam_cdev_notif_unregister(struct ssam_cdev_client *client,
> +				       const struct ssam_cdev_notifier_desc __user *d)
> +{
> +	struct ssam_cdev_notifier_desc desc;
> +	long ret;
> +
> +	ret = copy_struct_from_user(&desc, sizeof(desc), d, sizeof(*d));
> +	if (ret)
> +		return ret;
> +
> +	return ssam_cdev_notifier_unregister(client, desc.target_category);
> +}
> +
> +
> +/* -- File operations. ------------------------------------------------------ */
> +
> +static int ssam_cdev_device_open(struct inode *inode, struct file *filp)
> +{
> +	struct miscdevice *mdev = filp->private_data;
> +	struct ssam_cdev_client *client;
> +	struct ssam_cdev *cdev = container_of(mdev, struct ssam_cdev, mdev);
> +
> +	/* Initialize client */
> +	client = vzalloc(sizeof(*client));
> +	if (!client)
> +		return -ENOMEM;
> +
> +	client->cdev = ssam_cdev_get(cdev);
> +
> +	INIT_LIST_HEAD(&client->node);
> +
> +	mutex_init(&client->notifier_lock);
> +
> +	mutex_init(&client->read_lock);
> +	mutex_init(&client->write_lock);
> +	INIT_KFIFO(client->buffer);
> +	init_waitqueue_head(&client->waitq);
> +
> +	filp->private_data = client;
> +
> +	/* Attach client. */
> +	down_write(&cdev->client_lock);
> +
> +	if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &cdev->flags)) {
> +		up_write(&cdev->client_lock);
> +		ssam_cdev_put(client->cdev);

You are missing the mutex_destroy() calls here which you are
doing in ssam_cdev_device_release().

Or maybe move the mutex_init calls below this check
(before the up_write()) since I don't think the client can
be accessed by any code until the up_write is done?

Regards,

Hans


> +		vfree(client);
> +		return -ENODEV;
> +	}
> +	list_add_tail(&client->node, &cdev->client_list);
> +
> +	up_write(&cdev->client_lock);
> +
> +	stream_open(inode, filp);
> +	return 0;
> +}
> +
> +static int ssam_cdev_device_release(struct inode *inode, struct file *filp)
> +{
> +	struct ssam_cdev_client *client = filp->private_data;
> +
> +	/* Force-unregister all remaining notifiers of this client. */
> +	ssam_cdev_notifier_unregister_all(client);
> +
> +	/* Detach client. */
> +	down_write(&client->cdev->client_lock);
> +	list_del(&client->node);
> +	up_write(&client->cdev->client_lock);
> +
> +	/* Free client. */
> +	mutex_destroy(&client->write_lock);
> +	mutex_destroy(&client->read_lock);
> +
> +	mutex_destroy(&client->notifier_lock);
> +
> +	ssam_cdev_put(client->cdev);
> +	vfree(client);
> +
> +	return 0;
> +}
> +
> +static long __ssam_cdev_device_ioctl(struct ssam_cdev_client *client, unsigned int cmd,
>  				     unsigned long arg)
>  {
>  	switch (cmd) {
>  	case SSAM_CDEV_REQUEST:
> -		return ssam_cdev_request(cdev, arg);
> +		return ssam_cdev_request(client, (struct ssam_cdev_request __user *)arg);
> +
> +	case SSAM_CDEV_NOTIF_REGISTER:
> +		return ssam_cdev_notif_register(client,
> +						(struct ssam_cdev_notifier_desc __user *)arg);
> +
> +	case SSAM_CDEV_NOTIF_UNREGISTER:
> +		return ssam_cdev_notif_unregister(client,
> +						  (struct ssam_cdev_notifier_desc __user *)arg);
>  
>  	default:
>  		return -ENOTTY;
>  	}
>  }
>  
> -static long ssam_cdev_device_ioctl(struct file *file, unsigned int cmd,
> -				   unsigned long arg)
> +static long ssam_cdev_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
> -	struct ssam_cdev *cdev = file->private_data;
> +	struct ssam_cdev_client *client = file->private_data;
>  	long status;
>  
>  	/* Ensure that controller is valid for as long as we need it. */
> +	if (down_read_killable(&client->cdev->lock))
> +		return -ERESTARTSYS;
> +
> +	if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &client->cdev->flags)) {
> +		up_read(&client->cdev->lock);
> +		return -ENODEV;
> +	}
> +
> +	status = __ssam_cdev_device_ioctl(client, cmd, arg);
> +
> +	up_read(&client->cdev->lock);
> +	return status;
> +}
> +
> +static ssize_t ssam_cdev_read(struct file *file, char __user *buf, size_t count, loff_t *offs)
> +{
> +	struct ssam_cdev_client *client = file->private_data;
> +	struct ssam_cdev *cdev = client->cdev;
> +	unsigned int copied;
> +	int status = 0;
> +
>  	if (down_read_killable(&cdev->lock))
>  		return -ERESTARTSYS;
>  
> -	if (!cdev->ctrl) {
> +	/* Make sure we're not shut down. */
> +	if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &cdev->flags)) {
>  		up_read(&cdev->lock);
>  		return -ENODEV;
>  	}
>  
> -	status = __ssam_cdev_device_ioctl(cdev, cmd, arg);
> +	do {
> +		/* Check availability, wait if necessary. */
> +		if (kfifo_is_empty(&client->buffer)) {
> +			up_read(&cdev->lock);
> +
> +			if (file->f_flags & O_NONBLOCK)
> +				return -EAGAIN;
> +
> +			status = wait_event_interruptible(client->waitq,
> +							  !kfifo_is_empty(&client->buffer) ||
> +							  test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT,
> +								   &cdev->flags));
> +			if (status < 0)
> +				return status;
> +
> +			if (down_read_killable(&cdev->lock))
> +				return -ERESTARTSYS;
> +
> +			/* Need to check that we're not shut down again. */
> +			if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &cdev->flags)) {
> +				up_read(&cdev->lock);
> +				return -ENODEV;
> +			}
> +		}
> +
> +		/* Try to read from FIFO. */
> +		if (mutex_lock_interruptible(&client->read_lock)) {
> +			up_read(&cdev->lock);
> +			return -ERESTARTSYS;
> +		}
> +
> +		status = kfifo_to_user(&client->buffer, buf, count, &copied);
> +		mutex_unlock(&client->read_lock);
> +
> +		if (status < 0) {
> +			up_read(&cdev->lock);
> +			return status;
> +		}
> +
> +		/* We might not have gotten anything, check this here. */
> +		if (copied == 0 && (file->f_flags & O_NONBLOCK)) {
> +			up_read(&cdev->lock);
> +			return -EAGAIN;
> +		}
> +	} while (copied == 0);
>  
>  	up_read(&cdev->lock);
> -	return status;
> +	return copied;
> +}
> +
> +static __poll_t ssam_cdev_poll(struct file *file, struct poll_table_struct *pt)
> +{
> +	struct ssam_cdev_client *client = file->private_data;
> +	__poll_t events = 0;
> +
> +	if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &client->cdev->flags))
> +		return EPOLLHUP | EPOLLERR;
> +
> +	poll_wait(file, &client->waitq, pt);
> +
> +	if (!kfifo_is_empty(&client->buffer))
> +		events |= EPOLLIN | EPOLLRDNORM;
> +
> +	return events;
> +}
> +
> +static int ssam_cdev_fasync(int fd, struct file *file, int on)
> +{
> +	struct ssam_cdev_client *client = file->private_data;
> +
> +	return fasync_helper(fd, file, on, &client->fasync);
>  }
>  
>  static const struct file_operations ssam_controller_fops = {
>  	.owner          = THIS_MODULE,
>  	.open           = ssam_cdev_device_open,
>  	.release        = ssam_cdev_device_release,
> +	.read           = ssam_cdev_read,
> +	.poll           = ssam_cdev_poll,
> +	.fasync         = ssam_cdev_fasync,
>  	.unlocked_ioctl = ssam_cdev_device_ioctl,
>  	.compat_ioctl   = ssam_cdev_device_ioctl,
> -	.llseek         = noop_llseek,
> +	.llseek         = no_llseek,
>  };
>  
> +
> +/* -- Device and driver setup ----------------------------------------------- */
> +
>  static int ssam_dbg_device_probe(struct platform_device *pdev)
>  {
>  	struct ssam_controller *ctrl;
> @@ -236,6 +616,7 @@ static int ssam_dbg_device_probe(struct platform_device *pdev)
>  	kref_init(&cdev->kref);
>  	init_rwsem(&cdev->lock);
>  	cdev->ctrl = ctrl;
> +	cdev->dev = &pdev->dev;
>  
>  	cdev->mdev.parent   = &pdev->dev;
>  	cdev->mdev.minor    = MISC_DYNAMIC_MINOR;
> @@ -243,6 +624,9 @@ static int ssam_dbg_device_probe(struct platform_device *pdev)
>  	cdev->mdev.nodename = "surface/aggregator";
>  	cdev->mdev.fops     = &ssam_controller_fops;
>  
> +	init_rwsem(&cdev->client_lock);
> +	INIT_LIST_HEAD(&cdev->client_list);
> +
>  	status = misc_register(&cdev->mdev);
>  	if (status) {
>  		kfree(cdev);
> @@ -256,8 +640,32 @@ static int ssam_dbg_device_probe(struct platform_device *pdev)
>  static int ssam_dbg_device_remove(struct platform_device *pdev)
>  {
>  	struct ssam_cdev *cdev = platform_get_drvdata(pdev);
> +	struct ssam_cdev_client *client;
>  
> -	misc_deregister(&cdev->mdev);
> +	/*
> +	 * Mark device as shut-down. Prevent new clients from being added and
> +	 * new operations from being executed.
> +	 */
> +	set_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &cdev->flags);
> +
> +	down_write(&cdev->client_lock);
> +
> +	/* Remove all notifiers registered by us. */
> +	list_for_each_entry(client, &cdev->client_list, node) {
> +		ssam_cdev_notifier_unregister_all(client);
> +	}
> +
> +	/* Wake up async clients. */
> +	list_for_each_entry(client, &cdev->client_list, node) {
> +		kill_fasync(&client->fasync, SIGIO, POLL_HUP);
> +	}
> +
> +	/* Wake up blocking clients. */
> +	list_for_each_entry(client, &cdev->client_list, node) {
> +		wake_up_interruptible(&client->waitq);
> +	}
> +
> +	up_write(&cdev->client_lock);
>  
>  	/*
>  	 * The controller is only guaranteed to be valid for as long as the
> @@ -266,8 +674,11 @@ static int ssam_dbg_device_remove(struct platform_device *pdev)
>  	 */
>  	down_write(&cdev->lock);
>  	cdev->ctrl = NULL;
> +	cdev->dev = NULL;
>  	up_write(&cdev->lock);
>  
> +	misc_deregister(&cdev->mdev);
> +
>  	ssam_cdev_put(cdev);
>  	return 0;
>  }
> diff --git a/include/uapi/linux/surface_aggregator/cdev.h b/include/uapi/linux/surface_aggregator/cdev.h
> index fbcce04abfe9..4f393fafc235 100644
> --- a/include/uapi/linux/surface_aggregator/cdev.h
> +++ b/include/uapi/linux/surface_aggregator/cdev.h
> @@ -6,7 +6,7 @@
>   * device. This device provides direct user-space access to the SSAM EC.
>   * Intended for debugging and development.
>   *
> - * Copyright (C) 2020 Maximilian Luz <luzmaximilian@gmail.com>
> + * Copyright (C) 2020-2021 Maximilian Luz <luzmaximilian@gmail.com>
>   */
>  
>  #ifndef _UAPI_LINUX_SURFACE_AGGREGATOR_CDEV_H
> @@ -73,6 +73,43 @@ struct ssam_cdev_request {
>  	} response;
>  } __attribute__((__packed__));
>  
> -#define SSAM_CDEV_REQUEST	_IOWR(0xA5, 1, struct ssam_cdev_request)
> +/**
> + * struct ssam_cdev_notifier_desc - Notifier descriptor.
> + * @priority:        Priority value determining the order in which notifier
> + *                   callbacks will be called. A higher value means higher
> + *                   priority, i.e. the associated callback will be executed
> + *                   earlier than other (lower priority) callbacks.
> + * @target_category: The event target category for which this notifier should
> + *                   receive events.
> + *
> + * Specifies the notifier that should be registered or unregistered,
> + * specifically with which priority and for which target category of events.
> + */
> +struct ssam_cdev_notifier_desc {
> +	__s32 priority;
> +	__u8 target_category;
> +} __attribute__((__packed__));
> +
> +/**
> + * struct ssam_cdev_event - SSAM event sent by the EC.
> + * @target_category: Target category of the event source. See &enum ssam_ssh_tc.
> + * @target_id:       Target ID of the event source.
> + * @command_id:      Command ID of the event.
> + * @instance_id:     Instance ID of the event source.
> + * @length:          Length of the event payload in bytes.
> + * @data:            Event payload data.
> + */
> +struct ssam_cdev_event {
> +	__u8 target_category;
> +	__u8 target_id;
> +	__u8 command_id;
> +	__u8 instance_id;
> +	__u16 length;
> +	__u8 data[];
> +} __attribute__((__packed__));
> +
> +#define SSAM_CDEV_REQUEST		_IOWR(0xA5, 1, struct ssam_cdev_request)
> +#define SSAM_CDEV_NOTIF_REGISTER	_IOW(0xA5, 2, struct ssam_cdev_notifier_desc)
> +#define SSAM_CDEV_NOTIF_UNREGISTER	_IOW(0xA5, 3, struct ssam_cdev_notifier_desc)
>  
>  #endif /* _UAPI_LINUX_SURFACE_AGGREGATOR_CDEV_H */
> 

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

[Hans de Goede]

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(-)
> 

Re: [PATCH 4/7] platform/surface: aggregator_cdev: Add support for forwarding events to user-space

[Maximilian Luz]

On 6/4/21 1:32 PM, Hans de Goede wrote:
> Hi,
> 
> I've one review remark inline below.
> 
> On 6/4/21 1:45 AM, Maximilian Luz wrote:

[...]

>> +static int ssam_cdev_device_open(struct inode *inode, struct file *filp)
>> +{
>> +	struct miscdevice *mdev = filp->private_data;
>> +	struct ssam_cdev_client *client;
>> +	struct ssam_cdev *cdev = container_of(mdev, struct ssam_cdev, mdev);
>> +
>> +	/* Initialize client */
>> +	client = vzalloc(sizeof(*client));
>> +	if (!client)
>> +		return -ENOMEM;
>> +
>> +	client->cdev = ssam_cdev_get(cdev);
>> +
>> +	INIT_LIST_HEAD(&client->node);
>> +
>> +	mutex_init(&client->notifier_lock);
>> +
>> +	mutex_init(&client->read_lock);
>> +	mutex_init(&client->write_lock);
>> +	INIT_KFIFO(client->buffer);
>> +	init_waitqueue_head(&client->waitq);
>> +
>> +	filp->private_data = client;
>> +
>> +	/* Attach client. */
>> +	down_write(&cdev->client_lock);
>> +
>> +	if (test_bit(SSAM_CDEV_DEVICE_SHUTDOWN_BIT, &cdev->flags)) {
>> +		up_write(&cdev->client_lock);
>> +		ssam_cdev_put(client->cdev);
> 
> You are missing the mutex_destroy() calls here which you are
> doing in ssam_cdev_device_release().

Thank you for noticing this! This code is based on Surface DTX code
which has the same problem, I'll send in a fix for that shortly.

> Or maybe move the mutex_init calls below this check
> (before the up_write()) since I don't think the client can
> be accessed by any code until the up_write is done?

Yes, that would also be possible, but I'd prefer adding the
mutex_destroy() calls in the failure path. To me that seems a bit easier
to reason about.

Thanks,
Max

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

[Maximilian Luz]

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