[v4,2/6] i3c: master: pre-reserve boardinfo->init_dyn_addr when available
diff mbox series

Message ID 20191210101502.8401-3-pgaj@cadence.com
State New
Headers show
Series
  • I3C device addresing adjustments
Related show

Commit Message

Przemysław Gaj Dec. 10, 2019, 10:14 a.m. UTC
It may be the case that SETDASA fails for some reason. Reserve
->init_dyn_addr when it's defined to prevent assigning this address
to another slave device. This way when device shows up we don't
have to re-assign addresses.

Signed-off-by: Przemysław Gaj <pgaj@cadence.com>
---
 drivers/i3c/master.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Boris Brezillon Dec. 10, 2019, 1:29 p.m. UTC | #1
On Tue, 10 Dec 2019 11:14:58 +0100
Przemysław Gaj <pgaj@cadence.com> wrote:

> It may be the case that SETDASA fails for some reason. Reserve
> ->init_dyn_addr when it's defined to prevent assigning this address  
> to another slave device. This way when device shows up we don't
> have to re-assign addresses.
> 
> Signed-off-by: Przemysław Gaj <pgaj@cadence.com>
> ---
>  drivers/i3c/master.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 5c06c41e6660..fab6e0609fca 100755
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1263,7 +1263,8 @@ static void i3c_master_put_i3c_addrs(struct i3c_dev_desc *dev)
>  					     I3C_ADDR_SLOT_FREE);
>  
>  	if (dev->boardinfo && dev->boardinfo->init_dyn_addr)
> -		i3c_bus_set_addr_slot_status(&master->bus, dev->info.dyn_addr,
> +		i3c_bus_set_addr_slot_status(&master->bus,
> +					     dev->boardinfo->init_dyn_addr,
>  					     I3C_ADDR_SLOT_FREE);

Should be fixed in a separate patch, or at least mentioned in the
commit message.

>  }
>  
> @@ -1675,6 +1676,11 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  				ret = -EBUSY;
>  				goto err_detach_devs;
>  			}
> +
> +			/* Reserve the slot. */
> +			i3c_bus_set_addr_slot_status(&master->bus,
> +						     i3cboardinfo->init_dyn_addr,
> +						     I3C_ADDR_SLOT_I3C_DEV);

Looks like the "reserve/release init_dyn_addr slot" logic is
asymmetric. I wonder if that's not a problem. Can't we end up in a
situation where the ->init_dyn_addr is released (when SETDASA() fails)
and then re-used without being reserved (when the device is later
discovered through a regular DAA)?

So maybe I was wrong and we should indeed reserve the address in the
i3c_master_get_i3c_addrs() path.

>  		}
>  
>  		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
Vitor Soares Dec. 10, 2019, 3:24 p.m. UTC | #2
Hi Przemysław,

From: Przemysław Gaj <pgaj@cadence.com>
Date: Tue, Dec 10, 2019 at 10:14:58

> It may be the case that SETDASA fails for some reason. Reserve
> ->init_dyn_addr when it's defined to prevent assigning this address
> to another slave device. This way when device shows up we don't
> have to re-assign addresses.
> 
> Signed-off-by: Przemysław Gaj <pgaj@cadence.com>
> ---
>  drivers/i3c/master.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 5c06c41e6660..fab6e0609fca 100755
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1263,7 +1263,8 @@ static void i3c_master_put_i3c_addrs(struct i3c_dev_desc *dev)
>  					     I3C_ADDR_SLOT_FREE);
>  
>  	if (dev->boardinfo && dev->boardinfo->init_dyn_addr)
> -		i3c_bus_set_addr_slot_status(&master->bus, dev->info.dyn_addr,
> +		i3c_bus_set_addr_slot_status(&master->bus,
> +					     dev->boardinfo->init_dyn_addr,
>  					     I3C_ADDR_SLOT_FREE);
>  }
>  
> @@ -1675,6 +1676,11 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  				ret = -EBUSY;
>  				goto err_detach_devs;
>  			}
> +
> +			/* Reserve the slot. */
> +			i3c_bus_set_addr_slot_status(&master->bus,
> +						     i3cboardinfo->init_dyn_addr,
> +						     I3C_ADDR_SLOT_I3C_DEV);
>  		}
>  
>  		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
> -- 
> 2.14.0

In my experience we should pre-reserve all DT DA entries and not use them 
during the ENTDAA for safety reasons.
That would be one of my next steps in this patch series.

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

The 12/10/2019 15:24, Vitor Soares wrote:
> 
> Hi Przemysław,
> 
> From: Przemysław Gaj <pgaj@cadence.com>
> Date: Tue, Dec 10, 2019 at 10:14:58
> 
> > It may be the case that SETDASA fails for some reason. Reserve
> > ->init_dyn_addr when it's defined to prevent assigning this address
> > to another slave device. This way when device shows up we don't
> > have to re-assign addresses.
> > 
> > Signed-off-by: Przemysław Gaj <pgaj@cadence.com>
> > ---
> >  drivers/i3c/master.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 5c06c41e6660..fab6e0609fca 100755
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -1263,7 +1263,8 @@ static void i3c_master_put_i3c_addrs(struct i3c_dev_desc *dev)
> >  					     I3C_ADDR_SLOT_FREE);
> >  
> >  	if (dev->boardinfo && dev->boardinfo->init_dyn_addr)
> > -		i3c_bus_set_addr_slot_status(&master->bus, dev->info.dyn_addr,
> > +		i3c_bus_set_addr_slot_status(&master->bus,
> > +					     dev->boardinfo->init_dyn_addr,
> >  					     I3C_ADDR_SLOT_FREE);
> >  }
> >  
> > @@ -1675,6 +1676,11 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> >  				ret = -EBUSY;
> >  				goto err_detach_devs;
> >  			}
> > +
> > +			/* Reserve the slot. */
> > +			i3c_bus_set_addr_slot_status(&master->bus,
> > +						     i3cboardinfo->init_dyn_addr,
> > +						     I3C_ADDR_SLOT_I3C_DEV);
> >  		}
> >  
> >  		i3cdev = i3c_master_alloc_i3c_dev(master, &info);
> > -- 
> > 2.14.0
> 
> In my experience we should pre-reserve all DT DA entries and not use them 
> during the ENTDAA for safety reasons.
> That would be one of my next steps in this patch series.

Actually, I reserve DT DA address above. The only thing I will rework, I
won't free up the address. That will be documented in the code.

We can rework it later if really needed.

> 
> Best regards,
> Vitor Soares
>

Patch
diff mbox series

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 5c06c41e6660..fab6e0609fca 100755
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1263,7 +1263,8 @@  static void i3c_master_put_i3c_addrs(struct i3c_dev_desc *dev)
 					     I3C_ADDR_SLOT_FREE);
 
 	if (dev->boardinfo && dev->boardinfo->init_dyn_addr)
-		i3c_bus_set_addr_slot_status(&master->bus, dev->info.dyn_addr,
+		i3c_bus_set_addr_slot_status(&master->bus,
+					     dev->boardinfo->init_dyn_addr,
 					     I3C_ADDR_SLOT_FREE);
 }
 
@@ -1675,6 +1676,11 @@  static int i3c_master_bus_init(struct i3c_master_controller *master)
 				ret = -EBUSY;
 				goto err_detach_devs;
 			}
+
+			/* Reserve the slot. */
+			i3c_bus_set_addr_slot_status(&master->bus,
+						     i3cboardinfo->init_dyn_addr,
+						     I3C_ADDR_SLOT_I3C_DEV);
 		}
 
 		i3cdev = i3c_master_alloc_i3c_dev(master, &info);