Re: [PATCH 1/2] drm/xe/i2c: Not for review, Squash of I2C/AMC series

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

New issues:
- [High] Interrupt storm due to missing IRQ masking for SMBus Alert.
- [High] Use-After-Free due to dangling pointer and missing synchronization in `xe_amc_exit`.
- [High] `xe_i2c_write` forces `DW_IC_ENABLE` to remain enabled, silently breaking I2C timing configuration.
- [High] Data race on I2C_CONFIG_CMD MMIO register due to non-atomic read-modify-write across contexts.

Pre-existing issues:
- [High] Use-After-Free due to uncancelled asynchronous work during I2C teardown.
- [High] Missing SR-IOV VF guard in I2C probe allows Virtual Functions to access PF-only resources.
--

--- Patch [1]: [PATCH 1/2] drm/xe/i2c: Not for review, Squash of I2C/AMC series ---
commit 067320578064c382a7fa5cb790273a2f63391cd5
Author: Heikki Krogerus <[email protected]>

drm/xe/i2c: Not for review, Squash of I2C/AMC series

This commit squashes the I2C and AMC patch series for the Intel Xe driver.
It introduces the Add-In Management Controller (AMC) alert handler and
integrates DesignWare I2C register definitions.

Link: https://patchwork.freedesktop.org/series/168930/

> diff --git a/drivers/gpu/drm/xe/xe_amc.c b/drivers/gpu/drm/xe/xe_amc.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_amc.c
[ ... ]
> +static void xe_amc_work(struct work_struct *work)
> +{
[ ... ]
> +	alert_reason = response.value;
> +	dev_dbg(&client->dev, "Alert reason: %d\n", alert_reason);
> +
> +out_reassert_interrupt:
> +	xe_mmio_rmw32(amc->i2c->mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);

[Severity: High]
Does this introduce a data race regression with the hardirq handler?
xe_amc_work() executes in process context, while
xe_i2c_handle_smbus_alert() executes in hardirq context. Both perform an
unprotected xe_mmio_rmw32() on the I2C_CONFIG_CMD register.

Because xe_mmio_rmw32() performs a non-atomic read, bitwise modify, and
write sequence, if the hardirq preempts the workqueue exactly between the
read and write steps, the hardirq's modifications will be clobbered when
the workqueue resumes and writes back its stale value. Does this need
locking or atomic bit operations?

[ ... ]
> +void xe_amc_exit(struct xe_i2c *i2c)
> +{
> +	if (i2c->amc) {
> +		cancel_work_sync(&i2c->amc->work);
> +		kfree(i2c->amc);
> +	}
> +}

[Severity: High]
Can this code leave a dangling pointer that causes a use-after-free
regression? When i2c->amc is freed, the pointer is not set to NULL, and
there is no synchronization with the active IRQ handler.

If an I2C interrupt fires concurrently, xe_i2c_handle_smbus_alert() will
observe i2c->amc as non-NULL and call xe_amc_handle_alert(), which will
execute queue_work() on the freed memory. Should i2c->amc be set to NULL
here, combined with proper IRQ synchronization?

> 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
[ ... ]
> +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);
> +
> +	if (stat & DW_IC_SMBUS_INTR_ALERT && i2c->amc)
> +		xe_amc_handle_alert(i2c);
> +	else
> +		xe_mmio_rmw32(i2c->mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);
> +}

[Severity: High]
Can this branch cause an interrupt storm regression? The AMC asserts the
level-triggered SMBus Alert line. When the alert is present, this handler
queues a work item via xe_amc_handle_alert(), but does not set
PCI_COMMAND_INTX_DISABLE to mask the INTx at the PCI level.

Since the AMC holds the physical line low until the workqueue runs,
returning from this handler without masking the interrupt will cause the
DW I2C controller to immediately re-assert the interrupt. Would it be safer
to mask the interrupt before returning?

[ ... ]
> @@ -267,8 +258,33 @@ static int xe_i2c_write(void *context, unsigned int reg, unsigned int val)
>  {
>  	struct xe_i2c *i2c = context;
>  
> -	xe_mmio_write32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET), val);
> +	switch (reg) {
[ ... ]
> +	case DW_IC_ENABLE:
> +		i2c->ic_enable = val;
> +		/* Other fields can be updated except the enable bit. */
> +		val |= DW_IC_ENABLE_ENABLE;
> +		break;

[Severity: High]
Does forcing this enable bit to 1 introduce a regression that silently
breaks the I2C timing configuration?

The DesignWare core driver configures I2C timings (such as
DW_IC_SS_SCL_HCNT), which strictly requires the controller to be disabled
first. It attempts to disable the controller by writing 0 to DW_IC_ENABLE.

However, by intercepting this write and forcing DW_IC_ENABLE_ENABLE to 1,
the controller remains enabled. Will the hardware silently ignore all
subsequent writes to the timing registers because of this?

[ ... ]
> @@ -310,12 +326,15 @@ static void xe_i2c_remove(void *data)
>  	struct xe_i2c *i2c = data;
>  	unsigned int i;
>  
> -	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++)
> +	xe_amc_exit(i2c);
> +
> +	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);
> -	xe_i2c_remove_irq(i2c);
>  }

[Severity: High]
This isn't a regression introduced by this patch, but does this function
leave asynchronous work uncancelled during I2C teardown?

The I2C bus notifier schedules i2c->work (xe_i2c_client_work)
asynchronously, but this teardown function unregisters the notifier and
adapter without calling cancel_work_sync(&i2c->work). Since the devres
managed i2c structure is freed shortly after, could a pending work
execution access freed memory if it runs after this completes?

[ ... ]
> @@ -363,22 +382,18 @@ int xe_i2c_probe(struct xe_device *xe)
>  	if (ret)
>  		return ret;
>  
> -	ret = xe_i2c_create_irq(xe);
> -	if (ret)
> -		goto err_unregister_notifier;
> -
>  	ret = xe_i2c_register_adapter(i2c);

[Severity: High]
This isn't a regression introduced by this patch, but does this function
lack the required SR-IOV Virtual Function (VF) guard before accessing
hardware?

A Virtual Function attempting to load the driver will call xe_i2c_probe(),
which accesses PF-only I2C controller MMIO resources and registers devices.
Without an IS_SRIOV_VF(xe) early return check, can this lead to hardware
failures, MMIO timeouts, or incorrect driver behavior in an unprivileged
VF context?

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