Re: [PATCH v5 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 3 potential issue(s) to consider:
- [High] Stack-allocated buffers are passed directly to `i2c_transfer`, violating DMA constraints.
- [High] Race condition during driver removal leads to a use-after-free of `ucsi->connector` due to improper teardown ordering.
- [High] Unhandled hardware interrupts or I2C read failures return IRQ_HANDLED, causing an interrupt storm.
--

--- Patch [2]: [PATCH v5 2/2] usb: typec: ucsi: Add ITE IT8851/IT8853 Type-C PD controller driver ---
commit e9bebde0ac95c5792e010b2c998820bde558bc90
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. The driver communicates with the
controller over an I2C interface and forwards UCSI events to the core.

> diff --git a/drivers/usb/typec/ucsi/ucsi_itepd.c b/drivers/usb/typec/ucsi/ucsi_itepd.c
> new file mode 100644
> index 0000000000000..b95f5f9b91228
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_itepd.c
[ ... ]
> +static int itepd_read_reg(struct itepd *itepd, u8 reg, void *data, u32 len)
> +{
> +	struct i2c_client *client = itepd->client;
> +	struct i2c_msg msg[2] = {
> +		{
> +			.addr	= client->addr,
> +			.flags	= 0,
> +			.len	= 1,
> +			.buf	= &reg,
> +		},

[Severity: High]
Does this code pass a stack-allocated buffer to the I2C subsystem?

The reg pointer and the data argument (which points to local stack variables
like event and le_cci in caller functions) are passed directly into the
i2c_msg array. 

When i2c_transfer() passes these structures to the underlying I2C host
controller driver, it may map them for DMA. Since the stack is often
virtually mapped in the kernel (CONFIG_VMAP_STACK=y), passing stack memory
to the DMA API causes mapping failures. 

Depending on the platform, this can trigger kernel panics, DMA API debug
warnings, or silent memory corruption.

> +		{
> +			.addr	= client->addr,
> +			.flags	= I2C_M_RD,
> +			.len	= len,
> +			.buf	= data,
> +		}
> +
> +	};
> +	int ret;
[ ... ]
> +static int itepd_write_reg(struct itepd *itepd, u8 reg, const void *data, u32 len)
> +{
> +	struct i2c_client *client = itepd->client;
> +	u8 buf[sizeof(u64) + 1];
> +	struct i2c_msg msg[1] = {
> +		{
> +			.addr	= client->addr,
> +			.flags	= 0,
> +			.len	= len + 1,
> +			.buf	= buf,
> +		}

[Severity: High]
Similarly, buf is a local stack array used as the write buffer for
i2c_transfer(). 

Is it possible to allocate these buffers dynamically or use a pre-allocated
DMA-safe buffer in the itepd structure to avoid DMA mapping issues on stack
memory?

[ ... ]
> +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 this return value cause an interrupt storm if I2C communication fails?

If itepd_process_event() fails due to an I2C error, it propagates a negative
error code. At that point, the hardware interrupt source has not been
acknowledged or cleared via a write to the ITEPD_VENDOR_WC_INT register.

By returning IRQ_HANDLED, the handler falsely signals to the IRQ subsystem
that the interrupt was successfully processed. For a level-triggered interrupt,
this prevents the kernel's spurious interrupt detector from disabling the
unhandled IRQ, leading to an infinite interrupt loop that can starve the CPU.

Should this return IRQ_NONE on failure instead to allow the spurious IRQ
detector to properly mask it?

> +
> +	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]
Is there a race condition between tearing down the UCSI subsystem and freeing
the interrupt?

ucsi_unregister() sends an asynchronous command to disable notifications and
frees the ucsi->connector array. Since free_irq() has not yet been called to
synchronize and disable the IRQ thread, itepd_irq_thread_fn() can run
concurrently.

If the interrupt fires after ucsi->connector is freed, the IRQ handler will
call ucsi_notify_common(), which reads the freed ucsi->connector array. 

Should free_irq() be called before ucsi_unregister() to guarantee that no
interrupts are processed while the subsystem dismantles its structures?

> +	ucsi_destroy(itepd->ucsi);
> +}

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