| Message ID | cover.1567608245.git.vitor.soares@synopsys.com |
|---|---|
| Headers | show |
| Series | i3c: detach/free device fail pre_assign_dyn_addr() | expand |
Hi Boris, From: Vitor Soares <vitor.soares@synopsys.com> Date: Thu, Sep 05, 2019 at 11:00:34 > As for today, the I3C framework is keeping in memory and master->bus.devs > list the devices that fail during pre_assign_dyn_addr() and send them on > DEFSLVS command. > > According to MIPI I3C Bus spec the DEFSLVS command is used to inform any > Secondary Master about the Dynamic Addresses that were assigned to I3C > devices and the I2C devices present on the bus. > > This issue could be fixed by changing i3c_master_defslvs_locked() to > ignore unaddressed i3c devices but the i3c_dev_desc would be allocated and > attached to HC unnecessarily. This can cause that some HC aren't able to > do DAA for HJ capable devices due to lack of space in controller. > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > --- > Changes in v3: > - Change commit message > > 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 I think I clearly explain why the current solution is problematic and my choices for the proposal solution. Please let me know if there is anything else that I can improve in this patch. Best regards, Vitor Soares
On Thu, 5 Sep 2019 12:00:35 +0200 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > The newdev->boardinfo assignment was missing in > i3c_master_add_i3c_dev_locked() and hence the ->of_node info isn't > propagated to i3c_dev_desc. > > Fix this by trying to initialize device i3c_dev_boardinfo if available. > > Cc: <stable@vger.kernel.org> > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > --- > Change in v3: > - None > > Changes in v2: > - Change commit message > - Change i3c_master_search_i3c_boardinfo(newdev) to > i3c_master_init_i3c_dev_boardinfo(newdev) > - Add fixes, stable tags > > drivers/i3c/master.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 586e34f..9fb99bc 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -1798,6 +1798,22 @@ i3c_master_search_i3c_dev_duplicate(struct i3c_dev_desc *refdev) > return NULL; > } > > +static void i3c_master_init_i3c_dev_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; > + > + list_for_each_entry(boardinfo, &master->boardinfo.i3c, node) { > + if (dev->info.pid == boardinfo->pid) { > + dev->boardinfo = boardinfo; > + return; > + } > + } > +} > + > /** > * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus > * @master: master used to send frames on the bus > @@ -1818,8 +1834,9 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > u8 addr) > { > struct i3c_device_info info = { .dyn_addr = addr }; > - struct i3c_dev_desc *newdev, *olddev; > u8 old_dyn_addr = addr, expected_dyn_addr; > + enum i3c_addr_slot_status addrstatus; > + struct i3c_dev_desc *newdev, *olddev; > struct i3c_ibi_setup ibireq = { }; > bool enable_ibi = false; > int ret; > @@ -1878,6 +1895,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > if (ret) > goto err_detach_dev; > > + i3c_master_init_i3c_dev_boardinfo(newdev); > + > /* > * Depending on our previous state, the expected dynamic address might > * differ: > @@ -1895,7 +1914,11 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > else > expected_dyn_addr = newdev->info.dyn_addr; > > - if (newdev->info.dyn_addr != expected_dyn_addr) { > + addrstatus = i3c_bus_get_addr_slot_status(&master->bus, > + expected_dyn_addr); > + > + if (newdev->info.dyn_addr != expected_dyn_addr && > + addrstatus == I3C_ADDR_SLOT_FREE) { First, this change shouldn't be part of this patch, since the commit message only mentions the boardinfo init stuff, not the extra 'is slot free check'. Plus, I want the fix to be backported so we should avoid any unneeded deps. But even with those 2 things addressed, I'm still convinced the 'free desc when device is not reachable' change you do in patch 1 is not that great, and the fact that you can't pre-reserve the address to make sure no one uses it until the device had a chance to show up tends to prove me right. Can we please do what I suggest and solve the "not enough dev slots" problem later on (if we really have to). > /* > * Try to apply the expected dynamic address. If it fails, keep > * the address assigned by the master.
Hi Boris, From: Boris Brezillon <boris.brezillon@collabora.com> Date: Thu, Oct 03, 2019 at 15:29:43 > On Thu, 5 Sep 2019 12:00:35 +0200 > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > The newdev->boardinfo assignment was missing in > > i3c_master_add_i3c_dev_locked() and hence the ->of_node info isn't > > propagated to i3c_dev_desc. > > > > Fix this by trying to initialize device i3c_dev_boardinfo if available. > > > > Cc: <stable@vger.kernel.org> > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > > --- > > Change in v3: > > - None > > > > Changes in v2: > > - Change commit message > > - Change i3c_master_search_i3c_boardinfo(newdev) to > > i3c_master_init_i3c_dev_boardinfo(newdev) > > - Add fixes, stable tags > > > > /** > > * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus > > * @master: master used to send frames on the bus > > @@ -1818,8 +1834,9 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > > u8 addr) > > { > > struct i3c_device_info info = { .dyn_addr = addr }; > > - struct i3c_dev_desc *newdev, *olddev; > > u8 old_dyn_addr = addr, expected_dyn_addr; > > + enum i3c_addr_slot_status addrstatus; > > + struct i3c_dev_desc *newdev, *olddev; > > struct i3c_ibi_setup ibireq = { }; > > bool enable_ibi = false; > > int ret; > > @@ -1878,6 +1895,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > > if (ret) > > goto err_detach_dev; > > > > + i3c_master_init_i3c_dev_boardinfo(newdev); > > + > > /* > > * Depending on our previous state, the expected dynamic address might > > * differ: > > @@ -1895,7 +1914,11 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > > else > > expected_dyn_addr = newdev->info.dyn_addr; > > > > - if (newdev->info.dyn_addr != expected_dyn_addr) { > > + addrstatus = i3c_bus_get_addr_slot_status(&master->bus, > > + expected_dyn_addr); > > + > > + if (newdev->info.dyn_addr != expected_dyn_addr && > > + addrstatus == I3C_ADDR_SLOT_FREE) { > > First, this change shouldn't be part of this patch, since the commit > message only mentions the boardinfo init stuff, This is not an issue, I can create a patch just for boardinfo init fix. > not the extra 'is slot > free check'. Even ignoring patch 1, it is necessary to check if the slot is free because if SETDASA fails the boardinfo->init_dyn_addr can be assigned to another device. That's why we need to check if expected_dyn_addr is free. > Plus, I want the fix to be backported so we should avoid > any unneeded deps. > > But even with those 2 things addressed, I'm still convinced the > 'free desc when device is not reachable' change you do in patch 1 is > not that great, If I'm doing wrong I really appreciate you tell me the reason. > and the fact that you can't pre-reserve the address to > make sure no one uses it until the device had a chance to show up tends > to prove me right. This is a different corner case and I though we agreed that the framework doesn't provide guarantees to assign boardinfo->init_dyn_addr [1]. Yet, I don't disagree with the idea of pre-reserve the boardinfo->init_dyn_addr. I can do this but we need to align how it should be done. > > Can we please do what I suggest and solve the "not enough dev slots" > problem later on (if we really have to). I have this use case where the HC has only 4 slot for 4 devices. Sometimes the one or more devices can be sleeping and when they trigger HJ there is no space in HC. Best regards, Vitor Soares [1] https://patchwork.kernel.org/patch/11120841/
On Thu, 3 Oct 2019 17:37:40 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > Hi Boris, > > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Thu, Oct 03, 2019 at 15:29:43 > > > On Thu, 5 Sep 2019 12:00:35 +0200 > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > The newdev->boardinfo assignment was missing in > > > i3c_master_add_i3c_dev_locked() and hence the ->of_node info isn't > > > propagated to i3c_dev_desc. > > > > > > Fix this by trying to initialize device i3c_dev_boardinfo if available. > > > > > > Cc: <stable@vger.kernel.org> > > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") > > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > > > --- > > > Change in v3: > > > - None > > > > > > Changes in v2: > > > - Change commit message > > > - Change i3c_master_search_i3c_boardinfo(newdev) to > > > i3c_master_init_i3c_dev_boardinfo(newdev) > > > - Add fixes, stable tags > > > > > > /** > > > * i3c_master_add_i3c_dev_locked() - add an I3C slave to the bus > > > * @master: master used to send frames on the bus > > > @@ -1818,8 +1834,9 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > > > u8 addr) > > > { > > > struct i3c_device_info info = { .dyn_addr = addr }; > > > - struct i3c_dev_desc *newdev, *olddev; > > > u8 old_dyn_addr = addr, expected_dyn_addr; > > > + enum i3c_addr_slot_status addrstatus; > > > + struct i3c_dev_desc *newdev, *olddev; > > > struct i3c_ibi_setup ibireq = { }; > > > bool enable_ibi = false; > > > int ret; > > > @@ -1878,6 +1895,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > > > if (ret) > > > goto err_detach_dev; > > > > > > + i3c_master_init_i3c_dev_boardinfo(newdev); > > > + > > > /* > > > * Depending on our previous state, the expected dynamic address might > > > * differ: > > > @@ -1895,7 +1914,11 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > > > else > > > expected_dyn_addr = newdev->info.dyn_addr; > > > > > > - if (newdev->info.dyn_addr != expected_dyn_addr) { > > > + addrstatus = i3c_bus_get_addr_slot_status(&master->bus, > > > + expected_dyn_addr); > > > + > > > + if (newdev->info.dyn_addr != expected_dyn_addr && > > > + addrstatus == I3C_ADDR_SLOT_FREE) { > > > > First, this change shouldn't be part of this patch, since the commit > > message only mentions the boardinfo init stuff, > > This is not an issue, I can create a patch just for boardinfo init fix. > > > not the extra 'is slot > > free check'. > > Even ignoring patch 1, it is necessary to check if the slot is free > because if SETDASA fails the boardinfo->init_dyn_addr can be assigned to > another device. That's why we need to check if expected_dyn_addr is free. Correct. I thought we were already pre-reserving the init_addr (as described here [1]), but it looks like the code is buggy. That's probably something we should fix (we should reserve ->init_i3c_addr here [2], not ->dyn_addr). > > > Plus, I want the fix to be backported so we should avoid > > any unneeded deps. > > > > But even with those 2 things addressed, I'm still convinced the > > 'free desc when device is not reachable' change you do in patch 1 is > > not that great, > > If I'm doing wrong I really appreciate you tell me the reason. I just think it's easier to keep track of things (like reserved addresses) if the descriptor stays around even if the device is not yet accessible. > > > and the fact that you can't pre-reserve the address to > > make sure no one uses it until the device had a chance to show up tends > > to prove me right. > > This is a different corner case and I though we agreed that the framework > doesn't provide guarantees to assign boardinfo->init_dyn_addr [1]. Well, it doesn't, but we should try hard to not use addresses that have been requested by a device. > > Yet, I don't disagree with the idea of pre-reserve the > boardinfo->init_dyn_addr. > I can do this but we need to align how it should be done. Keep the device around even if SETDASA fails and make sure the ->init_dyn_addr is reserved. It's how it was supposed to work, there's just a bug in the logic. > > > > > Can we please do what I suggest and solve the "not enough dev slots" > > problem later on (if we really have to). > > I have this use case where the HC has only 4 slot for 4 devices. > Sometimes the one or more devices can be sleeping and when they trigger > HJ there is no space in HC. Let's address that separately please. I want to solve one problem at a time. [1]https://elixir.bootlin.com/linux/latest/source/drivers/i3c/master.c#L1330 [2]https://elixir.bootlin.com/linux/latest/source/drivers/i3c/master.c#L1307
As for today, the I3C framework is keeping in memory and master->bus.devs list the devices that fail during pre_assign_dyn_addr() and send them on DEFSLVS command. According to MIPI I3C Bus spec the DEFSLVS command is used to inform any Secondary Master about the Dynamic Addresses that were assigned to I3C devices and the I2C devices present on the bus. This issue could be fixed by changing i3c_master_defslvs_locked() to ignore unaddressed i3c devices but the i3c_dev_desc would be allocated and attached to HC unnecessarily. This can cause that some HC aren't able to do DAA for HJ capable devices due of lack of space. This patch-series propose to detach/free devices that are failing during pre_assign_dyn_addr() and to propagate i3c_boardinfo, if available, to i3c_dev_desc during i3c_master_add_i3c_dev_locked(). Besides the fix for the problem mention above, this change will permit to describe devices with a preferable dynamic address (important due to priority reason) but without a static address in DT. In addition, I'm improving the management of the Data Address Table in DW I3C Master by keeping the free slots consecutive. Change in v3: - Change cover letter - Change commit message for patch 1 - Add Rob rb-tags Change in v2: - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr() - Change i3c_master_search_i3c_boardinfo(newdev) to i3c_master_init_i3c_dev_boardinfo(newdev) - Add fixes, stable tags on patch 2 - Add a note for no guarantee of 'assigned-address' use Vitor Soares (5): i3c: master: detach and free device if pre_assign_dyn_addr() fails i3c: master: make sure ->boardinfo is initialized in add_i3c_dev_locked() dt-bindings: i3c: Make 'assigned-address' valid if static address == 0 dt-bindings: i3c: add a note for no guarantee of 'assigned-address' use i3c: master: dw: reattach device on first available location of address table Documentation/devicetree/bindings/i3c/i3c.txt | 15 ++++++-- drivers/i3c/master.c | 49 ++++++++++++++++++++++----- drivers/i3c/master/dw-i3c-master.c | 16 +++++++++ 3 files changed, 68 insertions(+), 12 deletions(-)