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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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@zju.edu.cn/
> > Cc: stable@vger.kernel.org
> > Assisted-by: Codex:gpt-5.6
> > Co-developed-by: Song Li <songl@zju.edu.cn>
> > Signed-off-by: Song Li <songl@zju.edu.cn>
> > Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>

LGTM

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> > ---
> > 
> > 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