| Message ID | 20240819-i3c_fix-v3-11-7d69f7b0a05e@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i3c: master: some fix and improvemnt for hotjoin | expand |
Hi Frank, Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:05 -0400: > svc_i3c_master_do_daa() { > ... > for (i = 0; i < dev_nb; i++) { > ret = i3c_master_add_i3c_dev_locked(m, addrs[i]); > if (ret) > goto rpm_out; > } > } > > If two devices (A and B) are detected in DAA and address 0xa is assigned to > device A and 0xb to device B, a failure in i3c_master_add_i3c_dev_locked() > for device A (addr: 0xa) could prevent device B (addr: 0xb) from being > registered on the bus. The I3C stack might still consider 0xb a free > address. If a subsequent Hotjoin occurs, 0xb might be assigned to Device A, > causing both devices A and B to use the same address 0xb, violating the I3C > specification. > > The return value for i3c_master_add_i3c_dev_locked() should not be checked > because subsequent steps will scan the entire I3C bus, independent of > whether i3c_master_add_i3c_dev_locked() returns success. > > If device A registration fails, there is still a chance to register device > B. i3c_master_add_i3c_dev_locked() can reset DAA if a failure occurs while > retrieving device information. > > Cc: stable@kernel.org > Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry") > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/i3c/master/svc-i3c-master.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > index 2010495906eb3..003565fddc261 100644 > --- a/drivers/i3c/master/svc-i3c-master.c > +++ b/drivers/i3c/master/svc-i3c-master.c > @@ -1042,11 +1042,8 @@ static int svc_i3c_master_do_daa(struct i3c_master_controller *m) > goto rpm_out; > > /* Register all devices who participated to the core */ > - for (i = 0; i < dev_nb; i++) { > - ret = i3c_master_add_i3c_dev_locked(m, addrs[i]); > - if (ret) > - goto rpm_out; > - } > + for (i = 0; i < dev_nb; i++) > + i3c_master_add_i3c_dev_locked(m, addrs[i]); Makes sense, but please explain why don't check the return value in a comment (your commit log is good). > > /* Configure IBI auto-rules */ > ret = svc_i3c_update_ibirules(master); > With the comment added, Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks, Miquèl
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c index 2010495906eb3..003565fddc261 100644 --- a/drivers/i3c/master/svc-i3c-master.c +++ b/drivers/i3c/master/svc-i3c-master.c @@ -1042,11 +1042,8 @@ static int svc_i3c_master_do_daa(struct i3c_master_controller *m) goto rpm_out; /* Register all devices who participated to the core */ - for (i = 0; i < dev_nb; i++) { - ret = i3c_master_add_i3c_dev_locked(m, addrs[i]); - if (ret) - goto rpm_out; - } + for (i = 0; i < dev_nb; i++) + i3c_master_add_i3c_dev_locked(m, addrs[i]); /* Configure IBI auto-rules */ ret = svc_i3c_update_ibirules(master);
svc_i3c_master_do_daa() { ... for (i = 0; i < dev_nb; i++) { ret = i3c_master_add_i3c_dev_locked(m, addrs[i]); if (ret) goto rpm_out; } } If two devices (A and B) are detected in DAA and address 0xa is assigned to device A and 0xb to device B, a failure in i3c_master_add_i3c_dev_locked() for device A (addr: 0xa) could prevent device B (addr: 0xb) from being registered on the bus. The I3C stack might still consider 0xb a free address. If a subsequent Hotjoin occurs, 0xb might be assigned to Device A, causing both devices A and B to use the same address 0xb, violating the I3C specification. The return value for i3c_master_add_i3c_dev_locked() should not be checked because subsequent steps will scan the entire I3C bus, independent of whether i3c_master_add_i3c_dev_locked() returns success. If device A registration fails, there is still a chance to register device B. i3c_master_add_i3c_dev_locked() can reset DAA if a failure occurs while retrieving device information. Cc: stable@kernel.org Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry") Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/i3c/master/svc-i3c-master.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)