Re: [PATCH v2 3/7] usb: typec: Auto enter control for alternate modes

[Abhishek Pandit-Subedi]

On Fri, Nov 1, 2024 at 6:59 AM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, Oct 31, 2024 at 03:48:45PM -0700, Abhishek Pandit-Subedi wrote:
> > On Thu, Oct 31, 2024 at 7:33 AM Heikki Krogerus
> > <heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Oct 30, 2024 at 02:28:34PM -0700, Abhishek Pandit-Subedi wrote:
> > > > Add controls for whether an alternate mode is automatically entered when
> > > > a partner connects. The auto_enter control is only available on ports
> > > > and applies immediately after a partner connects. The default behavior
> > > > is to enable auto enter and drivers must explicitly disable it.
> > > >
> > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > >  Documentation/ABI/testing/sysfs-bus-typec |  9 +++++++
> > > >  drivers/usb/typec/altmodes/displayport.c  |  6 +++--
> > > >  drivers/usb/typec/altmodes/thunderbolt.c  |  3 ++-
> > > >  drivers/usb/typec/class.c                 | 31 +++++++++++++++++++++++
> > > >  include/linux/usb/typec.h                 |  2 ++
> > > >  include/linux/usb/typec_altmode.h         |  2 ++
> > > >  6 files changed, 50 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-typec b/Documentation/ABI/testing/sysfs-bus-typec
> > > > index 205d9c91e2e1..f09d05727b82 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-typec
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-typec
> > > > @@ -12,6 +12,15 @@ Description:
> > > >
> > > >               Valid values are boolean.
> > > >
> > > > +What:                /sys/bus/typec/devices/.../auto_enter
> > > > +Date:                September 2024
> > > > +Contact:     Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > > +Description:
> > > > +             Controls whether a mode will be automatically entered when a partner is
> > > > +             connected.
> > > > +
> > > > +             This field is only valid and displayed on a port. Valid values are boolean.
> > >
> > > So, why can't this be controlled with the "active" property of the
> > > port altmode instead? That's why it's there.
> > >
> > > Sorry if I missed something in v1 related to this question.
> >
> > There was a bit of discussion around this in another patch in v1:
> > https://patchwork.kernel.org/project/chrome-platform/patch/20240925092505.8.Ic14738918e3d026fa2d85e95fb68f8e07a0828d0@changeid/
> > And this patch is probably a good place to continue that discussion.
> >
> > With the way altmodes drivers currently work, they will auto-enter
> > when probed. So if you have a partner that supports both displayport
> > and thunderbolt, they will both attempt to auto-enter on probe. I
> > think I could use the `active` field instead so that the port altmode
> > blocks entry until userspace enables it -- this would avoid the need
> > to add one more sysfs ABI. I'll actually go ahead and do this for the
> > next patch series I send up.
> >
> > However, the underlying problem I'm trying to solve still exists: how
> > do you choose a specific altmode to enter if there are multiple to
> > choose from? I tried to implement a method that first tries USB4 and
> > then Thunderbolt and then DP but I realized that the altmode drivers
> > don't necessarily bind immediately after a partner altmode is
> > registered so I can't just call `activate` (since no ops are attached
> > to the partner altmode yet). Do you have any thoughts about how to
> > handle multiple modes as well as how to handle fallback mode entry
> > (i.e. thunderbolt fails so you try DPAM next)?
>
> If the user space needs to take over control of the entry order, then
> can't it just de-activate all port alt modes by default, and then
> activate the one that needs to enter? The port driver probable needs
> to implent the "activate" callback for this.
>
> The user space can see when the driver is bound to a device by
> monitoring the uevents, no?

This requires userspace intervention to do the correct thing. Let's
take a real world example:

I have a TBT4 dock that supports DPAM (svid 0xff01), TBT (svid 0x8087)
and also USB4.

* When I plug in the dock, it enumerates and registers the partner
altmodes. The altmode bus matches typec_displayport and
typec_thunderbolt and loads the drivers for them. By default, both
drivers will try to activate their altmode on probe(). Having a
userspace daemon disable the altmode on the ports and enable them on
connection will probably work here.
* If I boot with the dock connected, the same thing happens but my
userspace daemon may not be running yet. What should the default
kernel behavior be to enter alt-modes then? When you throw USB4 into
the mix, this becomes another can of worms since you probably don't
want to downgrade from USB4 to DPAM.

On ChromeOS, prior to this patch series, our userspace daemon (typecd)
could handle all of this in userspace since it could just wait for
`num_alt_modes` to be filled on partner-attach before trying to enter
the right mode (via a side-band channel to the EC). After this change,
typecd will be in a similar bind -- it will have to wait until all
attached partner SVIDs have drivers attached (if available).

Underlying all this, I guess the real need here is for some sort of
signal that says: All partner modes are registered, any necessary
drivers for these modes are bound and you are ready to make a decision
on which mode to enter. Then, we could iteratively try to enter the
best mode (USB4 > TBT > DP) and report failure conditions on why it
couldn't be entered (i.e. Cable speed, partner problem / link
training, etc). This could be done in kernel or userspace depending on
the system.

>
> thanks,
>
> --
> heikki