[v2,1/5] i3c: master: detach and free device if pre_assign_dyn_addr() fails
diff mbox series

Message ID 105a3ac1653e9ae658056a5ec9ddc2a084a61669.1567437955.git.vitor.soares@synopsys.com
State Changes Requested
Headers show
Series
  • i3c: remove device if failed on pre_assign_dyn_addr()
Related show

Commit Message

Vitor Soares Sept. 3, 2019, 10:35 a.m. UTC
On pre_assing_dyn_addr() the devices that fail:
  i3c_master_setdasa_locked()
  i3c_master_reattach_i3c_dev()
  i3c_master_retrieve_dev_info()

are kept in memory and master->bus.devs list. This makes the i3c devices
without a dynamic address are sent on DEFSLVS CCC command. Fix this by
detaching and freeing the devices that fail on pre_assign_dyn_addr().

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
---
Changes in v2:
  - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
  - Convert i3c_master_pre_assign_dyn_addr() to return an int

 drivers/i3c/master.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

Comments

Boris Brezillon Sept. 3, 2019, 10:52 a.m. UTC | #1
On Tue,  3 Sep 2019 12:35:50 +0200
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> On pre_assing_dyn_addr() the devices that fail:
>   i3c_master_setdasa_locked()
>   i3c_master_reattach_i3c_dev()
>   i3c_master_retrieve_dev_info()
> 
> are kept in memory and master->bus.devs list. This makes the i3c devices
> without a dynamic address are sent on DEFSLVS CCC command. Fix this by
> detaching and freeing the devices that fail on pre_assign_dyn_addr().
> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
> Changes in v2:
>   - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()

So, you decided to ignore my comment about leaving the i3c_dev_desc
allocated and skipping entries that don't have a dynamic address when
forging the DEFSLVS frame instead of doing this
allocate/free/re-allocate dance, and more importantly, you didn't even
bother explaining why.

I'm not willing to accept this patch unless you come up with solid
reasons.
Vitor Soares Sept. 3, 2019, 11:10 a.m. UTC | #2
Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Tue, Sep 03, 2019 at 11:52:37

> On Tue,  3 Sep 2019 12:35:50 +0200
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > On pre_assing_dyn_addr() the devices that fail:
> >   i3c_master_setdasa_locked()
> >   i3c_master_reattach_i3c_dev()
> >   i3c_master_retrieve_dev_info()
> > 
> > are kept in memory and master->bus.devs list. This makes the i3c devices
> > without a dynamic address are sent on DEFSLVS CCC command. Fix this by
> > detaching and freeing the devices that fail on pre_assign_dyn_addr().
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> > Changes in v2:
> >   - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
> 
> So, you decided to ignore my comment about leaving the i3c_dev_desc
> allocated and skipping entries that don't have a dynamic address when
> forging the DEFSLVS frame instead of doing this
> allocate/free/re-allocate dance, and more importantly, you didn't even
> bother explaining why.
> 
> I'm not willing to accept this patch unless you come up with solid
> reasons.

I think I already give a strong reason for my decision. Let say that my 
controller only has space for 4 devices and one of them is offline during 
pre_assign_dyn_addr() and hence it fails. When device tries to do the HJ 
I won't be able to do the enumeration because there is no space left. 
Anyway, you are right and I need to add this to commit message.

BTW, It is not clear to me why should we keep non addressed devices 
allocated when we have i3c_dev_boardinfo list with ->of_node information 
and what is the problem with allocate/free/re-allocate dance?

Best regards,
Vitor Soares
Przemysław Gaj Sept. 3, 2019, 11:13 a.m. UTC | #3
Hi Vitor,

I'm sorry for the delay.

The 09/03/2019 12:35, Vitor Soares wrote:
> EXTERNAL MAIL
> 
> 
> On pre_assing_dyn_addr() the devices that fail:
>   i3c_master_setdasa_locked()
>   i3c_master_reattach_i3c_dev()
>   i3c_master_retrieve_dev_info()
> 
> are kept in memory and master->bus.devs list. This makes the i3c devices
> without a dynamic address are sent on DEFSLVS CCC command. Fix this by
> detaching and freeing the devices that fail on pre_assign_dyn_addr().

What is the problem with sending devices without dynamic address in DEFSLVS?
Shouldn't we still inform rest of the devices about that? About fact that
device with SA without DA exists on the bus?

I think that this is limitation for us. Espacially we have SA and DA fields in
DEFSLVS frame so we are able to distinguish devices with and without Dynamic
Address.

> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> ---
> Changes in v2:
>   - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
>   - Convert i3c_master_pre_assign_dyn_addr() to return an int
> 
>  drivers/i3c/master.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 5f4bd52..586e34f 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1426,19 +1426,19 @@ 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_dev_desc *dev)
>  {
>  	struct i3c_master_controller *master = i3c_dev_get_master(dev);
>  	int ret;
>  
>  	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
>  	    !dev->boardinfo->static_addr)
> -		return;
> +		return 0;
>  
>  	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
>  					dev->boardinfo->init_dyn_addr);
>  	if (ret)
> -		return;
> +		return ret;
>  
>  	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
>  	ret = i3c_master_reattach_i3c_dev(dev, 0);
> @@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
>  	if (ret)
>  		goto err_rstdaa;
>  
> -	return;
> +	return 0;
>  
>  err_rstdaa:
>  	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
> +
> +	return ret;
>  }
>  
>  static void
> @@ -1647,7 +1649,7 @@ 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 i3c_dev_desc *i3cdev, *i3ctmp;
>  	struct i2c_dev_desc *i2cdev;
>  	int ret;
>  
> @@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  	 * Pre-assign dynamic address and retrieve device information if
>  	 * needed.
>  	 */
> -	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
> -		i3c_master_pre_assign_dyn_addr(i3cdev);
> +	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
> +				 common.node) {
> +		ret = i3c_master_pre_assign_dyn_addr(i3cdev);
> +		if (ret) {
> +			i3c_master_detach_i3c_dev(i3cdev);
> +			i3c_master_free_i3c_dev(i3cdev);
> +		}
> +	}
>  
>  	ret = i3c_master_do_daa(master);
>  	if (ret)
> -- 
> 2.7.4
>
Vitor Soares Sept. 3, 2019, 11:57 a.m. UTC | #4
From: Przemyslaw Gaj <pgaj@cadence.com>
Date: Tue, Sep 03, 2019 at 12:13:57

> Hi Vitor,
> 
> I'm sorry for the delay.
> 
> The 09/03/2019 12:35, Vitor Soares wrote:
> > EXTERNAL MAIL
> > 
> > 
> > On pre_assing_dyn_addr() the devices that fail:
> >   i3c_master_setdasa_locked()
> >   i3c_master_reattach_i3c_dev()
> >   i3c_master_retrieve_dev_info()
> > 
> > are kept in memory and master->bus.devs list. This makes the i3c devices
> > without a dynamic address are sent on DEFSLVS CCC command. Fix this by
> > detaching and freeing the devices that fail on pre_assign_dyn_addr().
> 
> What is the problem with sending devices without dynamic address in DEFSLVS?

How do you address them?
For the DA field in DEFSLVS frame, the spec says: "shall contain the 
current value of the Slave's assigned 7-bit Dynamic address". If the 
device doesn't have the dynamic address assigned yet why send it?

> Shouldn't we still inform rest of the devices about that? About fact that
> device with SA without DA exists on the bus?

I would like to understand what is the use case for this? 

On I3C spec table "I3C Devices Roles vs Responsibilities", Secondary 
Master is not responsible to do Dynamic Address Assignment but it is 
optional to do Hot-Join Dynamic Address Assignment.

> 
> I think that this is limitation for us. Espacially we have SA and DA fields in
> DEFSLVS frame so we are able to distinguish devices with and without Dynamic
> Address.

I would say the reason behind is regarding how to distinguish i2c from 
i3c devices.

> 
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > ---
> > Changes in v2:
> >   - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
> >   - Convert i3c_master_pre_assign_dyn_addr() to return an int
> > 
> >  drivers/i3c/master.c | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 5f4bd52..586e34f 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -1426,19 +1426,19 @@ 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_dev_desc *dev)
> >  {
> >  	struct i3c_master_controller *master = i3c_dev_get_master(dev);
> >  	int ret;
> >  
> >  	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
> >  	    !dev->boardinfo->static_addr)
> > -		return;
> > +		return 0;
> >  
> >  	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
> >  					dev->boardinfo->init_dyn_addr);
> >  	if (ret)
> > -		return;
> > +		return ret;
> >  
> >  	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
> >  	ret = i3c_master_reattach_i3c_dev(dev, 0);
> > @@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> >  	if (ret)
> >  		goto err_rstdaa;
> >  
> > -	return;
> > +	return 0;
> >  
> >  err_rstdaa:
> >  	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
> > +
> > +	return ret;
> >  }
> >  
> >  static void
> > @@ -1647,7 +1649,7 @@ 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 i3c_dev_desc *i3cdev, *i3ctmp;
> >  	struct i2c_dev_desc *i2cdev;
> >  	int ret;
> >  
> > @@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> >  	 * Pre-assign dynamic address and retrieve device information if
> >  	 * needed.
> >  	 */
> > -	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
> > -		i3c_master_pre_assign_dyn_addr(i3cdev);
> > +	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
> > +				 common.node) {
> > +		ret = i3c_master_pre_assign_dyn_addr(i3cdev);
> > +		if (ret) {
> > +			i3c_master_detach_i3c_dev(i3cdev);
> > +			i3c_master_free_i3c_dev(i3cdev);
> > +		}
> > +	}
> >  
> >  	ret = i3c_master_do_daa(master);
> >  	if (ret)
> > -- 
> > 2.7.4
> > 
> 
> -- 
> -- 
> Przemyslaw Gaj

Best regards,
Vitor Soares
Przemysław Gaj Sept. 4, 2019, 4:47 a.m. UTC | #5
The 09/03/2019 11:57, Vitor Soares wrote:
> EXTERNAL MAIL
> 
> 
> From: Przemyslaw Gaj <pgaj@cadence.com>
> Date: Tue, Sep 03, 2019 at 12:13:57
> 
> > Hi Vitor,
> > 
> > I'm sorry for the delay.
> > 
> > The 09/03/2019 12:35, Vitor Soares wrote:
> > > EXTERNAL MAIL
> > > 
> > > 
> > > On pre_assing_dyn_addr() the devices that fail:
> > >   i3c_master_setdasa_locked()
> > >   i3c_master_reattach_i3c_dev()
> > >   i3c_master_retrieve_dev_info()
> > > 
> > > are kept in memory and master->bus.devs list. This makes the i3c devices
> > > without a dynamic address are sent on DEFSLVS CCC command. Fix this by
> > > detaching and freeing the devices that fail on pre_assign_dyn_addr().
> > 
> > What is the problem with sending devices without dynamic address in DEFSLVS?
> 
> How do you address them?
> For the DA field in DEFSLVS frame, the spec says: "shall contain the 
> current value of the Slave's assigned 7-bit Dynamic address". If the 
> device doesn't have the dynamic address assigned yet why send it?
>

What stops us from operating with this device in I2C mode using his SA?

> > Shouldn't we still inform rest of the devices about that? About fact that
> > device with SA without DA exists on the bus?
> 
> I would like to understand what is the use case for this? 

Look above. I2C mode still should be possible IMO. Just consider the case when
you really need some information from that device, main master can assign
dynamic address later but you can make some transfers. I know our framework
does not support that case but secondary master can be different system which
should know that this device exists and don't have DA yet, so I2C mode is
available.

> 
> On I3C spec table "I3C Devices Roles vs Responsibilities", Secondary 
> Master is not responsible to do Dynamic Address Assignment but it is 
> optional to do Hot-Join Dynamic Address Assignment.
> 

Yes, we discussed that few times during the review of Mastership patch series.

> > 
> > I think that this is limitation for us. Espacially we have SA and DA fields in
> > DEFSLVS frame so we are able to distinguish devices with and without Dynamic
> > Address.
> 
> I would say the reason behind is regarding how to distinguish i2c from 
> i3c devices.
> 
> > 
> > > 
> > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > > ---
> > > Changes in v2:
> > >   - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr()
> > >   - Convert i3c_master_pre_assign_dyn_addr() to return an int
> > > 
> > >  drivers/i3c/master.c | 22 +++++++++++++++-------
> > >  1 file changed, 15 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > index 5f4bd52..586e34f 100644
> > > --- a/drivers/i3c/master.c
> > > +++ b/drivers/i3c/master.c
> > > @@ -1426,19 +1426,19 @@ 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_dev_desc *dev)
> > >  {
> > >  	struct i3c_master_controller *master = i3c_dev_get_master(dev);
> > >  	int ret;
> > >  
> > >  	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
> > >  	    !dev->boardinfo->static_addr)
> > > -		return;
> > > +		return 0;
> > >  
> > >  	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
> > >  					dev->boardinfo->init_dyn_addr);
> > >  	if (ret)
> > > -		return;
> > > +		return ret;
> > >  
> > >  	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
> > >  	ret = i3c_master_reattach_i3c_dev(dev, 0);
> > > @@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> > >  	if (ret)
> > >  		goto err_rstdaa;
> > >  
> > > -	return;
> > > +	return 0;
> > >  
> > >  err_rstdaa:
> > >  	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
> > > +
> > > +	return ret;
> > >  }
> > >  
> > >  static void
> > > @@ -1647,7 +1649,7 @@ 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 i3c_dev_desc *i3cdev, *i3ctmp;
> > >  	struct i2c_dev_desc *i2cdev;
> > >  	int ret;
> > >  
> > > @@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> > >  	 * Pre-assign dynamic address and retrieve device information if
> > >  	 * needed.
> > >  	 */
> > > -	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
> > > -		i3c_master_pre_assign_dyn_addr(i3cdev);
> > > +	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
> > > +				 common.node) {
> > > +		ret = i3c_master_pre_assign_dyn_addr(i3cdev);
> > > +		if (ret) {
> > > +			i3c_master_detach_i3c_dev(i3cdev);
> > > +			i3c_master_free_i3c_dev(i3cdev);
> > > +		}
> > > +	}
> > >  
> > >  	ret = i3c_master_do_daa(master);
> > >  	if (ret)
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > -- 
> > Przemyslaw Gaj
> 
> Best regards,
> Vitor Soares
Vitor Soares Sept. 4, 2019, 9:06 a.m. UTC | #6
From: Przemyslaw Gaj <pgaj@cadence.com>
Date: Wed, Sep 04, 2019 at 05:47:18

> The 09/03/2019 11:57, Vitor Soares wrote:
> > EXTERNAL MAIL
> > 
> > 
> > From: Przemyslaw Gaj <pgaj@cadence.com>
> > Date: Tue, Sep 03, 2019 at 12:13:57
> > 
> > > Hi Vitor,
> > > 
> > > I'm sorry for the delay.
> > > 
> > > The 09/03/2019 12:35, Vitor Soares wrote:
> > > > EXTERNAL MAIL
> > > > 
> > > > 
> > > > On pre_assing_dyn_addr() the devices that fail:
> > > >   i3c_master_setdasa_locked()
> > > >   i3c_master_reattach_i3c_dev()
> > > >   i3c_master_retrieve_dev_info()
> > > > 
> > > > are kept in memory and master->bus.devs list. This makes the i3c devices
> > > > without a dynamic address are sent on DEFSLVS CCC command. Fix this by
> > > > detaching and freeing the devices that fail on pre_assign_dyn_addr().
> > > 
> > > What is the problem with sending devices without dynamic address in DEFSLVS?
> > 
> > How do you address them?
> > For the DA field in DEFSLVS frame, the spec says: "shall contain the 
> > current value of the Slave's assigned 7-bit Dynamic address". If the 
> > device doesn't have the dynamic address assigned yet why send it?
> >
> 
> What stops us from operating with this device in I2C mode using his SA?

So, send it as an I2C device as spec says.

> 
> > > Shouldn't we still inform rest of the devices about that? About fact that
> > > device with SA without DA exists on the bus?
> > 
> > I would like to understand what is the use case for this? 
> 
> Look above. I2C mode still should be possible IMO. Just consider the case when
> you really need some information from that device, main master can assign
> dynamic address later but you can make some transfers.

It is true, but again it is a I2C device and not I3C device. It won't 
reply to I3C commands until has a DA.
How will you know that the DA sent in DEFSLVS is not assigned yet?

> I know our framework
> does not support that case but secondary master can be different system which
> should know that this device exists and don't have DA yet, so I2C mode is
> available.

If the HJ happen in secondary master side, there is a change to describe 
in DT what should be DA. Please check 2nd patch.

> 
> > 
> > On I3C spec table "I3C Devices Roles vs Responsibilities", Secondary 
> > Master is not responsible to do Dynamic Address Assignment but it is 
> > optional to do Hot-Join Dynamic Address Assignment.
> > 
> 
> Yes, we discussed that few times during the review of Mastership patch series.

Hence, based on that table, secondary master won't do DAA just because he 
want, It needs a HJ to trigger DAA.
Sorry, but for me based on what spec says this use case is not feasible. 


Best regards,
Vitor Soares

Patch
diff mbox series

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5f4bd52..586e34f 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1426,19 +1426,19 @@  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_dev_desc *dev)
 {
 	struct i3c_master_controller *master = i3c_dev_get_master(dev);
 	int ret;
 
 	if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr ||
 	    !dev->boardinfo->static_addr)
-		return;
+		return 0;
 
 	ret = i3c_master_setdasa_locked(master, dev->info.static_addr,
 					dev->boardinfo->init_dyn_addr);
 	if (ret)
-		return;
+		return ret;
 
 	dev->info.dyn_addr = dev->boardinfo->init_dyn_addr;
 	ret = i3c_master_reattach_i3c_dev(dev, 0);
@@ -1449,10 +1449,12 @@  static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
 	if (ret)
 		goto err_rstdaa;
 
-	return;
+	return 0;
 
 err_rstdaa:
 	i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr);
+
+	return ret;
 }
 
 static void
@@ -1647,7 +1649,7 @@  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 i3c_dev_desc *i3cdev, *i3ctmp;
 	struct i2c_dev_desc *i2cdev;
 	int ret;
 
@@ -1746,8 +1748,14 @@  static int i3c_master_bus_init(struct i3c_master_controller *master)
 	 * Pre-assign dynamic address and retrieve device information if
 	 * needed.
 	 */
-	i3c_bus_for_each_i3cdev(&master->bus, i3cdev)
-		i3c_master_pre_assign_dyn_addr(i3cdev);
+	list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c,
+				 common.node) {
+		ret = i3c_master_pre_assign_dyn_addr(i3cdev);
+		if (ret) {
+			i3c_master_detach_i3c_dev(i3cdev);
+			i3c_master_free_i3c_dev(i3cdev);
+		}
+	}
 
 	ret = i3c_master_do_daa(master);
 	if (ret)