Re: [PATCH v3 2/2] usb: typec: Add onsemi FUSB15201 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: typec: fusb15201: unhandled interrupt bits cause interrupt storm
- [Medium] usb: typec: fusb15201: data race in probe allows spurious -ENOTCONN
--

commit d7feaab14acc6ee944a785f261bfb7dd837de605
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

usb: typec: Add onsemi FUSB15201 driver

The FUSB15201 is a dual-port USB Type-C and Power Delivery controller. This
driver handles Type-C detection, registers a Type-C port for each connector,
and forwards role swaps to the controller.

> diff --git a/drivers/usb/typec/fusb15201.c b/drivers/usb/typec/fusb15201.c
> new file mode 100644
> index 0000000000000..ab3b07368bdf1
> --- /dev/null
> +++ b/drivers/usb/typec/fusb15201.c
[ ... ]
> +static irqreturn_t fusb15201_irq(int irq, void *data)
> +{
[ ... ]
> +	for (i = 0; i < FUSB15201_NUM_PORTS; i++) {
> +		struct fusb15201_port *port = &chip->ports[i];
> +		unsigned int pending;
> +
> +		if (!port->port)
> +			continue;
> +
> +		if (regmap_read(chip->regmap, FUSB15201_REG_INTERRUPT(i), &pending))
> +			continue;
> +
> +		pending &= FUSB15201_INT_ALL;
> +		if (!pending)
> +			continue;
> +
> +		/* Write one to clear */
> +		regmap_write(chip->regmap, FUSB15201_REG_INTERRUPT(i), pending);

[Severity: High]
Could masking the pending interrupts before writing back cause an interrupt
storm?

If the hardware asserts any bit outside of FUSB15201_INT_ALL (such as a
reserved bit or a new status bit in newer firmware), the driver masks it out
and writes 0 for that bit, failing to clear it. 

If only unknown bits are asserted, pending becomes 0, and the handler returns
IRQ_NONE without clearing anything. The unhandled interrupt will then fire
continuously and lock up the CPU.

[ ... ]
> +static int fusb15201_hw_init(struct fusb15201 *chip)
> +{
[ ... ]
> +	for (i = 0; i < FUSB15201_NUM_PORTS; i++) {
> +		struct fusb15201_port *port = &chip->ports[i];
[ ... ]
> +		/* Discard anything that happened before we got here */
> +		ret = regmap_write(chip->regmap, FUSB15201_REG_INTERRUPT(i),
> +				   FUSB15201_INT_ALL);

[Severity: High]
Similar to the interrupt handler, this only clears known bits. Will any
pre-existing unhandled bits remain asserted and immediately trigger an
interrupt storm once the IRQ is requested?

Should this write a full mask to clear any unexpected bits during
initialization?

[ ... ]
> +	/* Seed the Type-C class with the current state before arming the IRQ */
> +	for (i = 0; i < FUSB15201_NUM_PORTS; i++) {
> +		if (chip->ports[i].port)
> +			fusb15201_hw_update(&chip->ports[i]);
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Can this lockless initialization race with sysfs commands?

Because typec_register_port() is called before fusb15201_hw_init() in probe,
sysfs attributes and uevents are already active. When fusb15201_hw_update()
is called locklessly here, it invokes typec_register_partner(), which can
trigger additional uevents before port->status is updated.

If userspace reacts to these uevents and requests a data or power role swap,
fusb15201_command() will acquire chip->lock and read port->status. Since
this hardware update does not hold the lock, fusb15201_command() could observe
a stale port->status == 0 and spuriously reject the valid swap request with
-ENOTCONN.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916135918.1017527-1-shengchao.guo@oss.qualcomm.com?part=2