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

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

[Raag Jadav]

+ 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>
> ---
> 
> 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;
>  }
> 

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

[Heikki Krogerus]

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

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

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