| Message ID | PH7PR07MB95385E2766D01F3741D418ABDDC22@PH7PR07MB9538.namprd07.prod.outlook.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] usb: xhci: lack of clearing xHC resources | expand |
On Wed, Feb 26, 2025 at 07:23:16AM +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 host to suspend > - reconnect hub attached to root port > - wake host > > For this scenario during enumeration of USB LS/FS device the Cadence xHC > reports completion error code for xHC commands because the xHC resources > used for devices has not been property released. s/property/properly/ > 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 resources should > be cleared starting form the last usb device in tree toward the root hub. > To fix this issue usbcore driver should call hcd->driver->reset_device > for 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> > > --- > Changelog: > v2: > - Replaced disconnection procedure with releasing only the xHC resources > > drivers/usb/core/hub.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index a76bb50b6202..d3f89528a414 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -6065,6 +6065,36 @@ void usb_hub_cleanup(void) > usb_deregister(&hub_driver); > } /* usb_hub_cleanup() */ > > +/** > + * hub_hc_release_resources - clear resources used by host controller > + * @pdev: pointer to device being released > + * > + * Context: task context, might sleep > + * > + * Function releases the host controller resources in correct order before > + * making any operation on resuming usb device. The host controller resources > + * allocated for devices in tree should be released starting from the last > + * usb device in tree toward the root hub. This function is used only during > + * resuming device when usb device require reinitialization - that is, when > + * flag udev->reset_resume is set. > + * > + * This call is synchronous, and may not be used in an interrupt context. > + */ > +static void hub_hc_release_resources(struct usb_device *udev) > +{ > + struct usb_hub *hub = usb_hub_to_struct_hub(udev); > + struct usb_hcd *hcd = bus_to_hcd(udev->bus); > + int i; > + > + /* Release up resources for all children before this device */ > + for (i = 0; i < udev->maxchild; i++) > + if (hub->ports[i]->child) > + hub_hc_release_resources(hub->ports[i]->child); > + > + if (hcd->driver->reset_device) > + hcd->driver->reset_device(hcd, udev); > +} > + > /** > * usb_reset_and_verify_device - perform a USB port reset to reinitialize a device > * @udev: device to reset (not in SUSPENDED or NOTATTACHED state) > @@ -6131,6 +6161,9 @@ static int usb_reset_and_verify_device(struct usb_device *udev) > > mutex_lock(hcd->address0_mutex); > > + if (udev->reset_resume) > + hub_hc_release_resources(udev); Don't you want to do this before taking the address0_mutex lock? > + > for (i = 0; i < PORT_INIT_TRIES; ++i) { > if (hub_port_stop_enumerate(parent_hub, port1, i)) { > ret = -ENODEV; Doing it this way, you will call hcd->driver->reset_device() multiple times for the same device: once for the hub(s) above the device and then once for the device itself. But since this isn't a hot path, maybe that doesn't matter. Alan Stern
>> 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 host to suspend >> - reconnect hub attached to root port >> - wake host >> >> For this scenario during enumeration of USB LS/FS device the Cadence >> xHC reports completion error code for xHC commands because the xHC >> resources used for devices has not been property released. > >s/property/properly/ > >> 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 resources >> should be cleared starting form the last usb device in tree toward the root >hub. >> To fix this issue usbcore driver should call hcd->driver->reset_device >> for 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> >> >> --- >> Changelog: >> v2: >> - Replaced disconnection procedure with releasing only the xHC >> resources >> >> drivers/usb/core/hub.c | 33 +++++++++++++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index >> a76bb50b6202..d3f89528a414 100644 >> --- a/drivers/usb/core/hub.c >> +++ b/drivers/usb/core/hub.c >> @@ -6065,6 +6065,36 @@ void usb_hub_cleanup(void) >> usb_deregister(&hub_driver); >> } /* usb_hub_cleanup() */ >> >> +/** >> + * hub_hc_release_resources - clear resources used by host controller >> + * @pdev: pointer to device being released >> + * >> + * Context: task context, might sleep >> + * >> + * Function releases the host controller resources in correct order >> +before >> + * making any operation on resuming usb device. The host controller >> +resources >> + * allocated for devices in tree should be released starting from the >> +last >> + * usb device in tree toward the root hub. This function is used only >> +during >> + * resuming device when usb device require reinitialization - that >> +is, when >> + * flag udev->reset_resume is set. >> + * >> + * This call is synchronous, and may not be used in an interrupt context. >> + */ >> +static void hub_hc_release_resources(struct usb_device *udev) { >> + struct usb_hub *hub = usb_hub_to_struct_hub(udev); >> + struct usb_hcd *hcd = bus_to_hcd(udev->bus); >> + int i; >> + >> + /* Release up resources for all children before this device */ >> + for (i = 0; i < udev->maxchild; i++) >> + if (hub->ports[i]->child) >> + hub_hc_release_resources(hub->ports[i]->child); >> + >> + if (hcd->driver->reset_device) >> + hcd->driver->reset_device(hcd, udev); } >> + >> /** >> * usb_reset_and_verify_device - perform a USB port reset to reinitialize a >device >> * @udev: device to reset (not in SUSPENDED or NOTATTACHED state) @@ >> -6131,6 +6161,9 @@ static int usb_reset_and_verify_device(struct >> usb_device *udev) >> >> mutex_lock(hcd->address0_mutex); >> >> + if (udev->reset_resume) >> + hub_hc_release_resources(udev); > >Don't you want to do this before taking the address0_mutex lock? I will move it. > >> + >> for (i = 0; i < PORT_INIT_TRIES; ++i) { >> if (hub_port_stop_enumerate(parent_hub, port1, i)) { >> ret = -ENODEV; > >Doing it this way, you will call hcd->driver->reset_device() multiple times for the >same device: once for the hub(s) above the device and then once for the device >itself. But since this isn't a hot path, maybe that doesn't matter. Yes, it true but the function xhci_discover_or_reset_device which is associated with hcd->driver->reset_device() include the checking whether device is in SLOT_STATE_DISABLED. It allows to detect whether device has been already reset and do not repeat the reset procedure. Thanks Pawel Laszczak > >Alan Stern
Hi Pawel, kernel test robot noticed the following build warnings: [auto build test WARNING on usb/usb-testing] [also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.14-rc4] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Pawel-Laszczak/usb-xhci-lack-of-clearing-xHC-resources/20250226-153837 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing patch link: https://lore.kernel.org/r/PH7PR07MB95385E2766D01F3741D418ABDDC22%40PH7PR07MB9538.namprd07.prod.outlook.com patch subject: [PATCH v2] usb: xhci: lack of clearing xHC resources config: i386-buildonly-randconfig-001-20250227 (https://download.01.org/0day-ci/archive/20250227/202502271523.jt3l4VVu-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250227/202502271523.jt3l4VVu-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202502271523.jt3l4VVu-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/usb/core/hub.c:6084: warning: Function parameter or struct member 'udev' not described in 'hub_hc_release_resources' >> drivers/usb/core/hub.c:6084: warning: Excess function parameter 'pdev' description in 'hub_hc_release_resources' vim +6084 drivers/usb/core/hub.c 6067 6068 /** 6069 * hub_hc_release_resources - clear resources used by host controller 6070 * @pdev: pointer to device being released 6071 * 6072 * Context: task context, might sleep 6073 * 6074 * Function releases the host controller resources in correct order before 6075 * making any operation on resuming usb device. The host controller resources 6076 * allocated for devices in tree should be released starting from the last 6077 * usb device in tree toward the root hub. This function is used only during 6078 * resuming device when usb device require reinitialization - that is, when 6079 * flag udev->reset_resume is set. 6080 * 6081 * This call is synchronous, and may not be used in an interrupt context. 6082 */ 6083 static void hub_hc_release_resources(struct usb_device *udev) > 6084 { 6085 struct usb_hub *hub = usb_hub_to_struct_hub(udev); 6086 struct usb_hcd *hcd = bus_to_hcd(udev->bus); 6087 int i; 6088 6089 /* Release up resources for all children before this device */ 6090 for (i = 0; i < udev->maxchild; i++) 6091 if (hub->ports[i]->child) 6092 hub_hc_release_resources(hub->ports[i]->child); 6093 6094 if (hcd->driver->reset_device) 6095 hcd->driver->reset_device(hcd, udev); 6096 } 6097
BTW, since the patch doesn't touch the xHCI driver, the Subject: shouldn't say "usb: xhci: ...". It would be better to put "usb: hub: ..." On Thu, Feb 27, 2025 at 07:05:17AM +0000, Pawel Laszczak wrote: > >Doing it this way, you will call hcd->driver->reset_device() multiple times for the > >same device: once for the hub(s) above the device and then once for the device > >itself. But since this isn't a hot path, maybe that doesn't matter. > > Yes, it true but the function xhci_discover_or_reset_device which is associated with > hcd->driver->reset_device() include the checking whether device is in SLOT_STATE_DISABLED. > It allows to detect whether device has been already reset and do not repeat the > reset procedure. Okay. Please resubmit the patch with the changes we discussed (and fix the kerneldoc problem pointed out by the kernel build checker). Alan Stern
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index a76bb50b6202..d3f89528a414 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -6065,6 +6065,36 @@ void usb_hub_cleanup(void) usb_deregister(&hub_driver); } /* usb_hub_cleanup() */ +/** + * hub_hc_release_resources - clear resources used by host controller + * @pdev: pointer to device being released + * + * Context: task context, might sleep + * + * Function releases the host controller resources in correct order before + * making any operation on resuming usb device. The host controller resources + * allocated for devices in tree should be released starting from the last + * usb device in tree toward the root hub. This function is used only during + * resuming device when usb device require reinitialization - that is, when + * flag udev->reset_resume is set. + * + * This call is synchronous, and may not be used in an interrupt context. + */ +static void hub_hc_release_resources(struct usb_device *udev) +{ + struct usb_hub *hub = usb_hub_to_struct_hub(udev); + struct usb_hcd *hcd = bus_to_hcd(udev->bus); + int i; + + /* Release up resources for all children before this device */ + for (i = 0; i < udev->maxchild; i++) + if (hub->ports[i]->child) + hub_hc_release_resources(hub->ports[i]->child); + + if (hcd->driver->reset_device) + hcd->driver->reset_device(hcd, udev); +} + /** * usb_reset_and_verify_device - perform a USB port reset to reinitialize a device * @udev: device to reset (not in SUSPENDED or NOTATTACHED state) @@ -6131,6 +6161,9 @@ static int usb_reset_and_verify_device(struct usb_device *udev) mutex_lock(hcd->address0_mutex); + if (udev->reset_resume) + hub_hc_release_resources(udev); + for (i = 0; i < PORT_INIT_TRIES; ++i) { if (hub_port_stop_enumerate(parent_hub, port1, i)) { ret = -ENODEV;
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 host to suspend - reconnect hub attached to root port - wake host For this scenario during enumeration of USB LS/FS device the Cadence xHC reports completion error code for xHC commands because the xHC resources used for devices has not been property released. 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 resources should be cleared starting form the last usb device in tree toward the root hub. To fix this issue usbcore driver should call hcd->driver->reset_device for 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> --- Changelog: v2: - Replaced disconnection procedure with releasing only the xHC resources drivers/usb/core/hub.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)