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

On Mon, Jul 13, 2026 at 01:50:32PM +0000, [email protected] wrote:
> 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.

Please check this sashiko review result?

Frank

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

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