linux-kernel - Re: [PATCH] usb: hcd: xhci: Add set command timer delay API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d88dbe7e-4558-970d-5601-d4d906829d47@linux.intel.com>
Date:   Tue, 29 Aug 2023 16:57:54 +0300
From:   Mathias Nyman <mathias.nyman@...ux.intel.com>
To:     Hardik Gajjar <hgajjar@...adit-jv.com>
Cc:     gregkh@...uxfoundation.org, mathias.nyman@...el.com,
        stern@...land.harvard.edu, yangyingliang@...wei.com,
        jinpu.wang@...os.com, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, erosca@...adit-jv.com
Subject: Re: [PATCH] usb: hcd: xhci: Add set command timer delay API

On 21.8.2023 12.55, Hardik Gajjar wrote:
> On Fri, Aug 18, 2023 at 04:18:30PM +0300, Mathias Nyman wrote:
>> On 18.8.2023 12.23, Hardik Gajjar wrote:
>>> xHCI driver starts the response timer after sending each
>>> command to the device. The default value of this timer is
>>> 5 seconds (XHCI_CMD_DEFAULT_TIMEOUT = HZ*5). This seems
>>> too high in time crtical use case.
>>>
>>> This patch provides an API to change the default value of
>>> the timer from the vendor USB driver.
>>>
>>> The default value will be XHCI_CMD_DEFAULT_TIMEOUT (5 sec)
>>>
>>> Use case:
>>> According to the Smartphone integration certification
>>> requirement in the automotive, the phone connected via USB
>>> should complete enumeration and user space handshake
>>> within 3 sec.
>>
>> The above incorrectly makes it sound as if the command timeout
>> timer causes the delay.
>>
> Thank you, Mathias, for your prompt response. I will enhance the message
> to provide more specificity in the subsequent patch.
>>>
>>> Reducing the response waiting time by setting the smaller
>>> command timer delay helps to speed up overall re-enumeration
>>> process of the USB device in case of device is not responding
>>> properly in first enumeration iteration.
>>
>> So is this a case where addressing a usb device behind xHC always
>> fail on the first attempt, i.e. address device command in xhci
>> never completes. Solution proposed here is to fail faster and
>> retry?
>>
>> Is the rootcause known why first enumeration fails?
>>
>> Does setting old_scheme_first module parameter help?
>>
> Yes, you are correct. The problem occurs when setting the address,
> and in such cases, this patch helps to fail faster and retry.
> 
> Unfortunately, the root cause is unknown. This problem is mainly
> observed with Android phones. Upon analyzing the USB analyzer logs,
> it seems that the device is not responding to the SET_ADDRESS request.

Is this only seen when connecting the device to a Linux xHCI host at USB 3 speeds?
How about connecting to a windows machine? or USB 2 Linux machine with a EHCI host?

> 
> I tried using "old_scheme_first=Y," but that did not help. Below are
> the short logs of the first iteration enumeration failure.
>>
>> The xhci command timeout is more of a xhci internal thing, not sure it's a good
>> idea to add this to hcd.
>>
>> Would it make sense to add a timeout parameter to hcd->driver->address_device(hcd, udev)
>> instead?
>>
>> First priority should of course be finding out why the first enumeration fails,
>> and solve that.
>>
>> Thanks
>> Mathias
> Thanks for the suggestion to modify hcd->driver->address_device.
> I will definitely implement it.However to confirm.
> 
> So, if I understand correctly, the idea is to avoid exposing any
> API from the xhci driver, but instead, create an interface in hcd.c (such as sysfs or API)
> and incorporate the delay in address_device as an additional parameter.

On second thought it only makes sense to do this if we can identify the device in advance
and make a quirk for it. But at this stage we don't know anything about the device.

So I guess this depends on the width of the problem.

If this works on windows then we need to figure out what we do differently.

If this fails with all hosts (well xHCI and EHCI) then hcd level change is probably needed.
For xhci we would pass timeout parameter when calling address_device, for other hosts
the timeout for the SET_ADDRESS control transfer would be adjusted.

If this only fails when connected behind a xHCI host then a local xHCI change should do.

> However, in that case, modifying xhci is still necessary as the timer is controlled from there.

Yes, xhci changes will be needed.

I suggest adding a timeout parameter to struct xhci_command, and use that when calling
xhci_mod_cmd_timer(). This way we can tailor different timeouts for different commands.

-Mathias