Re: [PATCH v5 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 3 potential issue(s) to consider:

New issues:
- [High] A race condition between `enable_store` and `port_store` leads to unbalanced PHY power operations and resource leaks due to lockless state modification.
- [High] Concurrent writes to the `enable` sysfs attribute lack serialization, causing double-initialization and leakage of PHY resources.

Pre-existing issues:
- [High] The sysfs `enable_store` callback fails to clear the `chip->enabled` flag when disabling the EUD, permanently locking the device state.
--

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

usb: misc: qcom_eud: add per-path High-Speed PHY control
    
EUD hardware can support multiple High-Speed USB paths, each routed
through its own PHY. The active path is selected in hardware via the
EUD_PORT_SEL register. As a High-Speed hub, EUD requires access to
the High-Speed PHY associated with the active path. To support this
multi-path capability, the driver must manage PHY resources on a
per-path basis, ensuring that the PHY for the currently selected
path is properly initialized and powered.

[ ... ]

> 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
[ ... ]
> @@ -39,26 +41,84 @@ static const char * const eud_port_names[] = {
>  	"secondary",
>  };
>  
> +struct eud_path {
> +	struct eud_chip		*chip;
> +	struct phy		*phy;
> +	u8			num;
> +};
> +
>  struct eud_chip {
>  	struct device			*dev;
>  	struct usb_role_switch		*role_sw;
>  	void __iomem			*base;
> +	struct eud_path			*paths[EUD_MAX_PORTS];
>  	phys_addr_t			mode_mgr;
>  	unsigned int			int_status;
>  	int				irq;
>  	bool				enabled;
>  	bool				usb_attached;
> +	bool				phy_enabled;
>  	u8				port_idx;
>  };
>  
> +static int eud_phy_enable(struct eud_chip *chip)
> +{
> +	struct phy *phy;
> +	int ret;
> +
> +	if (chip->phy_enabled)
> +		return 0;

[Severity: High]
Does this lack serialization for concurrent sysfs writes? If two threads
concurrently write '1' to the 'enable' attribute, both could invoke
enable_eud() and reach here simultaneously. 

Since there is no locking, both threads could evaluate chip->phy_enabled as
false at the same time and proceed to call phy_init() and phy_power_on().

Would this double-increment the PHY's reference counts, meaning a later call
to eud_phy_disable() would only execute once and leave the PHY references
permanently leaked?

> +
> +	phy = chip->paths[chip->port_idx]->phy;
> +
> +	ret = phy_init(phy);

[ ... ]

> @@ -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;
>  	writel(port, chip->base + EUD_REG_PORT_SEL);
>  	chip->port_idx = port;

[Severity: High]
Can this race with enable_store()? If thread A writes to the 'enable'
attribute and calls enable_eud(), which enables the PHY for the current
port_idx, could thread B write to 'port' before thread A sets
chip->enabled = true?

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

Thread B in port_store():
    if (chip->enabled)
        return -EBUSY;
    writel(port, chip->base + EUD_REG_PORT_SEL);
    chip->port_idx = port;

If thread B updates chip->port_idx locklessly during this window, wouldn't a
subsequent disable operation in eud_phy_disable() use the new port_idx,
disabling a PHY that was never enabled and leaking the one that was?


[Severity: High]
This is a pre-existing issue, but does enable_store() fail to clear the
chip->enabled flag when disabling the EUD?

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

    if (enable) {
        ret = enable_eud(chip);
    } else {
        ret = disable_eud(chip);
    }

If the else branch handles disabling but omits clearing chip->enabled,
wouldn't writing '0' to 'enable' disable the hardware while leaving
chip->enabled true? This would cause subsequent attempts to write to
'port' to permanently fail with -EBUSY, breaking the new port switching
functionality introduced here.

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