Re: [PATCH v5 2/9] usb: misc: qcom_eud: add sysfs attribute for port selection

[sashiko-bot@kernel.org]

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

New issues:
- [High] Lockless race condition between `enable_store` and `port_store` allows unsafe concurrent hardware register mutation.
- [Medium] The initial software state for the USB port is out of sync with the uninitialized hardware register at probe time.

Pre-existing issues:
- [High] The pre-existing failure to clear `chip->enabled` when disabling the EUD permanently locks the newly added `port` sysfs attribute.
- [High] Calling `readl_poll_timeout()` from hardirq context causes a 'scheduling while atomic' panic.
--

--- Patch [2]: [PATCH v5 2/9] usb: misc: qcom_eud: add sysfs attribute for port selection ---
commit d4f0438c23f8813f8cdd1221aa0f9078442b7817
Author: Elson Serrao <elson.serrao@oss.qualcomm.com>

usb: misc: qcom_eud: add sysfs attribute for port selection

EUD can be mapped to either the primary USB port or the secondary USB port
depending on the value of the EUD_PORT_SEL register. Add a 'port' sysfs
attribute to allow userspace to select which port EUD should operate on
and update the ABI documentation.

[ ... ]

> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 926419ca560fc..25a6a8f110ec8 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -40,6 +48,7 @@ struct eud_chip {
>  	int				irq;
>  	bool				enabled;
>  	bool				usb_attached;
> +	u8				port_idx;

[Severity: Medium]
Does the initial software state of the USB port get out of sync with the
uninitialized hardware register at probe time?

During eud_probe(), devm_kzalloc() implicitly zeroes chip->port_idx, which
corresponds to 'primary':

drivers/usb/misc/qcom_eud.c:eud_probe() {
    ...
    chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
    ...
}

However, the driver's probe sequence never writes to EUD_REG_PORT_SEL to
enforce this default, nor does it read the register to populate chip->port_idx.

If the hardware defaults to or is left in the 'secondary' state by a bootloader,
the sysfs attribute will incorrectly report the 'primary' port is selected.

>  };

[ ... ]

> @@ -104,8 +113,38 @@ static ssize_t enable_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(enable);
>  
> +static ssize_t port_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct eud_chip *chip = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%s\n", eud_port_names[chip->port_idx]);
> +}
> +
> +static ssize_t port_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t count)
> +{
> +	struct eud_chip *chip = dev_get_drvdata(dev);
> +	int port;
> +
> +	port = sysfs_match_string(eud_port_names, buf);
> +	if (port < 0)
> +		return port;
> +
> +	/* Port selection must be done before enabling EUD */
> +	if (chip->enabled)
> +		return -EBUSY;

[Severity: High]
This is a pre-existing issue, but because enable_store() never sets
chip->enabled to false when disabling the device, will this check
permanently lock out port changes once the EUD has been enabled?

If a user writes '1' to /sys/.../enable, and then writes '0', the device
successfully disables. However, because chip->enabled is never updated to false:

drivers/usb/misc/qcom_eud.c:enable_store() {
    ...
    } else {
        ret = disable_eud(chip);
        /* chip->enabled = false; is missing */
    }
    ...
}

Any subsequent write to /sys/.../port will evaluate chip->enabled as true and
reject the operation with -EBUSY indefinitely.

[Severity: High]
Does this lockless check allow unsafe concurrent hardware register mutation?

If there are concurrent writes to the enable and port sysfs attributes,
port_store() validates chip->enabled without taking a lock. Concurrently,
enable_store() could call enable_eud(), which turns on the hardware and sets
chip->enabled to true:

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

This would allow port_store() to overwrite the EUD_REG_PORT_SEL register while
the EUD is actively being enabled, leading to undefined hardware behavior.

> +
> +	writel(port, chip->base + EUD_REG_PORT_SEL);
> +	chip->port_idx = port;
> +
> +	return count;
> +}

[ ... ]

[Severity: High]
This is a pre-existing issue, but does calling readl_poll_timeout() from
hardirq context cause a 'scheduling while atomic' panic?

The hardware triggers an EUD_INT_SAFE_MODE interrupt, and the primary
interrupt handler handle_eud_irq() invokes 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(), readl_poll_timeout() is called with a 1-microsecond delay:

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

Since readl_poll_timeout() internally expands to usleep_range(), it attempts
to sleep while interrupts are disabled, which triggers a panic.

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