Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [Critical] Calling a sleeping function `usleep_range()` inside `xe_i2c_disable()` triggers a 'BUG: scheduling while atomic' panic because it executes under a regmap spinlock. - [High] Keeping the hardware permanently enabled by intercepting IC_ENABLE causes critical configuration registers (clock timings, FIFO thresholds) to be silently dropped by the I2C controller. - [High] Intercepting IC_ENABLE breaks the DesignWare I2C ABORT command, preventing error recovery and causing spurious timeouts. - [High] Unhandled disable timeout drops IC_TAR writes, sending I2C messages to the wrong target. Pre-existing issues: - [High] Missing `cancel_work_sync()` in the teardown path leads to a potential Use-After-Free of `struct xe_i2c`. -- commit 468f953254a43b46d9448f5006c92c7e5fe3466a Author: Heikki Krogerus <[email protected]> drm/xe/mcu_i2c: Take over control of the controller enabling This commit intercepts IC_ENABLE register accesses to keep the I2C controller enabled, as some platforms tie the power state to this register. It temporarily disables the controller only when updating IC_CON, IC_TAR, or IC_SAR registers. > diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c > index 732ad6ee05e9c..45fa9094b6142 100644 > --- a/drivers/gpu/drm/xe/xe_i2c.c > +++ b/drivers/gpu/drm/xe/xe_i2c.c [ ... ] > @@ -261,11 +262,49 @@ static void xe_i2c_remove_irq(struct xe_i2c *i2c) > irq_domain_remove(i2c->irqdomain); > } > > +#define IC_CON 0x00 > +#define IC_TAR 0x04 > +#define IC_SAR 0x08 > +#define IC_ENABLE 0x6c > +#define IC_ENABLE_STATUS 0x9c > + > +/* See "Disabling DW_apb_i2c" in the DesignWare DW_abp_i2c databook. */ > +static int xe_i2c_disable(struct xe_i2c *i2c) > +{ > + int timeout = 100; > + u32 status; > + > + do { > + /* Disable.*/ > + xe_mmio_rmw32(i2c->mmio, XE_REG(IC_ENABLE + > I2C_MEM_SPACE_OFFSET), 1, 0); > + > + /* Verify. */ > + status = xe_mmio_read32(i2c->mmio, XE_REG(IC_ENABLE_STATUS + > I2C_MEM_SPACE_OFFSET)); > + if (!(status & 1)) > + return 0; > + > + /* Repeat. */ > + usleep_range(25, 250); [Severity: Critical] Does this sleeping function execute in an atomic context? The regmap instance for this I2C controller is configured with .fast_io = true in i2c_regmap_config, which means the regmap core protects register operations with a spinlock (spin_lock_irqsave). Since xe_i2c_disable() is called from the regmap write callback xe_i2c_write(), calling usleep_range() while holding this spinlock will trigger a scheduling while atomic panic. > + } while (timeout--); > + > + return -ETIMEDOUT; > +} > + > static int xe_i2c_read(void *context, unsigned int reg, unsigned int *val) > { > struct xe_i2c *i2c = context; > > - *val = xe_mmio_read32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET)); > + switch (reg) { > + case IC_ENABLE: > + *val = i2c->ic_enable; > + break; > + case IC_ENABLE_STATUS: > + *val = i2c->ic_enable & 1; /* NOTE: Checking only the enable > bit */ > + break; [Severity: High] Will returning a cached value here break the I2C ABORT sequence? When an I2C transfer stalls, the i2c-designware driver initiates error recovery by writing the ABORT bit (bit 1) to IC_ENABLE and polling the register until the hardware auto-clears the bit. If xe_i2c_read() returns the cached i2c->ic_enable value where the ABORT bit remains set, the polling loop in i2c_dw_xfer() will unconditionally time out. > + default: > + *val = xe_mmio_read32(i2c->mmio, XE_REG(reg + > I2C_MEM_SPACE_OFFSET)); > + break; > + } > > return 0; > } > @@ -273,8 +312,30 @@ static int xe_i2c_read(void *context, unsigned int reg, > unsigned int *val) > static int xe_i2c_write(void *context, unsigned int reg, unsigned int val) > { > struct xe_i2c *i2c = context; > + int ret; > > - xe_mmio_write32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET), val); > + switch (reg) { > + case IC_CON: > + case IC_TAR: > + case IC_SAR: > + /* Disable the controller. */ > + ret = xe_i2c_disable(i2c); > + if (ret) > + return ret; > + > + /* Write the register. */ > + xe_mmio_write32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET), > val); [Severity: High] If xe_i2c_disable() times out, is it safe to skip the register write? The i2c-designware master driver ignores the return value of regmap_write() when programming the target address (DW_IC_TAR). If this write is skipped due to a timeout, the driver will proceed to transmit the message using the previously configured target address, which could send data to the wrong peripheral. > + > + /* Enable the controller. */ > + xe_mmio_rmw32(i2c->mmio, XE_REG(IC_ENABLE + > I2C_MEM_SPACE_OFFSET), 0, 1); > + break; > + case IC_ENABLE: > + i2c->ic_enable = val; > + break; [Severity: High] Does caching the IC_ENABLE write prevent error recovery? Because the value is only cached in i2c->ic_enable and not forwarded to the hardware, the physical controller will never execute the abort sequence requested by the i2c-designware driver. > + default: > + xe_mmio_write32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET), > val); > + break; [Severity: High] Will the hardware ignore writes to timing and configuration registers? The DesignWare APB I2C hardware requires the controller to be disabled before modifying configuration registers like clock timings (IC_*_SCL_HCNT) and FIFO thresholds. Since these registers fall through to this default case, they are sent to the hardware while it is still actively enabled. The hardware drops these writes, which could leave the controller operating with incorrect default settings after system resume. > + } > > return 0; > } drivers/gpu/drm/xe/xe_i2c.c:xe_i2c_remove() { ... for (i = 0; i < XE_I2C_MAX_CLIENTS; i++) i2c_unregister_device(i2c->client[i]); 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 bug introduced by this patch, but is there a missing cancel_work_sync() in the teardown path? The probe function initializes i2c->work, which is scheduled asynchronously. Since struct xe_i2c is managed by devres, it will be freed immediately after xe_i2c_remove() returns. If the work is still pending or executing, it could dereference the freed i2c memory, leading to a use-after-free. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
