Re: [PATCH v2 2/3] 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:
- [Medium] usb: dwc2: drd: wrong PHY mode for USB_DR_MODE_HOST on
  USB_ROLE_NONE
- [Medium] usb: dwc2: drd: phy_set_mode() called while PHY is exited in
  dwc2_drd_role_sw_set()
--

commit 2436b9203ac01c46908592e593c8d76d66df4918
Author: Inochi Amaoto <inochiama@gmail.com>

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

This commit updates dwc2_drd_init(), dwc2_drd_role_sw_set(), and
dwc2_drd_resume() to synchronize the generic PHY mode with the requested
or default USB role.

> diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c
> index 1ad8fa3f862a1..3dc194657e821 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,13 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
>  			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]
Can this call phy_set_mode() while hsotg->phy is powered off and exited?

In USB_DR_MODE_PERIPHERAL, dwc2_driver_probe() and dwc2_hsotg_udc_stop()
call dwc2_lowlevel_hw_disable(), which runs phy_power_off() and phy_exit()
and clears hsotg->ll_hw_enabled.  When dwc2_drd_role_sw_set() runs before
udc_start (for example when a cable is plugged in at boot), only
hsotg->clk is enabled while hsotg->phy remains uninitialized.

phy_set_mode_ext() invokes phy->ops->set_mode() without checking
phy->init_count, so PHY drivers that gate clocks or assert resets in
phy_exit() (such as phy-meson-gxl-usb2 or phy-ingenic-usb) will perform
unclocked register accesses.  When dwc2_hsotg_udc_start() later calls
dwc2_lowlevel_hw_enable(), phy_init() also resets the PHY without
re-applying the mode.

[ ... ]

> @@ -160,26 +166,40 @@ static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
[ ... ]
>  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->role_sw_default_mode == USB_DR_MODE_HOST;
> +	else
> +		is_host = hsotg->dr_mode == USB_DR_MODE_HOST;
> +
> +	ret = phy_set_mode(hsotg->phy,
> +			   is_host ? PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE);
> +	if (ret)
> +		return dev_err_probe(hsotg->dev, ret,
> +				     "failed to set default PHY mode\n");
[ ... ]
> @@ -230,6 +251,11 @@ void dwc2_drd_resume(struct dwc2_hsotg *hsotg)
>  				role = USB_ROLE_DEVICE;
>  		}
>  
> +		ret = phy_set_mode(hsotg->phy, role == USB_ROLE_HOST ?
> +				   PHY_MODE_USB_HOST : PHY_MODE_USB_DEVICE);
> +		if (ret)
> +			return ret;

[Severity: Medium]
Does this switch hsotg->phy to PHY_MODE_USB_DEVICE on USB_ROLE_NONE when
hsotg->dr_mode is USB_DR_MODE_HOST and role_sw_default_mode is
USB_DR_MODE_UNKNOWN?

When role is USB_ROLE_NONE and role-switch-default-mode is not set, role
stays USB_ROLE_NONE so that A-session valid overrides are not asserted.
Both dwc2_drd_role_sw_set() and dwc2_drd_resume() then pass
PHY_MODE_USB_DEVICE to phy_set_mode() even when hsotg->dr_mode is
USB_DR_MODE_HOST, leaving the PHY in device mode while the controller stays
in host mode.

In addition, if hsotg->dr_mode is USB_DR_MODE_HOST while
role_sw_default_mode is USB_DR_MODE_PERIPHERAL, dwc2_drd_init() sets
is_host to false and configures PHY_MODE_USB_DEVICE, whereas
dwc2_ovr_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));
}

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