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

On Wed, Jul 15, 2026 at 12:32:01PM +0300, Claudiu Beznea wrote:
>
>
> On 7/14/26 21:57, Frank Li wrote:
> > On Tue, Jul 14, 2026 at 12:30:13PM +0300, Claudiu Beznea wrote:
> > >
> > > On 7/13/26 19:59, Frank Li wrote:
> > > > 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?
> > > I tried to address this though the versions v2-v4 variant of this patch that
> > > I've posted previously but after more investigation and thinking it looked
> > > to me that the solution to this is not that simple and related only to this
> > > driver (see below).
> > >
> > > Last time I've asked for some guidance [1] but got no input on it.
> > >
> > > Apart from v4, I tried (before posting v5) to use separate free_pos list to
> > > be used while resuming, and still use only i3c_master_add_i3c_dev_locked()
> > > in renesas_i3c_daa() but that didn't work either, because the devices
> > > (present before suspend and) discovered at resume, ended up to be added to
> > > the new list then deleted by the code of i3c_master_add_i3c_dev_locked().
> > >
> > > I have also thought about increasing the free_pos bitmask with one extra
> > > entry while resuming to allow attach, detach, reattach work (for the devices
> > > connected before suspend), but I don't think that is going to work either,
> > > and looks hackish to me.
> > >
> > > I also tried calling i3c_master_detach_free_devs() (only the code for I3C
> > > devices) before running DAA on resume but that wasn't enough for the
> > > solution to work.
> > >
> > > I think this issue reported by sashiko can be encountered (at some point) on
> > > all the drivers that track and limit the number of attached devices with a
> > > bitmask (or other mechanism) and check that bitmask in the ->attach_i3c_dev.
> > >
> > > E.g., on the Renesas RZ/G3S I can end up to that point even w/o the code
> > > from this patch doing the following:
> > > 1/ connect 2 I3C devices to the bus (I'm using the NXP P3T1085UK-ARD with 2
> > >     temperature sensors)
> > > 2/ after the 2 I3C devices are attached, simulate the I3C bus is
> > >     full (no entries in the free_pos mask) (with the diff patch below)
> > > 3/ cut the power to the NXP P3T1085UK-ARD board
> > > 4/ connect the power to the NXP P3T1085UK-ARD
> > > 5/ at this point if I run do_daa the devices are not going to attach anymore
> > >
> > > I can simulate this on my side. I think it is the case for any other driver
> > > following the above mentioned pattern for attaching.
> > >
> > > That was the reason I have currently dropped the attempt to solve the
> > > problem highlighted by sashiko as its solution looks to me to be more
> > > complicated and involved more testing to be done, not only related to this
> > > driver.
> > >
> > > Since the issue is not only related to this patch but more on how the attach
> > > procedure works, and can be reproduced w/o this patch, I considered only the
> > > approach in this patch was be enough for suspend/resume fix and later fixes
> > > should be added to cover this complicated scenario.
> > >
> > > Please let me know if you have any suggestions on how to go forward with
> > > this problem.
> > >
> > > This is the patch I used for simulating full bus:
> > If problem only exist when full, it should be fine.
>
> Yes, it's only when the bus is full.

Reviewed-by: Frank Li <[email protected]>
>
> Thank you,
> Claudiu
>
> > It is less possible to
> > happen at real world.
> >
> > You explain it.
> >
> > Frank
>

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