Re: [PATCH v6 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:

Pre-existing issues:
- [High] Writing '0' to the 'enable' sysfs attribute disables the hardware but fails to update the internal `chip->enabled` state to false.
- [High] The hardirq handler calls `readl_poll_timeout()`, which internally uses `usleep_range()` and causes a 'scheduling while atomic' panic.
- [High] Concurrent sysfs writes to 'enable' can cause PHY reference count leaks due to missing state locking.
--

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

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

The EUD hardware can support multiple High-Speed USB paths, each connected
to different USB controllers. This commit restructures the driver to support
per-path role switches and removes the single chip-level role switch. It
also removes the role switch call from enable_eud().

> 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

[ ... ]

> @@ -123,7 +126,7 @@ static int enable_eud(struct eud_chip *priv)
>  	writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE,
>  			priv->base + EUD_REG_INT1_EN_MASK);
>  
> -	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
> +	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does writing '0' to the
'enable' sysfs attribute leave the software state permanently enabled?

Looking at enable_store() in drivers/usb/misc/qcom_eud.c, it calls
disable_eud() but fails to update the internal chip->enabled state to false:

        } else {
                ret = disable_eud(chip);
        }

Because port_store() checks if chip->enabled is true and returns -EBUSY,
does this lock the user out of changing the port configuration after the
EUD is enabled and disabled once?

[Severity: High]
This isn't a bug introduced by this patch, but can concurrent sysfs writes
to 'enable' cause PHY reference count leaks?

Looking at enable_store() in drivers/usb/misc/qcom_eud.c, it calls
enable_eud() without acquiring any state lock:

        if (enable) {
                ret = enable_eud(chip);

Inside eud_phy_enable(), chip->phy_enabled is checked and modified without
locking:

        if (chip->phy_enabled)
                return 0;

        phy = chip->paths[chip->port_idx]->phy;

If two threads simultaneously write '1' to the 'enable' sysfs attribute,
both could observe chip->phy_enabled as false and initialize the PHY
multiple times. When disable_eud() is later called, it will only power
off the PHY once. 

Does this leak the PHY reference counts and leave the hardware permanently
powered on?

[ ... ]

> @@ -270,12 +273,15 @@ static irqreturn_t handle_eud_irq(int irq, void *data)

[Severity: High]
This isn't a bug introduced by this patch, but does the hardirq handler call
a sleeping function?

When the EUD_INT_SAFE_MODE interrupt triggers, the primary interrupt handler
handle_eud_irq() in drivers/usb/misc/qcom_eud.c (which executes in hardirq
context) invokes pet_eud():

        case EUD_INT_SAFE_MODE:
                pet_eud(chip);
                return IRQ_HANDLED;

Inside pet_eud(), it calls readl_poll_timeout() with a non-zero delay_us:

                ret = readl_poll_timeout(chip->base + EUD_REG_SW_ATTACH_DET,
                                        reg, (reg == 0), 1, 100);

Since a non-zero delay_us causes readl_poll_timeout() to internally call
usleep_range(), will this cause a 'scheduling while atomic' panic?

>  static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>  {
>  	struct eud_chip *chip = data;
> +	struct eud_path *path;
>  	int ret;

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