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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Unnecessary heavy workqueue allocation and teardown when a controller does not support IBIs.

Pre-existing issues:
- [Critical] Potential use-after-free of generic IBI pool slots and `dev->ibi` when disabling an active IBI fails during teardown.
--

commit f38c11b5d94a35f5b99f6a1f23bbb449f122ded4
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

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. Add controller-only helpers
that invoke the controller callbacks without updating the bus device list,
address-slot state, or generic IBI lifecycle. Also release the generic IBI
workqueue and state when the controller request callback fails.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 03fb41f0786c5..88379b6809a64 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
[ ... ]
> @@ -3600,13 +3752,9 @@ EXPORT_SYMBOL_GPL(i3c_dev_enable_ibi_locked);
>  int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
>  			       const struct i3c_ibi_setup *req)
>  {
> -	struct i3c_master_controller *master = i3c_dev_get_master(dev);
>  	struct i3c_device_ibi_info *ibi;
>  	int ret;
>  
> -	if (!master->ops->request_ibi)
> -		return -EOPNOTSUPP;
> -
>  	if (dev->ibi)
>  		return -EBUSY;
>  
[ ... ]
> @@ -3627,8 +3775,15 @@ int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
>  	ibi->num_slots = req->num_slots;
>  
>  	dev->ibi = ibi;
> -	ret = master->ops->request_ibi(dev, req);
> +	ret = i3c_dev_request_ibi_controller_locked(dev, req);
>  	if (ret) {
> +		/*
> +		 * The controller request callback failed, so tear down the
> +		 * workqueue allocated above before freeing the IBI object.
> +		 * This is the owner of the workqueue, so it must destroy it
> +		 * here to avoid leaking it on the error path.
> +		 */
> +		destroy_workqueue(ibi->wq);
>  		kfree(ibi);
>  		dev->ibi = NULL;
>  	}

[Severity: Medium]
Since the check for master->ops->request_ibi was moved into
i3c_dev_request_ibi_controller_locked(), this code now unconditionally
creates a new ordered workqueue during device probe even when the
underlying controller doesn't support IBIs.

Does this unnecessary allocation and destruction of a workqueue thread
(which involves sysfs registration and RCU synchronization) add a noticeable
performance delay for devices on controllers without IBI support?

[ ... ]
> @@ -3667,7 +3843,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: Critical]
This is a pre-existing issue, but can calling
i3c_dev_free_ibi_controller_locked() before destroy_workqueue(dev->ibi->wq)
cause a use-after-free?

If i3c_dev_disable_ibi_locked() fails (e.g. due to a bus timeout), the
teardown continues and invokes the controller's free_ibi callback, which
immediately frees the generic IBI pool memory.

If there are still pending IBI work items because destroy_workqueue() hasn't
been called to drain them yet, will the workqueue process them and
dereference the freed generic IBI slots?

Additionally, could an active hardware interrupt still fire and race with
the controller's free_ibi callback, leading to memory corruption inside the
interrupt handler?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826103819.1614843-1-lakshay.piplani@nxp.com?part=2