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

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 <[email protected]>

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/[email protected]?part=1

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c