Re: [PATCH V2 7/7] i3c: master: Reconcile dynamic addresses after DAA

[Adrian Hunter]

On 08/06/2026 21:06, Frank Li wrote:
> On Mon, Jun 08, 2026 at 10:58:00AM +0300, Adrian Hunter wrote:
>> After Dynamic Address Assignment (DAA), there may be cases where
>> devices have been assigned dynamic addresses on the bus, but are not
>> successfully registered in the device model.  This can happen, for
>> example, if errors occur during device addition, leaving the bus state
>> and software state inconsistent.
>>
>> Introduce a reconciliation step to resolve such inconsistencies.
>>
>> Scan all address slots marked as I3C devices by the bus, and compare
>> them against the set of devices currently registered.  For any dynamic
>> address that is marked occupied but has no corresponding i3c_dev_desc,
>> probe for device presence using a GETSTATUS CCC.
>>
>> Retry the probe (with exponential backoff delay) to handle transient NACK
>> conditions.  If a device responds, register it via
>> i3c_master_add_i3c_dev_locked().  Otherwise, free the address
>> slot so it may be reused in future DAA operations.
>>
>> Note, i3c_master_add_i3c_dev_locked() may fail (again), in which case the
>> dynamic address remains marked as occupied.  A future DAA will try again.
>>
>> This also handles a corner case where a device is assigned a dynamic
>> address but not successfully added, and subsequently loses that address
>> (e.g. due to power management).  If DAA is run again, the device may
>> receive a new dynamic address while the old one remains marked as
>> occupied.  Repeated occurrences of this scenario could eventually
>> exhaust the dynamic address space.  The reconciliation step ensures that
>> stale addresses are detected and freed, preventing address leakage.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>> ---
>>
>>
>> Changes in V2:
>>
>> 	Add bitmap.h include for bitmap_zero() etc.
>> 	Re-base due to changes in previous patches.
>>
>>
>>  drivers/i3c/master.c | 115 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 113 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index 5021d25b718a..6656c8591fab 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
>> @@ -6,7 +6,9 @@
>>   */
>>
>>  #include <linux/atomic.h>
>> +#include <linux/bitmap.h>
>>  #include <linux/bug.h>
>> +#include <linux/delay.h>
>>  #include <linux/device.h>
>>  #include <linux/dma-mapping.h>
>>  #include <linux/err.h>
>> @@ -1613,6 +1615,56 @@ static int i3c_master_retrieve_dev_info(struct i3c_dev_desc *dev)
>>  	return 0;
>>  }
>>
>> +static int i3c_master_getstatus_locked(struct i3c_master_controller *master,
>> +				       u8 addr, u16 *status)
>> +{
>> +	struct i3c_ccc_getstatus *getstatus;
>> +	struct i3c_ccc_cmd_dest dest;
>> +	struct i3c_ccc_cmd cmd;
>> +	int ret;
>> +
>> +	getstatus = i3c_ccc_cmd_dest_init(&dest, addr, sizeof(*getstatus));
>> +	if (!getstatus)
>> +		return -ENOMEM;
>> +
>> +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETSTATUS, &dest, 1);
>> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
>> +	if (ret)
>> +		goto out;
>> +
>> +	if (dest.payload.len != sizeof(*getstatus)) {
>> +		ret = -EIO;
>> +		goto out;
>> +	}
>> +
>> +	if (status)
>> +		*status = be16_to_cpu(getstatus->status);
>> +out:
>> +	i3c_ccc_cmd_dest_cleanup(&dest);
>> +
>> +	return ret;
>> +}
>> +
>> +#define I3C_DEV_PROBE_INITIAL_DELAY_US	20
>> +#define I3C_DEV_PROBE_DELAY_FACTOR	2
>> +#define I3C_DEV_PROBE_CNT		5

They are somewhat arbitrary, but should give a device enough
opportunity to respond.

> 
> what's these value coming from? from i3c spec?
> 
>> +
>> +static bool i3c_master_i3c_dev_present(struct i3c_master_controller *master, unsigned int addr)
>> +{
>> +	int delay = I3C_DEV_PROBE_INITIAL_DELAY_US;
>> +
>> +	for (int i = 0; i < I3C_DEV_PROBE_CNT; i++) {
>> +		if (i) {
>> +			fsleep(delay);
>> +			delay *= I3C_DEV_PROBE_DELAY_FACTOR;
>> +		}
>> +		if (!i3c_master_getstatus_locked(master, addr, NULL))
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
> ...
>>
>> +static void i3c_master_reconcile_dyn_addrs(struct i3c_master_controller *master)
>> +{
>> +	DECLARE_BITMAP(dev_dyn_addrs, I2C_MAX_ADDR + 1);
>> +	enum i3c_addr_slot_status status;
>> +	struct i3c_dev_desc *desc;
>> +
>> +	/* Mark all devices' dynamic addresses in the bitmap */
>> +	bitmap_zero(dev_dyn_addrs, I2C_MAX_ADDR + 1);
>> +	i3c_bus_for_each_i3cdev(&master->bus, desc)
>> +		__set_bit(desc->info.dyn_addr, dev_dyn_addrs);

This misses static addresses.  I will add:

		if (desc->info.static_addr)
			__set_bit(desc->info.static_addr, dev_dyn_addrs);

>> +	/* Reconcile the bitmap with the bus address slot status */
>> +	for (unsigned int addr = 0; addr <= I2C_MAX_ADDR; addr++) {
>> +		status = i3c_bus_get_addr_slot_status(&master->bus, addr);
>> +		if (status != I3C_ADDR_SLOT_I3C_DEV || test_bit(addr, dev_dyn_addrs))
>> +			continue;
>> +		i3c_bus_set_addr_slot_status(&master->bus, addr, I3C_ADDR_SLOT_FREE);
>> +		/* Try to add the device, but probe to see if it is really present */
>> +		__i3c_master_add_i3c_dev_locked(master, addr, true);
> 
> If dev add success for addr, but you free addr at previous line, any
> issue?

Currently, addr is always I3C_ADDR_SLOT_FREE on all paths that call
__i3c_master_add_i3c_dev_locked(), so this is consistent.

There is no i3c_dev_desc for this addr since that was checked just
above.

Then a success path will use i3c_master_attach_i3c_dev() which sets
I3C_ADDR_SLOT_I3C_DEV via i3c_master_get_i3c_addrs()

> 
> Frank
>> +	}
>> +}
>> +
>>  /**
>>   * i3c_master_do_daa_ext() - Dynamic Address Assignment (extended version)
>>   * @master: controller
>> @@ -2445,6 +2551,11 @@ int i3c_master_do_daa_ext(struct i3c_master_controller *master, bool rstdaa)
>>  		if (rstdaa)
>>  			rstret = i3c_master_rstdaa_locked(master, I3C_BROADCAST_ADDR);
>>  		ret = master->ops->do_daa(master);
>> +		/*
>> +		 * Handle cases where a dynamic address was assigned but the
>> +		 * device was not successfully added.
>> +		 */
>> +		i3c_master_reconcile_dyn_addrs(master);
>>  	}
>>
>>  	i3c_bus_maintenance_unlock(&master->bus);
>> --
>> 2.51.0
>>