RE: [PATCH] usb: xhci: lack of clearing xHC resources

[Pawel Laszczak]

Sorry about that. 

I've checked it with checkpatch.pl but I had to forward patch from Outlook.
It reformatted the email.

I sent it again.

Regards,
Pawel

>
>On Thu, Feb 13, 2025 at 10:27:00AM +0000, Pawel Laszczak wrote:
>> The xHC resources allocated for USB devices are not released in correct order
>after resuming in case when while suspend device was reconnected.
>
>Please wrap your changelog text properly, checkpatch.pl should have
>caught this, did you forget to run it?
>
>>
>> This issue has been detected during the fallowing scenario:
>> - connect hub HS to root port
>> - connect LS/FS device to hub port
>> - wait for enumeration to finish
>> - force DUT to suspend
>> - reconnect hub attached to root port
>> - wake DUT
>>
>> For this scenario during enumeration of USB LS/FS device the Cadence xHC
>reports completion error code for xHCi commands because the devices was not
>property disconnected and in result the xHC resources has not been correct
>freed.
>> XHCI specification doesn't mention that device can be reset in any order so,
>we should not treat this issue as Cadence xHC controller bug.
>
>But if it operates unlike all other xhci controllers, isn't that a bug
>on its side?
>
>> Similar as during disconnecting in this case the device should be cleared
>starting form the last usb device in tree toward the root hub.
>> To fix this issue usbcore driver should disconnect all USB devices connected
>to hub which was reconnected while suspending.
>>
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP
>DRD Driver")
>> cc: <stable@xxxxxxxxxxxxxxx>
>> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
>> ---
>>  drivers/usb/core/hub.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index
>0cd44f1fd56d..2473cbf317a8 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -3627,10 +3627,12 @@ static int finish_port_resume(struct usb_device
>*udev)
>>  		 * the device will be rediscovered.
>>  		 */
>>   retry_reset_resume:
>> -		if (udev->quirks & USB_QUIRK_RESET)
>> +		if (udev->quirks & USB_QUIRK_RESET) {
>>  			status = -ENODEV;
>> -		else
>> +		} else {
>> +			hub_disconnect_children(udev);
>
>This feels odd, and will hit more than just xhci controllers, right?
>You aren't really disconnecting the hub, only resetting it (well the
>logical disconnect will cause a real disconnect later on, so this should
>be called from that code path, right?
>
>thanks,
>
>greg k-h