Re: [PATCH v5 5/9] usb: misc: qcom_eud: improve enable_store API

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the sysfs store handlers (`enable_store` and `port_store`) lack synchronization, exposing a critical TOCTOU race and hardware state corruption. While this patch attempts to avoid redundant hardware operations by adding a check `if (chip->enabled == enable)`, the check is performed without a lock. If multiple threads concurrently write '1' to the `enable` attribute, both can observe `chip->enabled == false`, bypass the check, and invoke `enable_eud()` simultaneously. This results in duplicate unsynchronized MMIO writes and unbalanced calls to `phy_init()` and `phy_power_on()`, permanently leaking PHY internal power/init reference counts. Furthermore, because `port_store` is also unprotected, a concurrent write to `port` can modify `chip->port_idx` while `enable_eud()` is executing. This can cause the driver to enable the PHY on one port but later disable the PHY on the other port, leading to permanent hardware resource leaks and corrupted PHY state.
--

--- Patch [5]: [PATCH v5 5/9] usb: misc: qcom_eud: improve enable_store API ---
commit daa70bf1af36bae927820d34ca275135da4718d1
Author: Elson Serrao <elson.serrao@oss.qualcomm.com>

usb: misc: qcom_eud: improve enable_store API

This commit adds a state check to enable_store() to skip redundant 
operations when the EUD is already in the desired state. It also 
improves error handling by explicitly logging failures.

> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -162,18 +162,27 @@ static ssize_t enable_store(struct device *dev,
>  	if (kstrtobool(buf, &enable))
>  		return -EINVAL;
>  
> +	/* Skip operation if already in desired state */
> +	if (chip->enabled == enable)
> +		return count;
> +
>  	if (enable) {
>  		ret = enable_eud(chip);

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of 
synchronization in this sysfs store handler expose a race condition?

If multiple threads concurrently write to the enable attribute, could both
observe chip->enabled == false, bypass this new check, and invoke
enable_eud() simultaneously?

Would this result in duplicate unsynchronized MMIO writes and unbalanced
calls to phy_init() and phy_power_on(), permanently leaking PHY internal
power and initialization reference counts?

Furthermore, because port_store() is also unprotected, can a concurrent
write to port modify chip->port_idx while enable_eud() is executing?

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;
...
}

Could this interleaving cause the driver to enable the PHY on one port but
later disable the PHY on the other port, leading to corrupted hardware state?

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