Re: [PATCH V3] xhci: Reset TI USB controller on xhci shutdown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] xhci: uninitialized variable err in xhci_shutdown()
- [Low] xhci: build failure in xhci_shutdown() when CONFIG_PCI is disabled
--

commit b61fa6ad71541414d2813b27997006fef547aa43
Author: Wen Xiong <wenxiong@linux.ibm.com>

xhci: Reset TI USB controller on xhci shutdown

This commit adds an XHCI_RESET_ON_SHUTDOWN quirk for TI 0x8241 xHCI PCI
controllers and calls pci_reset_function_locked() in xhci_shutdown() so the
controller can initialize properly in a kexec-booted kernel.

> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index a9e47e178c280..8e5e25ebb6b5e 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -750,6 +750,7 @@ EXPORT_SYMBOL_GPL(xhci_stop);
>  void xhci_shutdown(struct usb_hcd *hcd)
>  {
>  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	int err;
>  
>  	if (xhci->quirks & XHCI_SPURIOUS_REBOOT)
>  		usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev));
> @@ -781,6 +782,13 @@ void xhci_shutdown(struct usb_hcd *hcd)
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
>  			"xhci_shutdown completed - status = %x",
>  			readl(&xhci->op_regs->status));
> +
> +	/*
> +	 * TI xHCI controllers fail to initialize in a kexec
> +	 * kernel without a PCI function reset
> +	 */
> +	if (err && (xhci->quirks & XHCI_RESET_ON_SHUTDOWN))

[Severity: High]
Is err used uninitialized here in xhci_shutdown() in drivers/usb/host/xhci.c?

In xhci_shutdown(), err is declared on the stack without an initializer and is
never assigned a value before being checked in:

	if (err && (xhci->quirks & XHCI_RESET_ON_SHUTDOWN))
		pci_reset_function_locked(to_pci_dev(hcd->self.sysdev));

When system shutdown or a kexec reboot runs:

  device_shutdown()
    -> pci_device_shutdown()
      -> usb_hcd_pci_shutdown()
        -> xhci_pci_shutdown()
          -> xhci_shutdown()

this reads uninitialized stack memory (and triggers an uninitialized variable
warning at build time), and when stack variables are zero-initialized,
pci_reset_function_locked() will be skipped completely.

Should err be dropped so the condition only checks xhci->quirks &
XHCI_RESET_ON_SHUTDOWN?

> +		pci_reset_function_locked(to_pci_dev(hcd->self.sysdev));

[Severity: Low]
Will building drivers/usb/host/xhci.c fail when CONFIG_USB_XHCI_HCD is enabled
and CONFIG_PCI is disabled?

In include/linux/pci.h, pci_reset_function_locked() is only declared inside
the #ifdef CONFIG_PCI block and does not have a stub when CONFIG_PCI is
disabled:

  #ifdef CONFIG_PCI
  ...
  int __pci_reset_function_locked(struct pci_dev *dev);
  int pci_reset_function(struct pci_dev *dev);
  int pci_reset_function_locked(struct pci_dev *dev);

Since drivers/usb/host/xhci.c can be built without CONFIG_PCI, calling
pci_reset_function_locked() directly in xhci_shutdown() causes an implicit
function declaration error on non-PCI configurations.

Would it be better to perform this PCI function reset in xhci_pci_shutdown()
in drivers/usb/host/xhci-pci.c alongside the existing PCI-specific shutdown
handling?

>  }
>  EXPORT_SYMBOL_GPL(xhci_shutdown);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021160608.8229-1-wenxiong@linux.ibm.com?part=1