Re: [PATCH v6 3/9] usb: misc: qcom_eud: add per-path High-Speed PHY control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Lack of locking in sysfs handlers allows concurrent execution and TOCTOU races, leading to unbalanced PHY operations and driver state corruption.
--

--- Patch [3]: [PATCH v6 3/9] usb: misc: qcom_eud: add per-path High-Speed PHY control ---
commit 0126e533bbae398f1f9f6831389bcefdb1ad1252
Author: Elson Serrao <elson.serrao@oss.qualcomm.com>

usb: misc: qcom_eud: add per-path High-Speed PHY control

This patch restructures the EUD driver to implement per-path PHY management,
powering the appropriate PHY based on the selected UTMI path. This ensures
the PHY is explicitly initialized and powered when EUD is enabled.

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

[ ... ]

> @@ -130,6 +192,10 @@ static ssize_t port_store(struct device *dev, struct device_attribute *attr,
>  	if (port < 0)
>  		return port;
>  
> +	/* Check if the corresponding path is available */
> +	if (!chip->paths[port])
> +		return -EOPNOTSUPP;
> +
>  	/* Port selection must be done before enabling EUD */
>  	if (chip->enabled)
>  		return -EBUSY;

[Severity: High]
Is there a lock missing here in port_store() to synchronize with enable_store()?

Looking at enable_store(), it uses guard(mutex)(&chip->state_lock) before
checking chip->enabled, but port_store() appears to check chip->enabled and
mutate chip->port_idx without holding this lock.

Could this allow a race where port_store() changes chip->port_idx while
enable_store() is midway through enablement?

For example, if port_store() executes right after enable_eud() initializes
the PHY in enable_store() but before chip->enabled is set to true:

Thread 1 in enable_store():
    guard(mutex)(&chip->state_lock);
    enable_eud(chip);

Thread 2 in port_store():
    if (chip->enabled)
        return -EBUSY;
    chip->port_idx = port;

Thread 1 in enable_store():
    chip->enabled = true;

Would this cause subsequent disablements to read the new port_idx in
eud_phy_disable(), leaving the original PHY powered on and attempting to
power off the new PHY?

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