| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 27 Oct 2021 18:03:08 -0700
From: Prashant Malani <pmalani@...omium.org>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: Benson Leung <bleung@...gle.com>,
Adam Thomson <Adam.Thomson.Opensource@...semi.com>,
Guenter Roeck <linux@...ck-us.net>,
Badhri Jagan Sridharan <badhri@...gle.com>,
Jack Pham <jackp@...eaurora.org>,
"Gopal, Saranya" <saranya.gopal@...el.com>,
"Regupathy, Rajaram" <rajaram.regupathy@...el.com>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 2/4] usb: typec: Character device for USB Power
Delivery devices
Hi Heikki,
Thanks for sending the RFC.
On Tue, Oct 26, 2021 at 05:33:50PM +0300, Heikki Krogerus wrote:
> Interim.
>
> TODO/ideas:
> - Figure out a proper magic value for the ioctl and check if
> the ioctl range is OK.
> - Register separate PD device for the cdev, and register it
> only if the device (port, plug or partner) actually
> supports USB PD (or come up with some other solution?).
> - Introduce something like
>
> struct pd_request {
> struct pd_message request;
> struct pd_message __user *response;
> };
>
> and use it instead of only single struct pd_messages everywhere.
>
> - Add compat support.
> - What do we do with Alerts and Attentions?
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> ---
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> drivers/usb/typec/Makefile | 2 +-
> drivers/usb/typec/class.c | 42 ++++
> drivers/usb/typec/class.h | 4 +
> drivers/usb/typec/pd-dev.c | 210 ++++++++++++++++++
> drivers/usb/typec/pd-dev.h | 15 ++
> include/linux/usb/pd_dev.h | 22 ++
> include/linux/usb/typec.h | 8 +
> include/uapi/linux/usb/pd_dev.h | 55 +++++
> 9 files changed, 358 insertions(+), 1 deletion(-)
> create mode 100644 drivers/usb/typec/pd-dev.c
> create mode 100644 drivers/usb/typec/pd-dev.h
> create mode 100644 include/linux/usb/pd_dev.h
> create mode 100644 include/uapi/linux/usb/pd_dev.h
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 0ba463be6c588..fd443fd21f62a 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -175,6 +175,7 @@ Code Seq# Include File Comments
> 'P' 60-6F sound/sscape_ioctl.h conflict!
> 'P' 00-0F drivers/usb/class/usblp.c conflict!
> 'P' 01-09 drivers/misc/pci_endpoint_test.c conflict!
> +'P' 70-7F uapi/linux/usb/pd_dev.h <mailto:linux-usb@...r.kernel.org>
> 'Q' all linux/soundcard.h
> 'R' 00-1F linux/random.h conflict!
> 'R' 01 linux/rfkill.h conflict!
> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> index a0adb8947a301..be44528168013 100644
> --- a/drivers/usb/typec/Makefile
> +++ b/drivers/usb/typec/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_TYPEC) += typec.o
> -typec-y := class.o mux.o bus.o port-mapper.o
> +typec-y := class.o mux.o bus.o port-mapper.o pd-dev.o
> obj-$(CONFIG_TYPEC) += altmodes/
> obj-$(CONFIG_TYPEC_TCPM) += tcpm/
> obj-$(CONFIG_TYPEC_UCSI) += ucsi/
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index aeef453aa6585..19fcc5da175d7 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -15,6 +15,7 @@
>
> #include "bus.h"
> #include "class.h"
> +#include "pd-dev.h"
>
> static DEFINE_IDA(typec_index_ida);
>
> @@ -665,6 +666,11 @@ static const struct attribute_group *typec_partner_groups[] = {
> NULL
> };
>
> +char *typec_partner_devnode(struct device *dev, umode_t *mode, kuid_t *uid, kgid_t *gid)
> +{
> + return kasprintf(GFP_KERNEL, "pd%u/partner", to_typec_port(dev->parent)->id);
> +}
> +
> static void typec_partner_release(struct device *dev)
> {
> struct typec_partner *partner = to_typec_partner(dev);
> @@ -676,6 +682,7 @@ static void typec_partner_release(struct device *dev)
> const struct device_type typec_partner_dev_type = {
> .name = "typec_partner",
> .groups = typec_partner_groups,
> + .devnode = typec_partner_devnode,
> .release = typec_partner_release,
> };
>
> @@ -807,6 +814,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
>
> ida_init(&partner->mode_ids);
> partner->usb_pd = desc->usb_pd;
> + partner->pd_dev = desc->pd_dev;
> partner->accessory = desc->accessory;
> partner->num_altmodes = -1;
> partner->pd_revision = desc->pd_revision;
> @@ -826,6 +834,9 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
> partner->dev.type = &typec_partner_dev_type;
> dev_set_name(&partner->dev, "%s-partner", dev_name(&port->dev));
>
> + if (partner->pd_dev)
> + partner->dev.devt = MKDEV(PD_DEV_MAJOR, port->id * 4 + 3);
> +
> ret = device_register(&partner->dev);
> if (ret) {
> dev_err(&port->dev, "failed to register partner (%d)\n", ret);
> @@ -853,6 +864,13 @@ EXPORT_SYMBOL_GPL(typec_unregister_partner);
> /* ------------------------------------------------------------------------- */
> /* Type-C Cable Plugs */
>
> +char *typec_plug_devnode(struct device *dev, umode_t *mode, kuid_t *uid, kgid_t *gid)
> +{
> + return kasprintf(GFP_KERNEL, "pd%u/plug%d",
> + to_typec_port(dev->parent->parent)->id,
> + to_typec_plug(dev)->index);
> +}
> +
> static void typec_plug_release(struct device *dev)
> {
> struct typec_plug *plug = to_typec_plug(dev);
> @@ -891,6 +909,7 @@ static const struct attribute_group *typec_plug_groups[] = {
> const struct device_type typec_plug_dev_type = {
> .name = "typec_plug",
> .groups = typec_plug_groups,
> + .devnode = typec_plug_devnode,
> .release = typec_plug_release,
> };
>
> @@ -973,11 +992,16 @@ struct typec_plug *typec_register_plug(struct typec_cable *cable,
> ida_init(&plug->mode_ids);
> plug->num_altmodes = -1;
> plug->index = desc->index;
> + plug->pd_dev = desc->pd_dev;
> plug->dev.class = &typec_class;
> plug->dev.parent = &cable->dev;
> plug->dev.type = &typec_plug_dev_type;
> dev_set_name(&plug->dev, "%s-%s", dev_name(cable->dev.parent), name);
>
> + if (plug->pd_dev)
> + plug->dev.devt = MKDEV(PD_DEV_MAJOR,
> + to_typec_port(cable->dev.parent)->id * 4 + 1 + plug->index);
> +
> ret = device_register(&plug->dev);
> if (ret) {
> dev_err(&cable->dev, "failed to register plug (%d)\n", ret);
> @@ -1595,6 +1619,11 @@ static int typec_uevent(struct device *dev, struct kobj_uevent_env *env)
> return ret;
> }
>
> +char *typec_devnode(struct device *dev, umode_t *mode, kuid_t *uid, kgid_t *gid)
> +{
> + return kasprintf(GFP_KERNEL, "pd%u/port", to_typec_port(dev)->id);
> +}
> +
> static void typec_release(struct device *dev)
> {
> struct typec_port *port = to_typec_port(dev);
> @@ -1611,6 +1640,7 @@ const struct device_type typec_port_dev_type = {
> .name = "typec_port",
> .groups = typec_groups,
> .uevent = typec_uevent,
> + .devnode = typec_devnode,
> .release = typec_release,
> };
>
> @@ -2044,6 +2074,7 @@ struct typec_port *typec_register_port(struct device *parent,
>
> port->id = id;
> port->ops = cap->ops;
> + port->pd_dev = cap->pd_dev;
> port->port_type = cap->type;
> port->prefer_role = cap->prefer_role;
>
> @@ -2055,6 +2086,9 @@ struct typec_port *typec_register_port(struct device *parent,
> dev_set_name(&port->dev, "port%d", id);
> dev_set_drvdata(&port->dev, cap->driver_data);
>
> + if (port->pd_dev)
> + port->dev.devt = MKDEV(PD_DEV_MAJOR, id * 4);
> +
> port->cap = kmemdup(cap, sizeof(*cap), GFP_KERNEL);
> if (!port->cap) {
> put_device(&port->dev);
> @@ -2121,8 +2155,15 @@ static int __init typec_init(void)
> if (ret)
> goto err_unregister_mux_class;
>
> + ret = usbpd_dev_init();
> + if (ret)
> + goto err_unregister_class;
> +
> return 0;
>
> +err_unregister_class:
> + class_unregister(&typec_class);
> +
> err_unregister_mux_class:
> class_unregister(&typec_mux_class);
>
> @@ -2135,6 +2176,7 @@ subsys_initcall(typec_init);
>
> static void __exit typec_exit(void)
> {
> + usbpd_dev_exit();
> class_unregister(&typec_class);
> ida_destroy(&typec_index_ida);
> bus_unregister(&typec_bus);
> diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
> index aef03eb7e1523..87c072f2ad753 100644
> --- a/drivers/usb/typec/class.h
> +++ b/drivers/usb/typec/class.h
> @@ -14,6 +14,7 @@ struct typec_plug {
> enum typec_plug_index index;
> struct ida mode_ids;
> int num_altmodes;
> + const struct pd_dev *pd_dev;
> };
>
> struct typec_cable {
> @@ -33,6 +34,7 @@ struct typec_partner {
> int num_altmodes;
> u16 pd_revision; /* 0300H = "3.0" */
> enum usb_pd_svdm_ver svdm_version;
> + const struct pd_dev *pd_dev;
> };
>
> struct typec_port {
> @@ -59,6 +61,8 @@ struct typec_port {
> struct mutex port_list_lock; /* Port list lock */
>
> void *pld;
> +
> + const struct pd_dev *pd_dev;
> };
>
> #define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
> diff --git a/drivers/usb/typec/pd-dev.c b/drivers/usb/typec/pd-dev.c
> new file mode 100644
> index 0000000000000..436853e046ce4
> --- /dev/null
> +++ b/drivers/usb/typec/pd-dev.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * USB Power Delivery /dev entries
> + *
> + * Copyright (C) 2021 Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> + */
> +
> +#include <linux/cdev.h>
> +#include <linux/fs.h>
> +#include <linux/slab.h>
> +#include <linux/usb/pd_dev.h>
> +
> +#include "class.h"
> +
> +#define PD_DEV_MAX (MINORMASK + 1)
> +
> +dev_t usbpd_devt;
> +static struct cdev usb_pd_cdev;
> +
> +struct pddev {
> + struct device *dev;
> + struct typec_port *port;
> + const struct pd_dev *pd_dev;
> +};
> +
> +static ssize_t usbpd_read(struct file *file, char __user *buf, size_t count, loff_t *offset)
> +{
> + /* FIXME TODO XXX */
> +
> + /* Alert and Attention handling here (with poll) ? */
> +
> + return 0;
> +}
> +
> +static long usbpd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + struct pddev *pd = file->private_data;
> + void __user *p = (void __user *)arg;
> + unsigned int pwr_role;
> + struct pd_message msg;
> + u32 configuration;
> + int ret = 0;
> +
> + switch (cmd) {
> + case USBPDDEV_INFO:
> + if (copy_to_user(p, pd->pd_dev->info, sizeof(*pd->pd_dev->info)))
> + return -EFAULT;
> + break;
> + case USBPDDEV_CONFIGURE:
> + if (!pd->pd_dev->ops->configure)
> + return -ENOTTY;
> +
> + if (copy_from_user(&configuration, p, sizeof(configuration)))
> + return -EFAULT;
> +
> + ret = pd->pd_dev->ops->configure(pd->pd_dev, configuration);
> + if (ret)
> + return ret;
> + break;
> + case USBPDDEV_PWR_ROLE:
> + if (is_typec_plug(pd->dev))
> + return -ENOTTY;
> +
> + if (is_typec_partner(pd->dev)) {
> + if (pd->port->pwr_role == TYPEC_SINK)
> + pwr_role = TYPEC_SOURCE;
> + else
> + pwr_role = TYPEC_SINK;
> + } else {
> + pwr_role = pd->port->pwr_role;
> + }
> +
> + if (copy_to_user(p, &pwr_role, sizeof(unsigned int)))
> + return -EFAULT;
> + break;
> + case USBPDDEV_GET_MESSAGE:
> + if (!pd->pd_dev->ops->get_message)
> + return -ENOTTY;
> +
> + if (copy_from_user(&msg, p, sizeof(msg)))
> + return -EFAULT;
> +
> + ret = pd->pd_dev->ops->get_message(pd->pd_dev, &msg);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user(p, &msg, sizeof(msg)))
> + return -EFAULT;
> + break;
> + case USBPDDEV_SET_MESSAGE:
> + if (!pd->pd_dev->ops->set_message)
> + return -ENOTTY;
> +
> + ret = pd->pd_dev->ops->set_message(pd->pd_dev, &msg);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user(p, &msg, sizeof(msg)))
> + return -EFAULT;
> + break;
> + case USBPDDEV_SUBMIT_MESSAGE:
> + if (!pd->pd_dev->ops->submit)
> + return -ENOTTY;
> +
> + if (copy_from_user(&msg, p, sizeof(msg)))
> + return -EFAULT;
> +
> + ret = pd->pd_dev->ops->submit(pd->pd_dev, &msg);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user(p, &msg, sizeof(msg)))
> + return -EFAULT;
> + break;
Why is USBPDDEV_SUBMIT_MESSAGE different from USBPDDEV_SET_MESSAGE?
Shouldn't "setting" a PDO or property automatically "submit" it (using TCPM
or whatever interface is actually performing the PD messaging) if
appropriate (e.g Source Caps?). Is there a situation where one would
want to "set" a property but not "send" it?
It seems to me that the two can be combined into 1 rather than having
a separate command just for ports.
Best regards,
-Prashant
Powered by blists - more mailing lists