Re: [PATCH v1 1/2] drm/xe/i2c: Handler for SMBus Alerts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Calling `i2c_new_smbus_alert_device()` with an uninitialized `i2c->adapter` causes a kernel crash during probe.
- [High] `xe_i2c_remove()` unregisters the SMBus alert device twice, leading to a use-after-free and kernel crash.
- [High] The driver will fail to link (`undefined reference`) when `CONFIG_DRM_XE=y` and `CONFIG_I2C_SMBUS=m`.
- [High] Using `regmap_write()` to clear the SMBus alert interrupt register fails to clear it, resulting in an interrupt storm.
- [High] The SMBus alert client device is unregistered and freed during driver removal while the I2C hardirq handler remains active, causing a use-after-free.
- [High] Calling regmap APIs in the hardirq handler without raw spinlocks causes a "sleeping in atomic context" BUG on PREEMPT_RT kernels.
- [Medium] Disabling `CONFIG_I2C_SMBUS` causes the entire Xe driver probe to fail on Crescent Island.
--

commit 4babbf281e90eb4de9750bdaf56676cc4600f883
Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>

drm/xe/i2c: Handler for SMBus Alerts

Some devices that are attached to the I2C controller use the
SMBus Alert signal for example to inform the host about
thermal events, so registering the default SMBus Alert
device device for them. The alert device makes sure that
the alert is processed and passed to the correct I2C client
driver.

> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 8e7b146880f46..9c8af00cf37fd 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -153,6 +153,7 @@ xe-y += xe_bb.o \
>  	xe_wopcm.o
>  
>  xe-$(CONFIG_I2C)	+= xe_i2c.o
> +xe-$(CONFIG_I2C_SMBUS)	+= xe_i2c_smbus.o

[Severity: High]
Will this cause a build failure when CONFIG_DRM_XE=y and CONFIG_I2C_SMBUS=m?

If CONFIG_I2C_SMBUS is built as a module, xe_i2c_smbus.o is added to the xe-m
target. However, IS_ENABLED(CONFIG_I2C_SMBUS) in xe_i2c.h will still evaluate
to true, causing the built-in driver to expect the functions and resulting in
an undefined reference at link time.

> diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> index 706783863d07d..732ad6ee05e9c 100644
> --- a/drivers/gpu/drm/xe/xe_i2c.c
> +++ b/drivers/gpu/drm/xe/xe_i2c.c
[ ... ]
> @@ -182,7 +183,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->i2c->smbus_alert)
> +		xe_i2c_handle_smbus_alert(xe->i2c);

[Severity: High]
Is there a race condition here during driver teardown that could cause a
use-after-free?

During teardown, the smbus_alert device is unregistered and freed, but the
i2c->smbus_alert pointer is not set to NULL. If a hardware interrupt fires
concurrently before the IRQ is completely disabled, this handler will
evaluate xe->i2c->smbus_alert as true and dereference the freed pointer.

> +	else
> +		generic_handle_irq_safe(xe->i2c->adapter_irq);
>  
>  	/* Deassert after I2C adapter clears the interrupt */
[ ... ]
> @@ -232,6 +236,9 @@ static int xe_i2c_create_irq(struct xe_device *xe)
>  	    xe_survivability_mode_is_boot_enabled(xe))
>  		return 0;
>  
> +	if (xe->info.platform == XE_CRESCENTISLAND)
> +		return xe_i2c_register_smbus_alert(i2c);

[Severity: High]
Does this pass an uninitialized adapter to the SMBus registration?

This call happens during xe_i2c_probe() before the adapter is fully
initialized and registered. The i2c->adapter pointer is still NULL at
this point, which will cause i2c_new_smbus_alert_device() to crash when
it dereferences the adapter.

[Severity: Medium]
Should the driver fail to probe entirely if SMBus is disabled?

If the kernel is built without CONFIG_I2C_SMBUS, the stub for
xe_i2c_register_smbus_alert() returns -EOPNOTSUPP. Returning that error
here propagates up and causes the entire GPU driver probe to fail on this
platform.

> +
>  	domain = irq_domain_create_linear(dev_fwnode(i2c->drm_dev), 1, &xe_i2c_irq_ops, NULL);
>  	if (!domain)
>  		return -ENOMEM;
> @@ -244,6 +251,9 @@ static int xe_i2c_create_irq(struct xe_device *xe)
>  
>  static void xe_i2c_remove_irq(struct xe_i2c *i2c)
>  {
> +	if (i2c->smbus_alert)
> +		i2c_unregister_device(i2c->smbus_alert);
> +

[Severity: High]
Does this trigger a double-free of the alert device?

During device removal, xe_i2c_remove() unregisters the adapter before
calling xe_i2c_remove_irq(). Unregistering the adapter synchronously removes
and frees all child clients, including the SMBus alert device. Calling
i2c_unregister_device() on it again here will access freed memory.

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_i2c_smbus.c b/drivers/gpu/drm/xe/xe_i2c_smbus.c
> new file mode 100644
> index 0000000000000..185a6d2f0a508
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_i2c_smbus.c
[ ... ]
> +void xe_i2c_handle_smbus_alert(struct xe_i2c *i2c)
> +{
> +	u32 stat;
> +
> +	if (!i2c->smbus_alert)
> +		return;
> +
> +	regmap_read(i2c->regmap, DW_IC_SMBUS_INTR_STAT, &stat);

[Severity: High]
Can this sleep in atomic context?

This function is called from xe_i2c_irq_handler() which runs in hardirq
context. The regmap initialized for the I2C controller does not set
use_raw_spinlock to true.

On PREEMPT_RT kernels, the standard spinlock used by regmap becomes a
sleepable mutex, which will trigger a bug when acquired from hardirq context.

> +	if (!stat)
> +		return;
> +
> +	regmap_write(i2c->regmap, DW_IC_CLR_SMBUS_INTR, stat);

[Severity: High]
Does writing to this register properly clear the interrupt?

In Synopsys DesignWare I2C controllers, interrupt clear registers are
typically read-to-clear. A write operation is usually ignored by the
hardware. If the interrupt is not cleared, it will remain asserted and
cause an interrupt storm when the handler returns.

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