Re: [PATCH v2 2/2] usb: typec: add CIX Type-C PD controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] usb: typec: cix_pd: double fwnode put in cix_pd_init_ports()
  on parse error
- [Medium] usb: typec: cix_pd: broken role reset and race in
  cix_pd_resume()
- [Medium] usb: typec: cix_pd: missing data role update on DP-only
  connections
- [Medium] usb: typec: cix_pd: check enable_irq_wake() return in
  cix_pd_suspend()
- [Medium] usb: typec: cix_pd: unconditional IRQ_HANDLED in
  cix_pd_irq_handle()
--

commit 03dd05a69907e7c85cf26147fe9e902253dd9e55
Author: Gary Yang <gary.yang@cixtech.com>

usb: typec: add CIX Type-C PD controller driver

This commit adds an I2C driver for the CIX Type-C PD controller (Realtek
RTS5453H), reporting port status, DisplayPort alternate mode, orientation,
and USB role changes to the Type-C framework.

> diff --git a/drivers/usb/typec/cix_pd.c b/drivers/usb/typec/cix_pd.c
> new file mode 100644
> index 0000000000000..30c879f976c25
> --- /dev/null
> +++ b/drivers/usb/typec/cix_pd.c
> @@ -0,0 +1,687 @@

[ ... ]

> +static int cix_pd_typec_port_update(struct cix_pd *typec)
> +{
> +	struct typec_displayport_data *dp_data = &typec->dp_data;
> +	enum typec_orientation orientation;
> +	enum usb_role role;
> +	bool usb2, usb3, dp, connected;
> +	int ret, err;
> +
> +	usb2 = typec->data_status[1] & USB2_CONNECTION;
> +	usb3 = typec->data_status[1] & USB3_2_CONNECTION;
> +	dp = typec->data_status[1] & DP_CONNECTION;

[ ... ]

> +	if (!connected || !(usb2 || usb3))
> +		role = USB_ROLE_NONE;
> +	else if (typec->data_status[1] & USB_DATA_ROLE)
> +		role = USB_ROLE_DEVICE;
> +	else
> +		role = USB_ROLE_HOST;
> +
> +	if (typec->role != role) {
> +		err = 0;
> +		if (typec->role_sw) {
> +			err = usb_role_switch_set_role(typec->role_sw, role);
> +			if (err)
> +				dev_err(typec->dev,
> +					"typec port(%d): failed to set USB role: %d\n",
> +					typec->id, err);
> +		}
> +
> +		/* Only cache the role once the switch accepted it. */
> +		if (!err) {
> +			typec->role = role;
> +			if (role == USB_ROLE_DEVICE)
> +				typec_set_data_role(typec->port, TYPEC_DEVICE);
> +			else if (role == USB_ROLE_HOST)
> +				typec_set_data_role(typec->port, TYPEC_HOST);
> +		}
> +	}

[Severity: Medium]
Does skipping typec_set_data_role() when role is USB_ROLE_NONE leave
typec->port in TYPEC_DEVICE on 4-lane DisplayPort-only connections?

When a 4-lane DisplayPort partner connects (dp is true while usb2 and usb3
are false), role is set to USB_ROLE_NONE and typec_set_data_role() is not
called.  If the port was initialized to TYPEC_DEVICE (such as a dual-role
port without try-power-role set to source) or was previously in device
mode, registering cix_pd_dp_partner_altmode below fails in
dp_altmode_probe() with -EPROTO because it requires the port data role to
be TYPEC_HOST:

drivers/usb/typec/altmodes/displayport.c:dp_altmode_probe() {
	...
	/* Port can only be DFP_U. */
	if (typec_altmode_get_data_role(alt) != TYPEC_HOST)
		return -EPROTO;
	...
}

Updating typec->port's data role from USB_DATA_ROLE whenever connected is
true, independently of the USB controller's role switch state, would avoid
this.

[ ... ]

> +static int cix_pd_typec_parse_port_props(struct typec_capability *cap,
> +					 struct fwnode_handle *fwnode,
> +					 struct device *dev)
> +{
> +	int ret;
> +
> +	memset(cap, 0, sizeof(*cap));
> +	ret = typec_get_fw_cap(cap, fwnode);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to parse connector properties\n");
> +
> +	cap->fwnode = fwnode_handle_get(fwnode);
> +	cap->orientation_aware = true;
> +	/* Alternate modes are entered and exited by the controller firmware. */
> +	cap->no_mode_control = true;
> +
> +	return 0;
> +}

[ ... ]

> +static void cix_pd_unregister_ports(struct cix_pd *typec)
> +{

[ ... ]

> +	usb_role_switch_put(typec->role_sw);
> +	typec->role_sw = NULL;
> +	typec_mux_put(typec->mux);
> +	typec->mux = NULL;
> +	fwnode_handle_put(typec->caps.fwnode);
> +	typec->caps.fwnode = NULL;
> +}
> +
> +static irqreturn_t cix_pd_irq_handle(int irq, void *data)
> +{
> +	struct cix_pd *rts = data;
> +
> +	if (cix_pd_event_handler(rts, 1) == -ESHUTDOWN)
> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
Can returning IRQ_HANDLED unconditionally on a shared level-triggered
interrupt line cause an unthrottled interrupt storm?

cix_pd_event_handler() returns 0 whenever I2C transfers to this controller
succeed, even if this instance did not assert the interrupt.  If another
controller sharing the IRQ line holds it active while not yet probed,
probe-deferred, unbound, or unresponsive over I2C, this handler still
returns IRQ_HANDLED on every pass, preventing the IRQ core from detecting
and disabling the stuck interrupt line.  Additionally, cix_pd_probe() calls
cix_pd_event_handler(rts, 0) without acknowledging a pending interrupt
before requesting the IRQ.

> +
> +static int cix_pd_init_ports(struct cix_pd *typec)
> +{
> +	struct device *dev = typec->dev;
> +	struct typec_altmode_desc desc = cix_pd_dp_port_altmode;
> +	struct fwnode_handle *fwnode;
> +	int ret;
> +
> +	fwnode = device_get_named_child_node(dev, "connector");
> +	if (!fwnode)
> +		return dev_err_probe(dev, -ENODEV, "no connector node found\n");
> +
> +	ret = cix_pd_typec_get_handles(typec, fwnode, dev);
> +	if (ret)
> +		goto out_put_fwnode;
> +
> +	ret = cix_pd_typec_parse_port_props(&typec->caps, fwnode, dev);
> +	if (ret)
> +		goto out_unregister_ports;

[ ... ]

> +out_unregister_ports:
> +	cix_pd_unregister_ports(typec);
> +out_put_fwnode:
> +	fwnode_handle_put(fwnode);
> +	return ret;
> +}

[Severity: Medium]
Does cix_pd_init_ports() put fwnode twice when
cix_pd_typec_parse_port_props() fails?

typec_get_fw_cap() assigns cap->fwnode = fwnode before validating the
connector properties:

drivers/usb/typec/class.c:typec_get_fw_cap() {
	cap->fwnode = fwnode;

	ret = fwnode_property_read_string(fwnode, "power-role", &cap_str);
	if (ret < 0)
		return ret;
	...
}

When typec_get_fw_cap() returns an error, cix_pd_typec_parse_port_props()
returns before calling fwnode_handle_get(fwnode), leaving
typec->caps.fwnode pointing to fwnode without an extra reference.  Jumping
to out_unregister_ports then calls fwnode_handle_put(typec->caps.fwnode) in
cix_pd_unregister_ports() and fwnode_handle_put(fwnode) at out_put_fwnode,
underflowing the fwnode reference count.

[ ... ]

> +static int __maybe_unused cix_pd_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct cix_pd *rts = i2c_get_clientdata(client);
> +
> +	if (client->irq > 0 && device_may_wakeup(dev)) {
> +		mutex_lock(&rts->irq_lock);
> +		if (!rts->wake_enabled) {
> +			enable_irq_wake(client->irq);
> +			rts->wake_enabled = true;
> +		}
> +		mutex_unlock(&rts->irq_lock);
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
What happens if enable_irq_wake() fails in cix_pd_suspend()?

cix_pd_probe() marks the device wakeup-capable unconditionally, so
enable_irq_wake() can fail if the interrupt controller does not support
wakeup.  Setting rts->wake_enabled = true without checking the return value
causes cix_pd_resume() or cix_pd_remove() to call disable_irq_wake(),
triggering an "Unbalanced IRQ wake disable" warning in irq_set_irq_wake().

> +
> +static int __maybe_unused cix_pd_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct cix_pd *rts = i2c_get_clientdata(client);
> +	int ret;
> +
> +	mutex_lock(&rts->irq_lock);
> +	if (rts->wake_enabled) {
> +		disable_irq_wake(client->irq);
> +		rts->wake_enabled = false;
> +	}
> +	mutex_unlock(&rts->irq_lock);
> +
> +	/* Force the USB role to be reprogrammed after system resume. */
> +	rts->role = USB_ROLE_NONE;
> +
> +	ret = cix_pd_event_handler(rts, 0);

[Severity: Medium]
Does setting rts->role = USB_ROLE_NONE before cix_pd_event_handler() prevent
clearing the USB role when a partner was unplugged during suspend?

If the port was in host or device mode before suspend and the cable was
unplugged while suspended, cix_pd_typec_port_update() computes
role = USB_ROLE_NONE, sees typec->role == role, and skips
usb_role_switch_set_role(typec->role_sw, USB_ROLE_NONE), leaving the USB
controller stuck in its pre-suspend role.

Also, device interrupts are re-enabled before cix_pd_resume() runs, so
writing rts->role without holding rts->lock races with
cix_pd_irq_handle() -> cix_pd_event_handler():

CPU0                              CPU1
cix_pd_irq_handle()
  cix_pd_event_handler()
    mutex_lock(&rts->lock)
    cix_pd_typec_port_update()
      rts->role = USB_ROLE_HOST;
                                  cix_pd_resume()
                                    rts->role = USB_ROLE_NONE;  <-- data race
    mutex_unlock(&rts->lock)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109092400.202219-1-gary.yang@cixtech.com?part=2