| Message ID | 3e21481ddf53ea58f5899df6ec542b79b8cbcd68.1567071213.git.vitor.soares@synopsys.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | i3c: remove device if failed on pre_assign_dyn_addr() | expand |
On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <Vitor.Soares@synopsys.com> 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 <vitor.soares@synopsys.com> > --- > 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:
Hi Boris, From: Boris Brezillon <boris.brezillon@collabora.com> Date: Thu, Aug 29, 2019 at 11:44:57 > On Thu, 29 Aug 2019 12:19:33 +0200 > Vitor Soares <Vitor.Soares@synopsys.com> 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 <vitor.soares@synopsys.com> > > --- > > 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
On Thu, 29 Aug 2019 14:00:44 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > Hi Boris, > > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Thu, Aug 29, 2019 at 11:44:57 > > > On Thu, 29 Aug 2019 12:19:33 +0200 > > Vitor Soares <Vitor.Soares@synopsys.com> 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.
On Thu, 29 Aug 2019 16:39:18 +0200 Boris Brezillon <boris.brezillon@collabora.com> wrote: > On Thu, 29 Aug 2019 14:00:44 +0000 > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > Hi Boris, > > > > From: Boris Brezillon <boris.brezillon@collabora.com> > > Date: Thu, Aug 29, 2019 at 11:44:57 > > > > > On Thu, 29 Aug 2019 12:19:33 +0200 > > > Vitor Soares <Vitor.Soares@synopsys.com> 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
From: Boris Brezillon <boris.brezillon@collabora.com> Date: Thu, Aug 29, 2019 at 15:39:41 > On Thu, 29 Aug 2019 16:39:18 +0200 > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > On Thu, 29 Aug 2019 14:00:44 +0000 > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > Hi Boris, > > > > > > From: Boris Brezillon <boris.brezillon@collabora.com> > > > Date: Thu, Aug 29, 2019 at 11:44:57 > > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200 > > > > Vitor Soares <Vitor.Soares@synopsys.com> 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.
On Thu, 29 Aug 2019 15:07:08 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Thu, Aug 29, 2019 at 15:39:41 > > > On Thu, 29 Aug 2019 16:39:18 +0200 > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > On Thu, 29 Aug 2019 14:00:44 +0000 > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > > > Hi Boris, > > > > > > > > From: Boris Brezillon <boris.brezillon@collabora.com> > > > > Date: Thu, Aug 29, 2019 at 11:44:57 > > > > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200 > > > > > Vitor Soares <Vitor.Soares@synopsys.com> 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.
-----Original Message----- From: Boris Brezillon <boris.brezillon@collabora.com> Sent: Thursday, August 29, 2019 4:25 PM To: Vitor Soares <Vitor.Soares@synopsys.com> Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-i3c@lists.infradead.org; bbrezillon@kernel.org; robh+dt@kernel.org; mark.rutland@arm.com; Joao.Pinto@synopsys.com 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 <Vitor.Soares@synopsys.com> wrote: > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Thu, Aug 29, 2019 at 15:39:41 > > > On Thu, 29 Aug 2019 16:39:18 +0200 > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > On Thu, 29 Aug 2019 14:00:44 +0000 > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > > > Hi Boris, > > > > > > > > From: Boris Brezillon <boris.brezillon@collabora.com> > > > > Date: Thu, Aug 29, 2019 at 11:44:57 > > > > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200 > > > > > Vitor Soares <Vitor.Soares@synopsys.com> 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.
On Thu, 29 Aug 2019 15:57:32 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > -----Original Message----- > From: Boris Brezillon > <boris.brezillon@collabora.com> > Sent: Thursday, August 29, 2019 4:25 > PM > To: Vitor Soares <Vitor.Soares@synopsys.com> > Cc: > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > linux-i3c@lists.infradead.org; bbrezillon@kernel.org; robh+dt@kernel.org; > mark.rutland@arm.com; Joao.Pinto@synopsys.com > 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 <Vitor.Soares@synopsys.com> wrote: > > > From: Boris Brezillon > <boris.brezillon@collabora.com> > > Date: Thu, Aug 29, 2019 at 15:39:41 > > > > > > On Thu, 29 Aug 2019 16:39:18 +0200 > > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > On Thu, 29 Aug 2019 14:00:44 +0000 > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > > > > > Hi Boris, > > > > > > > > > > From: Boris Brezillon <boris.brezillon@collabora.com> > > > > > Date: Thu, Aug 29, 2019 at 11:44:57 > > > > > > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200 > > > > > > Vitor Soares <Vitor.Soares@synopsys.com> 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.
On Thu, 29 Aug 2019 12:19:33 +0200 Vitor Soares <Vitor.Soares@synopsys.com> 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 <vitor.soares@synopsys.com> > --- > 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:
From: Boris Brezillon <boris.brezillon@collabora.com> Date: Thu, Aug 29, 2019 at 17:15:20 > On Thu, 29 Aug 2019 15:57:32 +0000 > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > -----Original Message----- > > From: Boris Brezillon > > <boris.brezillon@collabora.com> > > Sent: Thursday, August 29, 2019 4:25 > > PM > > To: Vitor Soares <Vitor.Soares@synopsys.com> > > Cc: > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > > linux-i3c@lists.infradead.org; bbrezillon@kernel.org; robh+dt@kernel.org; > > mark.rutland@arm.com; Joao.Pinto@synopsys.com > > 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 <Vitor.Soares@synopsys.com> wrote: > > > > > From: Boris Brezillon > > <boris.brezillon@collabora.com> > > > Date: Thu, Aug 29, 2019 at 15:39:41 > > > > > > > > > On Thu, 29 Aug 2019 16:39:18 +0200 > > > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > > > On Thu, 29 Aug 2019 14:00:44 +0000 > > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > > > > > > > Hi Boris, > > > > > > > > > > > > From: Boris Brezillon <boris.brezillon@collabora.com> > > > > > > Date: Thu, Aug 29, 2019 at 11:44:57 > > > > > > > > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200 > > > > > > > Vitor Soares <Vitor.Soares@synopsys.com> 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
On Thu, 29 Aug 2019 16:33:43 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Thu, Aug 29, 2019 at 17:15:20 > > > On Thu, 29 Aug 2019 15:57:32 +0000 > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > -----Original Message----- > > > From: Boris Brezillon > > > <boris.brezillon@collabora.com> > > > Sent: Thursday, August 29, 2019 4:25 > > > PM > > > To: Vitor Soares <Vitor.Soares@synopsys.com> > > > Cc: > > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > > > linux-i3c@lists.infradead.org; bbrezillon@kernel.org; robh+dt@kernel.org; > > > mark.rutland@arm.com; Joao.Pinto@synopsys.com > > > 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 <Vitor.Soares@synopsys.com> wrote: > > > > > > > From: Boris Brezillon > > > <boris.brezillon@collabora.com> > > > > Date: Thu, Aug 29, 2019 at 15:39:41 > > > > > > > > > > > > On Thu, 29 Aug 2019 16:39:18 +0200 > > > > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > > > > > > > On Thu, 29 Aug 2019 14:00:44 +0000 > > > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > > > > > > > > > Hi Boris, > > > > > > > > > > > > > > From: Boris Brezillon <boris.brezillon@collabora.com> > > > > > > > Date: Thu, Aug 29, 2019 at 11:44:57 > > > > > > > > > > > > > > > On Thu, 29 Aug 2019 12:19:33 +0200 > > > > > > > > Vitor Soares <Vitor.Soares@synopsys.com> 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.
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:
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 <vitor.soares@synopsys.com> --- drivers/i3c/master.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)