Re: [PATCH v5 06/17] i3c: renesas: Perform Dynamic Address Assignment on resume

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removal of the DATBASn save/restore mechanism completely breaks the I3C bus upon resume for previously attached devices, and fails to resume if the bus was fully occupied.
--

--- Patch [6]: [PATCH v5 06/17] i3c: renesas: Perform Dynamic Address Assignment on resume ---
commit e4ac838c055dc33beb2ff4e8968a032da48a2a47
Author: Claudiu Beznea <[email protected]>

i3c: renesas: Perform Dynamic Address Assignment on resume

This commit updates the suspend/resume operations by moving them out of
the noirq phase and attempting to perform Dynamic Address Assignment on
resume. It also removes the save and restore of the DATBASn registers.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -1495,15 +1484,19 @@ static int renesas_i3c_resume_noirq(struct device *dev)
>  	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYADV |
>  		       MSDVAD_MDYAD(i3c->dyn_addr));
>  
> -	/* Restore Device Address Table values. */
> -	for (i = 0; i < i3c->maxdevs; i++)
> -		renesas_writel(i3c->regs, DATBAS(i), i3c->DATBASn[i]);
> -
>  	/* I3C hw init. */
>  	renesas_i3c_hw_init(i3c);
>  
> +	ret = i3c_master_do_daa_ext(&i3c->base, true);

[Severity: High]
Does removing the DATBASn save and restore mechanism break communication
with previously attached devices upon resume?

Looking at renesas_i3c_daa(), it explicitly skips hardware register
initialization for previously attached devices:

renesas_i3c_daa() {
        ...
        if (olddevs & BIT(pos))
                continue;
        ...
}

Since the controller was reset during suspend, won't these hardware
registers remain uninitialized (0) if DATBASn isn't manually restored here?

Additionally, if the bus was fully occupied before suspend, will this cause
the DAA process to abort entirely?

renesas_i3c_daa() {
        ...
        ret = renesas_i3c_get_free_pos(i3c);
        if (ret < 0)
                return ret;
        ...
}

If the bus is full, free_pos would be 0, causing this to return -ENOSPC
immediately. This aborts before the ENTDAA command is sent, which would
leave the previously attached devices permanently stranded without dynamic
addresses.

> +	if (ret)
> +		dev_err(dev, "DAA failed on resume, ret=%d", ret);
> +
>  	i2c_mark_adapter_resumed(&i3c->base.i2c);
>  
> +	/*
> +	 * I3C devices may have retained their dynamic address anyway. Do not
> +	 * fail the resume because of DAA error.
> +	 */
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6

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