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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Potential Use-After-Free in `itepd_remove` due to tearing down the IRQ handler after `ucsi_unregister`.
- [Low] The `itepd_id_table` array does not use named initializers.
--

--- Patch [2]: [PATCH v6 2/2] usb: typec: ucsi: Add ITE IT8851/IT8853 Type-C PD controller driver ---
commit d3337d48cd9d0171c8064301690b4facc2758392
Author: Amber Kao <amber.kao@ite.com.tw>

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. It implements the necessary UCSI
operations and uses a shared interrupt line to process events.

diff --git a/drivers/usb/typec/ucsi/ucsi_itepd.c b/drivers/usb/typec/ucsi/ucsi_itepd.c
new file mode 100644
index 0000000000000..a639b3d32535f
--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_itepd.c
[ ... ]
> +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]
Could this teardown order cause a use-after-free if an interrupt fires before
the IRQ is freed?

When ucsi_unregister() is called, it frees the ucsi->connector array. If a
hardware interrupt triggers right after that, but before free_irq() completes,
the interrupt handler will call ucsi_notify_common().

Inside ucsi_notify_common(), the freed array is accessed:

drivers/usb/typec/ucsi/ucsi.c:ucsi_notify_common() {
    ...
    if (UCSI_CCI_CONNECTOR(cci)) {
        if (!ucsi->cap.num_connectors || ...)
            ucsi_connector_change(ucsi, UCSI_CCI_CONNECTOR(cci));
    }
    ...
}

And ucsi_connector_change() uses the dangling pointer:

drivers/usb/typec/ucsi/ucsi.c:ucsi_connector_change() {
    ...
    con = &ucsi->connector[num - 1];

    if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
        schedule_work(&con->work);
    ...
}

Should free_irq() be called before ucsi_unregister() to ensure the handler
is fully disabled first?

> +	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]
Does this array need to use named initializers?

The I2C subsystem guidelines request that initialized arrays of type
struct i2c_device_id use named initializers, such as:
{ .name = "ucsi_itepd", .driver_data = 0 }

> +	{}
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-ucsi-itepd-v6-0-d25ae12b181f@ite.com.tw?part=2