Re: [PATCH v3 2/6] usb: dwc2: fix a possible interrupt storm in hcd resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: dwc2: unlocked dwc2_hcd_reinit() call in
  _dwc2_hcd_resume()
--

commit 57723fb6e396e93ffaf40174f532484ee2100bb5
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

usb: dwc2: fix a possible interrupt storm in hcd resume

This commit moves set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags) before
dwc2_hcd_reinit() in _dwc2_hcd_resume() so that host interrupts enabled
during reinitialization are not rejected as unhandled.

> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index d5275cd658d4c..28cde15e0cca4 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4469,14 +4469,14 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>  		 */
>  		dwc2_core_init(hsotg, false);
>  		dwc2_enable_global_interrupts(hsotg);
> -		dwc2_hcd_reinit(hsotg);
> -		spin_lock_irqsave(&hsotg->lock, flags);
>  
>  		/*
>  		 * Set HW accessible bit before powering on the controller
>  		 * since an interrupt may rise.
>  		 */
>  		set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +		dwc2_hcd_reinit(hsotg);
> +		spin_lock_irqsave(&hsotg->lock, flags);
>  		break;

[Severity: Medium]
Should spin_lock_irqsave(&hsotg->lock, flags) be acquired before setting
HCD_FLAG_HW_ACCESSIBLE and calling dwc2_hcd_reinit(hsotg)?

dwc2_hcd_reinit() requires interrupts disabled and hsotg->lock held, as
its only other caller _dwc2_hcd_start() does:

drivers/usb/dwc2/hcd.c:dwc2_hcd_reinit() {
/*
 * Initializes dynamic portions of the DWC_otg HCD state
 *
 * Must be called with interrupt disabled and spinlock held
 */
static void dwc2_hcd_reinit(struct dwc2_hsotg *hsotg)
}

With HCD_FLAG_HW_ACCESSIBLE now set before dwc2_hcd_reinit(hsotg) while
interrupts are enabled and hsotg->lock is unlocked, a host interrupt can
enter usb_hcd_irq() -> _dwc2_hcd_irq() -> dwc2_handle_hcd_intr() (or a
concurrent caller can take hsotg->lock) while dwc2_hcd_reinit() is
modifying hsotg->flags.d32, hsotg->free_hc_list, channel states, and host
registers:

_dwc2_hcd_resume()
  dwc2_enable_global_interrupts(hsotg)
  set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags)
  dwc2_hcd_reinit(hsotg)
    list_del_init(&chan->hc_list_entry)
    <hardirq>
      usb_hcd_irq()
        _dwc2_hcd_irq()
          dwc2_handle_hcd_intr()  <-- races on hsotg state and lists
  spin_lock_irqsave(&hsotg->lock, flags)

Neither set_bit() nor dwc2_hcd_reinit() sleeps, so acquiring hsotg->lock
before enabling global interrupts, setting HCD_FLAG_HW_ACCESSIBLE, and
calling dwc2_hcd_reinit() would avoid this race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-dwc2-low-power-fixes-stm32-v3-0-68d2a0e9bab6@foss.st.com?part=2