| Message ID | 1533015328-10260-1-git-send-email-sreekanth.reddy@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] mpt3sas: correct reset of smid while clearing scsi tracker | expand |
Bart, > 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. You asked for Sreekanth to make changes. Please review the result. Thanks!
On Thu, 2018-08-02 at 16:11 -0400, Martin K. Petersen 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. > > You asked for Sreekanth to make changes. Please review the result. Hello Martin, That update had escaped from my attention because I wasn't Cc-ed on the update. Anyway, I will review that patch first. Bart.
On Tue, 2018-07-31 at 01:35 -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. > > v1 changelog: > Added memory barriers before & after atomic_set() API. > > Signed-off-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com> > --- > 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 = &ioc->chain_lookup[smid - 1].chains_per_smid[chain_offset]; > + /* Adding memory barrier before atomic operation. */ > + smp_mb__before_atomic(); I know that checkpatch complains if it encounters a barrier without preceding comment. The comment above this barrier doesn't seem useful to me - it is so general that that comment could be added above every smp_mb__before_atomic() call. The comment above a barrier should explain which other code needs the barrier in order to execute correctly. > atomic_inc(&ioc->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(&ioc->chain_lookup[st->smid - 1].chain_offset, 0); > + /* Adding memory barrier after atomic operation. */ > + smp_mb__after_atomic(); > + st->smid = 0; Same comment here: the comments that have been added are not useful. I think it is clear that you want to enforce the order in which .chain_offset and .smid are written. But which is the other code that can race with this code and that depends on this write order? I think this information should have been mentioned in the patch description. Thanks, Bart.
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 = &ioc->chain_lookup[smid - 1].chains_per_smid[chain_offset]; + /* Adding memory barrier before atomic operation. */ + smp_mb__before_atomic(); atomic_inc(&ioc->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(&ioc->chain_lookup[st->smid - 1].chain_offset, 0); + /* Adding memory barrier after atomic operation. */ + smp_mb__after_atomic(); + st->smid = 0; } /**
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 <sreekanth.reddy@broadcom.com> --- drivers/scsi/mpt3sas/mpt3sas_base.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)