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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The early return optimization in `enable_store` causes a state desynchronization if the hardware was left enabled before probe (e.g., by the bootloader), preventing userspace from disabling it.

Pre-existing issues:
- [High] `pet_eud()` calls the sleeping function `readl_poll_timeout()` while executing in the top-half hardirq handler, which causes a kernel panic.
--

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

usb: misc: qcom_eud: improve enable_store API

This commit adds duplicate state checks to skip redundant enable or disable
requests when the EUD is already in the desired state. It also adds explicit
error logging to provide better feedback.

> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 48a4441b0c9c0..e83a4402e4b84 100644
> --- 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;
> +

[Severity: High]
Does this early return risk a state desynchronization if the hardware was
left enabled before probe?

Since chip is allocated via devm_kzalloc(), chip->enabled initializes to false
without reading the actual hardware state. If the bootloader or a previous
instance leaves the EUD active, and a boot script subsequently writes '0' to
the sysfs enable attribute to explicitly disable it, this check will evaluate
to false == false and return immediately without actually disabling the
hardware.

[ ... ]

Additionally, while reviewing this file, I noticed a pre-existing issue in the
interrupt handling path:

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

drivers/usb/misc/qcom_eud.c:pet_eud() {
        ...
        ret = readl_poll_timeout(chip->base + EUD_REG_SW_ATTACH_DET,
                                 reg, (reg == 0), 1, 100);
        ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does pet_eud() call a sleeping
function while executing in a top-half hardirq context?

When the EUD_INT_SAFE_MODE interrupt is asserted, the primary IRQ handler
handle_eud_irq() is invoked in hardirq context. It calls pet_eud(), which
uses readl_poll_timeout() with a sleep_us parameter of 1. This invokes
usleep_range(), forcing a sleep in an atomic context and likely causing a
scheduling while atomic panic.

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