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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] usb: dwc2: BVALOVAL cleared in host mode in
  dwc2_ovr_init()/dwc2_ovr_avalid()
- [High] usb: dwc2: inverted role_sw_default_mode check in
  dwc2_drd_init()
--

commit 46e81140c7728597dbfee9abd2cadd6fb5aee6a2
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 to keep the BVALID
override asserted in host mode and enables it on Sophgo CV1800 series SoCs
to avoid a persistent Session End Detected interrupt.

> diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c
> index 1ad3804cbc805..4f7441a87c857 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);

[Severity: High]
Are the if/else branches inverted for the role_sw_default_mode check in
dwc2_drd_init() introduced by commit "usb: dwc2: drd: Sync the generic PHY
mode with role requests"?

drivers/usb/dwc2/drd.c:dwc2_drd_init() {
    ...
	if (hsotg->role_sw_default_mode != USB_DR_MODE_UNKNOWN)
		is_host = hsotg->dr_mode == USB_DR_MODE_HOST;
	else
		is_host = hsotg->role_sw_default_mode == USB_DR_MODE_HOST;

	ret = phy_set_mode(hsotg->phy,
			   is_host ? PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE);
    ...
}

When dr_mode is USB_DR_MODE_OTG and role_sw_default_mode is
USB_DR_MODE_HOST, is_host evaluates to false and phy_set_mode() puts the
PHY into PHY_MODE_USB_DEVICE, while dwc2_ovr_init() then configures
GOTGCTL and forces the controller into host mode.  Likewise, when
role_sw_default_mode is USB_DR_MODE_UNKNOWN, the else branch compares
USB_DR_MODE_UNKNOWN against USB_DR_MODE_HOST, so is_host is false even
when dr_mode is USB_DR_MODE_HOST.

> @@ -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 leave
GOTGCTL_BVALOVAL cleared while the controller is in host mode when
force_bvalid_in_host is set?

In dwc2_drd_role_sw_set(), switching from USB_ROLE_HOST to USB_ROLE_NONE
calls dwc2_ovr_avalid(hsotg, false) and clears GOTGCTL_BVALOVAL while the
controller is still in host mode, then drops hsotg->lock and re-enables
interrupts before dwc2_force_mode() switches the controller to device 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);
    ...
}

On CV1800, having GOTGCTL_BVALOVAL cleared with GOTGCTL_BVALOEN enabled
while in host mode triggers the un-clearable GOTGINT_SES_END_DET interrupt
storm before dwc2_force_mode() completes (or permanently if dr_mode is
USB_DR_MODE_HOST).

Similarly, dwc2_ovr_init() forces host mode when either dr_mode or
role_sw_default_mode is USB_DR_MODE_HOST, but only sets GOTGCTL_BVALOVAL
when role_sw_default_mode is USB_DR_MODE_HOST, leaving GOTGCTL_BVALOVAL
cleared in host mode if dr_mode is USB_DR_MODE_HOST and
role_sw_default_mode is not.

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