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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `state_lock` acquisition in `port_store` and `eud_remove` introduces data races that can corrupt port state and leave hardware incorrectly enabled.
- [Medium] Missing error rollback in `eud_role_switch_set` leaves EUD hardware incorrectly enabled if the USB controller fails to switch roles.
--

commit 73436063bf4c0e29764237d75663c476d5f5b246
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. Keeping
EUD enabled in other roles provides no benefit and unnecessarily keeps
the associated PHY resources enabled. Furthermore, enabling EUD in host
role can cause the USB controller to misbehave because the EUD hub
supports only a single upstream-facing port.

> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 07522225a8f89..549c0d852bbba 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
[ ... ]
> @@ -156,17 +159,33 @@ static ssize_t enable_store(struct device *dev,
>  		const char *buf, size_t count)
>  {
>  	struct eud_chip *chip = dev_get_drvdata(dev);
> +	struct eud_path *path;
>  	bool enable;
>  	int ret;
>  
>  	if (kstrtobool(buf, &enable))
>  		return -EINVAL;
>  
> +	guard(mutex)(&chip->state_lock);
> +
>  	/* Skip operation if already in desired state */
>  	if (chip->enabled == enable)
>  		return count;
>  
>  	if (enable) {
> +		path = chip->paths[chip->port_idx];
> +
> +		/*
> +		 * If not yet in device role, honor the userspace request and defer
> +		 * EUD enablement until the port transitions to device role in the
> +		 * set_role callback.
> +		 */
> +		if (path->curr_role != USB_ROLE_DEVICE) {

[Severity: High]
Does this code race with port_store() and eud_remove() which do not take the
newly introduced state_lock?

Looking at port_store():

drivers/usb/misc/qcom_eud.c:port_store() {
    ...
	/* Port selection must be done before enabling EUD */
	if (chip->enabled)
		return -EBUSY;

	writel(port, chip->base + EUD_REG_PORT_SEL);
	chip->port_idx = port;
    ...
}

If a concurrent sysfs write to the port attribute changes chip->port_idx
after enable_store() has acquired state_lock and cached the path, wouldn't
enable_store() validate the role for the old port, but call enable_eud() which
will use the new port index locklessly updated by port_store()? This could
power on the PHY for the newly selected port regardless of its role.

Similarly, eud_remove() manipulates hardware state without the state_lock:

drivers/usb/misc/qcom_eud.c:eud_remove() {
    ...
	if (chip->enabled)
		disable_eud(chip);
    ...
}

Could this teardown logic race with ongoing concurrent role-switch operations
and unbalanced PHY power management on module removal?

> +			dev_info(chip->dev, "Deferring EUD enable until port enters device mode\n");
> +			chip->enabled = enable;
> +			return count;
> +		}
> +
>  		ret = enable_eud(chip);
>  		if (ret) {
>  			dev_err(chip->dev, "failed to enable eud\n");
[ ... ]
> @@ -304,7 +323,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);
> +

[Severity: Medium]
If hardware state changes are made here before calling
usb_role_switch_set_role(), what happens if the controller transition fails
below?

> +		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;
> +		}
> +	}
>  
>  	/* 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;
> +	}

If usb_role_switch_set_role() fails, this returns the error immediately without
rolling back the physical state changes made earlier and without updating
path->curr_role.

If a transition to USB_ROLE_DEVICE enabled the EUD, but the controller switch
failed, wouldn't a subsequent switch back to USB_ROLE_HOST skip disabling the
EUD because path->curr_role never became USB_ROLE_DEVICE? Could this permanently
leave the EUD active in host mode, triggering controller misbehavior and leaking
the PHY power?

> +
> +	path->curr_role = role;
> +
> +	return 0;
> +}

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