Re: [PATCH v4 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:
- [High] usb: typec: cix_pd: HPD_IRQ latched in prev_status in
  cix_pd_event_handler()
- [Medium] usb: typec: cix_pd: unacknowledged IRQ on status refresh in
  cix_pd_resume()
- [Medium] usb: typec: cix_pd: unconditional port update in
  cix_pd_event_handler()
- [Medium] usb: typec: cix_pd: unlocked device_is_bound() in
  cix_pd_typec_port_update()
- [Medium] usb: typec: cix_pd: IRQ re-enabled on wake error in
  cix_pd_suspend()
--

commit df6c3014329332ed5d36e38c30e57956677e5273
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 orientation, USB data role, power role, and DisplayPort
alternate mode state 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..b413a125c7e6a
> --- /dev/null
> +++ b/drivers/usb/typec/cix_pd.c

[ ... ]

> +static int cix_pd_typec_port_update(struct cix_pd *typec, bool force)
> +{

[ ... ]

> +	if (!connected) {
> +		if (typec->partner_altmode) {
> +			typec_altmode_update_active(typec->partner_altmode,
> +						    false);
> +			typec_unregister_altmode(typec->partner_altmode);
> +			typec->partner_altmode = NULL;
> +		}
> +		if (typec->partner) {
> +			dev_info(typec->dev, "typec port(%d): unregister partner\n",
> +				 typec->id);
> +			typec_unregister_partner(typec->partner);
> +			typec->partner = NULL;
> +		}
> +	} else {
> +		if (!typec->partner) {
> +			dev_info(typec->dev, "typec port(%d): register partner\n",
> +				 typec->id);
> +			typec->partner = typec_register_partner(typec->port,
> +								&typec->desc);
> +			if (IS_ERR(typec->partner)) {
> +				dev_err(typec->dev,
> +					"typec port(%d): failed to register partner: %ld\n",
> +					typec->id, PTR_ERR(typec->partner));
> +				typec->partner = NULL;
> +			}
> +		}
> +
> +		if (typec->partner_altmode && !dp) {
> +			typec_altmode_update_active(typec->partner_altmode,
> +						    false);
> +			typec_unregister_altmode(typec->partner_altmode);
> +			typec->partner_altmode = NULL;
> +		}
> +
> +		if (typec->partner && dp && !typec->partner_altmode) {
> +			typec->partner_altmode =
> +				typec_partner_register_altmode(typec->partner,
> +							       &cix_pd_dp_partner_altmode);
> +			if (IS_ERR(typec->partner_altmode)) {
> +				dev_err(typec->dev,
> +					"typec port(%d): failed to register DP altmode: %ld\n",
> +					typec->id,
> +					PTR_ERR(typec->partner_altmode));
> +				typec->partner_altmode = NULL;
> +			}
> +		}
> +
> +		/*
> +		 * typec_altmode_update_active() takes a reference to the
> +		 * alternate mode driver's module only when a driver is bound
> +		 * to the mode device. Reporting the mode as active without a
> +		 * driver would leave that reference unbalanced, and the later
> +		 * deactivation would drop a reference that was never taken.
> +		 * Report the mode only once a driver is bound, and retry on
> +		 * the next status update so that a driver loaded after the
> +		 * cable was attached still learns about the active mode.
> +		 */
> +		if (typec->partner_altmode &&
> +		    device_is_bound(&typec->partner_altmode->dev))
> +			typec_altmode_update_active(typec->partner_altmode, true);
> +	}

[Severity: Medium]
Does calling device_is_bound(&typec->partner_altmode->dev) and
typec_altmode_update_active() without holding
device_lock(&typec->partner_altmode->dev) race with driver unbind?

device_is_bound() requires the caller to hold device_lock(dev).  During driver
unbind, __device_release_driver() runs typec_remove() ->
typec_altmode_update_active(adev, false) and device_unbind_cleanup() (which
sets adev->dev.driver = NULL) before detaching knode_driver:

CPU0                                  CPU1
cix_pd_typec_port_update()            device_release_driver_internal()
                                        __device_release_driver(dev)
                                          typec_remove(dev)
                                            typec_altmode_update_active(..., false)
  device_is_bound(dev) == true
  typec_altmode_update_active(..., true)
    if (adev->dev.driver)
                                          device_unbind_cleanup(dev)
                                            dev->driver = NULL
      try_module_get(adev->dev.driver->owner)
        <-- NULL pointer dereference

Depending on the exact timing against __device_release_driver(), this can also
leak a module reference, set adev->active = true without taking a module
reference, or double-put the module reference when
typec_altmode_update_active(typec->partner_altmode, false) runs on disconnect
or in cix_pd_unregister_ports().

In addition, when the DisplayPort alternate mode driver is built as a module
and loaded asynchronously by udev after typec_partner_register_altmode(),
device_is_bound() is false during the initial status update; if the hardware
raises no further interrupt after the module binds, partner_altmode remains
inactive in sysfs.

[ ... ]

> +static int cix_pd_event_handler(struct cix_pd *rts, u8 clear_irq, bool force)
> +{
> +	int ret, err = 0;
> +	bool changed;
> +	u8 data_control[3] = {0};
> +
> +	mutex_lock(&rts->lock);
> +	if (clear_irq) {
> +		data_control[0] = 2;
> +		data_control[1] = I2C_INT_ACK_BIT;
> +		data_control[2] = 0;
> +
> +		ret = cix_pd_block_write(rts, RTS_DATA_CONTROL, data_control, 3);
> +		if (ret)
> +			goto i2c_err;
> +	}
> +
> +	ret = cix_pd_block_read(rts, RTS_DATA_STATUS, rts->data_status, 6);
> +	if (ret)
> +		goto i2c_err;
> +
> +	dev_dbg(rts->dev,
> +		"typec port(%d): data status = 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x\n",
> +		rts->id, rts->data_status[0], rts->data_status[1],
> +		rts->data_status[2], rts->data_status[3], rts->data_status[4],
> +		rts->data_status[5]);
> +
> +	/*
> +	 * Only bytes 1..3 are decoded by this driver. Byte 0 and bytes 4..5
> +	 * have no known meaning, so they are excluded from the comparison: a
> +	 * change in an undecoded bit must not be reported as a port event.
> +	 */
> +	changed = !rts->status_valid ||
> +		  memcmp(&rts->data_status[1], &rts->prev_status[1],
> +			 DATA_STATUS_COUNT - 3);
> +	memcpy(rts->prev_status, rts->data_status, DATA_STATUS_COUNT);
> +	rts->status_valid = true;
> +
> +	err = cix_pd_typec_port_update(rts, force);

[Severity: Medium]
Should cix_pd_event_handler() skip cix_pd_typec_port_update() when neither
changed nor force is true?

Because all controller instances share one interrupt line, an interrupt on one
port runs cix_pd_event_handler() on every sibling instance.  Calling
cix_pd_typec_port_update() unconditionally when !changed && !force causes
every unchanged sibling port to call typec_set_orientation() and
typec_mux_set(), which reprogram the orientation switch and mode mux and emit
spurious KOBJ_CHANGE uevents and sysfs notifications on every unaffected port.

> +	if (err)
> +		dev_err(rts->dev, "typec port(%d): failed to update port: %d\n",
> +			rts->id, err);
> +
> +	if (rts->data_status[2] & HPD_IRQ) {
> +		data_control[0] = 2;
> +		data_control[1] = 0;
> +		data_control[2] = HPD_IRQ_ACK;
> +		ret = cix_pd_block_write(rts, RTS_DATA_CONTROL, data_control, 3);
> +		if (ret)
> +			goto i2c_err;
> +	}

[Severity: High]
Does copying rts->data_status into rts->prev_status with HPD_IRQ still set
break shared-interrupt attribution on subsequent interrupts?

When a DisplayPort sink generates an HPD_IRQ pulse, rts->prev_status[2] saves
HPD_IRQ = 1 (and cix_pd_typec_port_update() saves DP_STATUS_IRQ_HPD in
typec->hpd_status), and then writing HPD_IRQ_ACK clears HPD_IRQ in the
controller hardware.  If the sink generates a second HPD_IRQ while the rest of
data_status[1..3] is unchanged, rts->data_status[2] again has HPD_IRQ = 1,
matching the stale HPD_IRQ = 1 in rts->prev_status[2]; memcmp() reports no
change and cix_pd_irq_handle() returns IRQ_NONE for a valid interrupt it just
acknowledged, which can cause the spurious interrupt detector to disable the
shared IRQ line.  Conversely, if a sibling port interrupts next while this
port has HPD_IRQ = 0, this port sees a difference against its stale
rts->prev_status[2], returns IRQ_HANDLED for the sibling's interrupt, and logs
a duplicate mode message because dp_data->status no longer matches
typec->hpd_status.

[ ... ]

> +static int __maybe_unused cix_pd_suspend(struct device *dev)
> +{

[ ... ]

> +	disable_irq(client->irq);
> +	rts->irq_disabled = true;
> +
> +	if (device_may_wakeup(dev)) {
> +		ret = enable_irq_wake(client->irq);
> +		if (ret) {
> +			dev_err(dev, "failed to enable IRQ wake: %d\n", ret);
> +			enable_irq(client->irq);
> +			rts->irq_disabled = false;
> +		} else {
> +			rts->wake_enabled = true;
> +		}
> +	}
> +	mutex_unlock(&rts->irq_lock);
> +
> +	return 0;
> +}

[Severity: Medium]
If enable_irq_wake() fails, doesn't re-enabling client->irq while returning 0
allow the interrupt handler to run before the I2C adapter resumes?

Returning 0 lets system suspend proceed with the IRQ enabled, so
resume_device_irqs() unmasks the line before the I2C adapter is resumed and
cix_pd_irq_handle() can attempt I2C transfers on a suspended bus.  Also, when
multiple controller instances share client->irq, calling disable_irq() in each
instance increments the IRQ disable depth to the number of instances, whereas
suspend_device_irq() on IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND chips decrements the
depth only once, leaving the shared wakeup interrupt masked during sleep.

> +
> +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;
> +
> +	if (client->irq > 0) {
> +		mutex_lock(&rts->irq_lock);
> +		if (rts->wake_enabled) {
> +			disable_irq_wake(client->irq);
> +			rts->wake_enabled = false;
> +		}
> +		if (rts->irq_disabled) {
> +			enable_irq(client->irq);
> +			rts->irq_disabled = false;
> +		}
> +		mutex_unlock(&rts->irq_lock);
> +	}
> +
> +	/* Force the USB role to be reprogrammed after system resume. */
> +	ret = cix_pd_event_handler(rts, 0, true);

[Severity: Medium]
Does calling cix_pd_event_handler(rts, 0, true) after enable_irq() consume
sleep-time status changes without acknowledging the controller interrupt?

If a port status change occurred while the system was suspended, the
controller keeps the shared level-triggered interrupt line asserted until
I2C_INT_ACK_BIT is written.  When multiple instances share client->irq,
enable_irq() keeps the line masked until the last instance resumes, while each
earlier instance's cix_pd_event_handler(rts, 0, true) updates rts->prev_status
without clearing the hardware interrupt (and on the last or sole instance,
enable_irq() races the IRQ thread against cix_pd_event_handler()).  When
cix_pd_irq_handle() then runs for the still-asserted line, rts->prev_status
already matches rts->data_status, so every instance returns IRQ_NONE.

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