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

[Mario Limonciello <mario.limonciello@amd.com>]

On 6/25/2024 09:45, Mika Westerberg wrote:
> 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 :)

I think it's a good idea as well.  Could you leave a dynamic debugging 
statement with the result?

I think this could be helpful for rooting out unexpected BIOS problems 
with this.