Re: [PATCH v6 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:
- [Medium] The `port` attribute unconditionally allows selecting a secondary port on single-port hardware platforms.
- [Medium] The initial software state of `port_idx` is not synchronized with the hardware configuration during device probe.

Pre-existing issues:
- [High] The `chip->enabled` flag is never cleared when EUD is disabled, permanently blocking subsequent port changes.
- [High] Simultaneous active interrupts cause an unhandled interrupt storm in `handle_eud_irq`.
--

--- Patch [2]: [PATCH v6 2/9] usb: misc: qcom_eud: add sysfs attribute for port selection ---
commit 6f4483594010ca910c4a59bd283f5067e4e3b31e
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
> @@ -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]);

[Severity: Medium]
Could the initial software state of chip->port_idx be out of sync with the
hardware?

If the bootloader configures EUD_REG_PORT_SEL to the secondary port prior
to kernel boot, eud_probe() leaves port_idx as 0 from its initial
allocation.

Will port_show() erroneously return "primary" while the hardware is
actually configured for the secondary port?

> +}
> +
> +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 since the new port_store() function
relies on chip->enabled, does the driver ever clear this flag when EUD is
disabled?

Looking at enable_store(), when a user writes '0', it calls disable_eud()
but it doesn't appear to set chip->enabled to false:

drivers/usb/misc/qcom_eud.c:enable_store() {
    ...
    } else {
        ret = disable_eud(chip);
    }
    ...
}

Once enabled, will chip->enabled stay true forever and permanently block
subsequent port changes?

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

[Severity: Medium]
Does this code allow selecting a secondary port on hardware platforms that
only physically support a single port?

I don't see a check against device tree capabilities or a mechanism like
is_visible() to hide this attribute on single-port devices. Unconditionally
configuring the hardware register to a non-existent port could lead to
undefined behavior.

> +
> +	return count;
> +}
> +

[ ... ]

While reviewing the rest of the file, I noticed another detail:

[Severity: High]
This is a pre-existing issue, but can the interrupt handler experience an
unhandled interrupt storm if multiple statuses are asserted?

If both EUD_INT_VBUS and EUD_INT_SAFE_MODE are asserted simultaneously,
the switch statement in handle_eud_irq() looks like it might fail to handle
the combined bitmask:

drivers/usb/misc/qcom_eud.c:handle_eud_irq() {
    ...
    switch (reg & EUD_INT_ALL) {
    case EUD_INT_VBUS:
        usb_attach_detach(chip);
        return IRQ_WAKE_THREAD;
    case EUD_INT_SAFE_MODE:
        pet_eud(chip);
        return IRQ_HANDLED;
    default:
        return IRQ_NONE;
    }
}

Since EUD_INT_ALL includes both bits, wouldn't reg & EUD_INT_ALL evaluate
to 0x14, fall through to the default case, and return IRQ_NONE without
clearing either interrupt?

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