Re: [PATCH v9 6/8] usb: misc: qcom_eud: add role-based EUD control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: qcom_eud: missing hardware and state rollback on role switch failure
--

commit ac5ffb8b40f5366751bb4683118798fac6f6bb75
Author: Elson Serrao <elson.serrao@oss.qualcomm.com>

usb: misc: qcom_eud: add role-based EUD control

EUD functions by presenting itself as a USB device to the host PC and is
therefore only applicable when the port operates in device role. This patch
addresses this by treating EUD as usable only in device role, enabling or
disabling it as the port enters or leaves device role.

> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index e227d05cfecd4..5a21a2dd0b0cd 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c

[ ... ]

> @@ -306,7 +331,30 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>  static int eud_role_switch_set(struct usb_role_switch *sw, enum usb_role role)
>  {
>  	struct eud_path *path = usb_role_switch_get_drvdata(sw);
> -	int ret;
> +	struct eud_chip *chip = path->chip;
> +	int ret = 0;
> +
> +	guard(mutex)(&chip->state_lock);
> +
> +	/*
> +	 * EUD is usable only in device role. Power it down for every other
> +	 * role to avoid keeping an unusable module 'ON'. chip->enabled
> +	 * preserves user's sysfs configuration and is not modified across
> +	 * role transitions.
> +	 */
> +	if (chip->enabled && path->num == chip->port_idx && role != path->curr_role) {
> +		if (role == USB_ROLE_DEVICE)
> +			ret = enable_eud(chip);
> +		else if (path->curr_role == USB_ROLE_DEVICE)
> +			ret = disable_eud(chip);
> +
> +		if (ret) {
> +			dev_err(chip->dev, "failed to %s EUD for role %s: %d\n",
> +				str_enable_disable(role == USB_ROLE_DEVICE),
> +				usb_role_string(role), ret);
> +			return ret;
> +		}
> +	}
>  
>  	/* curr_role tracks the role from EUD's point of view */
>  	path->curr_role = role;
> 
>  	/* Forward the role request to the USB controller */
>  	ret = usb_role_switch_set_role(path->controller_sw, role);
>  	if (ret) {
>  		dev_err(path->chip->dev, "failed to set role %s for port %u: %d\n",
>  			usb_role_string(role), path->num, ret);
>  		return ret;
>  	}

[Severity: High]
Does this error path leave the hardware and state permanently desynchronized
if the downstream controller switch fails?

If a role switch to USB_ROLE_DEVICE is requested, enable_eud() succeeds and
path->curr_role is updated.  Then usb_role_switch_set_role() is called to
forward the switch to the controller.

If this downstream switch fails and returns an error, the function returns
early without reverting path->curr_role or powering down the EUD via
disable_eud().

As noted in the commit message, enabling EUD in host role can cause the USB
controller to misbehave. Could this sequence leave the EUD hardware enabled
while the underlying USB controller remains in its previous role?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929213513.2401005-1-elson.serrao@oss.qualcomm.com?part=6