Re: [PATCH] usb: hub: Fix usb enumeration issue due to address0 race

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 18.11.2021 15.50, Mathias Nyman wrote:
> On 18.11.2021 13.19, Marek Szyprowski wrote:
>> Hi,
>>
>> On 15.11.2021 23:16, Mathias Nyman wrote:
>>> xHC hardware can only have one slot in default state with address 0
>>> waiting for a unique address at a time, otherwise "undefined behavior
>>> may occur" according to xhci spec 5.4.3.4
>>>
>>> The address0_mutex exists to prevent this across both xhci roothubs.
>>>
>>> If hub_port_init() fails, it may unlock the mutex and exit with a xhci
>>> slot in default state. If the other xhci roothub calls hub_port_init()
>>> at this point we end up with two slots in default state.
>>>
>>> Make sure the address0_mutex protects the slot default state across
>>> hub_port_init() retries, until slot is addressed or disabled.
>>>
>>> Note, one known minor case is not fixed by this patch.
>>> If device needs to be reset during resume, but fails all hub_port_init()
>>> retries in usb_reset_and_verify_device(), then it's possible the slot is
>>> still left in default state when address0_mutex is unlocked.
>>>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>
>> This patch landed in linux next-20211118 as commit 6ae6dc22d2d1 ("usb: 
>> hub: Fix usb enumeration issue due to address0 race"). On my test 
>> systems it triggers the following deplock warning during system 
>> suspend/resume cycle:
>>
>> ======================================================
>> WARNING: possible circular locking dependency detected
>> 5.16.0-rc1-00014-g6ae6dc22d2d1 #4126 Not tainted
>> ------------------------------------------------------
>> kworker/u16:8/738 is trying to acquire lock:
>> cf81f738 (hcd->address0_mutex){+.+.}-{3:3}, at: 
>> usb_reset_and_verify_device+0xe8/0x3e4
>>
>> but task is already holding lock:
>> cf80ab3c (&port_dev->status_lock){+.+.}-{3:3}, at: 
>> usb_port_resume+0xa0/0x7e8
>>
> 
> Thanks, I see it now.
> 
> Lock order is:
>   mutex_lock(&port_dev->status_lock)
>     mutex_lock(hcd->address0_mutex)
>     mutex_unlock(hcd->address0_mutex)
>   mutex_unlock(&port_dev->status_lock)
> in hub_port_connect(), usb_port_resume() and usb_reset_device()
> 
> But patch changed the status_lock and address0_mutex lock order in hub_port_connect().
> Lets see if we can take the status_lock a bit earlier in hub_port_connect() to
> solve this.
> 

I can easily reproduce this myself now.
I'll send a patch on top of this one to fix it.
Lockdep warnings are gone for me after applying it,
Also fixes an unbalanced address0_mutex unlock in error codepath.

Grateful if someone else could try it out as well.

Thanks 
-Mathias