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

[sashiko-bot@kernel.org]

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

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

usb: typec: Add onsemi FUSB15201 driver

This commit adds a Type-C class driver for the onsemi FUSB15201 dual-port
USB Type-C and Power Delivery controller. It registers each connector
described in firmware, updates partner and role states via regmap and
threaded interrupts, and forwards role swap requests 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 void fusb15201_hw_update(struct fusb15201_port *port)
> +{
> +	struct fusb15201 *chip = port->chip;
> +	enum typec_data_role data_role = TYPEC_DEVICE;
> +	enum typec_role pwr_role;
> +	unsigned int status;
> +	int ret;
> +
> +	ret = regmap_read(chip->regmap, FUSB15201_REG_STATUS(port->index), &status);
> +	if (ret) {
> +		dev_warn(chip->dev, "port%u: failed to read status: %d\n",
> +			 port->index, ret);
> +		return;
> +	}

[ ... ]

> +	if (!(port->status & FUSB15201_STATUS_ATTACHED)) {
> +		struct typec_partner_desc desc = {
> +			.usb_pd = !!(status & FUSB15201_STATUS_USBPD),
> +		};
> +
> +		port->partner = typec_register_partner(port->port, &desc);
> +		if (IS_ERR(port->partner)) {
> +			dev_err(chip->dev,
> +				"port%u: failed to register partner: %pe\n",
> +				port->index, port->partner);
> +			port->partner = NULL;
> +		}
> +	}
> +
> +	typec_set_orientation(port->port, status & FUSB15201_STATUS_CC2 ?
> +			      TYPEC_ORIENTATION_REVERSE :
> +			      TYPEC_ORIENTATION_NORMAL);
> +
> +	/*
> +	 * The controller only tells us whether the partner is PD capable. The
> +	 * Type-C current advertisement is not reported, so 1.5A and 3.0A
> +	 * operation cannot be distinguished from the default.
> +	 */
> +	typec_set_pwr_opmode(port->port, status & FUSB15201_STATUS_USBPD ?
> +			     TYPEC_PWR_MODE_PD : TYPEC_PWR_MODE_USB);
> +
> +	pwr_role = status & FUSB15201_STATUS_SINK ? TYPEC_SINK : TYPEC_SOURCE;
> +	data_role = status & FUSB15201_STATUS_DFP ? TYPEC_HOST : TYPEC_DEVICE;
> +	fusb15201_set_roles(port, pwr_role, data_role, true);
> +
> +	port->status = status;
> +}

[ ... ]

> +static int fusb15201_command(struct fusb15201_port *port, unsigned int cmd)
> +{
> +	struct fusb15201 *chip = port->chip;
> +
> +	guard(mutex)(&chip->lock);
> +
> +	if (!(port->status & FUSB15201_STATUS_ATTACHED))
> +		return -ENOTCONN;
> +
> +	return regmap_write(chip->regmap, FUSB15201_REG_COMMANDS(port->index), cmd);
> +}

[ ... ]

> +static int fusb15201_hw_init(struct fusb15201 *chip)
> +{

[ ... ]

> +	/* 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]
As reported by Sashiko on v3 [1], could calling fusb15201_hw_update() in
fusb15201_hw_init() without holding chip->lock cause concurrent sysfs role
swap requests to fail spuriously with -ENOTCONN?

In fusb15201_probe(), fusb15201_register_port() calls typec_register_port(),
which exposes the port sysfs attributes (data_role and power_role) before
fusb15201_hw_init() runs.

When a port is already attached at probe time, fusb15201_hw_update() registers
the partner and updates the port orientation, power operation mode, and roles
(emitting uevents) before finally setting port->status = status at the end of
the function:

fusb15201_probe()
  fusb15201_register_port()
    typec_register_port() -> exposes data_role and power_role in sysfs
  fusb15201_hw_init()
    fusb15201_hw_update() -> called without chip->lock
      typec_register_partner() -> emits partner KOBJ_ADD
      fusb15201_set_roles()    -> emits port KOBJ_CHANGE
      port->status = status;

If userspace reacts to the port or partner uevents by writing to data_role or
power_role before fusb15201_hw_update() finishes, fusb15201_dr_set() or
fusb15201_pr_set() will call fusb15201_command(). Because fusb15201_hw_init()
does not hold chip->lock, fusb15201_command() can acquire chip->lock while
port->status is still 0 and return -ENOTCONN even though the partner and roles
have already been published.

Should fusb15201_hw_init() hold chip->lock around the fusb15201_hw_update()
calls (or update port->status under chip->lock before publishing the state to
the Type-C class)?

[1] https://lore.kernel.org/r/20260916135918.1017527-3-shengchao.guo@oss.qualcomm.com

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