Message-ID: <aFLCLL_7Ov8J9Wdx@kuha.fi.intel.com>
Date: Wed, 18 Jun 2025 16:42:04 +0300
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Andrei Kuchynski <akuchynski@...omium.org>,
	Dmitry Baryshkov <lumag@...nel.org>,
	Jameson Thies <jthies@...gle.com>,
	Benson Leung <bleung@...omium.org>,
	Tzung-Bi Shih <tzungbi@...nel.org>,
	Guenter Roeck <groeck@...omium.org>,
	Pooja Katiyar <pooja.katiyar@...el.com>,
	Badhri Jagan Sridharan <badhri@...gle.com>,
	RD Babiera <rdbabiera@...gle.com>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, chrome-platform@...ts.linux.dev
Subject: Re: [PATCH 04/10] usb: typec: Expose alternate mode priorities via
 sysfs

Hi Abhishek,

On Tue, Jun 17, 2025 at 07:47:18PM -0700, Abhishek Pandit-Subedi wrote:
> On Tue, Jun 17, 2025 at 6:28 AM Greg Kroah-Hartman
> <gregkh@...uxfoundation.org> wrote:
> >
> > On Tue, Jun 17, 2025 at 02:38:04PM +0200, Andrei Kuchynski wrote:
> > > On Tue, Jun 17, 2025 at 11:50 AM Heikki Krogerus
> > > <heikki.krogerus@...ux.intel.com> wrote:
> > > >
> > > > On Mon, Jun 16, 2025 at 01:31:41PM +0000, Andrei Kuchynski wrote:
> > > > > This sysfs attribute specifies the preferred order for enabling
> > > > > DisplayPort alt-mode, Thunderbolt alt-mode, and USB4 mode.
> > > > >
> > > > > Signed-off-by: Andrei Kuchynski <akuchynski@...omium.org>
> > > > > ---
> > > > >  Documentation/ABI/testing/sysfs-class-typec | 17 ++++
> > > > >  drivers/usb/typec/Makefile                  |  2 +-
> > > > >  drivers/usb/typec/class.c                   | 26 ++++++
> > > > >  drivers/usb/typec/class.h                   |  2 +
> > > > >  drivers/usb/typec/mode_selection.c          | 93 +++++++++++++++++++++
> > > > >  drivers/usb/typec/mode_selection.h          |  5 ++
> > > > >  include/linux/usb/typec_altmode.h           |  7 ++
> > > > >  7 files changed, 151 insertions(+), 1 deletion(-)
> > > > >  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..46eee82042ab 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-class-typec
> > > > > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > > > > @@ -162,6 +162,23 @@ Description:     Lists the supported USB Modes. The default USB mode that is used
> > > > >               - usb3 (USB 3.2)
> > > > >               - usb4 (USB4)
> > > > >
> > > > > +What:                /sys/class/typec/<port>/altmode_priorities
> > > > > +Date:                June 2025
> > > > > +Contact:     Andrei Kuchynski <akuchynski@...omium.org>
> > > > > +Description: Lists the alternate modes supported by the port and their
> > > > > +             priorities. The priority setting determines the order in which
> > > > > +             Displayport alt-mode, Thunderbolt alt-mode and USB4 mode will be
> > > > > +             activated, indicating the preferred selection sequence. A value of -1
> > > > > +             disables automatic entry into a specific mode, while lower numbers
> > > > > +             indicate higher priority. The default priorities can be modified by
> > > > > +             assigning new values. Modes without explicitly set values default to -1,
> > > > > +             effectively disabling them.
> > > > > +
> > > > > +             Example values:
> > > > > +             - "USB4=0 TBT=1 DP=2"
> > > > > +             - "USB4=-1 TBT=0"
> > > > > +             - "DP=-1 USB4=-1 TBT=-1"
> > > >
> > > > No. If you want to disable entry to a mode by default, then you
> > > > deactivate that mode, so -1 is not needed. USB4 is also not an alt
> > > > mode, so this at the very least should be named differently.
> 
> USB4 should be called an alt-mode though based on its behavior and I
> think the spec makes a mistake not doing so. It's mutually exclusive
> with alternate modes, it modifies the functionality of the data lanes
> and it's not a new data mode (just a tunneling protocol). But that's
> an aside for this patch...

It's still a big no from me for this one as it is, sorry. You are
trying to achieve too many things with these files.

The first thing here is that you want to offer the user a way to
define the order in which the modes are entered, and that's actually
something that we do *not* have any support for at the moment. I'm not
against this part. As long as you justify the need for this, and
ideally offer a way to setup the priorities also in tcpm.c, I'm okay
with it. But you do need to justify the need for this IMO. The specs
actually dictate the order in which these modes should be entered does
it not

But the second part is that you want to also use this file to disable
the modes. For that we do have the mechanisms already in place, so
there is no point in trying to include that here. It does not even
sound like the primary goal for you - your main goal is to just be
able to define the order, right?

So this is my suggestion: Change this so that you can only supply the
preferred order for the modes - the priorities. Get rid of the numeric
values - by simply listing the modes in the file becomes more machine
readable while still being perfectly human readable.

And the name. I agree that USB4 should be an alt mode, but it's not.
This should really be a no-brainer. Just s/altmode/mode/. That way
the naming is aligned with what you are proposing in patch 5/6.

> > > > But I'm not sure this is the correct way to handle the modes in
> > > > general. Can you please explain what exactly is the use case you are
> > > > thinking?
> > >
> > > Hi Heikki,
> > >
> > > This implements the mode selection logic within the kernel, replacing
> > > its userspace counterpart. Implementing this in the kernel offers
> > > advantages, making the process both more robust and far easier to
> > > manage.
> > > This eliminates the need for user space to verify port, partner, or
> > > cable capabilities, and to control the mode entry process. User space
> > > doesn't even need to know if USB4 mode is supported by the port or
> > > partner; unsupported modes are automatically skipped.
> >
> > But that's all things that userspace can do, so it should be doing it.
> > You need to justify why userspace can NOT do something in order to move
> > that logic into the kernel.
> 
> Userspace "can" do it but it is not doing it today. There is no
> userspace alt-mode manager (that I'm aware of) that's writing to
> /sys/class/typec (typecd in ChromeOS only reads and does mode
> entry/exit via a sideband mechanism) and boltd (userspace thunderbolt
> manager; https://github.com/gicmo/bolt) only deals with tunnels and
> not mode entry directly.
> 
> I think the inclusion of TBT and USB4 in the kernel makes this change
> necessary and the existing way of doing it is not widely used yet.
> 
> >
> > > User space's role is now limited to high-level tasks like security,
> > > with the kernel managing technical implementation. Mode selection and
> > > activation can occur without user space intervention at all if no
> > > special rules apply and the default policy (USB4 -> TBT -> DP) is
> > > acceptable.
> >
> > What is wrong with "userspace intervention"?  Is the current api broken
> > somehow?
> 
> There were some previous discussions on this:
> https://lore.kernel.org/linux-usb/ZyjW0CMXgGIt-usC@kuha.fi.intel.com/
> https://lore.kernel.org/chrome-platform/CAA8EJpqiF_0bgYT8boFa4UPJWcxgw89mmfbdMVKeAP-xnFOP4g@mail.gmail.com/
> 
> Some background:
> * The Type-C port driver will add a partner alt-mode to the altmode bus.
> * An altmode driver will probe and attach if available on the system.
>     * Currently, the probe function of these altmode drivers always
> enter the given mode. The expectation is that if a system allows mode
> override, it should always be done by the alt mode driver. This
> doesn't scale beyond one alt-mode driver – between DP and TBT, which
> altmode driver should win entry? Prior to TBT driver being added in
> December and USB4 support circa similar time frame, DP was always the
> default choice if override was supported.
>     * Userspace could make this decision but then what about USB4? The
> current APIs don't expose USB4 information in the same way and the
> auto-enter behavior is different (it doesn't enter).
> 
> In a lot of existing designs, mode entry is entirely handled by the
> firmware and no mode selection is possible from the kernel. This is
> covered by patch 3 of this series (alt_mode_override) flag. Windows
> designs usually fall into this category as they use PD controllers
> implementing UCSI (and usually a very old UCSI version 1.2).
> 
> A few designs allow overriding the current mode:
> * Chromebooks with cros_ec_ucsi which support Thunderbolt will allow
> override of the alternate mode (via UCSI set_new_cam) and USB mode
> (via UCSI Set_USB). They will enter DPAM instead of TBT by default if
> possible.
> * Chromebooks with the cros_ec_typec driver which support Thunderbolt
> will require the OS to make the mode entry decision (no mode entry by
> default on attach).
> * Pixel phone TCPM currently requires mode entry from the kernel as
> there's no userspace mode selection component.
> * Potentially some Windows designs that implement AltMode override via
> UCSI (ucsi_acpi). These should behave similarly to cros_ec_ucsi but
> will likely automatically enter TBT mode.
> 
> In these cases, the kernel is a better place to make alternate mode decisions:
> * Quicker than having a userspace implementation (and more consistent
> during boot).
> * Works consistently across drivers and accounts for both overridable
> and non-overridable systems.
> 
> The main thing userspace should really be doing with mode entry is
> setting policy around what modes they want to enter instead of doing
> low level checking of compatibility and sequencing of mode entry
> operations. And the main policy we're trying to enforce on ChromeOS is
> to not enter TBT by default (based on our security posture described
> at https://www.chromium.org/chromium-os/developer-library/reference/security/usb4/).
> 
> Given that Thunderbolt altmode driver support and USB4 support was
> only added around December of last year, I don't imagine there are
> many userspace usages yet and we should fix these API issues before
> they become too widely used. I think mode sequencing belongs in the
> kernel and not userspace.

My two cents - I'm sure this is clear to most of you, but not to
everybody.

The problem with the USB Type-C framework that I wrote is that it was
primary designed to support tcpm.c, but unfortunately it does not work
very well with the PD controllers when it comes to the alternate modes.

With tcpm.c we need to handle the alternate mode communication inside
kernel, but with PD controller we of course can't do that. The altmode
bus and altmode drivers are not needed with PD controllers, in fact
they only complicate things with the PD controllers.

So there are basically two separate ways of handling the same thing.
That's why I think we may need something like these files that you
guys are suggesting. We need a way to support the controlling of
alternate modes with PD controllers, or perhaps I should say
with interfaces that support it.

But that said, this series is not acceptable as it is quite yet. You
are trying to push a lot of information and control to these couple of
files which I'm not convinced is always necessary, and possibly even
prone to errors. I'm also really concerned that you want to only deal
with a predefined list of modes. It will probable not be a problem as
long as we can fallback to ABI that we already have. But during my
short career I've already seen several cases where assumptions have
been made about how a specific technology will be used, and it has in
practice always backfired.

thanks,

-- 
heikki

Powered by blists - more mailing lists