Re: [PATCH v3] drm/xe/i2c: cancel the client work on remove

[Rodrigo Vivi]

On Thu, Oct 01, 2026 at 03:34:40PM +0200, Heikki Krogerus wrote:
> On Tue, Sep 29, 2026 at 08:18:07AM +0200, Raag Jadav wrote:
> > + Heikki to comment on this.
> > 
> > On Mon, Sep 28, 2026 at 01:30:30AM +0000, Fan Wu wrote:
> > > xe_i2c_notifier() stores the DesignWare adapter in i2c->adapter and
> > > schedules i2c->work when the adapter is registered under the xe I2C
> > > platform device, and xe_i2c_client_work() then instantiates the AMC
> > > client device on that adapter.
> > > 
> > > xe_i2c_remove() tears down the AMC, unregisters the client devices,
> > > the bus notifier and the adapter platform device, but it never drains
> > > i2c->work. A work item that is still queued or running when the
> > > adapter is unregistered dereferences i2c->adapter in
> > > i2c_new_client_device() after platform_device_unregister() has
> > > released the adapter. The work item is also embedded in the
> > > devm-allocated struct xe_i2c, so a work item still queued after the
> > > drm device devm unwind frees that allocation runs its callback on
> > > freed memory.
> > > 
> > > The bus notifier is the only thing that schedules this work, and it is
> > > unregistered after the client devices. An instance that is still queued
> > > when the teardown runs can therefore write
> > > i2c->client[XE_I2C_CLIENT_AMC] while the loop is unregistering and
> > > clearing the same array, and an AMC client it instantiates late is only
> > > cleaned up by the adapter's own child sweep in i2c_del_adapter().
> > > 
> > > Move bus_unregister_notifier() in front of the client teardown loop
> > > and cancel the work right after it, so no new instance can be
> > > scheduled and a queued instance is drained before the client array is
> > > touched. A running instance still finds a live adapter, since the
> > > adapter is unregistered later.
> > > 
> > > This issue was found by an in-house static analysis tool.
> > > 
> > > Fixes: f0e53aadd702 ("drm/xe: Support for I2C attached MCUs")
> > > Link: https://lore.kernel.org/intel-xe/20260912085932.101598-1-fanwu01@xxxxxxxxxx/
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Assisted-by: Codex:gpt-5.6
> > > Co-developed-by: Song Li <songl@xxxxxxxxxx>
> > > Signed-off-by: Song Li <songl@xxxxxxxxxx>
> > > Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>
> 
> LGTM
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>

pushed to drm-xe-next after passing CI.

thank you all

> 
> > > ---
> > > 
> > > Changes in v3:
> > > - rebase onto drm-xe-next, after "drm/xe/i2c: Disable IRQ on unbind"
> > > - discussion: Link: above points at the v2 thread
> > > - unregister the bus notifier before the client teardown loop and
> > >   cancel the work before the loop as well: v2 cancelled the work only
> > >   after the loop, so an event arriving during the loop could still
> > >   schedule the work to race with the array teardown and leak a freshly
> > >   instantiated AMC client
> > >  drivers/gpu/drm/xe/xe_i2c.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> > > index f4f3819..b82caaf 100644
> > > --- a/drivers/gpu/drm/xe/xe_i2c.c
> > > +++ b/drivers/gpu/drm/xe/xe_i2c.c
> > > @@ -324,12 +324,15 @@ static void xe_i2c_remove(void *data)
> > >  	xe_i2c_irq_reset(xe);
> > >  	xe_amc_exit(i2c);
> > >  
> > > +	/* Stop the notifier from arming the client work before teardown. */
> > > +	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
> > > +	cancel_work_sync(&i2c->work);
> > > +
> > >  	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 = NULL;
> > >  }
> > > 
> 
> -- 
> heikki