| Message ID | ed18fd927b5759a6a1edb351113ceca615283189.1567608245.git.vitor.soares@synopsys.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | i3c: detach/free device fail pre_assign_dyn_addr() | expand |
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
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) { /* * Try to apply the expected dynamic address. If it fails, keep * the address assigned by the master.
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(-)