Re: [PATCH v7 2/2] usb: typec: ucsi: Add ITE IT8851/IT8853 Type-C PD controller driver

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Calling `ucsi_unregister()` before `free_irq()` causes a use-after-free if an interrupt fires during teardown.
- [High] Returning `IRQ_HANDLED` when an I2C error prevents clearing the hardware interrupt causes an interrupt storm.
- [Low] The `itepd_id_table` array uses positional initializers for `struct i2c_device_id`, violating I2C subsystem rules.
--

--- Patch [2]: [PATCH v7 2/2] usb: typec: ucsi: Add ITE IT8851/IT8853 Type-C PD controller driver ---
commit cf5d1292ced23eec9cf9ddfbbd017ff90f67a0b1
Author: Amber Kao <[email protected]>

usb: typec: ucsi: Add ITE IT8851/IT8853 Type-C PD controller driver

This commit adds a UCSI driver for the ITE IT8851 and IT8853 I2C-based
USB Type-C Power Delivery controllers.

> diff --git a/drivers/usb/typec/ucsi/ucsi_itepd.c b/drivers/usb/typec/ucsi/ucsi_itepd.c
> new file mode 100644
> index 0000000000000..d1887435a2e15
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_itepd.c

[ ... ]

> +static irqreturn_t itepd_irq_process(struct itepd *itepd)
> +{
> +	u32 cci;
> +	int ret;
> +
> +	ret = itepd_process_event(itepd, &cci);
> +	if (ret == ITEPD_EVENT_NONE)
> +		return IRQ_NONE;
> +
> +	if (ret < 0)
> +		return IRQ_HANDLED;

[Severity: High]
Could returning IRQ_HANDLED here cause an interrupt storm?

If itepd_process_event() fails due to an I2C error, the hardware interrupt
is never cleared on the controller. Because the IRQ subsystem thinks the
event was serviced, it will unmask the line, causing the level-triggered
interrupt to fire again immediately in an infinite loop.

> +
> +	if (ret == ITEPD_EVENT_UCSI)
> +		ucsi_notify_common(itepd->ucsi, cci);
> +
> +	return IRQ_HANDLED;
> +}

[ ... ]

> +static void itepd_remove(struct i2c_client *client)
> +{
> +	struct itepd *itepd = i2c_get_clientdata(client);
> +
> +	ucsi_unregister(itepd->ucsi);
> +	free_irq(client->irq, itepd);

[Severity: High]
Can this teardown sequence result in a use-after-free?

The ucsi_unregister() function frees the ucsi->connector array. If an
interrupt fires immediately after this (for example, if disabling
notifications takes time or an event was pending), the IRQ thread runs
and calls ucsi_notify_common().

This would then call ucsi_connector_change(), which attempts to schedule work
using the already freed connector structures.

Should free_irq() be called before ucsi_unregister() to synchronize with
the interrupt handler?

> +	ucsi_destroy(itepd->ucsi);
> +}
> +
> +static const struct of_device_id itepd_of_match_table[] = {
> +	{ .compatible = "ite,it8851" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, itepd_of_match_table);
> +
> +static const struct i2c_device_id itepd_id_table[] = {
> +	{ "ucsi_itepd", 0 },

[Severity: Low]
This isn't a bug, but the I2C subsystem guidelines explicitly require
initialized arrays of type struct i2c_device_id to use named initializers
to prevent issues when struct layouts change.

Could this be updated to use named initializers like:
{ .name = "ucsi_itepd", .driver_data = 0 }?

> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, itepd_id_table);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2