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

[Inochi Amaoto <inochiama@gmail.com>]

On Tue, Nov 10, 2026 at 07:18:48AM +0000, sashiko-bot@kernel.org wrote:
> 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).
> 

Yes, the check can be removed

> [ ... ]
> 
> > @@ -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));
> }
> 

Yes, it is inverted.

I will fix them in the next version.

Regards,
Inochi

> [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