[PATCH v3] i3c: dw: reduce do_daa time if there's no client

[PATCH v3] i3c: dw: reduce do_daa time if there's no client

[Jisheng Zhang]

dw_i3c_master_daa() derives the number of newly assigned dynamic
addresses from cmd->rx_len, the ISR sets it to the number of address
slots ENTDAA left unassigned. It starts out as zero, which already
means "every address was assigned", so a timed out transfer leaves
that value in place and it gets used as a result.

If there's no client connected, the addr assign cmd times out, then
the driver calls i3c_master_add_i3c_dev_locked() to add devices that
are not there, each costing about 1s, thus adds non necessary boot
time up to (maxdev * 1)s.

Start from maxdevs instead: no address is assigned before ENTDAA runs,
and the existing rx_count >= maxdevs check then reports an empty bus.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---

since v2:
 - rebase on the lastest i3c/next
 - collect Reviewed-by tag

since v1:
 - use another method to remove the non-necessary calling of
   i3c_master_add_i3c_dev_locked() if there's no client
 - update commit msg
 drivers/i3c/master/dw-i3c-master.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 0a3799a4ce87..4563d8761ba0 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -921,6 +921,7 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
 		goto rpm_out;
 	}
 	cmd = &xfer->cmds[0];
+	cmd->rx_len = master->maxdevs;
 	cmd->cmd_hi = COMMAND_PORT_TRANSFER_ARG;
 	cmd->cmd_lo = COMMAND_PORT_DEV_COUNT(master->maxdevs - pos) |
 		      COMMAND_PORT_DEV_INDEX(pos) |
-- 
2.51.0


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v3] i3c: dw: reduce do_daa time if there's no client

[Jisheng Zhang]

dw_i3c_master_daa() derives the number of newly assigned dynamic
addresses from cmd->rx_len, the ISR sets it to the number of address
slots ENTDAA left unassigned. It starts out as zero, which already
means "every address was assigned", so a timed out transfer leaves
that value in place and it gets used as a result.

If there's no client connected, the addr assign cmd times out, then
the driver calls i3c_master_add_i3c_dev_locked() to add devices that
are not there, each costing about 1s, thus adds non necessary boot
time up to (maxdev * 1)s.

Start from maxdevs instead: no address is assigned before ENTDAA runs,
and the existing rx_count >= maxdevs check then reports an empty bus.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---

since v2:
 - rebase on the lastest i3c/next
 - collect Reviewed-by tag

since v1:
 - use another method to remove the non-necessary calling of
   i3c_master_add_i3c_dev_locked() if there's no client
 - update commit msg
 drivers/i3c/master/dw-i3c-master.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 0a3799a4ce87..4563d8761ba0 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -921,6 +921,7 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
 		goto rpm_out;
 	}
 	cmd = &xfer->cmds[0];
+	cmd->rx_len = master->maxdevs;
 	cmd->cmd_hi = COMMAND_PORT_TRANSFER_ARG;
 	cmd->cmd_lo = COMMAND_PORT_DEV_COUNT(master->maxdevs - pos) |
 		      COMMAND_PORT_DEV_INDEX(pos) |
-- 
2.51.0

Re: [PATCH v3] i3c: dw: reduce do_daa time if there's no client

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Preexisting bug: `dw_i3c_master_daa` assumes `master->free_pos` is contiguous. A fragmented DAT can cause the hardware to overwrite existing devices and the software to drop newly assigned devices.
--

--- Patch [1]: [PATCH v3] i3c: dw: reduce do_daa time if there's no client ---
commit e31611e77943457ade6c812c1e49e6efc9e01ce2
Author: Jisheng Zhang <jszhang@kernel.org>

i3c: dw: reduce do_daa time if there's no client

This commit modifies dw_i3c_master_daa() to initialize cmd->rx_len to
master->maxdevs. This prevents the driver from attempting to add nonexistent
devices when an address assignment command times out on an empty bus.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 0a3799a4ce872..4563d8761ba0b 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -921,6 +921,7 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
>  		goto rpm_out;
>  	}
>  	cmd = &xfer->cmds[0];
> +	cmd->rx_len = master->maxdevs;
>  	cmd->cmd_hi = COMMAND_PORT_TRANSFER_ARG;
>  	cmd->cmd_lo = COMMAND_PORT_DEV_COUNT(master->maxdevs - pos) |
>  		      COMMAND_PORT_DEV_INDEX(pos) |

[Severity: High]
This is a pre-existing issue, but does this hardware instruction safely handle
fragmented Device Address Tables (DAT)?

If master->free_pos is fragmented (for example, devices are present at index 0
and 2, but index 1 is detached), dw_i3c_master_get_free_pos() returns pos = 1.
The hardware is then instructed to assign addresses sequentially for 
master->maxdevs - 1 devices starting at DEV_INDEX 1.

If the DesignWare I3C hardware sequentially iterates from DEV_INDEX without
skipping occupied slots, won't it assign a new device to the pre-existing
address at DAT[2] during a DAA (like Hot-Join), causing a bus collision?

Furthermore, looking later in dw_i3c_master_daa():

	newdevs &= ~olddevs;

If the hardware sequentially overwrote slot 2, that index was originally in
olddevs and is now masked out by this calculation. Won't the driver then
silently drop the newly assigned device since it gets cleared from the
newdevs bitmask?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819044833.32611-1-jszhang@kernel.org?part=1

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c