Re: [PATCH 2/4] usb: dwc2: drd: Sync the generic PHY mode with role requests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] usb: dwc2: drd: inverted default mode check in dwc2_drd_init()
- [Medium] usb: dwc2: drd: unupdated PHY mode on USB_ROLE_NONE in
  dwc2_drd_role_sw_set()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] usb: dwc2: early role switch registration race in
  dwc2_driver_probe() [1]
--

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

usb: dwc2: drd: Sync the generic PHY mode with role requests

This commit updates the generic PHY mode via phy_set_mode() during DRD
initialization, role switch updates, and system resume.

> diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c
> index 1ad8fa3f862a1..1ad3804cbc805 100644
> --- a/drivers/usb/dwc2/drd.c
> +++ b/drivers/usb/dwc2/drd.c

[ ... ]

> @@ -111,14 +113,11 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
>  	 * mode and sessions. It is the case if cable is plugged at boot.
>  	 */
>  	if (!hsotg->ll_hw_enabled && hsotg->clk) {
> -		int ret = clk_prepare_enable(hsotg->clk);
> -
> +		ret = clk_prepare_enable(hsotg->clk);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	spin_lock_irqsave(&hsotg->lock, flags);
> -
>  	if (role == USB_ROLE_NONE) {
>  		/* default operation mode when usb role is USB_ROLE_NONE */
>  		if (hsotg->role_sw_default_mode == USB_DR_MODE_HOST)
> @@ -127,6 +126,15 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
>  			role = USB_ROLE_DEVICE;
>  	}
>  
> +	if (role == USB_ROLE_HOST || role == USB_ROLE_DEVICE) {
> +		ret = phy_set_mode(hsotg->phy, role == USB_ROLE_HOST ?
> +				   PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE);
> +		if (ret)
> +			goto out_clk;
> +	}

[Severity: Medium]
Does guarding phy_set_mode() on USB_ROLE_HOST || USB_ROLE_DEVICE leave
hsotg->phy stuck in PHY_MODE_USB_HOST when switching from USB_ROLE_HOST to
USB_ROLE_NONE?

When hsotg->role_sw_default_mode is USB_DR_MODE_UNKNOWN and hsotg->dr_mode
is USB_DR_MODE_OTG, role remains USB_ROLE_NONE.  This skips phy_set_mode(),
whereas dwc2_force_mode(hsotg, role == USB_ROLE_HOST) below forces the
controller into device mode (matching dwc2_drd_init() and
dwc2_drd_resume(), which both set PHY_MODE_USB_DEVICE for USB_ROLE_NONE).

[ ... ]

> @@ -160,26 +168,40 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
>  		/* This will raise a Connector ID Status Change Interrupt */
>  		dwc2_force_mode(hsotg, role == USB_ROLE_HOST);

[Severity: High]
This problem wasn't introduced by this patch, but registering the role
switch in dwc2_drd_init() before dwc2_driver_probe() finishes initializing
the core and workqueue allows concurrent role switches to race with probe
[1]:

CPU0 (probe)                      CPU1 (role switch / IRQ)
dwc2_driver_probe()
  dwc2_drd_init()
    usb_role_switch_register()
                                  dwc2_drd_role_sw_set()
                                    dwc2_force_mode()
                                      /* unlocked RMW on GUSBCFG */
  dwc2_core_init()
    /* RMW on GUSBCFG */
                                  dwc2_handle_conn_id_status_change_intr()
                                    if (hsotg->wq_otg) /* NULL */
                                      queue_work(...)  <-- event dropped

dwc2_force_mode() modifies GUSBCFG without holding hsotg->lock, racing with
dwc2_core_init().  In addition, because hsotg->wq_otg is not allocated yet,
dwc2_handle_conn_id_status_change_intr() drops the connector ID status
change interrupt, leaving the controller in a mismatched state.

>  
> -	if (!hsotg->ll_hw_enabled && hsotg->clk)
> -		clk_disable_unprepare(hsotg->clk);
> -
>  	dev_dbg(hsotg->dev, "%s-session valid\n",
>  		role == USB_ROLE_NONE ? "No" :
>  		role == USB_ROLE_HOST ? "A" : "B");
>  
> -	return 0;
> +out_clk:
> +	if (!hsotg->ll_hw_enabled && hsotg->clk)
> +		clk_disable_unprepare(hsotg->clk);
> +
> +	return ret;
>  }
>  
>  int dwc2_drd_init(struct dwc2_hsotg *hsotg)
>  {
>  	struct usb_role_switch_desc role_sw_desc = {0};
>  	struct usb_role_switch *role_sw;
> +	bool is_host;
>  	int ret;
>  
>  	if (!device_property_read_bool(hsotg->dev, "usb-role-switch"))
>  		return 0;
>  
>  	hsotg->role_sw_default_mode = usb_get_role_switch_default_mode(hsotg->dev);
> +
> +	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);

[Severity: High]
Are the if and else branches inverted here?

When role_sw_default_mode is configured (for example USB_DR_MODE_HOST on an
OTG controller where dr_mode is USB_DR_MODE_OTG), the if branch ignores
role_sw_default_mode and checks dr_mode instead.  When role_sw_default_mode
is USB_DR_MODE_UNKNOWN, the else branch evaluates USB_DR_MODE_UNKNOWN ==
USB_DR_MODE_HOST, which is always false.  This sets hsotg->phy to
PHY_MODE_USB_DEVICE while dwc2_ovr_init() at the end of dwc2_drd_init()
forces the controller into host mode:

drivers/usb/dwc2/drd.c:dwc2_ovr_init() {
    ...
	dwc2_force_mode(hsotg, (hsotg->dr_mode == USB_DR_MODE_HOST) ||
				(hsotg->role_sw_default_mode == USB_DR_MODE_HOST));
}

[1] https://sashiko.dev/#/bug/linux-a825f666-6a3b-40af-8377-023f8afa7caf

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