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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Flawed interrupt handling leaves unknown bits unmasked and uncleared, leading to persistent interrupt storms.
- [High] The driver incorrectly enables global Dual Role Power (DRP) toggling if any port is configured as Sink-only, violating the port capabilities.
- [Medium] The USB role switch is not reset to USB_ROLE_NONE during driver unbind, leaving the external USB controller in an active state.
--

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

usb: typec: Add onsemi FUSB15201 driver

The commit introduces the onsemi FUSB15201 dual-port USB Type-C and Power
Delivery controller driver. It handles Type-C state detection, PD negotiation,
and allows for role swapping via sysfs.

> diff --git a/drivers/usb/typec/fusb15201.c b/drivers/usb/typec/fusb15201.c
> new file mode 100644
> index 0000000000000..920c561b62761
> --- /dev/null
> +++ b/drivers/usb/typec/fusb15201.c

[ ... ]

> +static irqreturn_t fusb15201_irq(int irq, void *data)
> +{
> +	struct fusb15201 *chip = data;
> +	irqreturn_t ret = IRQ_NONE;
> +	unsigned int i;
> +
> +	guard(mutex)(&chip->lock);
> +
> +	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]
If the hardware asserts an undocumented or reserved interrupt bit (e.g.
bits 3-7), does this masking leave those bits permanently uncleared? Since
they are masked out of the pending variable here, regmap_write won't clear
them. This might cause a persistent level-triggered IRQ storm if an unknown
bit triggers.

[ ... ]

> +static void fusb15201_put_role_sw(void *data)
> +{
> +	usb_role_switch_put(data);
> +}

[Severity: Medium]
Does this leave the external USB controller stuck in an active state if the
driver is unbound while attached? The driver correctly sets the role to
USB_ROLE_NONE on detach during normal operation, but that doesn't appear
to be called during this devres cleanup path.

[ ... ]

> +static int fusb15201_hw_init(struct fusb15201 *chip)
> +{
> +	bool source_only = true;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < FUSB15201_NUM_PORTS; i++) {
> +		struct fusb15201_port *port = &chip->ports[i];
> +
> +		if (!port->port) {
> +			/* Nothing describes this port, keep it quiet. */
> +			ret = regmap_write(chip->regmap,
> +					   FUSB15201_REG_INT_MASK(i),
> +					   FUSB15201_INT_ALL);

[Severity: High]
Since FUSB15201_INT_ALL is 0x07, does writing it to the mask register
mask bits 0-2 but leave bits 3-7 unmasked? This could expose the system to
the un-clearable interrupts mentioned above for unconfigured ports.

> +			if (ret)
> +				return ret;
> +
> +			continue;
> +		}
> +
> +		if (port->cap.type != TYPEC_PORT_SRC)
> +			source_only = false;
> +
> +		ret = regmap_write(chip->regmap, FUSB15201_REG_INT_MASK(i), 0);

[Severity: High]
Writing 0 appears to unmask all 8 bits, not just the known bits 0-2. Could
this also allow unknown interrupts to trigger and cause an IRQ storm on
configured ports?

> +		if (ret)
> +			return ret;
> +
> +		/* Discard anything that happened before we got here */
> +		ret = regmap_write(chip->regmap, FUSB15201_REG_INTERRUPT(i),
> +				   FUSB15201_INT_ALL);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Dual role toggling is the only configurable part of the Type-C state
> +	 * machine, and it is chip wide rather than per port, which is why it is
> +	 * set up here instead of from a port_type_set() callback.
> +	 */
> +	return regmap_update_bits(chip->regmap, FUSB15201_REG_MASTER_CONTROL,
> +				  FUSB15201_MASTER_DRP_DISABLE,
> +				  source_only ? FUSB15201_MASTER_DRP_DISABLE : 0);
> +}

[Severity: High]
If a port is configured as Sink-only (TYPEC_PORT_SNK), source_only becomes
false, which clears the MASTER_DRP_DISABLE bit. Doesn't this globally
enable Dual Role Power (DRP) toggling? If so, it would force fixed
Sink-only ports to periodically advertise as Sources, which could backdrive
power into attached devices or cause detection failures.

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