Message-ID: <CAMMMRMck=YzbN6g8-4qUx=YX+=S4HgKqHaYoUuQ0CsJUYd9GOw@mail.gmail.com>
Date: Thu, 11 Sep 2025 10:46:52 +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 <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 4/5] usb: typec: Implement alternate mode priority handling

On Thu, Sep 11, 2025 at 7:07 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Wed, Sep 10, 2025 at 07:35:42PM +0000, Andrei Kuchynski wrote:
> > On Wed, Sep 10, 2025 at 1:31 PM Greg Kroah-Hartman
> > <gregkh@...uxfoundation.org> wrote:
> > >
> > > On Fri, Sep 05, 2025 at 02:22:05PM +0000, Andrei Kuchynski wrote:
> > > > This patch introduces APIs to manage the priority of USB Type-C alternate
> > > > modes. These APIs allow for setting and retrieving a priority number for
> > > > each mode. If a new priority value conflicts with an existing mode's
> > > > priority, the priorities of the conflicting mode and all subsequent modes
> > > > are automatically incremented to ensure uniqueness.
> > > >
> > > > Signed-off-by: Andrei Kuchynski <akuchynski@...omium.org>
> > > > ---
> > > >  drivers/usb/typec/Makefile         |  2 +-
> > > >  drivers/usb/typec/mode_selection.c | 38 ++++++++++++++++++++++++++++++
> > > >  drivers/usb/typec/mode_selection.h |  6 +++++
> > > >  include/linux/usb/typec_altmode.h  |  1 +
> > > >  4 files changed, 46 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/usb/typec/mode_selection.c
> > > >  create mode 100644 drivers/usb/typec/mode_selection.h
> > > >
> > > > 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/mode_selection.c b/drivers/usb/typec/mode_selection.c
> > > > new file mode 100644
> > > > index 000000000000..2179bf25f5d4
> > > > --- /dev/null
> > > > +++ b/drivers/usb/typec/mode_selection.c
> > > > @@ -0,0 +1,38 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * Copyright 2025 Google LLC.
> > > > + */
> > > > +
> > > > +#include "mode_selection.h"
> > > > +#include "class.h"
> > > > +#include "bus.h"
> > > > +
> > > > +static int increment_duplicated_priority(struct device *dev, void *data)
> > > > +{
> > > > +     struct typec_altmode **alt_target = (struct typec_altmode **)data;
> > > > +
> > > > +     if (is_typec_altmode(dev)) {
> > > > +             struct typec_altmode *alt = to_typec_altmode(dev);
> > > > +
> > > > +             if (alt != *alt_target && alt->priority == (*alt_target)->priority) {
> > > > +                     alt->priority++;
> > > > +                     *alt_target = alt;
> > > > +                     return 1;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +void typec_mode_set_priority(struct typec_altmode *alt,
> > > > +             const unsigned int priority)
> > > > +{
> > > > +     struct typec_port *port = to_typec_port(alt->dev.parent);
> > > > +     int res = 1;
> > > > +
> > > > +     alt->priority = priority;
> > > > +
> > > > +     while (res)
> > > > +             res = device_for_each_child(&port->dev, &alt,
> > > > +                             increment_duplicated_priority);
> > > > +}
> > > > diff --git a/drivers/usb/typec/mode_selection.h b/drivers/usb/typec/mode_selection.h
> > > > new file mode 100644
> > > > index 000000000000..cbf5a37e6404
> > > > --- /dev/null
> > > > +++ b/drivers/usb/typec/mode_selection.h
> > > > @@ -0,0 +1,6 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +
> > > > +#include <linux/usb/typec_altmode.h>
> > > > +
> > > > +void typec_mode_set_priority(struct typec_altmode *alt,
> > > > +             const unsigned int priority);
> > > > diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h
> > > > index b3c0866ea70f..571c6e00b54f 100644
> > > > --- a/include/linux/usb/typec_altmode.h
> > > > +++ b/include/linux/usb/typec_altmode.h
> > > > @@ -28,6 +28,7 @@ struct typec_altmode {
> > > >       int                             mode;
> > > >       u32                             vdo;
> > > >       unsigned int                    active:1;
> > > > +     unsigned int                    priority;
> > >
> > > What is the range of this?  And this value is only incremented, never
> > > decremented?
> > >
> >
> > The range extends from 0 to UINT_MAX. The value is only incremented.
> > The only exception is that If the user sets UINT_MAX for two alternate
> > modes in turn, the priority of the first mode becomes 0. This does not
> > break the algorithm, and the user can check all priorities via
> > ‘priority’ attributes.
>
> Why not use u32 to define a sane range?  Or u8?  How many different
> priorities will actually be used in the real world?
>

The priority can be u32 or u8, but not bool. I use unsigned int as the
precise bit count is not important here.

Three different priorities are enough for DisplayPort, Thunderbolt,
USB4, at least for now. The algorithm is designed to accommodate any
number of modes, as it does not rely on predefined MAX_ALTMODE or
MAX_PRIORITY values.

Priority numbers are used only with the '<' operator.
In fact, we need a list of modes to determine the order of entry.
Previously, a list was used in the 'mode selection' series, but
parsing it is more complex than using kstrtouint().

> And what happens when it wraps?
>

If a user sets all priorities (either 2 or 3) to UINT_MAX, the
resulting mode sequence will not be as expected due to overflow.

> > I am unsure if a check for this specific case is necessary, as it
> > would require examining priorities across all modes, not just a simple
> > 'if' statement.
> > There are a few ideas in this algorithm:
> > - all priorities must always be valid and unique
>
> Is that true?  Where is that validated?
>

typec_mode_set_priority() ensures all priorities remain unique. Any
numerical value is valid as long as it can be compared to another,
kstrtouint() will return this value.

> > - no unnecessary restrictions for the user
>
> What does the user actually want to do here?
>

User wants to set the highest priority mode. For this, a single '0'
write operation is required. Also user can prioritize all modes by
assigning '0' to the 'priority' attribute for each mode, beginning
with the lowest prioritized mode and moving upwards.

For example, USB4 is the preferred mode, with priority 0. If a partner
(or port) device does not support USB4, the user can set the next
preferred mode (TBT or DP) based on security policy by assigning
priority 1 for only one mode. This mode is TBT when a user is logged
in, and DP otherwise.

> > - as simple as possible
>
> But not too simple :)
>
> thanks,
>
> greg k-h

Thanks,
Andrei

Powered by blists - more mailing lists