[PATCH v3 2/5] i3c: master: make sure ->boardinfo is initialized in add_i3c_dev_locked()
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) { /* * Try to apply the expected dynamic address. If it fails, keep * the address assigned by the master.
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
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
linux-stable-mirror@lists.linaro.org
-
Boris Brezillon -
Vitor Soares