Re: [PATCH V2 4/8] i3c: master: Add helper to query bus wakeup requirements

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless access to other devices' `ibi` structures in `adi_i3c_master_disable_ibi()` leads to Use-After-Free.
--

--- Patch [4]: [PATCH V2 4/8] i3c: master: Add helper to query bus wakeup requirements ---
commit 26d68568db9401a6324a05672a046e8465035dde
Author: Adrian Hunter <adrian.hunter@intel.com>

i3c: master: Add helper to query bus wakeup requirements

Adds i3c_master_any_wakeup_enabled() to iterate over devices on an I3C
bus and report whether any are enabled for system wakeup and have ibi
enabled. Controller drivers can use this helper to determine whether
wakeup support must remain available while the system is suspended.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 9d56ca08569a1..dd49652425b48 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -2048,6 +2048,36 @@ static void i3c_master_reg_work_fn(struct work_struct *work)
>  	i3c_master_register_new_i3c_devs(master);
>  }
>  
> +/**
> + * i3c_master_any_wakeup_enabled() - check if any device can wake the system
> + * @master: I3C master controller
> + *
> + * Iterate over devices on the bus and return true if any device has
> + * system wakeup enabled and IBI enabled.
> + *
> + * Return: true if any device may wake the system via IBI, false otherwise.
> + */
> +bool i3c_master_any_wakeup_enabled(struct i3c_master_controller *master)
> +{
> +	struct i3c_dev_desc *desc;
> +	bool wakeup = false;
> +
> +	i3c_bus_normaluse_lock(&master->bus);
> +	i3c_bus_for_each_i3cdev(&master->bus, desc) {
> +		if (!desc->dev || !device_may_wakeup(&desc->dev->dev))
> +			continue;
> +		guard(mutex)(&desc->ibi_lock);

[Severity: High]
This is a pre-existing issue, but seeing the correct locking here in
i3c_master_any_wakeup_enabled() brings to mind a similar loop in
adi_i3c_master_disable_ibi() in drivers/i3c/master/adi-i3c-master.c.

Does that loop also need to hold the ibi_lock? It currently accesses
i3cdev->ibi of other devices on the bus without holding their respective
ibi_lock:

drivers/i3c/master/adi-i3c-master.c:adi_i3c_master_disable_ibi() {
    ...
    i3c_bus_for_each_i3cdev(&m->bus, i3cdev) {
        if (dev != i3cdev && i3cdev->ibi)
            enabled |= i3cdev->ibi->enabled;
    }
    ...
}

Could this lead to a use-after-free if another device's ibi resources are
concurrently freed? For example, if i3c_device_free_ibi() is called for
another device during concurrent unbinds, it takes the same bus read lock
but could free the ibi pointer while this loop is evaluating
i3cdev->ibi->enabled.

> +		if (desc->ibi && desc->ibi->enabled) {
> +			wakeup = true;
> +			break;
> +		}
> +	}
> +	i3c_bus_normaluse_unlock(&master->bus);
> +
> +	return wakeup;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728155308.142713-1-adrian.hunter@intel.com?part=4

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c