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

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 [off-list ref]
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.

[ ... ]

+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