Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker
Tomas, > Fixes: 2b548dd3790f95c2e174d51c2c8ada71b6505b4e > scsi: mpt3sas: Fix calltrace observed while running IO & reset > > Maybe both patches could be merged. Merged the fix into the patch above. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker
On Wed, 2018-07-18 at 01:22 -0400, Sreekanth Reddy wrote: > In mpt3sas_base_clear_st() function smid value is reseted in wrong line, > i.e. driver should reset smid value to zero after decrementing chain_offset > counter in chain_lookup table but in current code, driver is resetting smid > value before decrementing the chain_offset counter. which we are correcting > with this patch. > > Signed-off-by: Sreekanth Reddy > --- > drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c > b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 902610d..94b939b 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -3283,8 +3283,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > return; > st->cb_idx = 0xFF; > st->direct_io = 0; > - st->smid = 0; > atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); > + st->smid = 0; > } > > /** Reviewed-by: Bart Van Assche
Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker
On Tue, Jul 24, 2018 at 10:37 AM, Sreekanth Reddy wrote: > Any update on this patch!. Just Ping once again. Any update on this patch. > Thanks, > Sreekanth > > On Fri, Jul 20, 2018 at 5:56 PM, Sreekanth Reddy > wrote: >> In mpt3sas_base_clear_st() function smid value is reseted in wrong line, >> i.e. driver should reset smid value to zero after decrementing chain_offset >> counter in chain_lookup table but in current code, driver is resetting smid >> value before decrementing the chain_offset counter. which we are correcting >> with this patch. >> >> v1 changelog: >> Added memory barriers before & after atomic_set() API. >> >> Signed-off-by: Sreekanth Reddy >> --- >> drivers/scsi/mpt3sas/mpt3sas_base.c | 8 +++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c >> b/drivers/scsi/mpt3sas/mpt3sas_base.c >> index 902610d..94359d8 100644 >> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c >> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c >> @@ -1702,6 +1702,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) >> return NULL; >> >> chain_req = >chain_lookup[smid - >> 1].chains_per_smid[chain_offset]; >> + /* Adding memory barrier before atomic operation. */ >> + smp_mb__before_atomic(); >> atomic_inc(>chain_lookup[smid - 1].chain_offset); >> return chain_req; >> } >> @@ -3283,8 +3285,12 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER >> *ioc, >> return; >> st->cb_idx = 0xFF; >> st->direct_io = 0; >> - st->smid = 0; >> + /* Adding memory barrier before atomic operation. */ >> + smp_mb__before_atomic(); >> atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); >> + /* Adding memory barrier after atomic operation. */ >> + smp_mb__after_atomic(); >> + st->smid = 0; >> } >> >> /** >> -- >> 1.8.3.1 >>
Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker
Any update on this patch!. Thanks, Sreekanth On Fri, Jul 20, 2018 at 5:56 PM, Sreekanth Reddy wrote: > In mpt3sas_base_clear_st() function smid value is reseted in wrong line, > i.e. driver should reset smid value to zero after decrementing chain_offset > counter in chain_lookup table but in current code, driver is resetting smid > value before decrementing the chain_offset counter. which we are correcting > with this patch. > > v1 changelog: > Added memory barriers before & after atomic_set() API. > > Signed-off-by: Sreekanth Reddy > --- > drivers/scsi/mpt3sas/mpt3sas_base.c | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c > b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 902610d..94359d8 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -1702,6 +1702,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) > return NULL; > > chain_req = >chain_lookup[smid - > 1].chains_per_smid[chain_offset]; > + /* Adding memory barrier before atomic operation. */ > + smp_mb__before_atomic(); > atomic_inc(>chain_lookup[smid - 1].chain_offset); > return chain_req; > } > @@ -3283,8 +3285,12 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > return; > st->cb_idx = 0xFF; > st->direct_io = 0; > - st->smid = 0; > + /* Adding memory barrier before atomic operation. */ > + smp_mb__before_atomic(); > atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); > + /* Adding memory barrier after atomic operation. */ > + smp_mb__after_atomic(); > + st->smid = 0; > } > > /** > -- > 1.8.3.1 >
[PATCH] mpt3sas: correct reset of smid while clearing scsi tracker
In mpt3sas_base_clear_st() function smid value is reseted in wrong line, i.e. driver should reset smid value to zero after decrementing chain_offset counter in chain_lookup table but in current code, driver is resetting smid value before decrementing the chain_offset counter. which we are correcting with this patch. v1 changelog: Added memory barriers before & after atomic_set() API. Signed-off-by: Sreekanth Reddy --- drivers/scsi/mpt3sas/mpt3sas_base.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c index 902610d..94359d8 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_base.c +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c @@ -1702,6 +1702,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) return NULL; chain_req = >chain_lookup[smid - 1].chains_per_smid[chain_offset]; + /* Adding memory barrier before atomic operation. */ + smp_mb__before_atomic(); atomic_inc(>chain_lookup[smid - 1].chain_offset); return chain_req; } @@ -3283,8 +3285,12 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, return; st->cb_idx = 0xFF; st->direct_io = 0; - st->smid = 0; + /* Adding memory barrier before atomic operation. */ + smp_mb__before_atomic(); atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); + /* Adding memory barrier after atomic operation. */ + smp_mb__after_atomic(); + st->smid = 0; } /** -- 1.8.3.1
Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker
On Wed, Jul 18, 2018 at 7:38 PM, Bart Van Assche wrote: > On Wed, 2018-07-18 at 01:22 -0400, Sreekanth Reddy wrote: >> In mpt3sas_base_clear_st() function smid value is reseted in wrong line, >> i.e. driver should reset smid value to zero after decrementing chain_offset >> counter in chain_lookup table but in current code, driver is resetting smid >> value before decrementing the chain_offset counter. which we are correcting >> with this patch. >> >> Signed-off-by: Sreekanth Reddy >> --- >> drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c >> b/drivers/scsi/mpt3sas/mpt3sas_base.c >> index 902610d..94b939b 100644 >> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c >> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c >> @@ -3283,8 +3283,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, >> return; >> st->cb_idx = 0xFF; >> st->direct_io = 0; >> - st->smid = 0; >> atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); >> + st->smid = 0; >> } > > How can this patch be correct without memory barrier between the atomic set > and the > st->smid assignment? Yes I agree that I need to add memory barrier around atomic_set() API. I was not aware of this before. Is it fine to use smp_mb__before_atomic() API before atomic_set() & smp_mb__after_atomic() API after atomic_set? Also in other places in driver, we are using atomic_inc() & atomic_dec() APIs without memory barrier, I need to add memory barrier their too. So is it fine to post separate patch for adding memory barrier around atomic_xyz() APIs? if yes then please consider this patch and I will post separate patch for adding memory barriers around the atomic_xyz() APIs. Thanks, Sreekanth > > Bart. > >
Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker
On Wed, 2018-07-18 at 01:22 -0400, Sreekanth Reddy wrote: > In mpt3sas_base_clear_st() function smid value is reseted in wrong line, > i.e. driver should reset smid value to zero after decrementing chain_offset > counter in chain_lookup table but in current code, driver is resetting smid > value before decrementing the chain_offset counter. which we are correcting > with this patch. > > Signed-off-by: Sreekanth Reddy > --- > drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c > b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 902610d..94b939b 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -3283,8 +3283,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > return; > st->cb_idx = 0xFF; > st->direct_io = 0; > - st->smid = 0; > atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); > + st->smid = 0; > } How can this patch be correct without memory barrier between the atomic set and the st->smid assignment? Bart.
Re: [PATCH] mpt3sas: correct reset of smid while clearing scsi tracker
On 07/18/2018 07:22 AM, Sreekanth Reddy wrote: > In mpt3sas_base_clear_st() function smid value is reseted in wrong line, > i.e. driver should reset smid value to zero after decrementing chain_offset > counter in chain_lookup table but in current code, driver is resetting smid > value before decrementing the chain_offset counter. which we are correcting > with this patch. > > Signed-off-by: Sreekanth Reddy Fixes: 2b548dd3790f95c2e174d51c2c8ada71b6505b4e scsi: mpt3sas: Fix calltrace observed while running IO & reset Maybe both patches could be merged. Reviewed-by: Tomas Henzl Tomas > --- > drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c > b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 902610d..94b939b 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -3283,8 +3283,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > return; > st->cb_idx = 0xFF; > st->direct_io = 0; > - st->smid = 0; > atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); > + st->smid = 0; > } > > /**
[PATCH] mpt3sas: correct reset of smid while clearing scsi tracker
In mpt3sas_base_clear_st() function smid value is reseted in wrong line, i.e. driver should reset smid value to zero after decrementing chain_offset counter in chain_lookup table but in current code, driver is resetting smid value before decrementing the chain_offset counter. which we are correcting with this patch. Signed-off-by: Sreekanth Reddy --- drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c index 902610d..94b939b 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_base.c +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c @@ -3283,8 +3283,8 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, return; st->cb_idx = 0xFF; st->direct_io = 0; - st->smid = 0; atomic_set(>chain_lookup[st->smid - 1].chain_offset, 0); + st->smid = 0; } /** -- 1.8.3.1
