| 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
| ||
|
Message-ID: <CANFp7mVUFZyF8z0dN-Mo7ntPOXh06ZD0RH5GyvJJymOXrhSD1g@mail.gmail.com>
Date: Mon, 11 Aug 2025 11:22:38 -0700
From: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: Andrei Kuchynski <akuchynski@...omium.org>, Benson Leung <bleung@...omium.org>,
Jameson Thies <jthies@...gle.com>, Tzung-Bi Shih <tzungbi@...nel.org>, linux-usb@...r.kernel.org,
chrome-platform@...ts.linux.dev, Guenter Roeck <groeck@...omium.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>, "Christian A. Ehrhardt" <lk@...e.de>,
Venkat Jayaraman <venkat.jayaraman@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 04/10] usb: typec: Expose mode priorities via sysfs
On Mon, Aug 11, 2025 at 7:25 AM Heikki Krogerus
<heikki.krogerus@...ux.intel.com> wrote:
>
> Hi Andrei,
>
> On Mon, Aug 04, 2025 at 09:03:33AM +0000, Andrei Kuchynski wrote:
> > This patch introduces new sysfs attributes to allow users to configure
> > and view Type-C mode priorities.
> >
> > `priority`, `usb4_priority` attributes allow users to assign a numeric
> > priority to DisplayPort alt-mode, Thunderbolt alt-mode, and USB4 mode.
> >
> > `mode_priorities` - read-only attribute that displays an ordered list
> > of all modes based on their configured priorities.
> >
> > Signed-off-by: Andrei Kuchynski <akuchynski@...omium.org>
> > ---
> > Documentation/ABI/testing/sysfs-class-typec | 33 +++++
> > drivers/usb/typec/Makefile | 2 +-
> > drivers/usb/typec/class.c | 103 +++++++++++++++-
> > drivers/usb/typec/class.h | 1 +
> > drivers/usb/typec/mode_selection.c | 130 ++++++++++++++++++++
> > drivers/usb/typec/mode_selection.h | 23 ++++
> > include/linux/usb/typec_altmode.h | 7 ++
> > 7 files changed, 295 insertions(+), 4 deletions(-)
> > create mode 100644 drivers/usb/typec/mode_selection.c
> > create mode 100644 drivers/usb/typec/mode_selection.h
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > index 38e101c17a00..575dd94f33ab 100644
> > --- a/Documentation/ABI/testing/sysfs-class-typec
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -162,6 +162,39 @@ Description: Lists the supported USB Modes. The default USB mode that is used
> > - usb3 (USB 3.2)
> > - usb4 (USB4)
> >
> > + What: /sys/class/typec/<port>/<alt-mode>/priority
> > +Date: July 2025
> > +Contact: Andrei Kuchynski <akuchynski@...omium.org>
> > +Description:
> > + Displays and allows setting the priority for a specific alt-mode.
> > + When read, it shows the current integer priority value. Lower numerical
> > + values indicate higher priority (0 is the highest priority).
> > + If the new value is already in use by another mode, the priority of the
> > + conflicting mode and any subsequent modes will be incremented until they
> > + are all unique.
> > + This attribute is visible only if the kernel supports mode selection.
> > +
> > + What: /sys/class/typec/<port>/usb4_priority
> > +Date: July 2025
> > +Contact: Andrei Kuchynski <akuchynski@...omium.org>
> > +Description:
> > + Displays and allows setting the priority for USB4 mode. Its behavior and
> > + priority numbering scheme are identical to the general alt-mode
> > + "priority" attributes.
>
> I'm not sure those above two file make any sense.
>
> > +What: /sys/class/typec/<port>/mode_priorities
> > +Date: July 2025
> > +Contact: Andrei Kuchynski <akuchynski@...omium.org>
> > +Description: This read-only file lists the modes supported by the port,
> > + ordered by their activation priority. It reflects the preferred sequence
> > + the kernel will attempt to activate modes (DisplayPort alt-mode,
> > + Thunderbolt alt-mode, USB4 mode).
> > + This attribute is visible only if the kernel supports mode selection.
> > +
> > + Example values:
> > + - "USB4 Thunderbolt3 DisplayPort"
> > + - "DisplayPort": the port only supports Displayport alt-mode
>
> Why not just use this one instead so that you write the highest
> priority mode to it?
Feedback from Greg on
https://lore.kernel.org/linux-usb/2025070159-judgingly-baggage-042a@gregkh/:
"quote":
Multiple value sysfs files are generally frowned apon. sysfs files that
also have to be manually parsed in the kernel are also frowned apon.
Are you _SURE_ there is no other way that you could possibly do this?
The reason we originally suggested a single "mode priorities" was
because we weren't sure what to do about USB4. Otherwise, it makes
sense to push a priority field to each alt_mode sysfs group and keep
it internally ordered. This is where I really wish we just treated
USB4 as an alternate mode :)
As such, our current API recommendation looks like the following:
* On each port, we lay out priorities for all supported alternate modes + USB4.
* We expose a file to trigger the mode selection task. Reading from it
gives you the current status of mode selection (single value).
* Detailed results from mode entry are pushed to the mode sysfs group
(via entry_results). Converting these to UEVENT is fine but a more
persistent value in debugfs would be useful for debugging.
>
> > USB Type-C partner devices (eg. /sys/class/typec/port0-partner/)
> >
> > What: /sys/class/typec/<port>-partner/accessory_mode
> > diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> > index 7a368fea61bc..8a6a1c663eb6 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 pd.o retimer.o
> > +typec-y := class.o mux.o bus.o pd.o retimer.o mode_selection.o
> > typec-$(CONFIG_ACPI) += port-mapper.o
> > obj-$(CONFIG_TYPEC) += altmodes/
> > obj-$(CONFIG_TYPEC_TCPM) += tcpm/
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index a72325ff099a..414d94c45ab9 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -19,6 +19,7 @@
> > #include "bus.h"
> > #include "class.h"
> > #include "pd.h"
> > +#include "mode_selection.h"
> >
> > static DEFINE_IDA(typec_index_ida);
> >
> > @@ -445,11 +446,45 @@ svid_show(struct device *dev, struct device_attribute *attr, char *buf)
> > }
> > static DEVICE_ATTR_RO(svid);
> >
> > +static ssize_t priority_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t size)
> > +{
> > + struct typec_altmode *adev = to_typec_altmode(dev);
> > + unsigned int val;
> > + int err = kstrtouint(buf, 10, &val);
> > +
> > + if (!err) {
> > + err = typec_mode_set_priority(to_typec_port(adev->dev.parent),
> > + typec_svid_to_altmode(adev->svid), val);
> > + if (!err)
> > + return size;
> > + }
> > +
> > + return err;
> > +}
> > +
> > +static ssize_t priority_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct typec_altmode *adev = to_typec_altmode(dev);
> > + int val;
> > + const int err = typec_mode_get_priority(to_typec_port(adev->dev.parent),
> > + typec_svid_to_altmode(adev->svid), &val);
> > +
> > + if (err)
> > + return err;
> > +
> > + return sprintf(buf, "%d\n", val);
> > +}
> > +static DEVICE_ATTR_RW(priority);
> > +
> > static struct attribute *typec_altmode_attrs[] = {
> > &dev_attr_active.attr,
> > &dev_attr_mode.attr,
> > &dev_attr_svid.attr,
> > &dev_attr_vdo.attr,
> > + &dev_attr_priority.attr,
> > NULL
> > };
> >
> > @@ -458,7 +493,7 @@ static umode_t typec_altmode_attr_is_visible(struct kobject *kobj,
> > {
> > struct typec_altmode *adev = to_typec_altmode(kobj_to_dev(kobj));
> >
> > - if (attr == &dev_attr_active.attr)
> > + if (attr == &dev_attr_active.attr) {
> > if (!is_typec_port(adev->dev.parent)) {
> > struct typec_partner *partner =
> > to_typec_partner(adev->dev.parent);
> > @@ -469,6 +504,15 @@ static umode_t typec_altmode_attr_is_visible(struct kobject *kobj,
> > !adev->ops->activate)
> > return 0444;
> > }
> > + } else if (attr == &dev_attr_priority.attr) {
> > + if (is_typec_port(adev->dev.parent)) {
> > + struct typec_port *port = to_typec_port(adev->dev.parent);
> > +
> > + if (!port->alt_mode_override)
> > + return 0;
> > + } else
> > + return 0;
> > + }
> >
> > return attr->mode;
> > }
> > @@ -1942,6 +1986,44 @@ static ssize_t orientation_show(struct device *dev,
> > }
> > static DEVICE_ATTR_RO(orientation);
> >
> > +static ssize_t mode_priorities_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + return typec_mode_get_priority_list(to_typec_port(dev), buf);
> > +}
> > +static DEVICE_ATTR_RO(mode_priorities);
> > +
> > +static ssize_t usb4_priority_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct typec_port *port = to_typec_port(dev);
> > + int val;
> > + const int err = typec_mode_get_priority(port, TYPEC_USB4_MODE, &val);
> > +
> > + if (err)
> > + return err;
> > +
> > + return sprintf(buf, "%d\n", val);
> > +}
> > +
> > +static ssize_t usb4_priority_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t size)
> > +{
> > + struct typec_port *port = to_typec_port(dev);
> > + unsigned int val;
> > + int err = kstrtouint(buf, 10, &val);
> > +
> > + if (!err) {
> > + err = typec_mode_set_priority(port, TYPEC_USB4_MODE, val);
> > + if (!err)
> > + return size;
> > + }
> > +
> > + return err;
> > +}
> > +static DEVICE_ATTR_RW(usb4_priority);
> > +
> > static struct attribute *typec_attrs[] = {
> > &dev_attr_data_role.attr,
> > &dev_attr_power_operation_mode.attr,
> > @@ -1954,6 +2036,8 @@ static struct attribute *typec_attrs[] = {
> > &dev_attr_port_type.attr,
> > &dev_attr_orientation.attr,
> > &dev_attr_usb_capability.attr,
> > + &dev_attr_mode_priorities.attr,
> > + &dev_attr_usb4_priority.attr,
> > NULL,
> > };
> >
> > @@ -1992,6 +2076,13 @@ static umode_t typec_attr_is_visible(struct kobject *kobj,
> > return 0;
> > if (!port->ops || !port->ops->default_usb_mode_set)
> > return 0444;
> > + } else if (attr == &dev_attr_mode_priorities.attr) {
> > + if (!port->alt_mode_override)
> > + return 0;
>
> I think the mode order could be visible even when it's read only.
>
> > + } else if (attr == &dev_attr_usb4_priority.attr) {
> > + if (!port->alt_mode_override ||
> > + !(port->cap->usb_capability & USB_CAPABILITY_USB4))
> > + return 0;
> > }
> >
> > return attr->mode;
> > @@ -2029,6 +2120,7 @@ static void typec_release(struct device *dev)
> > typec_mux_put(port->mux);
> > typec_retimer_put(port->retimer);
> > kfree(port->cap);
> > + typec_mode_selection_destroy(port);
> > kfree(port);
> > }
> >
> > @@ -2496,6 +2588,8 @@ typec_port_register_altmode(struct typec_port *port,
> > to_altmode(adev)->retimer = retimer;
> > }
> >
> > + typec_mode_set_priority(port, typec_svid_to_altmode(adev->svid), -1);
> > +
> > return adev;
> > }
> > EXPORT_SYMBOL_GPL(typec_port_register_altmode);
> > @@ -2645,9 +2739,12 @@ struct typec_port *typec_register_port(struct device *parent,
> > port->con.attach = typec_partner_attach;
> > port->con.deattach = typec_partner_deattach;
> >
> > - if (cap->usb_capability & USB_CAPABILITY_USB4)
> > + typec_mode_selection_init(port);
> > +
> > + if (cap->usb_capability & USB_CAPABILITY_USB4) {
> > port->usb_mode = USB_MODE_USB4;
> > - else if (cap->usb_capability & USB_CAPABILITY_USB3)
> > + typec_mode_set_priority(port, TYPEC_USB4_MODE, -1);
> > + } else if (cap->usb_capability & USB_CAPABILITY_USB3)
> > port->usb_mode = USB_MODE_USB3;
> > else if (cap->usb_capability & USB_CAPABILITY_USB2)
> > port->usb_mode = USB_MODE_USB2;
> > diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
> > index f05d9201c233..c6467e576569 100644
> > --- a/drivers/usb/typec/class.h
> > +++ b/drivers/usb/typec/class.h
> > @@ -82,6 +82,7 @@ struct typec_port {
> > struct device *usb3_dev;
> >
> > bool alt_mode_override;
> > + struct list_head mode_list;
> > };
> >
> > #define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
> > diff --git a/drivers/usb/typec/mode_selection.c b/drivers/usb/typec/mode_selection.c
> > new file mode 100644
> > index 000000000000..9a7185c07d0c
> > --- /dev/null
> > +++ b/drivers/usb/typec/mode_selection.c
> > @@ -0,0 +1,130 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright 2025 Google LLC.
> > + */
> > +
> > +#include <linux/usb/typec_altmode.h>
> > +#include <linux/slab.h>
> > +#include <linux/list.h>
> > +#include "mode_selection.h"
> > +#include "class.h"
> > +
> > +static const char * const mode_names[TYPEC_MODE_MAX] = {
> > + [TYPEC_DP_ALTMODE] = "DisplayPort",
> > + [TYPEC_TBT_ALTMODE] = "Thunderbolt3",
> > + [TYPEC_USB4_MODE] = "USB4",
> > +};
> > +
> > +static const int default_priorities[TYPEC_MODE_MAX] = {
> > + [TYPEC_DP_ALTMODE] = 2,
> > + [TYPEC_TBT_ALTMODE] = 1,
> > + [TYPEC_USB4_MODE] = 0,
> > +};
> > +
> > +/**
> > + * struct mode_selection_state - State tracking for a specific Type-C mode
> > + * @mode: The type of mode this instance represents
> > + * @name: Name string pointer
> > + * @priority: The mode priority. Higher values indicate a more preferred mode.
> > + * @list: List head to link this mode state into a prioritized list.
> > + */
> > +struct mode_selection_state {
> > + enum typec_mode_type mode;
> > + const char *name;
> > + int priority;
> > + struct list_head list;
> > +};
>
> The name member looks unnecessary, but maybe you use it out side of
> this file in the following patches.
>
> > +/* -------------------------------------------------------------------------- */
> > +/* port 'mode_priorities' attribute */
> > +void typec_mode_selection_init(struct typec_port *port)
> > +{
> > + INIT_LIST_HEAD(&port->mode_list);
> > +}
>
> Useless function.
>
> > +void typec_mode_selection_destroy(struct typec_port *port)
> > +{
> > + struct mode_selection_state *ms, *tmp;
> > +
> > + list_for_each_entry_safe(ms, tmp, &port->mode_list, list) {
> > + list_del(&ms->list);
> > + kfree(ms);
> > + }
> > +}
> > +
> > +int typec_mode_set_priority(struct typec_port *port,
> > + const enum typec_mode_type mode, const int priority)
> > +{
> > + struct mode_selection_state *ms_target = NULL;
> > + struct mode_selection_state *ms, *tmp;
> > +
> > + if (mode >= TYPEC_MODE_MAX || !mode_names[mode])
> > + return -EOPNOTSUPP;
> > +
> > + list_for_each_entry_safe(ms, tmp, &port->mode_list, list) {
> > + if (ms->mode == mode) {
> > + ms_target = ms;
> > + list_del(&ms->list);
> > + break;
> > + }
> > + }
> > +
> > + if (!ms_target) {
> > + ms_target = kzalloc(sizeof(struct mode_selection_state), GFP_KERNEL);
> > + if (!ms_target)
> > + return -ENOMEM;
> > + ms_target->mode = mode;
> > + ms_target->name = mode_names[mode];
> > + INIT_LIST_HEAD(&ms_target->list);
> > + }
> > +
> > + if (priority >= 0)
> > + ms_target->priority = priority;
> > + else
> > + ms_target->priority = default_priorities[mode];
> > +
> > + while (ms_target) {
> > + struct mode_selection_state *ms_peer = NULL;
> > +
> > + list_for_each_entry(ms, &port->mode_list, list)
> > + if (ms->priority >= ms_target->priority) {
> > + if (ms->priority == ms_target->priority)
> > + ms_peer = ms;
> > + break;
> > + }
> > +
> > + list_add_tail(&ms_target->list, &ms->list);
> > + ms_target = ms_peer;
> > + if (ms_target) {
> > + ms_target->priority++;
> > + list_del(&ms_target->list);
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int typec_mode_get_priority(struct typec_port *port,
> > + const enum typec_mode_type mode, int *priority)
> > +{
> > + struct mode_selection_state *ms;
> > +
> > + list_for_each_entry(ms, &port->mode_list, list)
> > + if (ms->mode == mode) {
> > + *priority = ms->priority;
> > + return 0;
> > + }
> > +
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +ssize_t typec_mode_get_priority_list(struct typec_port *port, char *buf)
> > +{
> > + struct mode_selection_state *ms;
> > + ssize_t count = 0;
> > +
> > + list_for_each_entry(ms, &port->mode_list, list)
> > + count += sysfs_emit_at(buf, count, "%s ", ms->name);
> > +
> > + return count + sysfs_emit_at(buf, count, "\n");
> > +}
> > diff --git a/drivers/usb/typec/mode_selection.h b/drivers/usb/typec/mode_selection.h
> > new file mode 100644
> > index 000000000000..151f0f8b6632
> > --- /dev/null
> > +++ b/drivers/usb/typec/mode_selection.h
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#include <linux/usb/typec_dp.h>
> > +#include <linux/usb/typec_tbt.h>
> > +
> > +static inline enum typec_mode_type typec_svid_to_altmode(const u16 svid)
> > +{
> > + switch (svid) {
> > + case USB_TYPEC_DP_SID:
> > + return TYPEC_DP_ALTMODE;
> > + case USB_TYPEC_TBT_SID:
> > + return TYPEC_TBT_ALTMODE;
> > + }
> > + return TYPEC_MODE_MAX;
> > +}
> > +
> > +void typec_mode_selection_init(struct typec_port *port);
> > +void typec_mode_selection_destroy(struct typec_port *port);
> > +int typec_mode_set_priority(struct typec_port *port,
> > + const enum typec_mode_type mode, const int priority);
> > +int typec_mode_get_priority(struct typec_port *port,
> > + const enum typec_mode_type mode, int *priority);
> > +ssize_t typec_mode_get_priority_list(struct typec_port *port, char *buf);
> > diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h
> > index b3c0866ea70f..5d14363e02eb 100644
> > --- a/include/linux/usb/typec_altmode.h
> > +++ b/include/linux/usb/typec_altmode.h
> > @@ -145,6 +145,13 @@ enum {
> >
> > #define TYPEC_MODAL_STATE(_state_) ((_state_) + TYPEC_STATE_MODAL)
> >
> > +enum typec_mode_type {
> > + TYPEC_DP_ALTMODE = 0,
> > + TYPEC_TBT_ALTMODE,
> > + TYPEC_USB4_MODE,
> > + TYPEC_MODE_MAX,
> > +};
> > +
> > struct typec_altmode *typec_altmode_get_plug(struct typec_altmode *altmode,
> > enum typec_plug_index index);
> > void typec_altmode_put_plug(struct typec_altmode *plug);
>
> It looks like this patch would allow the user space to write the mode
> priority order without it taking effect. You need to re-organise this
> series.
>
> Please introduce the kernel APIs first followed by the user space ABI
> changes. That should also make these a bit easier to review.
>
> thanks,
>
> --
> heikki
Powered by blists - more mailing lists