[PATCH v3] i3c: master: fix for SETDASA and DAA process

[PATCH v3] i3c: master: fix for SETDASA and DAA process

[Parshuram Thombare]

This patch fix following issue.
Controller slots blocked for devices with static_addr
but no init_dyn_addr may limit the number of I3C devices
on the bus which gets dynamic address in DAA. So
instead of attaching all the devices with static_addr,
now we only attach the devices which successfully
complete SETDASA. For remaining devices with init_dyn_addr,
i3c_master_add_i3c_dev_locked() will try to set requested
dynamic address after DAA.

Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
---
Changes between v2 and v3 are:
1. Keeping init_dyn_addr reserved.
2. Code refactoring and changes in comments.

Changes between v1 and v2 are:
1. Added boardinfo attach fix.
2. Removed reattach issue related fix.
3. Reserve init_dyn_addr initially, so that it will not
   be used in DAA and  attempt can be made to set those
   firmware requested dynamic address after DAA.
---
 drivers/i3c/master.c |  115 ++++++++++++++++++++++++++++----------------------
 1 files changed, 65 insertions(+), 50 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 3d995f2..24543d8 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1367,7 +1367,9 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
 	enum i3c_addr_slot_status status;
 	int ret;
 
-	if (dev->info.dyn_addr != old_dyn_addr) {
+	if (dev->info.dyn_addr != old_dyn_addr &&
+	    (!dev->boardinfo ||
+	     dev->info.dyn_addr != dev->boardinfo->init_dyn_addr)) {
 		status = i3c_bus_get_addr_slot_status(&master->bus,
 						      dev->info.dyn_addr);
 		if (status != I3C_ADDR_SLOT_FREE)
@@ -1426,33 +1428,49 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
 		master->ops->detach_i2c_dev(dev);
 }
 
-static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
+static int i3c_master_pre_assign_dyn_addr(struct i3c_master_controller *master,
+					  struct i3c_dev_boardinfo *boardinfo)
 {
-	struct i3c_master_controller *master = i3c_dev_get_master(dev);
+	struct i3c_device_info info = {
+		.static_addr = boardinfo->static_addr,
+	};
+	struct i3c_dev_desc *i3cdev;
 	int ret;
 
-	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
-	    !dev->boardinfo->static_addr)
-		return;
+	i3cdev = i3c_master_alloc_i3c_dev(master, &info);
+	if (IS_ERR(i3cdev))
+		return -ENOMEM;
+
+	i3cdev->boardinfo = boardinfo;
 
-	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
-					dev->boardinfo->init_dyn_addr);
+	ret = i3c_master_attach_i3c_dev(master, i3cdev);
 	if (ret)
-		return;
+		goto err_attach;
+
+	ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
+					i3cdev->boardinfo->init_dyn_addr);
+	if (ret)
+		goto err_setdasa;
 
-	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
-	ret = i3c_master_reattach_i3c_dev(dev, 0);
+	i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
+	ret = i3c_master_reattach_i3c_dev(i3cdev, 0);
 	if (ret)
 		goto err_rstdaa;
 
-	ret = i3c_master_retrieve_dev_info(dev);
+	ret = i3c_master_retrieve_dev_info(i3cdev);
 	if (ret)
 		goto err_rstdaa;
 
-	return;
+	return 0;
 
 err_rstdaa:
-	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
+	i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);
+err_setdasa:
+	i3c_master_detach_i3c_dev(i3cdev);
+err_attach:
+	i3c_master_free_i3c_dev(i3cdev);
+
+	return ret;
 }
 
 static void
@@ -1619,8 +1637,8 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
  * This function is following all initialisation steps described in the I3C
  * specification:
  *
- * 1. Attach I2C and statically defined I3C devs to the master so that the
- *    master can fill its internal device table appropriately
+ * 1. Attach I2C devs to the master so that the master can fill its internal
+ *    device table appropriately
  *
  * 2. Call &i3c_master_controller_ops->bus_init() method to initialize
  *    the master controller. That's usually where the bus mode is selected
@@ -1633,10 +1651,14 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
  * 4. Disable all slave events.
  *
  * 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. If only init_dyn_addr is available
+ *    or if SETDASA fails, reserve those init_dyn_addr to be used later to set
+ *    address using SETNEWDA after DAA.
  *
  * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
- *    remaining I3C devices
+ *    remaining I3C devices and attach them to the master if the dynamic address
+ *    assignment succeeds
  *
  * Once this is done, all I3C and I2C devices should be usable.
  *
@@ -1647,7 +1669,6 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 	enum i3c_addr_slot_status status;
 	struct i2c_dev_boardinfo *i2cboardinfo;
 	struct i3c_dev_boardinfo *i3cboardinfo;
-	struct i3c_dev_desc *i3cdev;
 	struct i2c_dev_desc *i2cdev;
 	int ret;
 
@@ -1679,34 +1700,6 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 			goto err_detach_devs;
 		}
 	}
-	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
-		struct i3c_device_info info = {
-			.static_addr = i3cboardinfo->static_addr,
-		};
-
-		if (i3cboardinfo->init_dyn_addr) {
-			status = i3c_bus_get_addr_slot_status(&master->bus,
-						i3cboardinfo->init_dyn_addr);
-			if (status != I3C_ADDR_SLOT_FREE) {
-				ret = -EBUSY;
-				goto err_detach_devs;
-			}
-		}
-
-		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
-		if (IS_ERR(i3cdev)) {
-			ret = PTR_ERR(i3cdev);
-			goto err_detach_devs;
-		}
-
-		i3cdev->boardinfo = i3cboardinfo;
-
-		ret = i3c_master_attach_i3c_dev(master, i3cdev);
-		if (ret) {
-			i3c_master_free_i3c_dev(i3cdev);
-			goto err_detach_devs;
-		}
-	}
 
 	/*
 	 * Now execute the controller specific ->bus_init() routine, which
@@ -1744,10 +1737,32 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 
 	/*
 	 * Pre-assign dynamic address and retrieve device information if
-	 * needed.
+	 * needed. And reserve the init_dyn_addr in case of failure, to retry
+	 * setting the requested address after DAA is done in
+	 * i3c_master_add_i3c_dev_locked().
 	 */
-	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
-		i3c_master_pre_assign_dyn_addr(i3cdev);
+	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
+		/*
+		 * We don't attach devices which are not addressable
+		 * (no static_addr and dyn_addr) and devices with static_addr
+		 * but no init_dyn_addr will participate in DAA.
+		 */
+		if (!i3cboardinfo->init_dyn_addr ||
+		    (i3cboardinfo->static_addr &&
+		     !i3c_master_pre_assign_dyn_addr(master, i3cboardinfo)))
+			continue;
+
+		ret = i3c_bus_get_addr_slot_status(&master->bus,
+						   i3cboardinfo->init_dyn_addr);
+		if (ret != I3C_ADDR_SLOT_FREE) {
+			ret = -EBUSY;
+			goto err_rstdaa;
+		}
+
+		i3c_bus_set_addr_slot_status(&master->bus,
+					     i3cboardinfo->init_dyn_addr,
+					     I3C_ADDR_SLOT_I3C_DEV);
+	}
 
 	ret = i3c_master_do_daa(master);
 	if (ret)
-- 
1.7.1


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

Re: [PATCH v3] i3c: master: fix for SETDASA and DAA process

[Boris Brezillon]

On Thu, 20 Aug 2020 15:38:26 +0200
Parshuram Thombare <pthombar@cadence.com> wrote:

> This patch fix following issue.
> Controller slots blocked for devices with static_addr
> but no init_dyn_addr may limit the number of I3C devices
> on the bus which gets dynamic address in DAA. So
> instead of attaching all the devices with static_addr,
> now we only attach the devices which successfully
> complete SETDASA. For remaining devices with init_dyn_addr,
> i3c_master_add_i3c_dev_locked() will try to set requested
> dynamic address after DAA.
> 
> Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")

Hm, not sure that qualifies as a fix. The current implementation was
correct, it was just reserving a slot in the device table for devices
that didn't have an init address or on which SETDASA failed.

> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
> ---
> Changes between v2 and v3 are:
> 1. Keeping init_dyn_addr reserved.
> 2. Code refactoring and changes in comments.
> 
> Changes between v1 and v2 are:
> 1. Added boardinfo attach fix.
> 2. Removed reattach issue related fix.
> 3. Reserve init_dyn_addr initially, so that it will not
>    be used in DAA and  attempt can be made to set those
>    firmware requested dynamic address after DAA.
> ---
>  drivers/i3c/master.c |  115 ++++++++++++++++++++++++++++----------------------
>  1 files changed, 65 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 3d995f2..24543d8 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1367,7 +1367,9 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
>  	enum i3c_addr_slot_status status;
>  	int ret;
>  
> -	if (dev->info.dyn_addr != old_dyn_addr) {
> +	if (dev->info.dyn_addr != old_dyn_addr &&
> +	    (!dev->boardinfo ||
> +	     dev->info.dyn_addr != dev->boardinfo->init_dyn_addr)) {
>  		status = i3c_bus_get_addr_slot_status(&master->bus,
>  						      dev->info.dyn_addr);
>  		if (status != I3C_ADDR_SLOT_FREE)
> @@ -1426,33 +1428,49 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
>  		master->ops->detach_i2c_dev(dev);
>  }
>  
> -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> +static int i3c_master_pre_assign_dyn_addr(struct i3c_master_controller *master,
> +					  struct i3c_dev_boardinfo *boardinfo)

That function now does more than just assigning a dynamic address: it
also creates the i3c_dev_desc. It should be renamed accordingly
(i3c_master_early_i3c_dev_add() maybe).

>  {
> -	struct i3c_master_controller *master = i3c_dev_get_master(dev);
> +	struct i3c_device_info info = {
> +		.static_addr = boardinfo->static_addr,
> +	};
> +	struct i3c_dev_desc *i3cdev;
>  	int ret;
>  
> -	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
> -	    !dev->boardinfo->static_addr)
> -		return;
> +	i3cdev = i3c_master_alloc_i3c_dev(master, &info);
> +	if (IS_ERR(i3cdev))
> +		return -ENOMEM;
> +
> +	i3cdev->boardinfo = boardinfo;
>  
> -	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
> -					dev->boardinfo->init_dyn_addr);
> +	ret = i3c_master_attach_i3c_dev(master, i3cdev);
>  	if (ret)
> -		return;
> +		goto err_attach;
> +
> +	ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
> +					i3cdev->boardinfo->init_dyn_addr);
> +	if (ret)
> +		goto err_setdasa;
>  
> -	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
> -	ret = i3c_master_reattach_i3c_dev(dev, 0);
> +	i3cdev->info.dyn_addr = i3cdev->boardinfo->init_dyn_addr;
> +	ret = i3c_master_reattach_i3c_dev(i3cdev, 0);
>  	if (ret)
>  		goto err_rstdaa;
>  
> -	ret = i3c_master_retrieve_dev_info(dev);
> +	ret = i3c_master_retrieve_dev_info(i3cdev);
>  	if (ret)
>  		goto err_rstdaa;
>  
> -	return;
> +	return 0;
>  
>  err_rstdaa:
> -	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
> +	i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);
> +err_setdasa:
> +	i3c_master_detach_i3c_dev(i3cdev);
> +err_attach:
> +	i3c_master_free_i3c_dev(i3cdev);
> +
> +	return ret;
>  }
>  
>  static void
> @@ -1619,8 +1637,8 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
>   * This function is following all initialisation steps described in the I3C
>   * specification:
>   *
> - * 1. Attach I2C and statically defined I3C devs to the master so that the
> - *    master can fill its internal device table appropriately
> + * 1. Attach I2C devs to the master so that the master can fill its internal
> + *    device table appropriately
>   *
>   * 2. Call &i3c_master_controller_ops->bus_init() method to initialize
>   *    the master controller. That's usually where the bus mode is selected
> @@ -1633,10 +1651,14 @@ static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
>   * 4. Disable all slave events.
>   *
>   * 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. If only init_dyn_addr is available
> + *    or if SETDASA fails, reserve those init_dyn_addr to be used later to set
> + *    address using SETNEWDA after DAA.
>   *
>   * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
> - *    remaining I3C devices
> + *    remaining I3C devices and attach them to the master if the dynamic address
> + *    assignment succeeds
>   *
>   * Once this is done, all I3C and I2C devices should be usable.
>   *
> @@ -1647,7 +1669,6 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  	enum i3c_addr_slot_status status;
>  	struct i2c_dev_boardinfo *i2cboardinfo;
>  	struct i3c_dev_boardinfo *i3cboardinfo;
> -	struct i3c_dev_desc *i3cdev;
>  	struct i2c_dev_desc *i2cdev;
>  	int ret;
>  
> @@ -1679,34 +1700,6 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  			goto err_detach_devs;
>  		}
>  	}
> -	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> -		struct i3c_device_info info = {
> -			.static_addr = i3cboardinfo->static_addr,
> -		};
> -
> -		if (i3cboardinfo->init_dyn_addr) {
> -			status = i3c_bus_get_addr_slot_status(&master->bus,
> -						i3cboardinfo->init_dyn_addr);
> -			if (status != I3C_ADDR_SLOT_FREE) {
> -				ret = -EBUSY;
> -				goto err_detach_devs;
> -			}
> -		}
> -
> -		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
> -		if (IS_ERR(i3cdev)) {
> -			ret = PTR_ERR(i3cdev);
> -			goto err_detach_devs;
> -		}
> -
> -		i3cdev->boardinfo = i3cboardinfo;
> -
> -		ret = i3c_master_attach_i3c_dev(master, i3cdev);
> -		if (ret) {
> -			i3c_master_free_i3c_dev(i3cdev);
> -			goto err_detach_devs;
> -		}
> -	}
>  
>  	/*
>  	 * Now execute the controller specific ->bus_init() routine, which
> @@ -1744,10 +1737,32 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  
>  	/*
>  	 * Pre-assign dynamic address and retrieve device information if
> -	 * needed.
> +	 * needed. And reserve the init_dyn_addr in case of failure, to retry
> +	 * setting the requested address after DAA is done in
> +	 * i3c_master_add_i3c_dev_locked().
>  	 */
> -	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
> -		i3c_master_pre_assign_dyn_addr(i3cdev);
> +	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> +		/*
> +		 * We don't attach devices which are not addressable
> +		 * (no static_addr and dyn_addr) and devices with static_addr
> +		 * but no init_dyn_addr will participate in DAA.
> +		 */
> +		if (!i3cboardinfo->init_dyn_addr ||
> +		    (i3cboardinfo->static_addr &&
> +		     !i3c_master_pre_assign_dyn_addr(master, i3cboardinfo)))
> +			continue;
> +
> +		ret = i3c_bus_get_addr_slot_status(&master->bus,
> +						   i3cboardinfo->init_dyn_addr);
> +		if (ret != I3C_ADDR_SLOT_FREE) {
> +			ret = -EBUSY;
> +			goto err_rstdaa;
> +		}
> +
> +		i3c_bus_set_addr_slot_status(&master->bus,
> +					     i3cboardinfo->init_dyn_addr,
> +					     I3C_ADDR_SLOT_I3C_DEV);

You should reserve the address before calling
i3c_master_pre_assign_dyn_addr():

		/*
		 * We don't attach devices which are not addressable
		 * (no static_addr and dyn_addr) and devices with
		 * static_addr but no init_dyn_addr will participate in DAA.
		 */
		if (!i3cboardinfo->init_dyn_addr ||
		    !i3cboardinfo->static_addr)
			continue;

		ret = i3c_bus_get_addr_slot_status(&master->bus,
						   i3cboardinfo->init_dyn_addr);
		if (ret != I3C_ADDR_SLOT_FREE) {
			ret = -EBUSY;
			goto err_rstdaa;
		}

		i3c_bus_set_addr_slot_status(&master->bus,
					     i3cboardinfo->init_dyn_addr,
					     I3C_ADDR_SLOT_I3C_DEV);

		i3c_master_pre_assign_dyn_addr(master, i3cboardinfo);

> +	}
>  
>  	ret = i3c_master_do_daa(master);
>  	if (ret)


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

RE: [PATCH v3] i3c: master: fix for SETDASA and DAA process

[Parshuram Raju Thombare]

>Hm, not sure that qualifies as a fix. The current implementation was
>correct, it was just reserving a slot in the device table for devices
>that didn't have an init address or on which SETDASA failed.
If I3C controllers like ours use hardware slots to store slave devices info, 
due to limited available slots this can cause issue. 
 If some slots are lost due to
1. only init_dyn_addr and no static_addr in DT 
OR
2. SETDASA failed
at the end of DAA some devices may be left without dyn_addr allocated from master
and hence can't work properly.
I think during our discussion we recognized this change as a bug.
That is the reason I added fixes tag, but if you think otherwise I can remove this tag.
 
>> -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
>> +static int i3c_master_pre_assign_dyn_addr(struct i3c_master_controller
>That function now does more than just assigning a dynamic address: it
>also creates the i3c_dev_desc. It should be renamed accordingly
>(i3c_master_early_i3c_dev_add() maybe).
Ok

>You should reserve the address before calling
>i3c_master_pre_assign_dyn_addr():
>
>		/*
>		 * We don't attach devices which are not addressable
>		 * (no static_addr and dyn_addr) and devices with
>		 * static_addr but no init_dyn_addr will participate in DAA.
>		 */
>		if (!i3cboardinfo->init_dyn_addr ||
>		    !i3cboardinfo->static_addr)
>			continue;
Don't we want to cover the case when only init_dyn_addr is present ?
I am not sure if we can't have init_dyn_addr without static_addr.
May be what we need is 
		if (!i3cboardinfo->init_dyn_addr)
			continue;

		ret = i3c_bus_get_addr_slot_status(&master->bus,
						   i3cboardinfo->init_dyn_addr);
		if (ret != I3C_ADDR_SLOT_FREE) {
			ret = -EBUSY;
			goto err_rstdaa;
		}

		i3c_bus_set_addr_slot_status(&master->bus,
					     i3cboardinfo->init_dyn_addr,
					     I3C_ADDR_SLOT_I3C_DEV);

		if (i3cboardinfo->static_addr)
			i3c_master_pre_assign_dyn_addr(master, i3cboardinfo);
IMHO this is functionally same to what I sent. Just that init_dyn_addr is reserved before,
and we leverage the change in reattach to bypass failure due to second attempt
to get init_dyn_addr in reattach called from i3c_master_pre_assign_dyn_addr().



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

Re: [PATCH v3] i3c: master: fix for SETDASA and DAA process

[Boris Brezillon]

On Thu, 20 Aug 2020 18:16:14 +0000
Parshuram Raju Thombare <pthombar@cadence.com> wrote:

> >Hm, not sure that qualifies as a fix. The current implementation was
> >correct, it was just reserving a slot in the device table for devices
> >that didn't have an init address or on which SETDASA failed.  
> If I3C controllers like ours use hardware slots to store slave devices info, 
> due to limited available slots this can cause issue. 
>  If some slots are lost due to
> 1. only init_dyn_addr and no static_addr in DT 
> OR
> 2. SETDASA failed

Well, having a slot with a static address is valid, though I agree
it's not really useful.

> at the end of DAA some devices may be left without dyn_addr allocated from master
> and hence can't work properly.

My point is, there's no address or device slot leak, it's just that
reserving a slot for I3C devices that only have a static address is
kind of useless. But let's be honest, given the number of I3C devices
available out there, I don't think it will hurt us before quite some
time :P. That's not to say we shouldn't address that, I just don't
think it deserves a Fixes tag.

> I think during our discussion we recognized this change as a bug.

IIRC, I was talking about the first patch in the series.

> That is the reason I added fixes tag, but if you think otherwise I can remove this tag.
>  
> >> -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> >> +static int i3c_master_pre_assign_dyn_addr(struct i3c_master_controller  
> >That function now does more than just assigning a dynamic address: it
> >also creates the i3c_dev_desc. It should be renamed accordingly
> >(i3c_master_early_i3c_dev_add() maybe).  
> Ok
> 
> >You should reserve the address before calling
> >i3c_master_pre_assign_dyn_addr():
> >
> >		/*
> >		 * We don't attach devices which are not addressable
> >		 * (no static_addr and dyn_addr) and devices with
> >		 * static_addr but no init_dyn_addr will participate in DAA.
> >		 */
> >		if (!i3cboardinfo->init_dyn_addr ||
> >		    !i3cboardinfo->static_addr)
> >			continue;  
> Don't we want to cover the case when only init_dyn_addr is present ?

Uh, yes, my bad.

> I am not sure if we can't have init_dyn_addr without static_addr.

You can, when you want to assign a specific dynamic address to a device
that doesn't have a static address (see the 'try to assign init_addr
dance' in i3c_dev_add()).

> May be what we need is 
> 		if (!i3cboardinfo->init_dyn_addr)
> 			continue;
> 
> 		ret = i3c_bus_get_addr_slot_status(&master->bus,
> 						   i3cboardinfo->init_dyn_addr);
> 		if (ret != I3C_ADDR_SLOT_FREE) {
> 			ret = -EBUSY;
> 			goto err_rstdaa;
> 		}
> 
> 		i3c_bus_set_addr_slot_status(&master->bus,
> 					     i3cboardinfo->init_dyn_addr,
> 					     I3C_ADDR_SLOT_I3C_DEV);
> 
> 		if (i3cboardinfo->static_addr)
> 			i3c_master_pre_assign_dyn_addr(master, i3cboardinfo);

Yep, that's correct.

> IMHO this is functionally same to what I sent. Just that init_dyn_addr is reserved before,
> and we leverage the change in reattach to bypass failure due to second attempt
> to get init_dyn_addr in reattach called from i3c_master_pre_assign_dyn_addr().

Unless I'm missing something, your solution didn't reserve the
init address when there's no static address, and we definitely want
that to happen, otherwise another device might steal it during DAA.

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

Re: [PATCH v3] i3c: master: fix for SETDASA and DAA process

[Boris Brezillon]

On Thu, 20 Aug 2020 21:03:11 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Thu, 20 Aug 2020 18:16:14 +0000
> Parshuram Raju Thombare <pthombar@cadence.com> wrote:
> 
> > >Hm, not sure that qualifies as a fix. The current implementation was
> > >correct, it was just reserving a slot in the device table for devices
> > >that didn't have an init address or on which SETDASA failed.    
> > If I3C controllers like ours use hardware slots to store slave devices info, 
> > due to limited available slots this can cause issue. 
> >  If some slots are lost due to
> > 1. only init_dyn_addr and no static_addr in DT 
> > OR
> > 2. SETDASA failed  
> 
> Well, having a slot with a static address is valid, though I agree
> it's not really useful.
> 
> > at the end of DAA some devices may be left without dyn_addr allocated from master
> > and hence can't work properly.  
> 
> My point is, there's no address or device slot leak, it's just that
> reserving a slot for I3C devices that only have a static address is
> kind of useless. But let's be honest, given the number of I3C devices
> available out there, I don't think it will hurt us before quite some
> time :P. That's not to say we shouldn't address that, I just don't
> think it deserves a Fixes tag.
> 
> > I think during our discussion we recognized this change as a bug.  
> 
> IIRC, I was talking about the first patch in the series.
> 
> > That is the reason I added fixes tag, but if you think otherwise I can remove this tag.
> >    
> > >> -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> > >> +static int i3c_master_pre_assign_dyn_addr(struct i3c_master_controller    
> > >That function now does more than just assigning a dynamic address: it
> > >also creates the i3c_dev_desc. It should be renamed accordingly
> > >(i3c_master_early_i3c_dev_add() maybe).    
> > Ok
> >   
> > >You should reserve the address before calling
> > >i3c_master_pre_assign_dyn_addr():
> > >
> > >		/*
> > >		 * We don't attach devices which are not addressable
> > >		 * (no static_addr and dyn_addr) and devices with
> > >		 * static_addr but no init_dyn_addr will participate in DAA.
> > >		 */
> > >		if (!i3cboardinfo->init_dyn_addr ||
> > >		    !i3cboardinfo->static_addr)
> > >			continue;    
> > Don't we want to cover the case when only init_dyn_addr is present ?  
> 
> Uh, yes, my bad.
> 
> > I am not sure if we can't have init_dyn_addr without static_addr.  
> 
> You can, when you want to assign a specific dynamic address to a device
> that doesn't have a static address (see the 'try to assign init_addr
> dance' in i3c_dev_add()).
> 
> > May be what we need is 
> > 		if (!i3cboardinfo->init_dyn_addr)
> > 			continue;
> > 
> > 		ret = i3c_bus_get_addr_slot_status(&master->bus,
> > 						   i3cboardinfo->init_dyn_addr);
> > 		if (ret != I3C_ADDR_SLOT_FREE) {
> > 			ret = -EBUSY;
> > 			goto err_rstdaa;
> > 		}
> > 
> > 		i3c_bus_set_addr_slot_status(&master->bus,
> > 					     i3cboardinfo->init_dyn_addr,
> > 					     I3C_ADDR_SLOT_I3C_DEV);
> > 
> > 		if (i3cboardinfo->static_addr)
> > 			i3c_master_pre_assign_dyn_addr(master, i3cboardinfo);  
> 
> Yep, that's correct.
> 
> > IMHO this is functionally same to what I sent. Just that init_dyn_addr is reserved before,
> > and we leverage the change in reattach to bypass failure due to second attempt
> > to get init_dyn_addr in reattach called from i3c_master_pre_assign_dyn_addr().  
> 
> Unless I'm missing something, your solution didn't reserve the
> init address when there's no static address, and we definitely want
> that to happen, otherwise another device might steal it during DAA.

My bad, it did. This being said, I find it much easier to follow when
the reservation happens in one place, so I'm still in favor of the new
version.

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