Re: [PATCH v7 3/8] 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] Missing locking in sysfs store handlers leads to concurrent accesses that corrupt EUD state, unbalance PHY reference counts, and cause resource leaks.
--

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

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

This commit introduces per-path High-Speed PHY management for the EUD
hardware. It restructures the driver to dynamically initialize and power
the PHY associated with the currently selected active path.

> 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]
Does this locklessly check chip->enabled and subsequently modify chip->port_idx,
creating a race condition with concurrent writes to the enable attribute?

If a privileged user concurrently writes to both the enable and port sysfs
attributes, the following sequence appears possible:

Thread A executes enable_store(true), reads chip->enabled as false, and starts
enable_eud(). It successfully powers on the PHY for the current port (e.g.,
port 0).

Before Thread A sets chip->enabled to true, Thread B executes port_store(1).

Because chip->enabled is still false, Thread B bypasses the EBUSY check above
and changes chip->port_idx to 1.

When EUD is later disabled, disable_eud() will read the new chip->port_idx
and power off the PHY for port 1. This would cause a reference count underflow
on the uninitialized PHY for port 1, while leaving the PHY for port 0
permanently powered on, leaking the resource.

While later patches in this series might add a state lock to enable_store(),
port_store() appears to remain completely lockless, leaving this race unresolved.
Should port_store() also be synchronized against concurrent modifications?

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