Hi,
This is the proposal for USB PD character devices that we could use to
communicate USB PD messages directly with the USB PD capable partners,
ports and cable plugs from user space. Originally I proposed this idea
here as a better way to get the PDOs from the partners (and ports and
plugs): https://lkml.org/lkml/2021/10/8/331
Initially you could read the PDOs with this, but it of course is not
only limited to PDOs - any messages should potentetially be suported.
This is in any case really just a draft, and most likely does not work
if you try it, but it should give an idea about what I'm proposing
(hopefully).
In this proposal I'm placing each device that you can have
behind a single USB PD capable Type-C port (so port, partner
and plugs) under its own folder, so you would end up with
something like this if also both plugs are supported:
/dev/pd0/port
/dev/pd0/plug0
/dev/pd0/plug1
/dev/pd0/partner
I'm also including an ugly test tool that you can try out to dump the
PDOs depending on the role of the device - tools/usb/pd-test.c. This
is what I got from a charger (and the port at the same time):
% pd-test /dev/pd0/port
Sink Capabilities:
PDO1: 0x3601912c
PDO2: 0x000640e1
PDO3: 0x9901912c
% pd-test /dev/pd0/partner
Source Capabilities:
PDO1: 0x0801912c
PDO2: 0x0003c12c
But please note that this whole series is really just meant to be PoC!
Don't assume it will work!
The core code that adds the character devices is here:
drivers/usb/typec/pd-dev.c.
It's actually really simple, and I don't think it would ever need to
be much more complicated than that.
Example driver support I added to UCSI:
drivers/usb/typec/ucsi/pd-dev.c
thanks,
Heikki Krogerus (4):
usb: pd: uapi header split
usb: typec: Character device for USB Power Delivery devices
usb: typec: ucsi: Add support for PD cdev
tools: usb: Hideous test tool for USB PD char device
.../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 +
drivers/usb/typec/ucsi/Makefile | 2 +-
drivers/usb/typec/ucsi/pd-dev.c | 125 +++++
drivers/usb/typec/ucsi/ucsi.c | 37 +-
drivers/usb/typec/ucsi/ucsi.h | 7 +
include/linux/usb/pd.h | 487 +----------------
include/linux/usb/pd_dev.h | 22 +
include/linux/usb/typec.h | 8 +
include/uapi/linux/usb/pd.h | 496 ++++++++++++++++++
include/uapi/linux/usb/pd_dev.h | 55 ++
tools/usb/Build | 1 +
tools/usb/Makefile | 8 +-
tools/usb/pd-test.c | 123 +++++
18 files changed, 1147 insertions(+), 498 deletions(-)
create mode 100644 drivers/usb/typec/pd-dev.c
create mode 100644 drivers/usb/typec/pd-dev.h
create mode 100644 drivers/usb/typec/ucsi/pd-dev.c
create mode 100644 include/linux/usb/pd_dev.h
create mode 100644 include/uapi/linux/usb/pd.h
create mode 100644 include/uapi/linux/usb/pd_dev.h
create mode 100644 tools/usb/pd-test.c
--
2.33.0
Interim.
The Makefile needs to be tuned so we can include to correct
files.
Signed-off-by: Heikki Krogerus <[email protected]>
---
tools/usb/Build | 1 +
tools/usb/Makefile | 8 ++-
tools/usb/pd-test.c | 123 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 131 insertions(+), 1 deletion(-)
create mode 100644 tools/usb/pd-test.c
diff --git a/tools/usb/Build b/tools/usb/Build
index 2ad6f97458168..7116198533a75 100644
--- a/tools/usb/Build
+++ b/tools/usb/Build
@@ -1,2 +1,3 @@
testusb-y += testusb.o
ffs-test-y += ffs-test.o
+pd-test-y += pd-test.o
diff --git a/tools/usb/Makefile b/tools/usb/Makefile
index 1b128e551b2e4..e3e41a3397f23 100644
--- a/tools/usb/Makefile
+++ b/tools/usb/Makefile
@@ -16,7 +16,7 @@ MAKEFLAGS += -r
override CFLAGS += -O2 -Wall -Wextra -g -D_GNU_SOURCE -I$(OUTPUT)include -I$(srctree)/tools/include
override LDFLAGS += -lpthread
-ALL_TARGETS := testusb ffs-test
+ALL_TARGETS := testusb ffs-test pd-test
ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
all: $(ALL_PROGRAMS)
@@ -36,6 +36,12 @@ $(FFS_TEST_IN): FORCE
$(OUTPUT)ffs-test: $(FFS_TEST_IN)
$(QUIET_LINK)$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
+PD_TEST_IN := $(OUTPUT)pd-test-in.o
+$(PD_TEST_IN): FORCE
+ $(Q)$(MAKE) $(build)=pd-test
+$(OUTPUT)pd-test: $(PD_TEST_IN)
+ $(QUIET_LINK)$(CC) $(CFLAGS) $< -o $@
+
clean:
rm -f $(ALL_PROGRAMS)
find $(if $(OUTPUT),$(OUTPUT),.) -name '*.o' -delete -o -name '\.*.d' -delete -o -name '\.*.o.cmd' -delete
diff --git a/tools/usb/pd-test.c b/tools/usb/pd-test.c
new file mode 100644
index 0000000000000..bb38dd4134581
--- /dev/null
+++ b/tools/usb/pd-test.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * USB Power Delivery device tester.
+ *
+ * Copyright (C) 2021 Intel Corporation
+ * Author: Heikki Krogerus <[email protected]>
+ */
+
+#include <stdio.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <linux/types.h>
+
+struct pd_message {
+ __le16 header;
+ __le32 payload[7];
+} __attribute__((packed));
+
+struct pd_info {
+ __u8 specification_revision;
+ __u32 ctrl_msgs_supported;
+ __u32 data_msgs_supported;
+ __u32 ext_msgs_supported;
+} __attribute__((packed));
+
+#define USBPDDEV_INFO _IOR('P', 0x70, struct pd_info)
+#define USBPDDEV_CONFIGURE _IOW('P', 0x71, __u32)
+#define USBPDDEV_PWR_ROLE _IOR('P', 0x72, int)
+#define USBPDDEV_GET_MESSAGE _IOWR('P', 0x73, struct pd_message)
+#define USBPDDEV_SET_MESSAGE _IOW('P', 0x74, struct pd_message)
+#define USBPDDEV_SUBMIT_MESSAGE _IOWR('P', 0x75, struct pd_message)
+
+enum pd_data_msg_type {
+ /* 0 Reserved */
+ PD_DATA_SOURCE_CAP = 1,
+ PD_DATA_REQUEST = 2,
+ PD_DATA_BIST = 3,
+ PD_DATA_SINK_CAP = 4,
+ PD_DATA_BATT_STATUS = 5,
+ PD_DATA_ALERT = 6,
+ PD_DATA_GET_COUNTRY_INFO = 7,
+ PD_DATA_ENTER_USB = 8,
+ /* 9-14 Reserved */
+ PD_DATA_VENDOR_DEF = 15,
+ /* 16-31 Reserved */
+};
+
+int dump_source_pdos(int fd)
+{
+ struct pd_message msg = {};
+ int ret;
+ int i;
+
+ msg.header = PD_DATA_SOURCE_CAP;
+ ret = ioctl(fd, USBPDDEV_GET_MESSAGE, &msg);
+ if (ret < 0) {
+ printf("No cached Source Capabilities %d\n", ret);
+ return ret;
+ }
+
+ printf("Source Capabilities:\n");
+
+ for (i = 0; i < (msg.header >> 12 & 7); i++)
+ printf(" PDO%d: 0x%08x\n", i + 1, msg.payload[i]);
+
+ return 0;
+}
+
+int dump_sink_pdos(int fd)
+{
+ struct pd_message msg = {};
+ int ret;
+ int i;
+
+ msg.header = PD_DATA_SINK_CAP;
+ ret = ioctl(fd, USBPDDEV_GET_MESSAGE, &msg);
+ if (ret < 0) {
+ printf("No cached Sink Capabilities %d\n", ret);
+ return ret;
+ }
+
+ printf("Sink Capabilities:\n");
+
+ for (i = 0; i < (msg.header >> 12 & 7); i++)
+ printf(" PDO%d: 0x%08x\n", i + 1, msg.payload[i]);
+
+ return 0;
+}
+
+int main(int argc, char **argv)
+{
+ unsigned int role;
+ int ret;
+ int fd;
+
+ if (argc != 2) {
+ fprintf(stderr, "Usage: %s [DEV]\n"
+ " %% %s /dev/pd0/port\n\n",
+ argv[0], argv[0]);
+ return -1;
+ }
+
+ fd = open(argv[1], O_RDWR);
+ if (fd < 0)
+ return fd;
+
+ ret = ioctl(fd, USBPDDEV_PWR_ROLE, &role);
+ if (ret < 0) {
+ printf("USBPDDEV_PWR_ROLE failed %d\n", ret);
+ goto err;
+ }
+
+ if (role)
+ ret = dump_source_pdos(fd);
+ else
+ ret = dump_sink_pdos(fd);
+err:
+ close(fd);
+
+ return ret;
+}
--
2.33.0
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 <[email protected]>
---
.../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:[email protected]>
'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 <[email protected]>
+ */
+
+#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;
+ default:
+ return -ENOTTY;
+ }
+
+ return 0;
+}
+
+static int usbpd_open(struct inode *inode, struct file *file)
+{
+ struct device *dev;
+ struct pddev *pd;
+
+ dev = class_find_device_by_devt(&typec_class, inode->i_rdev);
+ if (!dev)
+ return -ENODEV;
+
+ pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+ if (!pd) {
+ put_device(dev);
+ return -ENOMEM;
+ }
+
+ if (is_typec_partner(dev)) {
+ if (!to_typec_partner(dev)->usb_pd) {
+ put_device(dev);
+ kfree(pd);
+ return -ENODEV;
+ }
+ pd->port = to_typec_port(dev->parent);
+ pd->pd_dev = to_typec_partner(dev)->pd_dev;
+ } else if (is_typec_plug(dev)) {
+ pd->port = to_typec_port(dev->parent->parent);
+ pd->pd_dev = to_typec_plug(dev)->pd_dev;
+ } else {
+ pd->port = to_typec_port(dev);
+ pd->pd_dev = to_typec_port(dev)->pd_dev;
+ }
+
+ pd->dev = dev;
+ file->private_data = pd;
+
+ return 0;
+}
+
+static int usbpd_release(struct inode *inode, struct file *file)
+{
+ struct pddev *pd = file->private_data;
+
+ put_device(pd->dev);
+ kfree(pd);
+
+ return 0;
+}
+
+const struct file_operations usbpd_file_operations = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .read = usbpd_read,
+ .unlocked_ioctl = usbpd_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
+ .open = usbpd_open,
+ .release = usbpd_release,
+};
+
+int __init usbpd_dev_init(void)
+{
+ int ret;
+
+ ret = alloc_chrdev_region(&usbpd_devt, 0, PD_DEV_MAX, "usb_pd");
+ if (ret)
+ return ret;
+
+ /*
+ * FIXME!
+ *
+ * Now the cdev is always created, even when the device does not support
+ * USB PD.
+ */
+
+ cdev_init(&usb_pd_cdev, &usbpd_file_operations);
+
+ ret = cdev_add(&usb_pd_cdev, usbpd_devt, PD_DEV_MAX);
+ if (ret) {
+ unregister_chrdev_region(usbpd_devt, PD_DEV_MAX);
+ return ret;
+ }
+
+ return 0;
+}
+
+void __exit usbpd_dev_exit(void)
+{
+ cdev_del(&usb_pd_cdev);
+ unregister_chrdev_region(usbpd_devt, PD_DEV_MAX);
+}
diff --git a/drivers/usb/typec/pd-dev.h b/drivers/usb/typec/pd-dev.h
new file mode 100644
index 0000000000000..2d817167c4042
--- /dev/null
+++ b/drivers/usb/typec/pd-dev.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __USB_TYPEC_PDDEV__
+#define __USB_TYPEC_PDDEV__
+
+#include <linux/kdev_t.h>
+
+#define PD_DEV_MAJOR MAJOR(usbpd_devt)
+
+extern dev_t usbpd_devt;
+
+int usbpd_dev_init(void);
+void usbpd_dev_exit(void);
+
+#endif /* __USB_TYPEC_PDDEV__ */
diff --git a/include/linux/usb/pd_dev.h b/include/linux/usb/pd_dev.h
new file mode 100644
index 0000000000000..d451928f94e78
--- /dev/null
+++ b/include/linux/usb/pd_dev.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __LINUX_USB_PDDEV_H
+#define __LINUX_USB_PDDEV_H
+
+#include <uapi/linux/usb/pd_dev.h>
+
+struct pd_dev;
+
+struct pd_ops {
+ int (*configure)(const struct pd_dev *dev, u32 flags);
+ int (*get_message)(const struct pd_dev *dev, struct pd_message *msg);
+ int (*set_message)(const struct pd_dev *dev, struct pd_message *msg);
+ int (*submit)(const struct pd_dev *dev, struct pd_message *msg);
+};
+
+struct pd_dev {
+ const struct pd_info *info;
+ const struct pd_ops *ops;
+};
+
+#endif /* __LINUX_USB_PDDEV_H */
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index e2e44bb1dad85..6df7b096f769c 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -20,6 +20,7 @@ struct typec_port;
struct typec_altmode_ops;
struct fwnode_handle;
+struct pd_dev;
struct device;
enum typec_port_type {
@@ -159,11 +160,13 @@ enum typec_plug_index {
* struct typec_plug_desc - USB Type-C Cable Plug Descriptor
* @index: SOP Prime for the plug connected to DFP and SOP Double Prime for the
* plug connected to UFP
+ * @pd_dev: USB Power Delivery Character Device
*
* Represents USB Type-C Cable Plug.
*/
struct typec_plug_desc {
enum typec_plug_index index;
+ const struct pd_dev *pd_dev;
};
/*
@@ -189,6 +192,7 @@ struct typec_cable_desc {
* @accessory: Audio, Debug or none.
* @identity: Discover Identity command data
* @pd_revision: USB Power Delivery Specification Revision if supported
+ * @pd_dev: USB Power Delivery Character Device
*
* Details about a partner that is attached to USB Type-C port. If @identity
* member exists when partner is registered, a directory named "identity" is
@@ -204,6 +208,7 @@ struct typec_partner_desc {
enum typec_accessory accessory;
struct usb_pd_identity *identity;
u16 pd_revision; /* 0300H = "3.0" */
+ const struct pd_dev *pd_dev;
};
/**
@@ -241,6 +246,7 @@ enum usb_pd_svdm_ver {
* @fwnode: Optional fwnode of the port
* @driver_data: Private pointer for driver specific info
* @ops: Port operations vector
+ * @pd_dev: USB Power Delivery Character Device
*
* Static capabilities of a single USB Type-C port.
*/
@@ -258,6 +264,8 @@ struct typec_capability {
void *driver_data;
const struct typec_operations *ops;
+
+ const struct pd_dev *pd_dev;
};
/* Specific to try_role(). Indicates the user want's to clear the preference. */
diff --git a/include/uapi/linux/usb/pd_dev.h b/include/uapi/linux/usb/pd_dev.h
new file mode 100644
index 0000000000000..44027bc6b6339
--- /dev/null
+++ b/include/uapi/linux/usb/pd_dev.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _UAPI__LINUX_USB_PDDEV_H
+#define _UAPI__LINUX_USB_PDDEV_H
+
+#include <linux/types.h>
+#include <linux/usb/pd.h>
+
+/*
+ * struct pd_info - USB Power Delivery Device Information
+ * @specification_revision: USB Power Delivery Specification Revision
+ * @supported_ctrl_msgs: Supported Control Messages
+ * @supported_data_msgs: Supported Data Messages
+ * @supported_ext_msgs: Supported Extended Messages
+ *
+ * @specification_revision is in the same format as the Specification Revision
+ * Field in the Message Header. @supported_ctrl_msgs, @supported_data_msgs and
+ * @supported_ext_msgs list the messages, a bit for each, that can be used with
+ * USBPDDEV_SUBMIT_MESSAGE ioctl.
+ */
+struct pd_info {
+ __u8 specification_revision; /* XXX I don't know if this is useful? */
+ __u32 ctrl_msgs_supported;
+ __u32 data_msgs_supported;
+ __u32 ext_msgs_supported;
+} __attribute__ ((packed));
+
+/* Example configuration flags for ports. */
+#define USBPDDEV_CFPORT_ENTER_MODES BIT(0) /* Automatic alt mode entry. */
+
+/*
+ * For basic communication use USBPDDEV_SUBMIT_MESSAGE ioctl. GoodCRC is not
+ * supported. Response will also never be GoodCRC.
+ *
+ * To check cached objects (if they are cached) use USBPDDEV_GET_MESSAGE ioctl.
+ * Useful most likely with RDO and EUDO, but also with Identity etc.
+ * USBPDDEV_SET_MESSAGE is primarily meant to be used with ports. If supported,
+ * it can be used to assign the values for objects like EUDO that the port should
+ * use in future communication.
+ *
+ * The idea with USBPDDEV_CONFIGURE is that you could modify the behaviour of
+ * the underlying TCPM (or what ever interface you have) with some things. So
+ * for example, you could disable automatic alternate mode entry with it with
+ * that USBPDDEV_CFPORT_ENTER_MODES - It's just an example! - so basically, you
+ * could take over some things from TCPM with it.
+ */
+
+#define USBPDDEV_INFO _IOR('P', 0x70, struct pd_info)
+#define USBPDDEV_CONFIGURE _IOW('P', 0x71, __u32)
+#define USBPDDEV_PWR_ROLE _IOR('P', 0x72, int) /* The *current* role! */
+#define USBPDDEV_GET_MESSAGE _IOWR('P', 0x73, struct pd_message)
+#define USBPDDEV_SET_MESSAGE _IOW('P', 0x74, struct pd_message)
+#define USBPDDEV_SUBMIT_MESSAGE _IOWR('P', 0x75, struct pd_message)
+
+#endif /* _UAPI__LINUX_USB_PDDEV_H */
--
2.33.0
On Tue, Oct 26, 2021 at 05:33:48PM +0300, Heikki Krogerus wrote:
> Hi,
>
> This is the proposal for USB PD character devices that we could use to
> communicate USB PD messages directly with the USB PD capable partners,
> ports and cable plugs from user space. Originally I proposed this idea
> here as a better way to get the PDOs from the partners (and ports and
> plugs): https://lkml.org/lkml/2021/10/8/331
You should put the info there (and please use lore.kernel.org in the
future, not lkml.org as we have no control over that site), into this
0/X message as I have no idea _why_ you need a char device and why the
sysfs interface will not work.
So, why not sysfs? :)
thanks,
greg k-h
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.
Ick, no, new ioctls should never need compat support if you create them
properly. That is only for "old" ones.
Also, why not use the miscdev api instead? That should remove some code
of yours and make things simpler, if you really want to stick with a
char device node...
thanks,
greg k-h
Hi Greg,
On Tue, Oct 26, 2021 at 05:06:28PM +0200, Greg KH wrote:
> So, why not sysfs? :)
This is about allowing the user space to take over the USB Power
Delivery communication and policy decisions in some cases. The user
space needs to be able to send and receive raw USB Power Delivery
messages one way or the other. I don't care about what's the interface
that we use.
Here we are talking about the PDOs, so basically the power contract.
Even if we figured out a way how to expose all the information from
the Capability, Status, Alert and what ever messages you need to the
user space via sysfs, and then allow the user to separately send the
Request Message, we would have only covered the power contract. That
does not cover everything, but it would also be unnecessarily
complicated to handle with separate sysfs files IMO.
Even with the power contract it would make more sense to me to just
allow the user space to simply read and write the raw messages, but
when we go the other things like Vendor Specific Messages, I don't
think there is any other way.
So we really do need to be able to tap into the USB Power Delivery
protocol layer directly from user space. I don't care about how we do
that - character device is just a suggestion, although, it does still
feel correct to me. Is there some other way we could do this?
thanks,
--
heikki
On Wed, Oct 27, 2021 at 02:02:42PM +0300, Heikki Krogerus wrote:
> Hi Greg,
>
> On Tue, Oct 26, 2021 at 05:06:28PM +0200, Greg KH wrote:
> > So, why not sysfs? :)
>
> This is about allowing the user space to take over the USB Power
> Delivery communication and policy decisions in some cases. The user
> space needs to be able to send and receive raw USB Power Delivery
> messages one way or the other. I don't care about what's the interface
> that we use.
>
> Here we are talking about the PDOs, so basically the power contract.
> Even if we figured out a way how to expose all the information from
> the Capability, Status, Alert and what ever messages you need to the
> user space via sysfs, and then allow the user to separately send the
> Request Message, we would have only covered the power contract. That
> does not cover everything, but it would also be unnecessarily
> complicated to handle with separate sysfs files IMO.
>
> Even with the power contract it would make more sense to me to just
> allow the user space to simply read and write the raw messages, but
> when we go the other things like Vendor Specific Messages, I don't
> think there is any other way.
>
> So we really do need to be able to tap into the USB Power Delivery
> protocol layer directly from user space. I don't care about how we do
> that - character device is just a suggestion, although, it does still
> feel correct to me. Is there some other way we could do this?
Ok, a char device sounds fine, but _what_ userspace code is going to be
using this interface? We need to have a working version of that as well
before we could take this new interface, otherwise it wouldn't make much
sense.
And why does userspace have to do this, what is wrong with the kernel
doing it as it does today? I.e. what is broken that adding a new api to
the kernel is going to fix?
That needs to be documented really really well.
thanks,
greg k-h
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 <[email protected]>
> ---
> .../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:[email protected]>
> '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 <[email protected]>
> + */
> +
> +#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
On Wed, Oct 27, 2021 at 02:53:57PM +0200, Greg KH wrote:
> On Wed, Oct 27, 2021 at 02:02:42PM +0300, Heikki Krogerus wrote:
> > Hi Greg,
> >
> > On Tue, Oct 26, 2021 at 05:06:28PM +0200, Greg KH wrote:
> > > So, why not sysfs? :)
> >
> > This is about allowing the user space to take over the USB Power
> > Delivery communication and policy decisions in some cases. The user
> > space needs to be able to send and receive raw USB Power Delivery
> > messages one way or the other. I don't care about what's the interface
> > that we use.
> >
> > Here we are talking about the PDOs, so basically the power contract.
> > Even if we figured out a way how to expose all the information from
> > the Capability, Status, Alert and what ever messages you need to the
> > user space via sysfs, and then allow the user to separately send the
> > Request Message, we would have only covered the power contract. That
> > does not cover everything, but it would also be unnecessarily
> > complicated to handle with separate sysfs files IMO.
> >
> > Even with the power contract it would make more sense to me to just
> > allow the user space to simply read and write the raw messages, but
> > when we go the other things like Vendor Specific Messages, I don't
> > think there is any other way.
> >
> > So we really do need to be able to tap into the USB Power Delivery
> > protocol layer directly from user space. I don't care about how we do
> > that - character device is just a suggestion, although, it does still
> > feel correct to me. Is there some other way we could do this?
>
> Ok, a char device sounds fine, but _what_ userspace code is going to be
> using this interface? We need to have a working version of that as well
> before we could take this new interface, otherwise it wouldn't make much
> sense.
>
> And why does userspace have to do this, what is wrong with the kernel
> doing it as it does today? I.e. what is broken that adding a new api to
> the kernel is going to fix?
>
> That needs to be documented really really well.
Sure.
thanks,
--
heikki
Hi,
On Wed, Oct 27, 2021 at 06:03:08PM -0700, Prashant Malani wrote:
> 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.
USBPDDEV_SUBMIT_MESSAGE you use to send message directly to the partner.
USBPDDEV_SET_MESSAGE is meant to be used to store the values to a
cached message that the port manager should use next time there is
communication, but it does not send the message to the partner. So you
can use it even when there is no connection with a port, for example,
to store the values like the initial USB mode that should be used by
setting the EUDO message. Maybe the ioctl should be named
USBPDDEV_STORE_MESSAGE... I used "set" because it is sort of a
counterpart to USBPDDEV_GET_MESSAGE.
There is an explanation in include/uapi/linux/usb/pd_dev.h, please
check it. I'm curious also what you think about the idea with
USBPDDEV_CONFIGURE.
thanks,
--
heikki
Hi Heikki,
On Thu, Oct 28, 2021 at 12:36 AM Heikki Krogerus
<[email protected]> wrote:
>
> Hi,
>
> On Wed, Oct 27, 2021 at 06:03:08PM -0700, Prashant Malani wrote:
> > 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.
>
> USBPDDEV_SUBMIT_MESSAGE you use to send message directly to the partner.
>
> USBPDDEV_SET_MESSAGE is meant to be used to store the values to a
> cached message that the port manager should use next time there is
> communication, but it does not send the message to the partner. So you
> can use it even when there is no connection with a port, for example,
> to store the values like the initial USB mode that should be used by
> setting the EUDO message. Maybe the ioctl should be named
> USBPDDEV_STORE_MESSAGE... I used "set" because it is sort of a
> counterpart to USBPDDEV_GET_MESSAGE.
>
> There is an explanation in include/uapi/linux/usb/pd_dev.h, please
> check it.
Thanks for the further clarification. I guess I still don't see enough
need to differentiate SET/STORE
from SUBMIT; is there a situation where one would want to store the
source/sink caps for a port,
but not send/submit them immediately? When a partner is not connected
to a port, a set would
automatically just update the cached values and not perform a "submit"
(since there is nothing to
submit to). Perhaps there are (situations which require separate store
and submit), but I'm unable
to come up with one on the spot.
I'm curious also what you think about the idea with
> USBPDDEV_CONFIGURE.
It is indeed interesting. It seems like the specific interface for
this needs to be fleshed out more (will we
define a standard set of features which can be represented by the
|flags| and made configurable?). At present
I can't think of TCPM features which we'd want to toggle at runtime,
but I'm looking at it from a Chrome OS
perspective, so could be missing a bunch of use cases.
BTW, does poll work with this character device?
Best regards,