Re: [PATCH v7 4/8] usb: misc: qcom_eud: add per-path role switch support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a pre-existing issue that remains unaddressed in the series. Missing locking between sysfs `port_store` and `enable_store` leads to race conditions, unbalanced PHY operations, and potential PHY refcount corruption.
- [High] This problem wasn't introduced by this patch, but it is a critical pre-existing bug. The hardirq handler calls `readl_poll_timeout()` which uses `usleep_range()`, leading to sleeping in atomic context.
--

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

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

The commit replaces the single chip-level role switch with per-path role
switches to properly handle multi-path configurations. It also forwards
role change requests from the EUD to the respective USB controllers
and removes the unnecessary role switch call from enable_eud().

> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 03d1c200658c8..45c90d9f3b1f6 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
[ ... ]
> @@ -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 it is a critical pre-existing
bug that remains unaddressed in the series. The primary hardirq handler
handle_eud_irq() calls pet_eud():

drivers/usb/misc/qcom_eud.c:handle_eud_irq() {
    ...
	case EUD_INT_SAFE_MODE:
		pet_eud(chip);
		return IRQ_HANDLED;
    ...
}

Inside pet_eud(), we sleep in an atomic context:

drivers/usb/misc/qcom_eud.c:pet_eud() {
    ...
		/* Delay to make sure detach pet is done before attach pet */
		ret = readl_poll_timeout(chip->base + EUD_REG_SW_ATTACH_DET,
					reg, (reg == 0), 1, 100);
    ...
}

Does this code trigger a "BUG: scheduling while atomic" and lead to a kernel
panic when the EUD_INT_SAFE_MODE hardware interrupt fires?

The readl_poll_timeout() macro uses usleep_range() under the hood when a
non-zero delay is provided, which is invalid to call from a hardirq handler.

>  static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>  {
>  	struct eud_chip *chip = data;
[ ... ]

[Severity: High]
This is also a pre-existing issue, but I noticed a potential race condition
between sysfs callbacks that modify the port and enable states locklessly:

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

and

drivers/usb/misc/qcom_eud.c:enable_store() {
    ...
	if (enable) {
		ret = enable_eud(chip);
		if (!ret)
			chip->enabled = enable;
    ...
}

Could concurrent execution of port_store() and enable_store() lead to race
conditions since they locklessly read and write chip->enabled and
chip->port_idx?

Can this result in unbalanced PHY operations, PHY refcount underflow, or
permanent resource leaks if the port index changes during an enable
operation?

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