RE: [PATCH v2 2/2] i3c: master: fix for SETDASA and DAA process

[Parshuram Raju Thombare]

Hi Boris,

Thanks for your comments.

>> +	 * We anyway don't attach devices which are not addressable
>
>You can drop the anyway.
Sure, I will make above mentioned change in the comment.

>> +	 * (no static_addr and dyn_addr) and devices with static_addr
>> +	 * but no init_dyn_addr will participate in DAA.
>> +	 */
>> +	if (!boardinfo->static_addr || !boardinfo->init_dyn_addr)
>> +		return -EINVAL;
>
>If we consider this as an error, we should probably check that before
>calling i3c_master_pre_assign_dyn_addr() in i3c_master_bus_init().
Ok, I will move this check to i3c_master_bus_init(), before calling
i3c_master_pre_assign_dyn_addr. It will probably add extra if condition,
but will save one function call.

>>   * 5. Pre-assign dynamic addresses requested by the FW with SETDASA for I3C
>> - *    devices that have a static address
>> + *    devices that have a static address and attach corresponding statically
>> + *    defined I3C devices to the master.
>
>					     and attach them to the
>					     master if
>	  the dynamic address assignment succeeds
Sure, I will append above mentioned change to the comment.

>> +		/*
>> +		 * Free reserved init_dyn_addr so that attach can
>> +		 * get it before trying setnewda.
>> +		 */
>> +		if (i3cboardinfo->init_dyn_addr)
>> +			i3c_bus_set_addr_slot_status(&master->bus,
>> +						     init_dyn_addr,
>> +						     I3C_ADDR_SLOT_FREE);
>
>Hm, it's a bit more complicated. I don't think we can unconditionally
>release the init_dyn_addr here. Say you have a device that's been
>assigned its init_dyn_addr, and userspace decided to re-assign a new
>one (the feature is not available yet, but I thought about letting
>userspace write to the dyn_addr sysfs entry and wire that to a SETDA).
>The init_dyn_addr can now be re-assigned to a different device. After
>some time the device ends up resetting and thus lose its DA. A new DAA
>is issued to re-discover it, but you want this device to be assigned its
>last known address not the init address. And when
>i3c_master_attach_boardinfo() is called on this new device, you release
>a slot that's no longer yours.
>
>That was the rational behind the "address slots are attached to i3cdevs
>not boardinfo". Maybe we should have another list where we keep i3c
>devs that have not been discovered yet but have boardinfo attached to
>them. This way we can reserve dynamic addresses without blocking a
>slot in the master device table.

I think the sequence of events you are discussing here is
1. User assign address to device A with init_dyn_addr in boardinfo.
2. That particular init_dyn_addr is assigned to device B, which may be hotplugged ?
    and don't have boardinfo or init_dyn_addr in boardinfo ? 
3. Device A resets and trigger DAA due to hot plug ?
   A. Here now init_dyn_addr is already assigned to device B so device A shouldn't be freeing it.
   B. Now preferable dyn_addr is the one received from user in step 1.

If we are to prefer init_dyn_addr always, that will rule out possibility of making init_dyn_addr 
available to any other device when original device is assigned with user or master
provided address owing to SETDATA or SETNEWDA failures. And we can be sure of not freeing
init_dyn_addr inadvertently while it is being used by any other device.

Else if we want to prefer user provided address even across resets, since we don't need init_dyn_addr
anymore, it can be used to store user provided address. This will serve both the purposes A and B stated above.
 
And in my opinion this can be handled when we add code to allow user to change the device address.

Regards,
Parshuram Thombare