[PATCH v2 0/2] I3C SETDASA and DAA process fix

[PATCH v2 0/2] I3C SETDASA and DAA process fix

[Parshuram Thombare]

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.

Regards,
Parshuram Thombare

Parshuram Thombare (2):
  i3c: master add i3c_master_attach_boardinfo to preserve boardinfo
  i3c: master: fix for SETDASA and DAA process

 drivers/i3c/master.c | 138 +++++++++++++++++++++++++++----------------
 1 file changed, 88 insertions(+), 50 deletions(-)

-- 
2.17.1


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

[PATCH v2 1/2] i3c: master add i3c_master_attach_boardinfo to preserve boardinfo

[Parshuram Thombare]

Boardinfo was lost if I3C object for devices with boardinfo
available are not created or not added to the I3C device list
because of some failure e.g. SETDASA failed, retrieve info failed etc
This patch adds i3c_master_attach_boardinfo which scan boardinfo list
in the master object and 'attach' it to the I3C device object.

Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
---
 drivers/i3c/master.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5f4bd52121fe..3d995f247cb7 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1776,6 +1776,21 @@ static void i3c_master_bus_cleanup(struct i3c_master_controller *master)
 	i3c_master_detach_free_devs(master);
 }
 
+static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev)
+{
+	struct i3c_master_controller *master = i3cdev->common.master;
+	struct i3c_dev_boardinfo *i3cboardinfo;
+
+	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
+		if (i3cdev->info.pid != i3cboardinfo->pid)
+			continue;
+
+		i3cdev->boardinfo = i3cboardinfo;
+		i3cdev->info.static_addr = i3cboardinfo->static_addr;
+		return;
+	}
+}
+
 static struct i3c_dev_desc *
 i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
 {
@@ -1831,10 +1846,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 	if (ret)
 		goto err_detach_dev;
 
+	i3c_master_attach_boardinfo(newdev);
+
 	olddev = i3c_master_search_i3c_dev_duplicate(newdev);
 	if (olddev) {
-		newdev->boardinfo = olddev->boardinfo;
-		newdev->info.static_addr = olddev->info.static_addr;
 		newdev->dev = olddev->dev;
 		if (newdev->dev)
 			newdev->dev->desc = newdev;
-- 
2.17.1


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

[PATCH v2 2/2] 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>
---
 drivers/i3c/master.c | 119 ++++++++++++++++++++++++++-----------------
 1 file changed, 71 insertions(+), 48 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 3d995f247cb7..5e0438ecf95c 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1426,33 +1426,57 @@ 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;
+	/*
+	 * We anyway 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 (!boardinfo->static_addr || !boardinfo->init_dyn_addr)
+		return -EINVAL;
 
-	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
-					dev->boardinfo->init_dyn_addr);
+	i3cdev = i3c_master_alloc_i3c_dev(master, &info);
+	if (IS_ERR(i3cdev))
+		return -ENOMEM;
+
+	i3cdev->boardinfo = boardinfo;
+
+	ret = i3c_master_attach_i3c_dev(master, i3cdev);
 	if (ret)
-		return;
+		goto err_attach;
 
-	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
-	ret = i3c_master_reattach_i3c_dev(dev, 0);
+	ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
+					i3cdev->boardinfo->init_dyn_addr);
+	if (ret)
+		goto err_setdasa;
+
+	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 +1643,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,7 +1657,8 @@ 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.
  *
  * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
  *    remaining I3C devices
@@ -1647,7 +1672,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 +1703,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 +1740,26 @@ 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) {
+		ret = i3c_master_pre_assign_dyn_addr(master, i3cboardinfo);
+		if (!ret ||  !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);
+	}
 
 	ret = i3c_master_do_daa(master);
 	if (ret)
@@ -1780,6 +1792,7 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev)
 {
 	struct i3c_master_controller *master = i3cdev->common.master;
 	struct i3c_dev_boardinfo *i3cboardinfo;
+	u8 init_dyn_addr;
 
 	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
 		if (i3cdev->info.pid != i3cboardinfo->pid)
@@ -1787,6 +1800,16 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev)
 
 		i3cdev->boardinfo = i3cboardinfo;
 		i3cdev->info.static_addr = i3cboardinfo->static_addr;
+		init_dyn_addr = i3cboardinfo->init_dyn_addr;
+		/*
+		 * 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);
+
 		return;
 	}
 }
-- 
2.17.1


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

Re: [PATCH v2 1/2] i3c: master add i3c_master_attach_boardinfo to preserve boardinfo

[Boris Brezillon]

On Thu, 21 May 2020 11:32:22 +0200
Parshuram Thombare <pthombar@cadence.com> wrote:

> Boardinfo was lost if I3C object for devices with boardinfo
> available are not created or not added to the I3C device list
> because of some failure e.g. SETDASA failed, retrieve info failed etc
> This patch adds i3c_master_attach_boardinfo which scan boardinfo list
> in the master object and 'attach' it to the I3C device object.
> 
> Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>

This patch looks good to me. I'll apply it just after the merge window.

> ---
>  drivers/i3c/master.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 5f4bd52121fe..3d995f247cb7 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1776,6 +1776,21 @@ static void i3c_master_bus_cleanup(struct i3c_master_controller *master)
>  	i3c_master_detach_free_devs(master);
>  }
>  
> +static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev)
> +{
> +	struct i3c_master_controller *master = i3cdev->common.master;
> +	struct i3c_dev_boardinfo *i3cboardinfo;
> +
> +	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
> +		if (i3cdev->info.pid != i3cboardinfo->pid)
> +			continue;
> +
> +		i3cdev->boardinfo = i3cboardinfo;
> +		i3cdev->info.static_addr = i3cboardinfo->static_addr;
> +		return;
> +	}
> +}
> +
>  static struct i3c_dev_desc *
>  i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  {
> @@ -1831,10 +1846,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  	if (ret)
>  		goto err_detach_dev;
>  
> +	i3c_master_attach_boardinfo(newdev);
> +
>  	olddev = i3c_master_search_i3c_dev_duplicate(newdev);
>  	if (olddev) {
> -		newdev->boardinfo = olddev->boardinfo;
> -		newdev->info.static_addr = olddev->info.static_addr;
>  		newdev->dev = olddev->dev;
>  		if (newdev->dev)
>  			newdev->dev->desc = newdev;


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

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

[Boris Brezillon]

Hello Parshuram,

Sorry for the late reply :-/.

On Thu, 21 May 2020 11:33:01 +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")
> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
> ---
>  drivers/i3c/master.c | 119 ++++++++++++++++++++++++++-----------------
>  1 file changed, 71 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 3d995f247cb7..5e0438ecf95c 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1426,33 +1426,57 @@ 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;
> +	/*
> +	 * We anyway don't attach devices which are not addressable

You can drop the anyway.

> +	 * (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().

>  
> -	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
> -					dev->boardinfo->init_dyn_addr);
> +	i3cdev = i3c_master_alloc_i3c_dev(master, &info);
> +	if (IS_ERR(i3cdev))
> +		return -ENOMEM;
> +
> +	i3cdev->boardinfo = boardinfo;
> +
> +	ret = i3c_master_attach_i3c_dev(master, i3cdev);
>  	if (ret)
> -		return;
> +		goto err_attach;
>  
> -	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
> -	ret = i3c_master_reattach_i3c_dev(dev, 0);
> +	ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
> +					i3cdev->boardinfo->init_dyn_addr);
> +	if (ret)
> +		goto err_setdasa;
> +
> +	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 +1643,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,7 +1657,8 @@ 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.

					     and attach them to the
					     master if
	  the dynamic address assignment succeeds

>   *
>   * 6. Do a DAA (Dynamic Address Assignment) to assign dynamic addresses to all
>   *    remaining I3C devices
> @@ -1647,7 +1672,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 +1703,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 +1740,26 @@ 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) {
> +		ret = i3c_master_pre_assign_dyn_addr(master, i3cboardinfo);
> +		if (!ret ||  !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);
> +	}
>  
>  	ret = i3c_master_do_daa(master);
>  	if (ret)
> @@ -1780,6 +1792,7 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev)
>  {
>  	struct i3c_master_controller *master = i3cdev->common.master;
>  	struct i3c_dev_boardinfo *i3cboardinfo;
> +	u8 init_dyn_addr;
>  
>  	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
>  		if (i3cdev->info.pid != i3cboardinfo->pid)
> @@ -1787,6 +1800,16 @@ static void i3c_master_attach_boardinfo(struct i3c_dev_desc *i3cdev)
>  
>  		i3cdev->boardinfo = i3cboardinfo;
>  		i3cdev->info.static_addr = i3cboardinfo->static_addr;
> +		init_dyn_addr = i3cboardinfo->init_dyn_addr;
> +		/*
> +		 * 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.

> +
>  		return;
>  	}
>  }


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

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

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

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

[Boris Brezillon]

On Thu, 20 Aug 2020 09:23:25 +0000
Parshuram Raju Thombare <pthombar@cadence.com> wrote:

> 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.

No, that's not what I'm talking about. I meant:

1. Device A is assigned a default init address X in the DT.
2. Device B has no init address
3. The framework reserves address X for and assigns it to device A
   when it appears on the bus (DAA, SETDASA or HJ+DDA)
4. Device B is assigned address Y
5. User decides to explicitly assign a different address to device A by
   issuing "echo Z > /sys/bus/i3c/..../<i3c-dev>/dyn_addr" (not yet
   supported, but I think we should allow that at some point), such
   that device A gets a lower/higher priority
6. User manually assigns address X to device B (that should be allowed
   since device A no longer uses X)
7. Device A is reset for some reason and loses its dynamic address,
   thus requiring a new DAA (or HJ+DAA). During this new discovery,
   device A is re-assigned its last known address (Z), but in the
   meantime you've marked address X as free (when attaching boardinfo
   to the newdev object).

> 
> 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.

If we go for a temporary solution, I'd opt for relaxing the test done
in i3c_master_get_i3c_addrs() to not reserve the init address (since it
should have been reserved at probe time) and keep those init addressed
reserved.

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