Re: [PATCH] i3c: dw: use COMMAND_PORT_TRANSFER_ARG instead of hardcoding

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

Pre-existing issues:
- [High] Missing check for the transfer error status and timeout in `dw_i3c_master_daa()` leads to incorrectly registering ghost devices on an empty bus or upon hardware failure.
--

--- Patch [1]: [PATCH] i3c: dw: use COMMAND_PORT_TRANSFER_ARG instead of hardcoding ---
commit 22998f6bbcb44d600e23cd17f1e6c5b48d268e31
Author: Jisheng Zhang <[email protected]>

i3c: dw: use COMMAND_PORT_TRANSFER_ARG instead of hardcoding

This commit replaces the hardcoded value 1 with the well defined
COMMAND_PORT_TRANSFER_ARG macro.

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

[ ... ]

>  	dw_i3c_master_enqueue_xfer(master, xfer);
>  	if (!wait_for_completion_timeout(&xfer->comp, XFER_TIMEOUT))
>  		dw_i3c_master_dequeue_xfer(master, xfer);
>  
>  	/*
>  	 * cmd->rx_len holds the number of addresses ENTDAA left unassigned.
>  	 * On an empty bus rx_len == maxdevs, so avoid GENMASK(-1, 0).
>  	 */
>  	if (cmd->rx_len >= master->maxdevs)
>  		newdevs = 0;
>  	else
>  		newdevs = GENMASK(master->maxdevs - cmd->rx_len - 1, 0);

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in how
transfer errors and timeouts are handled here in dw_i3c_master_daa().

If wait_for_completion_timeout() times out due to a noisy or unresponsive
I3C bus, the completion timeout and any hardware transfer errors appear to
be ignored.

In a timeout scenario, cmd->rx_len remains 0. The code then falls through
and calculates newdevs = GENMASK(...) using that zero value.

Could this cause the driver to incorrectly interpret the timeout as a
successful discovery of devices for all free slots, leading it to register
phantom devices?

-- 
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