Re: [PATCH v2 3/3] usb: dwc2: Add USB DRD support on CV1800 series SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] usb: dwc2: BVALOVAL cleared in host mode in
  dwc2_ovr_init()/dwc2_ovr_avalid()
--

commit 5b3a06c8f6671fc6eef64472919667bd9e08400a
Author: Inochi Amaoto <inochiama@gmail.com>

usb: dwc2: Add USB DRD support on CV1800 series SoC

This commit adds a force_bvalid_in_host quirk parameter and enables it on
CV1800 series SoCs so that GOTGCTL_BVALOVAL remains set in host mode to
avoid a persistent Session End Detected interrupt storm.

> diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c
> index 3dc194657e821..3df63c0b88d80 100644
> --- a/drivers/usb/dwc2/drd.c
> +++ b/drivers/usb/dwc2/drd.c
> @@ -28,10 +28,13 @@ static void dwc2_ovr_init(struct dwc2_hsotg *hsotg)
>  	gotgctl = dwc2_readl(hsotg, GOTGCTL);
>  	dwc2_ovr_gotgctl(gotgctl);
>  	gotgctl &= ~(GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL);
> -	if (hsotg->role_sw_default_mode == USB_DR_MODE_HOST)
> +	if (hsotg->role_sw_default_mode == USB_DR_MODE_HOST) {
>  		gotgctl |= GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL;
> -	else if (hsotg->role_sw_default_mode == USB_DR_MODE_PERIPHERAL)
> +		if (hsotg->params.force_bvalid_in_host)
> +			gotgctl |= GOTGCTL_BVALOVAL;
> +	} else if (hsotg->role_sw_default_mode == USB_DR_MODE_PERIPHERAL) {
>  		gotgctl |= GOTGCTL_BVALOVAL | GOTGCTL_VBVALOVAL;
> +	}
>  	dwc2_writel(hsotg, gotgctl, GOTGCTL);
>  
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> @@ -45,18 +48,26 @@ static int dwc2_ovr_avalid(struct dwc2_hsotg *hsotg, bool valid)
>  	u32 gotgctl = dwc2_readl(hsotg, GOTGCTL);
>  
>  	/* Check if A-Session is already in the right state */
> -	if ((valid && (gotgctl & GOTGCTL_ASESVLD)) ||
> +	if ((valid && (gotgctl & GOTGCTL_ASESVLD) &&
> +	     (!hsotg->params.force_bvalid_in_host ||
> +	      (dwc2_is_host_mode(hsotg) && (gotgctl & GOTGCTL_BVALOVAL) &&
> +	       (gotgctl & GOTGCTL_AVALOVAL)))) ||
>  	    (!valid && !(gotgctl & GOTGCTL_ASESVLD)))
>  		return -EALREADY;
>  
>  	/* Always enable overrides to handle the resume case */
>  	dwc2_ovr_gotgctl(gotgctl);
>  
> -	gotgctl &= ~GOTGCTL_BVALOVAL;
> -	if (valid)
> +	if (valid) {
>  		gotgctl |= GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL;
> -	else
> -		gotgctl &= ~(GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL);
> +		if (hsotg->params.force_bvalid_in_host)
> +			gotgctl |= GOTGCTL_BVALOVAL;
> +		else
> +			gotgctl &= ~GOTGCTL_BVALOVAL;
> +	} else {
> +		gotgctl &= ~(GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL |
> +			     GOTGCTL_VBVALOVAL);
> +	}
>  	dwc2_writel(hsotg, gotgctl, GOTGCTL);
>  
>  	return 0;

[Severity: High]
Can dwc2_ovr_avalid(hsotg, false) and dwc2_ovr_init() still clear
GOTGCTL_BVALOVAL while the controller is in host mode, triggering the
GOTGINT_SES_END_DET interrupt storm this patch aims to prevent?

When switching from USB_ROLE_HOST to USB_ROLE_NONE, dwc2_drd_role_sw_set()
calls dwc2_ovr_avalid(hsotg, false) and drops hsotg->lock before
dwc2_force_mode() switches the controller out of host mode:

drivers/usb/dwc2/drd.c:dwc2_drd_role_sw_set() {
	...
		} else {
			dwc2_ovr_avalid(hsotg, false);
		}
	}

	spin_unlock_irqrestore(&hsotg->lock, flags);

	if (!already && hsotg->dr_mode == USB_DR_MODE_OTG)
		/* This will raise a Connector ID Status Change Interrupt */
		dwc2_force_mode(hsotg, role == USB_ROLE_HOST);
}

Because dwc2_ovr_avalid(hsotg, false) clears GOTGCTL_BVALOVAL while the
controller is still in host mode, interrupts are re-enabled at
spin_unlock_irqrestore() before dwc2_force_mode() switches to device mode
(and if dr_mode is USB_DR_MODE_HOST, dwc2_force_mode() is not called at
all).

Similarly, dwc2_ovr_init() forces host mode at the end of the function
whenever hsotg->dr_mode == USB_DR_MODE_HOST or hsotg->role_sw_default_mode
== USB_DR_MODE_HOST, but only sets GOTGCTL_BVALOVAL when
role_sw_default_mode == USB_DR_MODE_HOST. If dr_mode is USB_DR_MODE_HOST
and role_sw_default_mode is not, dwc2_ovr_init() enters host mode with
GOTGCTL_BVALOVAL cleared.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111055628.183891-1-inochiama@gmail.com?part=3