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

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

[Pawel Laszczak]

The xHC resources allocated for USB devices are not released in correct order after resuming in case when while suspend device was reconnected.

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.
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@vger.kernel.org>
Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 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);
 			status = usb_reset_and_verify_device(udev);
+		}
 	}
 
 	/* 10.5.4.5 says be sure devices in the tree are still there.
--
2.43.0

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

[gregkh]

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@vger.kernel.org>
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  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

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

[Pawel Laszczak]

The xHC resources allocated for USB devices are not released in correct
order after resuming in case when while suspend device was reconnected.

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.
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@vger.kernel.org>
Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 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);
 			status = usb_reset_and_verify_device(udev);
+		}
 	}
 
 	/* 10.5.4.5 says be sure devices in the tree are still there.
-- 
2.43.0

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@vger.kernel.org>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> ---
>>  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

Re: FW: [PATCH] usb: xhci: lack of clearing xHC resources

[gregkh]

On Thu, Feb 13, 2025 at 10:46:06AM +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.
> 
> 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.
> 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@vger.kernel.org>
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  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);
>  			status = usb_reset_and_verify_device(udev);
> +		}
>  	}
>  
>  	/* 10.5.4.5 says be sure devices in the tree are still there.
> -- 
> 2.43.0
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: FW: [PATCH] usb: xhci: lack of clearing xHC resources

[gregkh]

On Thu, Feb 13, 2025 at 10:46:06AM +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.
> 
> 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.
> 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@vger.kernel.org>
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>  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);
>  			status = usb_reset_and_verify_device(udev);
> +		}
>  	}
>  
>  	/* 10.5.4.5 says be sure devices in the tree are still there.
> -- 
> 2.43.0
> 
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

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

[stern]

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.
> 
> 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

DUT refers to the host, not the LS/FS device plugged into the hub, is 
that right?

> 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.
> 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.

No, that's not right at all.  We do not want to disconnect these devices 
if there's any way to avoid it.

There must be another way to tell the host controller to release the 
devices' resources.  Doesn't the usb_reset_and_verify_device() call do 
something like that anyway?  After all, the situation should be very 
similar to what happens when a device is simply reset.

Alan Stern

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

[Pawel Laszczak]

>
>
>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.
>>
>> 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
>
>DUT refers to the host, not the LS/FS device plugged into the hub, is that
>right?

Yes DUT refers to the HOST.

>
>> 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.
>> 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.
>
>No, that's not right at all.  We do not want to disconnect these devices if
>there's any way to avoid it.
>
>There must be another way to tell the host controller to release the devices'
>resources.  Doesn't the usb_reset_and_verify_device() call do something like
>that anyway?  After all, the situation should be very similar to what happens
>when a device is simply reset.
>
>Alan Stern


Yes, I had such idea too, but the current solution is simpler.
I don't understand why in this case we can't do disconnect
The hub connected to host was physically disconnected during suspend, so 
It seems quite logic to make disconnection. 
Can you comment why we should not make disconnection?

Thanks,
Pawel

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

[stern]

On Mon, Feb 17, 2025 at 06:25:35AM +0000, Pawel Laszczak wrote:
> >> 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.
> >> 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.
> >
> >No, that's not right at all.  We do not want to disconnect these devices if
> >there's any way to avoid it.
> >
> >There must be another way to tell the host controller to release the devices'
> >resources.  Doesn't the usb_reset_and_verify_device() call do something like
> >that anyway?  After all, the situation should be very similar to what happens
> >when a device is simply reset.
> >
> >Alan Stern
> 
> 
> Yes, I had such idea too, but the current solution is simpler.
> I don't understand why in this case we can't do disconnect
> The hub connected to host was physically disconnected during suspend, so 
> It seems quite logic to make disconnection. 
> Can you comment why we should not make disconnection?

Imagine that there is a disk drive plugged into the hub, and the drive 
is mounted with various files open or being written when the system gets 
suspended.  While the system is asleep, the user unplugs the hub but 
then plugs it back in before the system resumes.

Under those circumstances, the user expects that the drive will remain 
mounted, the files will remain open, and there won't be any data 
corruption on the drive when the system starts running again.  But if we 
said that all the devices below the hub were disconnected then none of 
those things would happen and the user would lose data.

Note that the same sort of thing can happen on some systems even if the 
hub does not get unplugged, because these systems do not provide suspend 
power to their USB controllers.  In addition, the same sort of thing 
happens every time a system goes into S4 hibernation.  You wouldn't want 
to force all users to unmount their USB drives whenever they hibernate 
their systems, would you?

This is why we should avoid saying that devices were disconnected during 
suspend.

Alan Stern