| Message ID | 20240820151917.3904956-1-ravindra.yashvant.shinde@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/1] i3c: master: Avoid sending DISEC command with old device address. | expand |
Hi Ravindra, On 8/20/2024 8:49 PM, Ravindra Yashvant Shinde 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. > After 2-3 reads i could understand what exactly you are trying. Could you please keep the complete commit log here ? " 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, update the olddev->ibi->enabled flag to false to avoid DISEC with OldAddr. " Make a complete story here, you don't need function to be added here since description should make it clear to understand. > i3c_master_add_i3c_dev_locked() > { > ... > olddev = i3c_master_search_i3c_dev_duplicate(newdev); > ... > if (olddev) { > enable_ibi = true; > ... > } > i3c_dev_free_ibi_locked(olddev); > ^^^^^^^^ > This function internally calls i3c_dev_disable_ibi_locked(addr) > function causing to send DISEC command with old Address. > > 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 Isn't this the same device who raised HOTJOIN and seeking a new dynamic address ? you intend to have no communication with older device address right ? > result in NACK or timeout. So, update the olddev->ibi->enabled > flag to false to avoid DISEC with OldAddr. > ... > } > > Signed-off-by: Ravindra Yashvant Shinde <ravindra.yashvant.shinde@nxp.com> > --- > change from v1 to v2 > - Fixed the author name. > - Unconditional set to the false & added some comments. > --- > drivers/i3c/master.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 7028f03c2c42..0dd8d1c28d58 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -2043,7 +2043,14 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > enable_ibi = true; > 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, update the olddev->ibi->enabled flag to false > + * to avoid DISEC with OldAddr. > + */ > + olddev->ibi->enabled = false; > i3c_dev_free_ibi_locked(olddev); > } > mutex_unlock(&olddev->ibi_lock);
On Thu, Sep 05, 2024 at 03:25:09PM +0530, Mukesh Kumar Savaliya wrote: > Hi Ravindra, > > > On 8/20/2024 8:49 PM, Ravindra Yashvant Shinde 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. > > > After 2-3 reads i could understand what exactly you are trying. > Could you please keep the complete commit log here ? > > " > 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, update the olddev->ibi->enabled > flag to false to avoid DISEC with OldAddr. > " > > Make a complete story here, you don't need function to be added here since > description should make it clear to understand. This one is only partial fix this problem. I merged it into https://lore.kernel.org/linux-i3c/20240829-i3c_fix-v4-1-ebcbd5efceba@nxp.com/T/#u This one is full fix. Frank > > > i3c_master_add_i3c_dev_locked() > > { > > ... > > olddev = i3c_master_search_i3c_dev_duplicate(newdev); > > ... > > if (olddev) { > > enable_ibi = true; > > ... > > } > > i3c_dev_free_ibi_locked(olddev); > > ^^^^^^^^ > > This function internally calls i3c_dev_disable_ibi_locked(addr) > > function causing to send DISEC command with old Address. > > > > 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 > Isn't this the same device who raised HOTJOIN and seeking a new dynamic > address ? you intend to have no communication with older device address > right ? > > result in NACK or timeout. So, update the olddev->ibi->enabled > > flag to false to avoid DISEC with OldAddr. > > ... > > } > > > > Signed-off-by: Ravindra Yashvant Shinde <ravindra.yashvant.shinde@nxp.com> > > --- > > change from v1 to v2 > > - Fixed the author name. > > - Unconditional set to the false & added some comments. > > --- > > drivers/i3c/master.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > index 7028f03c2c42..0dd8d1c28d58 100644 > > --- a/drivers/i3c/master.c > > +++ b/drivers/i3c/master.c > > @@ -2043,7 +2043,14 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > > enable_ibi = true; > > 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, update the olddev->ibi->enabled flag to false > > + * to avoid DISEC with OldAddr. > > + */ > > + olddev->ibi->enabled = false; > > i3c_dev_free_ibi_locked(olddev); > > } > > mutex_unlock(&olddev->ibi_lock);
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index 7028f03c2c42..0dd8d1c28d58 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -2043,7 +2043,14 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, enable_ibi = true; 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, update the olddev->ibi->enabled flag to false + * to avoid DISEC with OldAddr. + */ + olddev->ibi->enabled = false; i3c_dev_free_ibi_locked(olddev); } mutex_unlock(&olddev->ibi_lock);
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) { enable_ibi = true; ... } i3c_dev_free_ibi_locked(olddev); ^^^^^^^^ This function internally calls i3c_dev_disable_ibi_locked(addr) function causing to send DISEC command with old Address. 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, update the olddev->ibi->enabled flag to false to avoid DISEC with OldAddr. ... } Signed-off-by: Ravindra Yashvant Shinde <ravindra.yashvant.shinde@nxp.com> --- change from v1 to v2 - Fixed the author name. - Unconditional set to the false & added some comments. --- drivers/i3c/master.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)