| Message ID | 20240819-i3c_fix-v3-1-7d69f7b0a05e@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i3c: master: some fix and improvemnt for hotjoin | expand |
On Tue, Aug 20, 2024 at 12:02 AM Frank Li <Frank.Li@nxp.com> wrote: > > When a new device hotjoins, a new dynamic address is assigned. > i3c_master_add_i3c_dev_locked() identifies that the device was previously > attached to the bus and locates the olddev. > > i3c_master_add_i3c_dev_locked() > { > ... > olddev = i3c_master_search_i3c_dev_duplicate(newdev); > ... > if (olddev) { > ... > i3c_dev_disable_ibi_locked(olddev); > ^^^^^^ > The olddev should not receive any commands on the i3c bus as it > does not exist and has been assigned a new address. This will > result in NACK or timeout. So remove it. > } > } > > Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry") > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/i3c/master.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 7028f03c2c42e..852b32178b722 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -2039,10 +2039,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > ibireq.max_payload_len = olddev->ibi->max_payload_len; > ibireq.num_slots = olddev->ibi->num_slots; > > - if (olddev->ibi->enabled) { > + if (olddev->ibi->enabled) > enable_ibi = true; > - i3c_dev_disable_ibi_locked(olddev); > - } > > i3c_dev_free_ibi_locked(olddev); i3c_dev_free_ibi_locked will still encounter WARN_ON(dev->ibi->enabled). Thanks, Stanley > } > > -- > 2.34.1 > > > -- > linux-i3c mailing list > linux-i3c@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-i3c
On Tue, Aug 20, 2024 at 09:34:11AM +0800, Stanley Chu wrote: > On Tue, Aug 20, 2024 at 12:02 AM Frank Li <Frank.Li@nxp.com> wrote: > > > > When a new device hotjoins, a new dynamic address is assigned. > > i3c_master_add_i3c_dev_locked() identifies that the device was previously > > attached to the bus and locates the olddev. > > > > i3c_master_add_i3c_dev_locked() > > { > > ... > > olddev = i3c_master_search_i3c_dev_duplicate(newdev); > > ... > > if (olddev) { > > ... > > i3c_dev_disable_ibi_locked(olddev); > > ^^^^^^ > > The olddev should not receive any commands on the i3c bus as it > > does not exist and has been assigned a new address. This will > > result in NACK or timeout. So remove it. > > } > > } > > > > Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry") > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > drivers/i3c/master.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > index 7028f03c2c42e..852b32178b722 100644 > > --- a/drivers/i3c/master.c > > +++ b/drivers/i3c/master.c > > @@ -2039,10 +2039,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > > ibireq.max_payload_len = olddev->ibi->max_payload_len; > > ibireq.num_slots = olddev->ibi->num_slots; > > > > - if (olddev->ibi->enabled) { > > + if (olddev->ibi->enabled) > > enable_ibi = true; > > - i3c_dev_disable_ibi_locked(olddev); > > - } > > > > i3c_dev_free_ibi_locked(olddev); > > i3c_dev_free_ibi_locked will still encounter WARN_ON(dev->ibi->enabled). Thank you test it. The below patch should fix this problem. https://lore.kernel.org/imx/20240820043818.3352614-1-ravindra.yashvant.shinde@nxp.com/T/#u > > Thanks, > Stanley > > > } > > > > -- > > 2.34.1 > > > > > > -- > > linux-i3c mailing list > > linux-i3c@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-i3c
Hi Frank, Frank.li@nxp.com wrote on Tue, 20 Aug 2024 10:45:14 -0400: > On Tue, Aug 20, 2024 at 09:34:11AM +0800, Stanley Chu wrote: > > On Tue, Aug 20, 2024 at 12:02 AM Frank Li <Frank.Li@nxp.com> wrote: > > > > > > When a new device hotjoins, a new dynamic address is assigned. > > > i3c_master_add_i3c_dev_locked() identifies that the device was previously > > > attached to the bus and locates the olddev. > > > > > > i3c_master_add_i3c_dev_locked() > > > { > > > ... > > > olddev = i3c_master_search_i3c_dev_duplicate(newdev); > > > ... > > > if (olddev) { > > > ... > > > i3c_dev_disable_ibi_locked(olddev); > > > ^^^^^^ > > > The olddev should not receive any commands on the i3c bus as it > > > does not exist and has been assigned a new address. This will > > > result in NACK or timeout. So remove it. > > > } > > > } > > > > > > Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry") > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > --- > > > drivers/i3c/master.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > > index 7028f03c2c42e..852b32178b722 100644 > > > --- a/drivers/i3c/master.c > > > +++ b/drivers/i3c/master.c > > > @@ -2039,10 +2039,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > > > ibireq.max_payload_len = olddev->ibi->max_payload_len; > > > ibireq.num_slots = olddev->ibi->num_slots; > > > > > > - if (olddev->ibi->enabled) { > > > + if (olddev->ibi->enabled) > > > enable_ibi = true; > > > - i3c_dev_disable_ibi_locked(olddev); > > > - } > > > > > > i3c_dev_free_ibi_locked(olddev); > > > > i3c_dev_free_ibi_locked will still encounter WARN_ON(dev->ibi->enabled). > > Thank you test it. The below patch should fix this problem. > > https://lore.kernel.org/imx/20240820043818.3352614-1-ravindra.yashvant.shinde@nxp.com/T/#u It should indeed. With this change added: Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks, Miquèl
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index 7028f03c2c42e..852b32178b722 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -2039,10 +2039,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, ibireq.max_payload_len = olddev->ibi->max_payload_len; ibireq.num_slots = olddev->ibi->num_slots; - if (olddev->ibi->enabled) { + if (olddev->ibi->enabled) enable_ibi = true; - i3c_dev_disable_ibi_locked(olddev); - } i3c_dev_free_ibi_locked(olddev); }
When a new device hotjoins, a new dynamic address is assigned. i3c_master_add_i3c_dev_locked() identifies that the device was previously attached to the bus and locates the olddev. i3c_master_add_i3c_dev_locked() { ... olddev = i3c_master_search_i3c_dev_duplicate(newdev); ... if (olddev) { ... i3c_dev_disable_ibi_locked(olddev); ^^^^^^ The olddev should not receive any commands on the i3c bus as it does not exist and has been assigned a new address. This will result in NACK or timeout. So remove it. } } Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry") Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/i3c/master.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)