Re: [PATCH v15 2/8] i3c: master: Add controller-only device operation helpers

On Mon, Aug 17, 2026 at 10:50:59AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [Low] Incomplete refactoring: `i3c_master_handle_ibi()` bypasses the newly introduced `i3c_dev_recycle_ibi_slot_controller()` helper.
>
> Pre-existing issues:
> - [High] Missing memory barrier in `i3c_dev_disable_ibi_locked` causes a lost wakeup and indefinite hang.
> - [High] Use-after-free on error path in `i3c_dev_free_ibi_locked` when workqueue is drained after resources are freed.
> --
>
> --- Patch [2]: [PATCH v15 2/8] i3c: master: Add controller-only device operation helpers ---
> commit 85e9cb3ce264f7889b2b4406bc379307ad054a14
> Author: Lakshay Piplani <[email protected]>
>
> i3c: master: Add controller-only device operation helpers
>
> The generic I3C device attach and IBI paths perform both core-level
> bookkeeping and controller-specific operations. An I3C hub, however,
> maintains a logical device descriptor on the virtual downstream bus and
> a separate parent-facing descriptor used by the physical parent
> controller.
>
> >     Add helpers for:
> >       - attaching, reattaching, and detaching a device from a controller;
> >       - requesting and freeing controller IBI resources;
> >       - enabling and disabling IBIs at the controller;
> >       - recycling controller IBI slots.
> >
> >     Refactor the existing generic core paths to use these helpers, keeping
> >     the current behaviour unchanged for regular I3C devices.
>
> [Severity: Low]
> The commit message mentions refactoring the existing generic core paths to
> use the new controller-only helpers, including the one for recycling IBI
> slots.
>
> However, looking at the code for i3c_master_handle_ibi(), it appears to
> still bypass the newly introduced wrapper:
>
> drivers/i3c/master.c:i3c_master_handle_ibi() {
>         ...
>         if (master->ops->recycle_ibi_slot)
>                 master->ops->recycle_ibi_slot(dev, slot);
>         ...
> }
>
> Does this function need to be updated to use
> i3c_dev_recycle_ibi_slot_controller() to ensure the virtual controllers
> in the new I3C hub framework can properly intercept these operations?

Yes, please update it also.

Frank
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 03fb41f0786c5..7ae6b3aaa9b4c 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
>
> [ ... ]
>
> > @@ -3534,14 +3632,9 @@ EXPORT_SYMBOL_GPL(i3c_dev_do_xfers_locked);
> >   */
> >  int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev)
> >  {
> > -	struct i3c_master_controller *master;
> >  	int ret;
> >
> > -	if (!dev->ibi)
> > -		return -EINVAL;
> > -
> > -	master = i3c_dev_get_master(dev);
> > -	ret = master->ops->disable_ibi(dev);
> > +	ret = i3c_dev_disable_ibi_controller_locked(dev);
> >  	if (ret)
> >  		return ret;
>
> [Severity: High]
> This is a pre-existing issue, but does this sequence lack a memory barrier
> before checking the pending IBIs?
>
> Right after this return, the code does:
>
> drivers/i3c/master.c:i3c_dev_disable_ibi_locked() {
>         ...
>         reinit_completion(&dev->ibi->all_ibis_handled);
>         if (atomic_read(&dev->ibi->pending_ibis))
>                 wait_for_completion(&dev->ibi->all_ibis_handled);
>         ...
> }
>
> Since reinit_completion() performs a plain store that can be delayed or
> reordered after the atomic_read(), could a concurrent IBI worker
> decrementing the pending count and calling complete() race with this?
>
> If the worker executes between the load of pending_ibis and the delayed
> store, could the store overwrite the completion state back to 0, causing
> wait_for_completion() to hang indefinitely?
>
> [ ... ]
>
> > @@ -3667,7 +3835,7 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
> >  			dev_err(&master->dev, "Failed to disable IBI before freeing\n");
> >  	}
> >
> > -	master->ops->free_ibi(dev);
> > +	i3c_dev_free_ibi_controller_locked(dev);
> >
> >  	if (dev->ibi->wq) {
> >  		destroy_workqueue(dev->ibi->wq);
>
> [Severity: High]
> This is a pre-existing issue, but does this cleanup order create a
> use-after-free risk during an error path?
>
> If disabling the IBI fails (for example, due to a hardware timeout), the
> error path continues and calls i3c_dev_free_ibi_controller_locked(),
> which frees the underlying controller resources like the generic IBI pool.
>
> Then destroy_workqueue() is called to drain the workqueue. If there are
> pending or executing IBI works still in the workqueue, won't they try to
> access the freed slot pool when they run, resulting in a use-after-free?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c