Re: [PATCH v3 07/17] i3c: renesas: Do not attach devices if xfer failed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch prevents discovery of I3C devices by unconditionally discarding successfully attached devices when a DAA transfer terminates with an expected Address NACK, leading to permanently orphaned devices and address collisions.
--

commit 03ecc946e72f3aecbac0cad85676f818d253cd2c
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Do not attach devices if xfer failed

This commit attempts to fix a crash during device attachment when a transfer
fails. It modifies the logic to zero out the new devices mask if the
transfer reports an error, bypassing the attachment loop for those devices.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index c475d30a84c7c..f96848022c45d 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -780,10 +780,15 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>  
>  	renesas_i3c_wait_xfer(i3c, xfer);
>  
> -	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
> -	/* Re-attach all the I3C devices on resume. */
> -	if (!i3c->resuming)
> -		newdevs &= ~olddevs;
> +	/* Skip attaching if there are failures on the xfer. */
> +	if (xfer->ret) {
> +		newdevs = 0;
> +	} else {

[Severity: High]
Does this incorrectly discard successfully attached devices during a partial
device discovery?

During the ENTDAA procedure, the controller assigns addresses sequentially.
When there are fewer unassigned devices on the bus than the requested slots,
the controller eventually receives an address NACK, which is the standard
termination condition for the loop.

This expected NACK translates to a non-zero xfer->ret, but cmd->rx_count
still holds the valid count of unassigned slots. If three devices attach
successfully and the fourth attempt NACKs, xfer->ret is set, and this
logic will set newdevs to 0. 

Could this result in discarding the three successfully attached devices and
leaving them orphaned on the bus with active dynamic addresses, breaking
discovery for any bus that is not fully populated?

> +		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
> +		/* Re-attach all the I3C devices on resume. */
> +		if (!i3c->resuming)
> +			newdevs &= ~olddevs;
> +	}
>  
>  	for (pos = 0; pos < i3c->maxdevs; pos++) {
>  		if (!(newdevs & BIT(pos)))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=7

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