linux-kernel - Re: [PATCH v2] usb: host: xhci: Avoid XHCI resume delay if SSUSB device is not present

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] [day] [month] [year] [list]

Message-ID: <291f2270-5afc-7570-91cd-049c590b704f@quicinc.com>
Date:   Wed, 13 Sep 2023 14:08:27 -0700
From:   Wesley Cheng <quic_wcheng@...cinc.com>
To:     Mathias Nyman <mathias.nyman@...ux.intel.com>,
        <mathias.nyman@...el.com>, <gregkh@...uxfoundation.org>
CC:     <linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <quic_jackp@...cinc.com>
Subject: Re: [PATCH v2] usb: host: xhci: Avoid XHCI resume delay if SSUSB
 device is not present

Hi Mathias,

On 9/13/2023 7:21 AM, Mathias Nyman wrote:
> Hi
> 
> On 13.9.2023 0.51, Wesley Cheng wrote:
>> Hi Mathias,
>>
>>>> This is one way, but we can probably avoid re-reading all the usb3 
>>>> portsc registers
>>>> by checking if any bit is set in either:
>>>>
>>>>   // bitfield, set if xhci usb3 port neatly set to U3 with a hub 
>>>> request
>>>> xhci->usb3_rhub.bus_state.suspended_ports
>>>>
>>>> // bitfield, set if xhci usb3 port is forced to U3 during xhci suspend.
>>>> xhci->usb3_rhub.bus_state.bus_suspended
>>>>
>>>> But haven't checked this works in all corner cases.
>>>>
>>> Thanks for the suggestion.  I think I also looked at seeing if we 
>>> could use the suspended_ports param, and it was missing one of the 
>>> use cases we had.  I haven't thought on combining it with the 
>>> bus_suspend param also to see if it could work.  Let me give it a 
>>> try, and I'll get back to you.
>>>
>>
>> So in one of our normal use cases, which is to use an USB OTG adapter 
>> with our devices, we can have this connected with no device.  In this 
>> situation, the XHCI HCD and root hub are enumerated, and is in a state 
>> where nothing is connected to the port.  I added a print to the 
>> xhci_resume() path to check the status of "suspended_ports" and 
>> "bus_suspended" and they seem to reflect the same status as when there 
>> is something connected (to a device that supports autosuspend).  
>> Here's some pointers I've found on why these parameters may not work:
>>
>> 1.  bus_suspended is only set (for the bus) if we reach the 
>> bus_suspend() callback from USB HCD if the link is still in U0.  If 
>> USB autosuspend is enabled, then the suspending of the root hub udev, 
>> would have caused a call to suspend the port (usb_port_suspend()), and 
>> that would set "suspended_ports" and placed the link in U3 already.
>>
>> 2. "suspended_ports" can't differentiate if a device is connected or 
>> not after plugging in a USB3 device that has autosuspend enabled.  It 
>> looks like on device disconnection, the suspended_ports isn't cleared 
>> for that port number.  It is only cleared during the resume path where 
>> a get port status is queried:
>>
>> static void xhci_get_usb3_port_status(struct xhci_port *port, u32 
>> *status,
>>                        u32 portsc)
>> {
>> ...
>>       /* USB3 specific wPortStatus bits */
>>       if (portsc & PORT_POWER) {
>>           *status |= USB_SS_PORT_STAT_POWER;
>>           /* link state handling */
>>           if (link_state == XDEV_U0)
>>               bus_state->suspended_ports &= ~(1 << portnum);
>>       }
>>
>> IMO, this seems kind of weird, because the PLS shows that the port is 
>> in the RxDetect state, so it technically isn't suspended.  If you 
>> think we should clear suspended_ports on disconnect, then I think we 
>> can also change the logic to rely on it for avoiding the unnecessary 
>> delay in xhci_resume().
> 
> I think you found a bug.
> 
> We should clear suspended_ports bit if link state in portsc is anything 
> other than U3, Resume or Recovery.
> 
> Not doing so might cause USB_PORT_STAT_C_SUSPEND bit to be set 
> incorrectly in a USB2 get port status request.
> 
> So we want something like:
> if (suspended_ports bit is set) {
>      if (U3 || Resume || Recovery) {
>          don't touch anything
>      } else {
>          clear suspended_port bit
>          if ((U2 || U0) && USB2)
>              set bus_state->port_c_suspend bit
> }
> 
> I'll look into it
> 

Thanks, Mathias.  Will take some time to take a look as well since I 
have a reliable set up that observes this issue.  If you have any test 
code you might want to try, let me know!

Thanks
Wesley Cheng