Hi, On 12/3/20 10:26 PM, Maximilian Luz wrote: > Add a misc-device providing user-space access to the Surface Aggregator > EC, mainly intended for debugging, testing, and reverse-engineering. > This interface gives user-space applications the ability to send > requests to the EC and receive the corresponding responses. > > The device-file is managed by a pseudo platform-device and corresponding > driver to avoid dependence on the dedicated bus, allowing it to be > loaded in a minimal configuration. > > Signed-off-by: Maximilian Luz <luzmaximilian@xxxxxxxxx> 1 review comment inline: > --- > > Changes in v1 (from RFC): > - add copyright lines > - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later) > - move from debugfs to proper misc-device > - move interface definitions to uapi header > - remove versioning > - allocate platform device dynamically > - prioritize EFAULT error in IOCTL return > - use ENOTTY error for unknown IOCTLs > - fix IOCTL compatibility for 32-bit user-space on 64-bit kernels > - mark request struct as __attribute__((__packed__)) > - address issues when driver is unbound while file still open > > Changes in v2: > - add ioctl-number.rst entry > - improve documentation > - spell check comments and strings, fix typos > - unify comment style > - run checkpatch --strict, fix warnings and style issues > > --- > .../surface_aggregator/clients/cdev.rst | 85 +++++ > .../surface_aggregator/clients/index.rst | 12 +- > .../userspace-api/ioctl/ioctl-number.rst | 2 + > MAINTAINERS | 2 + > drivers/platform/surface/Kconfig | 17 + > drivers/platform/surface/Makefile | 1 + > .../surface/surface_aggregator_cdev.c | 299 ++++++++++++++++++ > include/uapi/linux/surface_aggregator/cdev.h | 58 ++++ > 8 files changed, 475 insertions(+), 1 deletion(-) > create mode 100644 Documentation/driver-api/surface_aggregator/clients/cdev.rst > create mode 100644 drivers/platform/surface/surface_aggregator_cdev.c > create mode 100644 include/uapi/linux/surface_aggregator/cdev.h > > diff --git a/Documentation/driver-api/surface_aggregator/clients/cdev.rst b/Documentation/driver-api/surface_aggregator/clients/cdev.rst > new file mode 100644 > index 000000000000..f30e77def838 > --- /dev/null > +++ b/Documentation/driver-api/surface_aggregator/clients/cdev.rst > @@ -0,0 +1,85 @@ > +.. 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_request_flags| replace:: :c:type:`enum ssam_request_flags <ssam_request_flags>` > + > +============================== > +User-Space EC Interface (cdev) > +============================== > + > +The ``surface_aggregator_cdev`` module provides a misc-device for the SSAM > +controller to allow for a (more or less) direct connection from user-space to > +the SAM EC. It is intended to be used for development and debugging, and > +therefore should not be used or relied upon in any other way. Note that this > +module is not loaded automatically, but instead must be loaded manually. > + > +The provided interface is accessible through the ``/dev/surface/aggregator`` > +device-file. All functionality of this interface is provided via IOCTLs. > +These IOCTLs and their respective input/output parameter structs are defined in > +``include/uapi/linux/surface_aggregator/cdev.h``. > + > + > +Controller IOCTLs > +================= > + > +The following IOCTLs are provided: > + > +.. flat-table:: Controller IOCTLs > + :widths: 1 1 1 1 4 > + :header-rows: 1 > + > + * - Type > + - Number > + - Direction > + - Name > + - Description > + > + * - ``0xA5`` > + - ``1`` > + - ``WR`` > + - ``REQUEST`` > + - Perform synchronous SAM request. > + > + > +``REQUEST`` > +----------- > + > +Defined as ``_IOWR(0xA5, 1, struct ssam_cdev_request)``. > + > +Executes a synchronous SAM request. The request specification is passed in > +as argument of type |ssam_cdev_request|, which is then written to/modified > +by the IOCTL to return status and result of the request. > + > +Request payload data must be allocated separately and is passed in via the > +``payload.data`` and ``payload.length`` members. If a response is required, > +the response buffer must be allocated by the caller and passed in via the > +``response.data`` member. The ``response.length`` member must be set to the > +capacity of this buffer, or if no response is required, zero. Upon > +completion of the request, the call will write the response to the response > +buffer (if its capacity allows it) and overwrite the length field with the > +actual size of the response, in bytes. > + > +Additionally, if the request has a response, this must be indicated via the > +request flags, as is done with in-kernel requests. Request flags can be set > +via the ``flags`` member and the values correspond to the values found in > +|ssam_request_flags|. > + > +Finally, the status of the request itself is returned in the ``status`` > +member (a negative errno value indicating failure). Note that failure > +indication of the IOCTL is separated from failure indication of the request: > +The IOCTL returns a negative status code if anything failed during setup of > +the request (``-EFAULT``) or if the provided argument or any of its fields > +are invalid (``-EINVAL``). In this case, the status value of the request > +argument may be set, providing more detail on what went wrong (e.g. > +``-ENOMEM`` for out-of-memory), but this value may also be zero. The IOCTL > +will return with a zero status code in case the request has been set up, > +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: > + > +.. kernel-doc:: include/uapi/linux/surface_aggregator/cdev.h > + :functions: ssam_cdev_request > diff --git a/Documentation/driver-api/surface_aggregator/clients/index.rst b/Documentation/driver-api/surface_aggregator/clients/index.rst > index 31e026d96102..ab260ec82cfb 100644 > --- a/Documentation/driver-api/surface_aggregator/clients/index.rst > +++ b/Documentation/driver-api/surface_aggregator/clients/index.rst > @@ -7,4 +7,14 @@ Client Driver Documentation > This is the documentation for client drivers themselves. Refer to > :doc:`../client` for documentation on how to write client drivers. > > -.. Place documentation for individual client drivers here. > +.. toctree:: > + :maxdepth: 1 > + > + cdev > + > +.. only:: subproject and html > + > + Indices > + ======= > + > + * :ref:`genindex` > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst > index 55a2d9b2ce33..a98895da62b3 100644 > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst > @@ -323,6 +323,8 @@ Code Seq# Include File Comments > <mailto:tlewis@xxxxxxxxxxxxxx> > 0xA3 90-9F linux/dtlk.h > 0xA4 00-1F uapi/linux/tee.h Generic TEE subsystem > +0xA5 01 linux/surface_aggregator/cdev.h Microsoft Surface Platform System Aggregator > + <mailto:luzmaximilian@xxxxxxxxx> > 0xAA 00-3F linux/uapi/linux/userfaultfd.h > 0xAB 00-1F linux/nbd.h > 0xAC 00-1F linux/raw.h > diff --git a/MAINTAINERS b/MAINTAINERS > index ff5d60875cec..f5a788f445a4 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -11699,7 +11699,9 @@ W: https://github.com/linux-surface/surface-aggregator-module > C: irc://chat.freenode.net/##linux-surface > F: Documentation/driver-api/surface_aggregator/ > F: drivers/platform/surface/aggregator/ > +F: drivers/platform/surface/surface_aggregator_cdev.c > F: include/linux/surface_aggregator/ > +F: include/uapi/linux/surface_aggregator/ > > MICROTEK X6 SCANNER > M: Oliver Neukum <oliver@xxxxxxxxxx> > diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig > index 5af22f5141ff..cecad7a0cb7b 100644 > --- a/drivers/platform/surface/Kconfig > +++ b/drivers/platform/surface/Kconfig > @@ -40,6 +40,23 @@ config SURFACE_3_POWER_OPREGION > This driver provides support for ACPI operation > region of the Surface 3 battery platform driver. > > +config SURFACE_AGGREGATOR_CDEV > + tristate "Surface System Aggregator Module User-Space Interface" > + depends on SURFACE_AGGREGATOR > + help > + Provides a misc-device interface to the Surface System Aggregator > + Module (SSAM) controller. > + > + This option provides a module (called surface_aggregator_cdev), that, > + when loaded, will add a client device (and its respective driver) to > + the SSAM controller. Said client device manages a misc-device > + interface (/dev/surface/aggregator), which can be used by user-space > + tools to directly communicate with the SSAM EC by sending requests and > + receiving the corresponding responses. > + > + The provided interface is intended for debugging and development only, > + and should not be used otherwise. > + > config SURFACE_GPE > tristate "Surface GPE/Lid Support Driver" > depends on ACPI > diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile > index 034134fe0264..161f0ad05795 100644 > --- a/drivers/platform/surface/Makefile > +++ b/drivers/platform/surface/Makefile > @@ -8,5 +8,6 @@ obj-$(CONFIG_SURFACE3_WMI) += surface3-wmi.o > obj-$(CONFIG_SURFACE_3_BUTTON) += surface3_button.o > obj-$(CONFIG_SURFACE_3_POWER_OPREGION) += surface3_power.o > obj-$(CONFIG_SURFACE_AGGREGATOR) += aggregator/ > +obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV) += surface_aggregator_cdev.o > obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o > obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o > diff --git a/drivers/platform/surface/surface_aggregator_cdev.c b/drivers/platform/surface/surface_aggregator_cdev.c > new file mode 100644 > index 000000000000..40c30627f33a > --- /dev/null > +++ b/drivers/platform/surface/surface_aggregator_cdev.c > @@ -0,0 +1,299 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * 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@xxxxxxxxx> > + */ > + > +#include <linux/fs.h> > +#include <linux/kernel.h> > +#include <linux/kref.h> > +#include <linux/miscdevice.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/rwsem.h> > +#include <linux/slab.h> > +#include <linux/uaccess.h> > + > +#include <linux/surface_aggregator/cdev.h> > +#include <linux/surface_aggregator/controller.h> > + > +#define SSAM_CDEV_DEVICE_NAME "surface_aggregator_cdev" > + > +struct ssam_cdev { > + struct kref kref; > + struct rw_semaphore lock; > + struct ssam_controller *ctrl; > + struct miscdevice mdev; > +}; > + > +static void __ssam_cdev_release(struct kref *kref) > +{ > + kfree(container_of(kref, struct ssam_cdev, kref)); > +} > + > +static struct ssam_cdev *ssam_cdev_get(struct ssam_cdev *cdev) > +{ > + if (cdev) > + kref_get(&cdev->kref); > + > + return cdev; > +} > + > +static void ssam_cdev_put(struct ssam_cdev *cdev) > +{ > + if (cdev) > + kref_put(&cdev->kref, __ssam_cdev_release); > +} > + > +static int ssam_cdev_device_open(struct inode *inode, struct file *filp) > +{ > + struct miscdevice *mdev = filp->private_data; > + struct ssam_cdev *cdev = container_of(mdev, struct ssam_cdev, mdev); > + > + filp->private_data = ssam_cdev_get(cdev); > + return stream_open(inode, filp); > +} > + > +static int ssam_cdev_device_release(struct inode *inode, struct file *filp) > +{ > + ssam_cdev_put(filp->private_data); > + return 0; > +} > + > +static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg) > +{ > + struct ssam_cdev_request __user *r; > + struct ssam_cdev_request rqst; > + struct ssam_request spec; > + struct ssam_response rsp; > + const void __user *plddata; > + 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; > + > + plddata = u64_to_user_ptr(rqst.payload.data); > + rspdata = u64_to_user_ptr(rqst.response.data); > + > + /* Setup basic request fields. */ > + spec.target_category = rqst.target_category; > + spec.target_id = rqst.target_id; > + spec.command_id = rqst.command_id; > + spec.instance_id = rqst.instance_id; > + spec.flags = rqst.flags; > + spec.length = rqst.payload.length; > + spec.payload = NULL; > + > + rsp.capacity = rqst.response.length; > + rsp.length = 0; > + rsp.pointer = NULL; > + > + /* Get request payload from user-space. */ > + if (spec.length) { > + if (!plddata) { > + ret = -EINVAL; > + goto out; > + } > + > + spec.payload = kzalloc(spec.length, GFP_KERNEL); > + if (!spec.payload) { > + status = -ENOMEM; > + ret = -EFAULT; > + goto out; > + } > + > + if (copy_from_user((void *)spec.payload, plddata, spec.length)) { > + ret = -EFAULT; > + goto out; > + } > + } > + > + /* Allocate response buffer. */ > + if (rsp.capacity) { > + if (!rspdata) { > + ret = -EINVAL; > + goto out; > + } > + > + rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL); > + if (!rsp.pointer) { > + status = -ENOMEM; > + ret = -EFAULT; This is weird, -EFAULT should only be used if a SEGFAULT would have been raised if the code was running in userspace rather then in kernelspace, IOW if userspace has provided an invalid pointer (or a too small buffer, causing the pointer to become invalid at some point in the buffer). IMHO you should simply do ret = -ENOMEM here. Otherwise this looks good to me. Regards, Hans > + goto out; > + } > + } > + > + /* Perform request. */ > + status = ssam_request_sync(cdev->ctrl, &spec, &rsp); > + if (status) > + goto out; > + > + /* Copy response to user-space. */ > + if (rsp.length && copy_to_user(rspdata, rsp.pointer, rsp.length)) > + ret = -EFAULT; > + > +out: > + /* Always try to set response-length and status. */ > + tmp = put_user(rsp.length, &r->response.length); > + if (tmp) > + ret = tmp; > + > + tmp = put_user(status, &r->status); > + if (tmp) > + ret = tmp; > + > + /* Cleanup. */ > + kfree(spec.payload); > + kfree(rsp.pointer); > + > + return ret; > +} > + > +static long __ssam_cdev_device_ioctl(struct ssam_cdev *cdev, unsigned int cmd, > + unsigned long arg) > +{ > + switch (cmd) { > + case SSAM_CDEV_REQUEST: > + return ssam_cdev_request(cdev, arg); > + > + default: > + return -ENOTTY; > + } > +} > + > +static long ssam_cdev_device_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + struct ssam_cdev *cdev = file->private_data; > + long status; > + > + /* Ensure that controller is valid for as long as we need it. */ > + if (down_read_killable(&cdev->lock)) > + return -ERESTARTSYS; > + > + if (!cdev->ctrl) { > + up_read(&cdev->lock); > + return -ENODEV; > + } > + > + status = __ssam_cdev_device_ioctl(cdev, cmd, arg); > + > + up_read(&cdev->lock); > + return status; > +} > + > +static const struct file_operations ssam_controller_fops = { > + .owner = THIS_MODULE, > + .open = ssam_cdev_device_open, > + .release = ssam_cdev_device_release, > + .unlocked_ioctl = ssam_cdev_device_ioctl, > + .compat_ioctl = ssam_cdev_device_ioctl, > + .llseek = noop_llseek, > +}; > + > +static int ssam_dbg_device_probe(struct platform_device *pdev) > +{ > + struct ssam_controller *ctrl; > + struct ssam_cdev *cdev; > + int status; > + > + ctrl = ssam_client_bind(&pdev->dev); > + if (IS_ERR(ctrl)) > + return PTR_ERR(ctrl) == -ENODEV ? -EPROBE_DEFER : PTR_ERR(ctrl); > + > + cdev = kzalloc(sizeof(*cdev), GFP_KERNEL); > + if (!cdev) > + return -ENOMEM; > + > + kref_init(&cdev->kref); > + init_rwsem(&cdev->lock); > + cdev->ctrl = ctrl; > + > + cdev->mdev.parent = &pdev->dev; > + cdev->mdev.minor = MISC_DYNAMIC_MINOR; > + cdev->mdev.name = "surface_aggregator"; > + cdev->mdev.nodename = "surface/aggregator"; > + cdev->mdev.fops = &ssam_controller_fops; > + > + status = misc_register(&cdev->mdev); > + if (status) { > + kfree(cdev); > + return status; > + } > + > + platform_set_drvdata(pdev, cdev); > + return 0; > +} > + > +static int ssam_dbg_device_remove(struct platform_device *pdev) > +{ > + struct ssam_cdev *cdev = platform_get_drvdata(pdev); > + > + misc_deregister(&cdev->mdev); > + > + /* > + * The controller is only guaranteed to be valid for as long as the > + * driver is bound. Remove controller so that any lingering open files > + * cannot access it any more after we're gone. > + */ > + down_write(&cdev->lock); > + cdev->ctrl = NULL; > + up_write(&cdev->lock); > + > + ssam_cdev_put(cdev); > + return 0; > +} > + > +static struct platform_device *ssam_cdev_device; > + > +static struct platform_driver ssam_cdev_driver = { > + .probe = ssam_dbg_device_probe, > + .remove = ssam_dbg_device_remove, > + .driver = { > + .name = SSAM_CDEV_DEVICE_NAME, > + .probe_type = PROBE_PREFER_ASYNCHRONOUS, > + }, > +}; > + > +static int __init ssam_debug_init(void) > +{ > + int status; > + > + ssam_cdev_device = platform_device_alloc(SSAM_CDEV_DEVICE_NAME, > + PLATFORM_DEVID_NONE); > + if (!ssam_cdev_device) > + return -ENOMEM; > + > + status = platform_device_add(ssam_cdev_device); > + if (status) > + goto err_device; > + > + status = platform_driver_register(&ssam_cdev_driver); > + if (status) > + goto err_driver; > + > + return 0; > + > +err_driver: > + platform_device_del(ssam_cdev_device); > +err_device: > + platform_device_put(ssam_cdev_device); > + return status; > +} > +module_init(ssam_debug_init); > + > +static void __exit ssam_debug_exit(void) > +{ > + platform_driver_unregister(&ssam_cdev_driver); > + platform_device_unregister(ssam_cdev_device); > +} > +module_exit(ssam_debug_exit); > + > +MODULE_AUTHOR("Maximilian Luz <luzmaximilian@xxxxxxxxx>"); > +MODULE_DESCRIPTION("User-space interface for Surface System Aggregator Module"); > +MODULE_LICENSE("GPL"); > diff --git a/include/uapi/linux/surface_aggregator/cdev.h b/include/uapi/linux/surface_aggregator/cdev.h > new file mode 100644 > index 000000000000..1a8bc0249f8e > --- /dev/null > +++ b/include/uapi/linux/surface_aggregator/cdev.h > @@ -0,0 +1,58 @@ > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */ > +/* > + * Surface System Aggregator Module (SSAM) user-space EC interface. > + * > + * Definitions, structs, and IOCTLs for the /dev/surface/aggregator misc > + * device. This device provides direct user-space access to the SSAM EC. > + * Intended for debugging and development. > + * > + * Copyright (C) 2020 Maximilian Luz <luzmaximilian@xxxxxxxxx> > + */ > + > +#ifndef _UAPI_LINUX_SURFACE_AGGREGATOR_CDEV_H > +#define _UAPI_LINUX_SURFACE_AGGREGATOR_CDEV_H > + > +#include <linux/ioctl.h> > +#include <linux/types.h> > + > +/** > + * struct ssam_cdev_request - Controller request IOCTL argument. > + * @target_category: Target category of the SAM request. > + * @target_id: Target ID of the SAM request. > + * @command_id: Command ID of the SAM request. > + * @instance_id: Instance ID of the SAM request. > + * @flags: SAM Request flags. > + * @status: Request status (output). > + * @payload: Request payload (input data). > + * @payload.data: Pointer to request payload data. > + * @payload.length: Length of request payload data (in bytes). > + * @response: Request response (output data). > + * @response.data: Pointer to response buffer. > + * @response.length: On input: Capacity of response buffer (in bytes). > + * On output: Length of request response (number of bytes > + * in the buffer that are actually used). > + */ > +struct ssam_cdev_request { > + __u8 target_category; > + __u8 target_id; > + __u8 command_id; > + __u8 instance_id; > + __u16 flags; > + __s16 status; > + > + struct { > + __u64 data; > + __u16 length; > + __u8 __pad[6]; > + } payload; > + > + struct { > + __u64 data; > + __u16 length; > + __u8 __pad[6]; > + } response; > +} __attribute__((__packed__)); > + > +#define SSAM_CDEV_REQUEST _IOWR(0xA5, 1, struct ssam_cdev_request) > + > +#endif /* _UAPI_LINUX_SURFACE_AGGREGATOR_CDEV_H */ > -- > 2.29.2 >
