| 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: <CAMMMRMeKyi56Pha-X86BaQwcHGCx-xu5F67HCGZg=Yhxuk==OQ@mail.gmail.com>
Date: Tue, 22 Jul 2025 17:08:15 +0200
From: Andrei Kuchynski <akuchynski@...omium.org>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Abhishek Pandit-Subedi <abhishekpandit@...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>,
Dmitry Baryshkov <lumag@...nel.org>, "Christian A. Ehrhardt" <lk@...e.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 05/10] usb: typec: Implement automated mode selection
On Tue, Jul 1, 2025 at 10:41 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Mon, Jun 30, 2025 at 02:12:34PM +0000, Andrei Kuchynski wrote:
> > This commit introduces mode_selection sysfs attribute to control automated
> > mode negotiation. Writing "yes" to this file activates the automated
> > selection process for DisplayPort, Thunderbolt alternate modes, and USB4
> > mode. Conversely, writing "no" will cancel any ongoing selection process
> > and exit the currently active mode.
> >
> > Signed-off-by: Andrei Kuchynski <akuchynski@...omium.org>
> > ---
> > Documentation/ABI/testing/sysfs-class-typec | 17 +
> > drivers/usb/typec/class.c | 52 ++-
> > drivers/usb/typec/class.h | 10 +
> > drivers/usb/typec/mode_selection.c | 413 ++++++++++++++++++++
> > drivers/usb/typec/mode_selection.h | 30 ++
> > include/linux/usb/pd_vdo.h | 2 +
> > include/linux/usb/typec_altmode.h | 5 +
> > 7 files changed, 527 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > index ff3296ee8e1c..0ffc71a7c374 100644
> > --- a/Documentation/ABI/testing/sysfs-class-typec
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -263,6 +263,23 @@ Description: The USB Modes that the partner device supports. The active mode
> > - usb3 (USB 3.2)
> > - usb4 (USB4)
> >
> > +What: /sys/class/typec/<port>-partner/mode_selection
> > +Date: June 2025
> > +Contact: Andrei Kuchynski <akuchynski@...omium.org>
> > +Description: Lists the partner-supported alternate modes and mode entry
> > + results with the currently entered mode bracketed. If a cable doesn't
> > + support a mode, it's marked as 'nc'. An ellipsis indicates a mode
> > + currently in progress. Automated mode selection is activated by writing
> > + "yes" to the file. Conversely, writing "no" will cancel any ongoing
> > + selection process and exit the currently active mode, if any.
> > +
> > + Example values:
> > + - "DP TBT=... USB4=nc": The cable does not support USB4 mode,
> > + The driver is currently attempting to enter Thunderbolt alt-mode.
> > + - "[DP] TBT=-EOPNOTSUPP USB4=-ETIME": USB4 mode entry failed due to
> > + a timeout, Thunderbolt failed with the result -EOPNOTSUPP,
> > + and DisplayPort is the active alt-mode.
>
> We don't print error codes to userspace like this :(
>
The intention is to provide detailed status updates regarding failures.
I will revise this to simplify string parsing in user-space and omit
any error codes.
> And "yes" and "no" are not the traditional sysfs apis for on/off, please
> use the in-kernel function for that instead that takes many more types
> of values.
>
kstrtobool() is used. It is just not documented here. I will update the
documentation
> And again, multiple values in a sysfs file are not usually a good idea
> at all as now userspace has to parse them properly. What userspace tool
> is going to do that?
>
By combining results into a single, complex status string, the user can
receive the information atomically. This is why type_mode_selection_get
function is mutex-protected. It prevents the user from encountering
inconsistent states when reading different files.
> > +
> > USB Type-C cable devices (eg. /sys/class/typec/port0-cable/)
> >
> > Note: Electronically Marked Cables will have a device also for one cable plug
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 93eadbcdd4c0..8455e07a9934 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -741,6 +741,33 @@ static ssize_t number_of_alternate_modes_show(struct device *dev, struct device_
> > }
> > static DEVICE_ATTR_RO(number_of_alternate_modes);
> >
> > +static ssize_t mode_selection_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct typec_partner *partner = to_typec_partner(dev);
> > +
> > + return typec_mode_selection_get(partner, buf);
> > +}
> > +
> > +static ssize_t mode_selection_store(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t size)
> > +{
> > + struct typec_partner *partner = to_typec_partner(dev);
> > + bool start;
> > + int ret = kstrtobool(buf, &start);
> > +
> > + if (!ret) {
> > + if (start)
> > + ret = typec_mode_selection_start(partner);
> > + else
> > + ret = typec_mode_selection_reset(partner);
> > + }
> > +
> > + return ret ? : size;
>
> Again, only use ? : if you have to (hint, you don't have to here.)
>
Will be replaced with an if/else statement.
> > +static unsigned int mode_selection_timeout = 4000;
> > +module_param(mode_selection_timeout, uint, 0644);
> > +MODULE_PARM_DESC(mode_selection_timeout, "The timeout mode entry, ms");
> > +
> > +static unsigned int mode_selection_delay = 1000;
> > +module_param(mode_selection_delay, uint, 0644);
> > +MODULE_PARM_DESC(mode_selection_delay,
> > + "The delay between attempts to enter or exit a mode, ms");
> > +
> > +static unsigned int mode_selection_entry_attempts = 4;
> > +module_param(mode_selection_entry_attempts, uint, 0644);
> > +MODULE_PARM_DESC(mode_selection_entry_attempts,
> > + "Max attempts to enter mode on BUSY result");
>
> This is not the 1990's, please NEVER add new module parameters
> (especially ones that you never even documented in the changelog!)
>
> This just will not work, attributes for functionality either need to
> "just work properly" or you need to make them on a per-controller type
> basis as remember, systems have multiple controllers in them...
>
The current values are suitable for all controllers we work with,
rendering per-controller adjustments unnecessary. Therefore, I will
proceed with removing these module parameters.
> > +
> > static const char * const mode_names[] = {
> > [TYPEC_DP_ALTMODE] = "DP",
> > [TYPEC_TBT_ALTMODE] = "TBT",
> > @@ -15,6 +31,15 @@ static const char * const mode_names[] = {
> > };
> > static const char * const default_priorities = "USB4 TBT DP";
> >
> > +struct mode_selection_state {
> > + int mode;
>
> Shouldn't this be an enum?
>
enum is much better, thanks
> > + bool enable;
> > + bool cable_capability;
> > + bool enter;
> > + int attempt_count;
> > + int result;
> > +};
>
> No documentation for what this structure is for?
>
This, of course, needs to be described.
> > +
> > /* -------------------------------------------------------------------------- */
> > /* port 'mode_priorities' attribute */
> > static int typec_mode_parse_priority_string(const char *str, int *list)
> > @@ -114,3 +139,391 @@ int typec_mode_priorities_get(struct typec_port *port, char *buf)
> >
> > return count + sysfs_emit_at(buf, count, "\n");
> > }
> > +
> > +/* -------------------------------------------------------------------------- */
> > +/* partner 'mod_selection' attribute */
> > +
> > +/**
> > + * mode_selection_next() - Process mode selection results and schedule next
> > + * action
> > + *
> > + * This function evaluates the outcome of the previous mode entry or exit
> > + * attempt. Based on this result, it determines the next mode to process and
> > + * schedules `mode_selection_work()` if further actions are required.
> > + *
> > + * If the previous mode entry was successful, the mode selection sequence is
> > + * considered complete for the current cycle.
> > + *
> > + * If the previous mode entry failed, this function schedules
> > + * `mode_selection_work()` to attempt exiting the mode that was partially
> > + * activated but not fully entered.
> > + *
> > + * If the previous operation was an exit (after a failed entry attempt),
> > + * `mode_selection_next()` then advances the internal list of candidate
> > + * modes to determine the next mode to enter.
> > + */
> > +static void mode_selection_next(
> > + struct typec_partner *partner, struct mode_selection_state *ms)
> > +{
> > + if (!ms->enter) {
> > + kfifo_skip(&partner->mode_sequence);
> > + } else if (!ms->result) {
> > + dev_info(&partner->dev, "%s mode entered\n", mode_names[ms->mode]);
>
> Please remove debugging code.
>
> > +
> > + partner->active_mode = ms;
> > + kfifo_reset(&partner->mode_sequence);
> > + } else {
> > + dev_err(&partner->dev, "%s mode entry failed: %pe\n",
> > + mode_names[ms->mode], ERR_PTR(ms->result));
>
> What can a user do with this error message?
>
All prints will be removed.
> > +
> > + if (ms->result != -EBUSY ||
> > + ms->attempt_count >= mode_selection_entry_attempts)
> > + ms->enter = false;
> > + }
> > +
> > + if (!kfifo_is_empty(&partner->mode_sequence))
> > + schedule_delayed_work(&partner->mode_selection_work,
> > + msecs_to_jiffies(mode_selection_delay));
> > +}
> > +
> > +static void mode_selection_complete(struct typec_partner *partner,
> > + const int mode, const int result)
> > +{
> > + struct mode_selection_state *ms;
> > +
> > + mutex_lock(&partner->mode_sequence_lock);
>
> You use a lock here, but not in the function above? Why?
>
`mode_sequence_lock` mutex protects `mode_sequence` kfifo state. It
should be locked before kfifo_peek()
> > + if (kfifo_peek(&partner->mode_sequence, &ms)) {
> > + if (ms->mode == mode) {
> > + ms->result = result;
> > + cancel_delayed_work(&partner->mode_selection_work);
> > + mode_selection_next(partner, ms);
>
> Ah, you need to have the lock held, you didn't document that in text or
> in a way the compiler can verify/check it :(
>
>
It is a good case for __must_hold macro. Thanks
> > + }
> > + }
> > + mutex_unlock(&partner->mode_sequence_lock);
> > +}
> > +
> > +void typec_mode_selection_altmode_complete(struct typec_altmode *alt,
> > + const int result)
> > +{
> > + mode_selection_complete(to_typec_partner(alt->dev.parent),
> > + typec_svid_to_altmode(alt->svid), result);
> > +}
> > +EXPORT_SYMBOL_GPL(typec_mode_selection_altmode_complete);
> > +
> > +void typec_mode_selection_usb4_complete(struct typec_partner *partner,
> > + const int result)
> > +{
> > + mode_selection_complete(partner, TYPEC_USB4_MODE, result);
> > +}
> > +EXPORT_SYMBOL_GPL(typec_mode_selection_usb4_complete);
> > +
> > +static void mode_selection_activate_usb4_mode(struct typec_partner *partner,
> > + struct mode_selection_state *ms)
> > +{
> > + struct typec_port *port = to_typec_port(partner->dev.parent);
> > + int result = -EOPNOTSUPP;
> > +
> > + if (port->ops && port->ops->enter_usb_mode) {
> > + if (ms->enter && port->usb_mode != USB_MODE_USB4)
> > + result = -EPERM;
> > + else
> > + result = port->ops->enter_usb_mode(port,
> > + ms->enter ? USB_MODE_USB4 : USB_MODE_USB3);
> > + }
> > +
> > + if (ms->enter)
> > + ms->result = result;
> > +}
> > +
> > +static int mode_selection_activate_altmode(struct device *dev, void *data)
> > +{
> > + struct typec_altmode *alt = to_typec_altmode(dev);
> > + struct mode_selection_state *ms = (struct mode_selection_state *)data;
> > + int result = -ENODEV;
> > +
> > + if (!strcmp(dev->type->name, ALTERNATE_MODE_DEVICE_TYPE_NAME)) {
> > + if (ms->mode == typec_svid_to_altmode(alt->svid)) {
> > + if (alt->ops && alt->ops->activate)
> > + result = alt->ops->activate(alt, ms->enter ? 1 : 0);
> > + else
> > + result = -EOPNOTSUPP;
> > + }
> > + }
> > +
> > + if (ms->enter)
> > + ms->result = result;
> > +
> > + return result == -ENODEV ? 0 : 1;
> > +}
> > +
> > +static void mode_selection_activate_mode(struct typec_partner *partner,
> > + struct mode_selection_state *ms)
> > +{
> > + dev_info(&partner->dev, "%s %s mode\n",
> > + ms->enter ? "Enter" : "Exit", mode_names[ms->mode]);
>
> Again, please remove debugging code.
>
> When drivers work properly, they are quiet.
>
> And this really is the only valid use for ? : around, so that's ok here :)
>
Sadly, the only good case will be removed :)
> > +void typec_mode_selection_add_cable(struct typec_partner *partner,
> > + struct typec_cable *cable)
> > +{
> > + const u32 id_header = cable->identity->id_header;
> > + const u32 vdo1 = cable->identity->vdo[0];
> > + const u32 type = PD_IDH_PTYPE(id_header);
> > + const u32 speed = VDO_TYPEC_CABLE_SPEED(vdo1);
> > + bool capability[] = {
> > + [TYPEC_DP_ALTMODE] = true,
> > + [TYPEC_TBT_ALTMODE] = false,
> > + [TYPEC_USB4_MODE] = false,
> > + };
>
> Why are these the default capabilities? Where is that documented? Why
> these specific values to start with?
>
These values are currently utilized in ChromeOS when VDO is 0 and the
cable is neither passive nor active. This unfortunately includes some USB
dongles with DisplayPort support. I will document this behavior.
Thanks for the review!
Andrei
> thanks,
>
> greg k-h
Powered by blists - more mailing lists