| Message ID | 1533292316-16076-1-git-send-email-sreekanth.reddy@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] mpt3sas: correct reset of smid while clearing scsi tracker | expand |
On Fri, 2018-08-03 at 06:31 -0400, Sreekanth Reddy wrote: > In mpt3sas_base_clear_st() function smid value is reseted in ^^^^^^^ That's not an English word. Please read https://forum.wordreference.com/threads/set-reset-vs-setted-resetted-programming.70776/ > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c > index 902610d..94f7236 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -1702,6 +1702,11 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) > return NULL; > > chain_req = &ioc->chain_lookup[smid - 1].chains_per_smid[chain_offset]; > + /* Added memory barrier to make sure that correct chain tracker > + * is retrieved before incrementing the smid pool's chain_offset > + * value in chain lookup table. > + */ Please verify your patch with checkpatch and use the proper kernel comment style. > + smp_mb__before_atomic(); The purpose of smp_mb__before_atomic() / smp_mb__after_atomic() is to implement acquire / release semantics. I think what you need here is a regular barrier (smp_mb()). > @@ -3283,8 +3288,13 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, > return; > st->cb_idx = 0xFF; > st->direct_io = 0; > - st->smid = 0; > atomic_set(&ioc->chain_lookup[st->smid - 1].chain_offset, 0); > + /* Added memory barrier to make sure that smid is set to zero > + * only after resetting corresponding smid pool's chain_offset to zero > + * in chain lookup table. > + */ > + smp_mb__after_atomic(); > + st->smid = 0; The two above comments also apply to these two changes. Thanks, Bart.
On Fri, Aug 3, 2018 at 5:06 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Fri, 2018-08-03 at 06:31 -0400, Sreekanth Reddy wrote: >> + /* Added memory barrier to make sure that correct chain tracker >> + * is retrieved before incrementing the smid pool's chain_offset >> + * value in chain lookup table. >> + */ > > Please verify your patch with checkpatch and use the proper kernel comment style. This kind of style is what net subsystem is using. Last time I heard about this was that checkpatch doesn't distinguish subsystem and thus allows this kind of style.
On Fri, 2018-08-03 at 18:55 +0300, Andy Shevchenko wrote: > On Fri, Aug 3, 2018 at 5:06 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > > On Fri, 2018-08-03 at 06:31 -0400, Sreekanth Reddy wrote: > > > + /* Added memory barrier to make sure that correct chain tracker > > > + * is retrieved before incrementing the smid pool's chain_offset > > > + * value in chain lookup table. > > > + */ > > > > Please verify your patch with checkpatch and use the proper kernel comment style. > > This kind of style is what net subsystem is using. Last time I heard > about this was that checkpatch doesn't distinguish subsystem and thus > allows this kind of style. Since consistency is appreciated in Linux kernel code and since most other comments in SCSI code start with "/*" on a line by itself I think that's the preferred style for SCSI code. Thanks, Bart.
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c index 902610d..94f7236 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_base.c +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c @@ -1702,6 +1702,11 @@ static int mpt3sas_remove_dead_ioc_func(void *arg) return NULL; chain_req = &ioc->chain_lookup[smid - 1].chains_per_smid[chain_offset]; + /* Added memory barrier to make sure that correct chain tracker + * is retrieved before incrementing the smid pool's chain_offset + * value in chain lookup table. + */ + smp_mb__before_atomic(); atomic_inc(&ioc->chain_lookup[smid - 1].chain_offset); return chain_req; } @@ -3283,8 +3288,13 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, return; st->cb_idx = 0xFF; st->direct_io = 0; - st->smid = 0; atomic_set(&ioc->chain_lookup[st->smid - 1].chain_offset, 0); + /* Added memory barrier to make sure that smid is set to zero + * only after resetting corresponding smid pool's chain_offset to zero + * in chain lookup table. + */ + 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. v2 changelog: Added proper comments to describe the need of using smp_mb__before_atomic() & smp_mb__after_atomic() APIs in the driver before calling these APIs. Used smp_mb__after_atomic() API in mpt3sas_base_clear_st() to make sure that smid is reset to zero only after corresponding smid pool's chain_offset in chain lookup table is reset to zero. Used smp_mb__before_atomic() API in _base_get_chain_buffer_tracker() to make sure that proper chain tracker is retrieved from the corresponding smid's pool from chain table before incrementing smid pool's chain offset value. Signed-off-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com> --- drivers/scsi/mpt3sas/mpt3sas_base.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)