Re: [PATCH 0/4] Add device links between tunneled USB3 devices and USB4 Host

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Tue, Jun 25, 2024 at 05:37:27PM +0300, Mathias Nyman wrote:
> On 24.6.2024 7.59, Mika Westerberg wrote:
> > On Fri, Jun 21, 2024 at 11:30:05AM -0500, Mario Limonciello wrote:
> > > On 6/21/2024 01:19, Mika Westerberg wrote:
> > > > Hi Mario,
> > > > 
> > > > On Thu, Jun 20, 2024 at 01:36:56PM -0500, Mario Limonciello wrote:
> > > > > On 6/20/2024 01:41, Mika Westerberg wrote:
> > > > > > +CC Mario from AMD side to check that we are good and don't break
> > > > > > anything accidentally.
> > > > > > 
> > > > > > On Wed, Jun 19, 2024 at 04:03:01PM +0300, Mathias Nyman wrote:
> > > > > > > The relationship between a USB4 Host Interface providing USB3 tunnels,
> > > > > > > and tunneled USB3 devices is not represented in device hierarchy.
> > > > > > > 
> > > > > > > This caused issues with power managment as devices may suspend and
> > > > > > > resume in incorrect order.
> > > > > > > A device link between the USB4 Host Interface and the USB3 xHCI was
> > > > > > > originally added to solve this, preventing the USB4 Host Interface from
> > > > > > > suspending if the USB3 xHCI Host was still active.
> > > > > > > This unfortunately also prevents USB4 Host Interface from suspending even
> > > > > > > if the USB3 xHCI Host is only serving native non-tunneled USB devices.
> > > > > > > 
> > > > > > > Improve the current powermanagement situation by creating device links
> > > > > > > directly from tunneled USB3 devices to the USB4 Host Interface they depend
> > > > > > > on instead of a device link between the hosts.
> > > > > > > This way USB4 host may suspend when the last tunneled device is
> > > > > > > disconnected.
> > > > > > > 
> > > > > > > Intel xHCI hosts are capable of reporting if connected USB3 devices are
> > > > > > > tunneled via vendor specific capabilities.
> > > > > > > Use this until a standard way is available.
> > > > > > > 
> > > > > > > Mathias Nyman (4):
> > > > > > >      xhci: Add USB4 tunnel detection for USB3 devices on Intel hosts
> > > > > > >      usb: Add tunneled parameter to usb device structure
> > > > > > >      usb: acpi: add device link between tunneled USB3 device and USB4 Host
> > > > > > >        Interface
> > > > > > >      thunderbolt: Don't create device link from USB4 Host Interface to USB3
> > > > > > >        xHC host
> > > > > > > 
> > > > > > >     drivers/thunderbolt/acpi.c       | 40 ++++++------------------
> > > > > > >     drivers/usb/core/usb-acpi.c      | 52 ++++++++++++++++++++++++++++++++
> > > > > > >     drivers/usb/host/xhci-ext-caps.h |  5 +++
> > > > > > >     drivers/usb/host/xhci-hub.c      | 29 ++++++++++++++++++
> > > > > > >     drivers/usb/host/xhci.c          | 12 ++++++++
> > > > > > >     drivers/usb/host/xhci.h          |  1 +
> > > > > > >     include/linux/usb.h              |  2 ++
> > > > > > >     7 files changed, 111 insertions(+), 30 deletions(-)
> > > > > > > 
> > > > > > > -- 
> > > > > > > 2.25.1
> > > > > 
> > > > > Hi Mika,
> > > > > 
> > > > > Thanks for looping me in.  Unfortunately with this is appears the XHCI
> > > > > controller link never gets created.  I've not checked functional impact from
> > > > > this, but I'd guess there "should" be some functional problems too.
> > > > 
> > > > Thanks for checking!
> > > > 
> > > > I think the code that sets up the device link based on ->tunneled should
> > > > do that always if the hardware cannot tell if this is native or tunneled
> > > > link to keep the existing functionality and only do the "optimization"
> > > > if the hardware is capable of identifying that.
> > > > 
> > > > Perhaps it can be a callback provided by the xHCI controller driver
> > > > (->is_tunneled()) that then defaults to true if the
> > > > "usb4-host-interface" property is there and in case of Intel hardware
> > > > also checks the proprietary register?
> 
> How about changing the boolean udev->tunneled into a 3 value enum with
> "link_unknown", "link_native", and "link_tunneled" options.
> 
> "link_unknown" would be default, xhci driver only changes this to "link_tunneled" or
> "link_native" if the host can detect tunnels.
> 
> device link to USB4 host would be created for USB3 devices that have
> "usb4-host-interface" property and udev->tunneled != native.

Sounds good to me :)