Re: [PATCH v5 4/9] usb: misc: qcom_eud: add per-path role switch support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing synchronization in sysfs handlers enable_store and port_store causes PHY reference count corruption and hardware desynchronization.
- [High] EUD remains physically enabled when the role switches to HOST, breaking host mode.

Pre-existing issues:
- [High] Sysfs enable_store fails to clear chip->enabled on disable, permanently locking port_store.
--

--- Patch [4]: [PATCH v5 4/9] usb: misc: qcom_eud: add per-path role switch support ---
commit b7772664d1b8d3bcc67e85e648539118fa47a9cd
Author: Elson Serrao <elson.serrao@oss.qualcomm.com>

usb: misc: qcom_eud: add per-path role switch support

This commit updates the EUD driver to support per-path role switches instead
of a single chip-level switch. It allows each path to handle independent role
management by relaying requests from the connector to the controller. The
unnecessary role switch call from enable_eud() is also removed.

> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 03d1c200658c8..48a4441b0c9c0 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c

[ ... ]

> @@ -286,17 +292,39 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static void eud_role_switch_release(void *data)
> +static int eud_role_switch_set(struct usb_role_switch *sw, enum usb_role role)
>  {
> -	struct eud_chip *chip = data;
> +	struct eud_path *path = usb_role_switch_get_drvdata(sw);
> +	int 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;
> +	}
> +
> +	path->curr_role = role;
> +
> +	return 0;
> +}

[Severity: High]
Does the EUD remain physically enabled when the role switches to USB_ROLE_HOST?

If a role switch to USB_ROLE_HOST occurs, eud_role_switch_set() forwards the
request to the USB controller but doesn't appear to dynamically disable the
EUD hardware itself. Since EUD is only usable in device role, would keeping it
enabled during host role disrupt the UTMI lines and break the host connection?

[Severity: High]
Could the missing synchronization between the sysfs handlers enable_store()
and port_store() cause PHY reference count corruption?

Looking at drivers/usb/misc/qcom_eud.c, if Thread A executes enable_store()
while Thread B concurrently executes port_store(), port_store() can modify
chip->port_idx locklessly:

Thread B in port_store():
    chip->port_idx = port;

Thread A in enable_store() would then execute enable_eud() without seeing the
updated port_idx or while it is changing:

Thread A in enable_store():
    ret = enable_eud(chip);
    if (!ret)
        chip->enabled = enable;

Because neither function holds a lock, port_store() can bypass the
chip->enabled check. This might cause eud_phy_enable() to power on the PHY
for the new port. When disable_eud() is later called, could it disable the
wrong PHY and leak the power-on state for the old PHY?

[Severity: High]
This is a pre-existing issue, but does enable_store() fail to clear the
enabled flag on disable?

Looking at enable_store() in drivers/usb/misc/qcom_eud.c:

enable_store() {
    ...
    } else {
        ret = disable_eud(chip);
    }

    return ret < 0 ? ret : count;
}

If a user writes 0 to the enable sysfs attribute, this calls disable_eud(chip)
but doesn't set chip->enabled = false. Could this permanently lock
port_store(), since subsequent writes will unconditionally fail with -EBUSY
when checking if (chip->enabled)?

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