Re: [PATCH v2 RESEND 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] Dangling fwnode pointer in the Type-C port device due to missing reference increment.
- [Medium] Data race on driver state and hardware updates during probe.
- [Medium] Unhandled hardware interrupts may cause an IRQ storm.
--

commit 8e96d9a2937eced73eec22a51fca02079409de61
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>
usb: typec: Add onsemi FUSB15201 driver

This commit adds a new driver for the onsemi FUSB15201 dual-port USB Type-C 
and Power Delivery controller. It registers a Type-C port for each connector 
described in the firmware, reporting attach/detach events and roles, and 
drives the USB role switch for OTG support.

> diff --git a/drivers/usb/typec/fusb15201.c b/drivers/usb/typec/fusb15201.c
> new file mode 100644
> index 0000000000000..897b2fabc449e
> --- /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: Medium]
Will this leave unknown interrupt bits permanently asserted, causing an IRQ
storm? If the hardware asserts an unhandled interrupt bit, it is masked out
from `pending` before the 'write one to clear' operation, so it is never
cleared.

[ ... ]

> +static int fusb15201_register_port(struct fusb15201 *chip,
> +				   struct fwnode_handle *fwnode)
> +{
> +	struct fusb15201_port *port;
> +	unsigned int index;
> +	int ret;

[ ... ]

> +	port->cap.revision = USB_TYPEC_REV_2_0;
> +	port->cap.orientation_aware = true;
> +	port->cap.driver_data = port;
> +	port->cap.ops = &fusb15201_typec_ops;
> +
> +	port->port = typec_register_port(chip->dev, &port->cap);

[Severity: High]
Does this create a dangling pointer to the fwnode?

The fwnode passed into typec_register_port() via `port->cap` is from
the device_for_each_child_node_scoped() iterator in fusb15201_probe().
Since the Type-C class does not take its own reference to the fwnode when
saving it to the device structure, the fwnode becomes a dangling pointer as
soon as the loop iteration ends and the scoped reference is dropped.

[ ... ]

> +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);
> +			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: Medium]
Does writing 0 unmask all interrupt bits for the active port, including any
undocumented ones? If the hardware asserts an unhandled interrupt, it might
not be cleared in fusb15201_irq() because it gets masked out, potentially
leading to an IRQ storm.

[ ... ]

> +static int fusb15201_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct fusb15201 *chip;
> +	unsigned int i, nports = 0;
> +	int ret;

[ ... ]

> +	ret = fusb15201_hw_init(chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to initialise\n");
> +
> +	/* 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]);
> +	}

[Severity: Medium]
Can this race with concurrent sysfs operations?

fusb15201_hw_update() updates shared driver state (like `port->status`)
without holding `chip->lock`.

Meanwhile, since the port is already registered and exposed to sysfs,
userspace can concurrently access it, invoking fusb15201_dr_set() ->
fusb15201_command(), which correctly acquires `chip->lock` but reads
the racing `port->status`.

> +
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL, fusb15201_irq,
> +					IRQF_ONESHOT, dev_name(dev), chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to request irq\n");
> +
> +	return 0;
> +}

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