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 <[email protected]> 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/[email protected]?part=2
