| Message ID | PH7PR07MB95384002E4FBBC7FE971862FDDFF2@PH7PR07MB9538.namprd07.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: xhci: lack of clearing xHC resources | expand |
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
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
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
> > >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
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
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.
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(-) -- 2.43.0