Re: [PATCH v2 2/3] drm/xe/i2c: Handler for SMBus Alerts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Unconditional dereference of a potentially NULL I2C client pointer when handling SMBus alerts.
- [High] Normal I2C adapter interrupts are not propagated to the adapter driver on the XE_CRESCENTISLAND platform, breaking standard I2C transfers.
- [High] Writing to a read-to-clear interrupt status register will fail to clear the interrupt, leading to an interrupt storm.
- [Low] DRM_XE unconditionally selects I2C_SMBUS, causing unmet dependencies and build failures when I2C is not enabled.

Pre-existing issues:
- [High] Missing `cancel_work_sync()` in the device cleanup path allows pending asynchronous work to access freed memory.
--

commit 99e285e8af69e8587ef5c6dca5bee3189103c64e
Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Subject: drm/xe/i2c: Handler for SMBus Alerts

This commit adds support for handling SMBus alerts in the XE I2C driver.
It introduces a new client for the alert device and an interrupt handling
path to forward alerts to the SMBus core.

> diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> --- a/drivers/gpu/drm/xe/Kconfig
> +++ b/drivers/gpu/drm/xe/Kconfig
> @@ -46,6 +46,7 @@ config DRM_XE
>  	select WANT_DEV_COREDUMP
>  	select AUXILIARY_BUS
>  	select REGMAP if I2C
> +	select I2C_SMBUS

[Severity: Low]
Does this select statement need to be guarded conditionally or have a
dependency on I2C added to DRM_XE?

Selecting I2C_SMBUS unconditionally without inheriting its dependency on I2C
can cause unmet dependency warnings and build failures when I2C is disabled.

> diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> --- a/drivers/gpu/drm/xe/xe_i2c.c
> +++ b/drivers/gpu/drm/xe/xe_i2c.c
[ ... ]
> @@ -61,6 +65,37 @@ static inline void xe_i2c_read_endpoint(struct xe_mmio *mmio, void *ep)
>  	val[1] = xe_mmio_read32(mmio, REG_SG_REMAP_ADDR_POSTFIX);
>  }
>  
> +static void xe_i2c_handle_smbus_alert(struct xe_i2c *i2c)
> +{
> +	u32 stat;
> +
> +	stat = xe_mmio_read32(i2c->mmio, I2C_REG(DW_IC_SMBUS_INTR_STAT));
> +	if (!stat)
> +		return;
> +
> +	xe_mmio_write32(i2c->mmio, I2C_REG(DW_IC_CLR_SMBUS_INTR), stat);

[Severity: High]
Will this successfully clear the interrupt?

Synopsys DesignWare I2C DW_IC_CLR_* registers typically use read-to-clear
semantics. Writing to this register might not clear the hardware interrupt,
potentially leading to an interrupt storm when the ISR returns.

> +
> +	if (stat & DW_IC_SMBUS_INTR_ALERT)
> +		i2c_handle_smbus_alert(i2c->client[XE_I2C_CLIENT_ALERT]);

[Severity: Critical]
Is it possible for i2c->client[XE_I2C_CLIENT_ALERT] to be NULL here?

The client registration happens asynchronously in xe_i2c_client_work(). If an
SMBus alert hardware interrupt fires before the async workqueue executes, or
if the client registration fails, passing a NULL pointer to
i2c_handle_smbus_alert() will cause a NULL pointer dereference.

> +}
[ ... ]
> @@ -182,7 +222,10 @@ void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl)
>  		return;
>  
>  	/* Forward interrupt to I2C adapter */
> -	generic_handle_irq_safe(xe->i2c->adapter_irq);
> +	if (xe->info.platform == XE_CRESCENTISLAND)
> +		xe_i2c_handle_smbus_alert(xe->i2c);
> +	else
> +		generic_handle_irq_safe(xe->i2c->adapter_irq);

[Severity: High]
Does this prevent regular I2C adapter interrupts from being handled on the
XE_CRESCENTISLAND platform?

By placing generic_handle_irq_safe() in the else branch, it seems normal I2C
transfers might time out and fail because their hardware interrupts are
entirely skipped on this platform.

>  	/* Deassert after I2C adapter clears the interrupt */
>  	xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, 0, PCI_COMMAND_INTX_DISABLE);
[ ... ]
> @@ -307,8 +350,10 @@ static void xe_i2c_remove(void *data)
>  	struct xe_i2c *i2c = data;
>  	unsigned int i;
>  
> -	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++)
> +	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++) {
>  		i2c_unregister_device(i2c->client[i]);
> +		i2c->client[i] = NULL;
> +	}
>  
>  	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
>  	xe_i2c_unregister_adapter(i2c);

[Severity: High]
This is a pre-existing issue, but is there a missing call to
cancel_work_sync() in this teardown path?

The asynchronous workqueue item i2c->work is scheduled when the I2C adapter
is added. If this device is removed while the work is still pending or
running, xe_i2c_client_work() could dereference freed memory after
xe_i2c_remove() completes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625125939.429078-1-heikki.krogerus@linux.intel.com?part=2