[2/4] i3c: master: Check if devices have i3c_dev_boardinfo on i3c_master_add_i3c_dev_locked()

Commit Message

Vitor Soares Aug. 29, 2019, 10:19 a.m. UTC

The I3C devices described in DT might not be attached to the master which
doesn't allow to assign a specific dynamic address.

This patch check if a device has i3c_dev_boardinfo and add it to
i3c_dev_desc structure. In this conditions, the framework will try to
assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.

Signed-off-by: Vitor Soares <[e-mail]>
---
 drivers/i3c/master.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Boris Brezillon Aug. 29, 2019, 10:44 a.m. UTC | #1

On Thu, 29 Aug 2019 12:19:33 +0200
Vitor Soares <[e-mail]> wrote:

> The I3C devices described in DT might not be attached to the master which
> doesn't allow to assign a specific dynamic address.

I remember testing this when developing the framework, so, unless
another patch regressed it, it should already work. I suspect patch 1
is actually the regressing this use case.

> 
> This patch check if a device has i3c_dev_boardinfo and add it to
> i3c_dev_desc structure. In this conditions, the framework will try to
> assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.
> 
> Signed-off-by: Vitor Soares <[e-mail]>
> ---
>  drivers/i3c/master.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 4d29e1f..85fbda6 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1795,6 +1795,23 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  	return NULL;
>  }
>  
> +static struct i3c_dev_boardinfo *
> +i3c_master_search_i3c_boardinfo(struct i3c_dev_desc *dev)
> +{
> +	struct i3c_master_controller *master = i3c_dev_get_master(dev);
> +	struct i3c_dev_boardinfo *boardinfo;
> +
> +	if (dev->boardinfo)
> +		return NULL;
> +
> +	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
> +		if (dev->info.pid == boardinfo->pid)
> +			return boardinfo;
> +	}
> +
> +	return NULL;
> +}
> +
>  /**
>   * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
>   * @master: master used to send frames on the bus
> @@ -1816,6 +1833,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  {
>  	struct i3c_device_info info = { .dyn_addr = addr };
>  	struct i3c_dev_desc *newdev, *olddev;
> +	struct i3c_dev_boardinfo *boardinfo;
>  	u8 old_dyn_addr = addr, expected_dyn_addr;
>  	struct i3c_ibi_setup ibireq = { };
>  	bool enable_ibi = false;
> @@ -1875,6 +1893,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  	if (ret)
>  		goto err_detach_dev;
>  
> +	boardinfo = i3c_master_search_i3c_boardinfo(newdev);
> +	if (boardinfo)
> +		newdev->boardinfo = boardinfo;
> +
>  	/*
>  	 * Depending on our previous state, the expected dynamic address might
>  	 * differ:

Vitor Soares Aug. 29, 2019, 2 p.m. UTC | #2

Hi Boris,

From: Boris Brezillon <[e-mail]>
Date: Thu, Aug 29, 2019 at 11:44:57

> On Thu, 29 Aug 2019 12:19:33 +0200
> Vitor Soares <[e-mail]> wrote:
> 
> > The I3C devices described in DT might not be attached to the master which
> > doesn't allow to assign a specific dynamic address.
> 
> I remember testing this when developing the framework, so, unless
> another patch regressed it, it should already work. I suspect patch 1
> is actually the regressing this use case.

For today it doesn't address the case where the device is described with 
static address = 0, which isn't attached to the controller.
With this change both cases are covered.

> 
> > 
> > This patch check if a device has i3c_dev_boardinfo and add it to
> > i3c_dev_desc structure. In this conditions, the framework will try to
> > assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.
> > 
> > Signed-off-by: Vitor Soares <[e-mail]>
> > ---
> >  drivers/i3c/master.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 4d29e1f..85fbda6 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -1795,6 +1795,23 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
> >  	return NULL;
> >  }
> >  
> > +static struct i3c_dev_boardinfo *
> > +i3c_master_search_i3c_boardinfo(struct i3c_dev_desc *dev)
> > +{
> > +	struct i3c_master_controller *master = i3c_dev_get_master(dev);
> > +	struct i3c_dev_boardinfo *boardinfo;
> > +
> > +	if (dev->boardinfo)
> > +		return NULL;
> > +
> > +	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
> > +		if (dev->info.pid == boardinfo->pid)
> > +			return boardinfo;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> >  /**
> >   * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
> >   * @master: master used to send frames on the bus
> > @@ -1816,6 +1833,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> >  {
> >  	struct i3c_device_info info = { .dyn_addr = addr };
> >  	struct i3c_dev_desc *newdev, *olddev;
> > +	struct i3c_dev_boardinfo *boardinfo;
> >  	u8 old_dyn_addr = addr, expected_dyn_addr;
> >  	struct i3c_ibi_setup ibireq = { };
> >  	bool enable_ibi = false;
> > @@ -1875,6 +1893,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> >  	if (ret)
> >  		goto err_detach_dev;
> >  
> > +	boardinfo = i3c_master_search_i3c_boardinfo(newdev);
> > +	if (boardinfo)
> > +		newdev->boardinfo = boardinfo;
> > +
> >  	/*
> >  	 * Depending on our previous state, the expected dynamic address might
> >  	 * differ:

Best regards,
Vitor Soares

Boris Brezillon Aug. 29, 2019, 2:39 p.m. UTC | #3

On Thu, 29 Aug 2019 14:00:44 +0000
Vitor Soares <[e-mail]> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <[e-mail]>
> Date: Thu, Aug 29, 2019 at 11:44:57
> 
> > On Thu, 29 Aug 2019 12:19:33 +0200
> > Vitor Soares <[e-mail]> wrote:
> >   
> > > The I3C devices described in DT might not be attached to the master which
> > > doesn't allow to assign a specific dynamic address.  
> > 
> > I remember testing this when developing the framework, so, unless
> > another patch regressed it, it should already work. I suspect patch 1
> > is actually the regressing this use case.  
> 
> For today it doesn't address the case where the device is described with 
> static address = 0, which isn't attached to the controller.

Hm, I'm pretty sure I had designed the code to support that case (see
[1]). It might be buggy, but nothing we can't fix I guess.

Boris Brezillon Aug. 29, 2019, 2:39 p.m. UTC | #4

On Thu, 29 Aug 2019 16:39:18 +0200
Boris Brezillon <[e-mail]> wrote:

> On Thu, 29 Aug 2019 14:00:44 +0000
> Vitor Soares <[e-mail]> wrote:
> 
> > Hi Boris,
> > 
> > From: Boris Brezillon <[e-mail]>
> > Date: Thu, Aug 29, 2019 at 11:44:57
> >   
> > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > Vitor Soares <[e-mail]> wrote:
> > >     
> > > > The I3C devices described in DT might not be attached to the master which
> > > > doesn't allow to assign a specific dynamic address.    
> > > 
> > > I remember testing this when developing the framework, so, unless
> > > another patch regressed it, it should already work. I suspect patch 1
> > > is actually the regressing this use case.    
> > 
> > For today it doesn't address the case where the device is described with 
> > static address = 0, which isn't attached to the controller.  
> 
> Hm, I'm pretty sure I had designed the code to support that case (see
> [1]). It might be buggy, but nothing we can't fix I guess.
> 

[1]https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/i3c/master.c#L1898

Vitor Soares Aug. 29, 2019, 3:07 p.m. UTC | #5

From: Boris Brezillon <[e-mail]>
Date: Thu, Aug 29, 2019 at 15:39:41

> On Thu, 29 Aug 2019 16:39:18 +0200
> Boris Brezillon <[e-mail]> wrote:
> 
> > On Thu, 29 Aug 2019 14:00:44 +0000
> > Vitor Soares <[e-mail]> wrote:
> > 
> > > Hi Boris,
> > > 
> > > From: Boris Brezillon <[e-mail]>
> > > Date: Thu, Aug 29, 2019 at 11:44:57
> > >   
> > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > Vitor Soares <[e-mail]> wrote:
> > > >     
> > > > > The I3C devices described in DT might not be attached to the master which
> > > > > doesn't allow to assign a specific dynamic address.    
> > > > 
> > > > I remember testing this when developing the framework, so, unless
> > > > another patch regressed it, it should already work. I suspect patch 1
> > > > is actually the regressing this use case.    
> > > 
> > > For today it doesn't address the case where the device is described with 
> > > static address = 0, which isn't attached to the controller.  
> > 
> > Hm, I'm pretty sure I had designed the code to support that case (see
> > [1]). It might be buggy, but nothing we can't fix I guess.
> > 
> 
> [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e= 

That is only valid if you have olddev which will only exist if static 
address != 0.

Boris Brezillon Aug. 29, 2019, 3:24 p.m. UTC | #6

On Thu, 29 Aug 2019 15:07:08 +0000
Vitor Soares <[e-mail]> wrote:

> From: Boris Brezillon <[e-mail]>
> Date: Thu, Aug 29, 2019 at 15:39:41
> 
> > On Thu, 29 Aug 2019 16:39:18 +0200
> > Boris Brezillon <[e-mail]> wrote:
> >   
> > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > Vitor Soares <[e-mail]> wrote:
> > >   
> > > > Hi Boris,
> > > > 
> > > > From: Boris Brezillon <[e-mail]>
> > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > >     
> > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > Vitor Soares <[e-mail]> wrote:
> > > > >       
> > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > doesn't allow to assign a specific dynamic address.      
> > > > > 
> > > > > I remember testing this when developing the framework, so, unless
> > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > is actually the regressing this use case.      
> > > > 
> > > > For today it doesn't address the case where the device is described with 
> > > > static address = 0, which isn't attached to the controller.    
> > > 
> > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > [1]). It might be buggy, but nothing we can't fix I guess.
> > >   
> > 
> > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=   
> 
> That is only valid if you have olddev which will only exist if static 
> address != 0.

Hm, if you revert patch 1 (and assuming the device is properly defined
in the DT), you should have olddev != NULL when reaching that point. If
that's not the case there's a bug somewhere that should be fixed.

Vitor Soares Aug. 29, 2019, 3:57 p.m. UTC | #7

-----Original Message-----
From: Boris Brezillon 
<[e-mail]> 
Sent: Thursday, August 29, 2019 4:25 
PM
To: Vitor Soares <[e-mail]>
Cc: 
[e-mail]; [e-mail]; 
[e-mail]; [e-mail]; [e-mail]; 
[e-mail]; [e-mail]
Subject: Re: [PATCH 2/4] 
i3c: master: Check if devices have i3c_dev_boardinfo on 
i3c_master_add_i3c_dev_locked()

On Thu, 29 Aug 2019 15:07:08 +0000
Vitor Soares <[e-mail]> wrote:

> From: Boris Brezillon 
<[e-mail]>
> Date: Thu, Aug 29, 2019 at 15:39:41
> 

> > On Thu, 29 Aug 2019 16:39:18 +0200
> > Boris Brezillon <[e-mail]> wrote:
> >   
> > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > Vitor Soares <[e-mail]> wrote:
> > >   
> > > > Hi Boris,
> > > > 
> > > > From: Boris Brezillon <[e-mail]>
> > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > >     
> > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > Vitor Soares <[e-mail]> wrote:
> > > > >       
> > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > doesn't allow to assign a specific dynamic address.      
> > > > > 
> > > > > I remember testing this when developing the framework, so, unless
> > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > is actually the regressing this use case.      
> > > > 
> > > > For today it doesn't address the case where the device is described with 
> > > > static address = 0, which isn't attached to the controller.    
> > > 
> > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > [1]). It might be buggy, but nothing we can't fix I guess.
> > >   
> > 
> > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=   
> 
> That is only valid if you have olddev which will only exist if static 
> address != 0.

Hm, if you revert patch 1 (and assuming the device is properly defined
in the DT), you should have olddev != NULL when reaching that point. If
that's not the case there's a bug somewhere that should be fixed.

No, because the device is not attached.

Boris Brezillon Aug. 29, 2019, 4:15 p.m. UTC | #8

On Thu, 29 Aug 2019 15:57:32 +0000
Vitor Soares <[e-mail]> wrote:

> -----Original Message-----
> From: Boris Brezillon 
> <[e-mail]> 
> Sent: Thursday, August 29, 2019 4:25 
> PM
> To: Vitor Soares <[e-mail]>
> Cc: 
> [e-mail]; [e-mail]; 
> [e-mail]; [e-mail]; [e-mail]; 
> [e-mail]; [e-mail]
> Subject: Re: [PATCH 2/4] 
> i3c: master: Check if devices have i3c_dev_boardinfo on 
> i3c_master_add_i3c_dev_locked()
> 
> On Thu, 29 Aug 2019 15:07:08 +0000
> Vitor Soares <[e-mail]> wrote:
> 
> > From: Boris Brezillon   
> <[e-mail]>
> > Date: Thu, Aug 29, 2019 at 15:39:41
> >   
> 
> > > On Thu, 29 Aug 2019 16:39:18 +0200
> > > Boris Brezillon <[e-mail]> wrote:
> > >     
> > > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > > Vitor Soares <[e-mail]> wrote:
> > > >     
> > > > > Hi Boris,
> > > > > 
> > > > > From: Boris Brezillon <[e-mail]>
> > > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > > >       
> > > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > > Vitor Soares <[e-mail]> wrote:
> > > > > >         
> > > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > > doesn't allow to assign a specific dynamic address.        
> > > > > > 
> > > > > > I remember testing this when developing the framework, so, unless
> > > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > > is actually the regressing this use case.        
> > > > > 
> > > > > For today it doesn't address the case where the device is described with 
> > > > > static address = 0, which isn't attached to the controller.      
> > > > 
> > > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > > [1]). It might be buggy, but nothing we can't fix I guess.
> > > >     
> > > 
> > > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=     
> > 
> > That is only valid if you have olddev which will only exist if static 
> > address != 0.  
> 
> Hm, if you revert patch 1 (and assuming the device is properly defined
> in the DT), you should have olddev != NULL when reaching that point. If
> that's not the case there's a bug somewhere that should be fixed.
> 
> No, because the device is not attached.

Oh, my bad, I see what you mean now. This is definitely a bug and should
have the Fixes tags. I mean, even if we don't care about dynamic
address assignment, I3C drivers might care about the ->of_node that's
attached to the device.

Boris Brezillon Aug. 29, 2019, 4:27 p.m. UTC | #9

On Thu, 29 Aug 2019 12:19:33 +0200
Vitor Soares <[e-mail]> wrote:

> The I3C devices described in DT might not be attached to the master which
> doesn't allow to assign a specific dynamic address.

Dynamic address assignment is not the only problem here (see my
comment about missing ->of_node info). I would simply say that
newdev->boardinfo assignment was missing in
i3c_master_add_i3c_dev_locked() which is already a bug by itself.

I would also change the subject to

"i3c: master: Make sure ->boardinfo is initialized in add_i3c_dev_locked()"

> 
> This patch check if a device has i3c_dev_boardinfo and add it to
> i3c_dev_desc structure. In this conditions, the framework will try to
> assign the i3c_dev_boardinfo->init_dyn_addr even if stactic address = 0.

I would get rid of this explanation and add Fixes/Cc-stable tags.

> 
> Signed-off-by: Vitor Soares <[e-mail]>
> ---
>  drivers/i3c/master.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 4d29e1f..85fbda6 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1795,6 +1795,23 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
>  	return NULL;
>  }
>  
> +static struct i3c_dev_boardinfo *
> +i3c_master_search_i3c_boardinfo(struct i3c_dev_desc *dev)
> +{
> +	struct i3c_master_controller *master = i3c_dev_get_master(dev);
> +	struct i3c_dev_boardinfo *boardinfo;
> +
> +	if (dev->boardinfo)
> +		return NULL;
> +
> +	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
> +		if (dev->info.pid == boardinfo->pid)
> +			return boardinfo;
> +	}
> +
> +	return NULL;
> +}

Can we instead have:

static void i3c_master_init_i3c_dev_boardinfo(struct i3c_dev_desc *dev)
{
	struct i3c_master_controller *master = i3c_dev_get_master(dev);

	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
		if (dev->info.pid == boardinfo->pid)
			dev->boardinfo = boardinfo;
	}
}

> +
>  /**
>   * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
>   * @master: master used to send frames on the bus
> @@ -1816,6 +1833,7 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  {
>  	struct i3c_device_info info = { .dyn_addr = addr };
>  	struct i3c_dev_desc *newdev, *olddev;
> +	struct i3c_dev_boardinfo *boardinfo;
>  	u8 old_dyn_addr = addr, expected_dyn_addr;
>  	struct i3c_ibi_setup ibireq = { };
>  	bool enable_ibi = false;
> @@ -1875,6 +1893,10 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  	if (ret)
>  		goto err_detach_dev;
>  
> +	boardinfo = i3c_master_search_i3c_boardinfo(newdev);
> +	if (boardinfo)
> +		newdev->boardinfo = boardinfo;
> +

And here:

	i3c_master_init_i3c_dev_boardinfo(newdev);

>  	/*
>  	 * Depending on our previous state, the expected dynamic address might
>  	 * differ:

Vitor Soares Aug. 29, 2019, 4:33 p.m. UTC | #10

From: Boris Brezillon <[e-mail]>
Date: Thu, Aug 29, 2019 at 17:15:20

> On Thu, 29 Aug 2019 15:57:32 +0000
> Vitor Soares <[e-mail]> wrote:
> 
> > -----Original Message-----
> > From: Boris Brezillon 
> > <[e-mail]> 
> > Sent: Thursday, August 29, 2019 4:25 
> > PM
> > To: Vitor Soares <[e-mail]>
> > Cc: 
> > [e-mail]; [e-mail]; 
> > [e-mail]; [e-mail]; [e-mail]; 
> > [e-mail]; [e-mail]
> > Subject: Re: [PATCH 2/4] 
> > i3c: master: Check if devices have i3c_dev_boardinfo on 
> > i3c_master_add_i3c_dev_locked()
> > 
> > On Thu, 29 Aug 2019 15:07:08 +0000
> > Vitor Soares <[e-mail]> wrote:
> > 
> > > From: Boris Brezillon   
> > <[e-mail]>
> > > Date: Thu, Aug 29, 2019 at 15:39:41
> > >   
> > 
> > > > On Thu, 29 Aug 2019 16:39:18 +0200
> > > > Boris Brezillon <[e-mail]> wrote:
> > > >     
> > > > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > > > Vitor Soares <[e-mail]> wrote:
> > > > >     
> > > > > > Hi Boris,
> > > > > > 
> > > > > > From: Boris Brezillon <[e-mail]>
> > > > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > > > >       
> > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > > > Vitor Soares <[e-mail]> wrote:
> > > > > > >         
> > > > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > > > doesn't allow to assign a specific dynamic address.        
> > > > > > > 
> > > > > > > I remember testing this when developing the framework, so, unless
> > > > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > > > is actually the regressing this use case.        
> > > > > > 
> > > > > > For today it doesn't address the case where the device is described with 
> > > > > > static address = 0, which isn't attached to the controller.      
> > > > > 
> > > > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > > > [1]). It might be buggy, but nothing we can't fix I guess.
> > > > >     
> > > > 
> > > > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=     
> > > 
> > > That is only valid if you have olddev which will only exist if static 
> > > address != 0.  
> > 
> > Hm, if you revert patch 1 (and assuming the device is properly defined
> > in the DT), you should have olddev != NULL when reaching that point. If
> > that's not the case there's a bug somewhere that should be fixed.
> > 
> > No, because the device is not attached.
> 
> Oh, my bad, I see what you mean now. This is definitely a bug and should
> have the Fixes tags. I mean, even if we don't care about dynamic
> address assignment, I3C drivers might care about the ->of_node that's
> attached to the device.

I didn't consider a bug because in dt-bindings says to not use 'assigned 
address' if SA = 0.
Do you think there is a better way to solve it?

Best regards,
Vitor Soares

Boris Brezillon Aug. 29, 2019, 4:37 p.m. UTC | #11

On Thu, 29 Aug 2019 16:33:43 +0000
Vitor Soares <[e-mail]> wrote:

> From: Boris Brezillon <[e-mail]>
> Date: Thu, Aug 29, 2019 at 17:15:20
> 
> > On Thu, 29 Aug 2019 15:57:32 +0000
> > Vitor Soares <[e-mail]> wrote:
> >   
> > > -----Original Message-----
> > > From: Boris Brezillon 
> > > <[e-mail]> 
> > > Sent: Thursday, August 29, 2019 4:25 
> > > PM
> > > To: Vitor Soares <[e-mail]>
> > > Cc: 
> > > [e-mail]; [e-mail]; 
> > > [e-mail]; [e-mail]; [e-mail]; 
> > > [e-mail]; [e-mail]
> > > Subject: Re: [PATCH 2/4] 
> > > i3c: master: Check if devices have i3c_dev_boardinfo on 
> > > i3c_master_add_i3c_dev_locked()
> > > 
> > > On Thu, 29 Aug 2019 15:07:08 +0000
> > > Vitor Soares <[e-mail]> wrote:
> > >   
> > > > From: Boris Brezillon     
> > > <[e-mail]>  
> > > > Date: Thu, Aug 29, 2019 at 15:39:41
> > > >     
> > >   
> > > > > On Thu, 29 Aug 2019 16:39:18 +0200
> > > > > Boris Brezillon <[e-mail]> wrote:
> > > > >       
> > > > > > On Thu, 29 Aug 2019 14:00:44 +0000
> > > > > > Vitor Soares <[e-mail]> wrote:
> > > > > >       
> > > > > > > Hi Boris,
> > > > > > > 
> > > > > > > From: Boris Brezillon <[e-mail]>
> > > > > > > Date: Thu, Aug 29, 2019 at 11:44:57
> > > > > > >         
> > > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200
> > > > > > > > Vitor Soares <[e-mail]> wrote:
> > > > > > > >           
> > > > > > > > > The I3C devices described in DT might not be attached to the master which
> > > > > > > > > doesn't allow to assign a specific dynamic address.          
> > > > > > > > 
> > > > > > > > I remember testing this when developing the framework, so, unless
> > > > > > > > another patch regressed it, it should already work. I suspect patch 1
> > > > > > > > is actually the regressing this use case.          
> > > > > > > 
> > > > > > > For today it doesn't address the case where the device is described with 
> > > > > > > static address = 0, which isn't attached to the controller.        
> > > > > > 
> > > > > > Hm, I'm pretty sure I had designed the code to support that case (see
> > > > > > [1]). It might be buggy, but nothing we can't fix I guess.
> > > > > >       
> > > > > 
> > > > > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v5.3-2Drc6_source_drivers_i3c_master.c-23L1898&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=IXS1ygIgEo5vwajk0iwd5aBDVBzRnVTjO3cg4iBmGNc&s=HC-AcYm-AZPrUBoALioej_BDnqOtJHltr39Z2yPkuU4&e=       
> > > > 
> > > > That is only valid if you have olddev which will only exist if static 
> > > > address != 0.    
> > > 
> > > Hm, if you revert patch 1 (and assuming the device is properly defined
> > > in the DT), you should have olddev != NULL when reaching that point. If
> > > that's not the case there's a bug somewhere that should be fixed.
> > > 
> > > No, because the device is not attached.  
> > 
> > Oh, my bad, I see what you mean now. This is definitely a bug and should
> > have the Fixes tags. I mean, even if we don't care about dynamic
> > address assignment, I3C drivers might care about the ->of_node that's
> > attached to the device.  
> 
> I didn't consider a bug because in dt-bindings says to not use 'assigned 
> address' if SA = 0.

The fact that we don't try to assign a predefined dynamic address
when !SA is indeed not a bug, but failing to propagate the ->of_node
info to the i3c_device is one.

> Do you think there is a better way to solve it?

Your patch is correct, I just proposed a few adjustments.

Patch

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 4d29e1f..85fbda6 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1795,6 +1795,23 @@  i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev)
 	return NULL;
 }
 
+static struct i3c_dev_boardinfo *
+i3c_master_search_i3c_boardinfo(struct i3c_dev_desc *dev)
+{
+	struct i3c_master_controller *master = i3c_dev_get_master(dev);
+	struct i3c_dev_boardinfo *boardinfo;
+
+	if (dev->boardinfo)
+		return NULL;
+
+	list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) {
+		if (dev->info.pid == boardinfo->pid)
+			return boardinfo;
+	}
+
+	return NULL;
+}
+
 /**
  * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus
  * @master: master used to send frames on the bus
@@ -1816,6 +1833,7 @@  int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 {
 	struct i3c_device_info info = { .dyn_addr = addr };
 	struct i3c_dev_desc *newdev, *olddev;
+	struct i3c_dev_boardinfo *boardinfo;
 	u8 old_dyn_addr = addr, expected_dyn_addr;
 	struct i3c_ibi_setup ibireq = { };
 	bool enable_ibi = false;
@@ -1875,6 +1893,10 @@  int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 	if (ret)
 		goto err_detach_dev;
 
+	boardinfo = i3c_master_search_i3c_boardinfo(newdev);
+	if (boardinfo)
+		newdev->boardinfo = boardinfo;
+
 	/*
 	 * Depending on our previous state, the expected dynamic address might
 	 * differ: