On Tue, 2018-07-31 at 01:35 -0400, Sreekanth Reddy wrote: +AD4- In mpt3sas+AF8-base+AF8-clear+AF8-st() function smid value is reseted in wrong line, +AD4- i.e. driver should reset smid value to zero after decrementing chain+AF8-offset +AD4- counter in chain+AF8-lookup table but in current code, driver is resetting smid +AD4- value before decrementing the chain+AF8-offset counter. which we are correcting +AD4- with this patch. +AD4- +AD4- v1 changelog: +AD4- Added memory barriers before +ACY- after atomic+AF8-set() API. +AD4- +AD4- Signed-off-by: Sreekanth Reddy +ADw-sreekanth.reddy+AEA-broadcom.com+AD4- +AD4- --- +AD4- drivers/scsi/mpt3sas/mpt3sas+AF8-base.c +AHw- 8 +-+-+-+-+-+-+-- +AD4- 1 file changed, 7 insertions(+-), 1 deletion(-) +AD4- +AD4- diff --git a/drivers/scsi/mpt3sas/mpt3sas+AF8-base.c b/drivers/scsi/mpt3sas/mpt3sas+AF8-base.c +AD4- index 902610d..94359d8 100644 +AD4- --- a/drivers/scsi/mpt3sas/mpt3sas+AF8-base.c +AD4- +-+-+- b/drivers/scsi/mpt3sas/mpt3sas+AF8-base.c +AD4- +AEAAQA- -1702,6 +-1702,8 +AEAAQA- static int mpt3sas+AF8-remove+AF8-dead+AF8-ioc+AF8-func(void +ACo-arg) +AD4- return NULL+ADs- +AD4- +AD4- chain+AF8-req +AD0- +ACY-ioc-+AD4-chain+AF8-lookup+AFs-smid - 1+AF0-.chains+AF8-per+AF8-smid+AFs-chain+AF8-offset+AF0AOw- +AD4- +- /+ACo- Adding memory barrier before atomic operation. +ACo-/ +AD4- +- smp+AF8-mb+AF8AXw-before+AF8-atomic()+ADs- 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+AF8-mb+AF8AXw-before+AF8-atomic() call. The comment above a barrier should explain which other code needs the barrier in order to execute correctly. +AD4- atomic+AF8-inc(+ACY-ioc-+AD4-chain+AF8-lookup+AFs-smid - 1+AF0-.chain+AF8-offset)+ADs- +AD4- return chain+AF8-req+ADs- +AD4- +AH0- +AD4- +AEAAQA- -3283,8 +-3285,12 +AEAAQA- void mpt3sas+AF8-base+AF8-clear+AF8-st(struct MPT3SAS+AF8-ADAPTER +ACo-ioc, +AD4- return+ADs- +AD4- st-+AD4-cb+AF8-idx +AD0- 0xFF+ADs- +AD4- st-+AD4-direct+AF8-io +AD0- 0+ADs- +AD4- - st-+AD4-smid +AD0- 0+ADs- +AD4- +- /+ACo- Adding memory barrier before atomic operation. +ACo-/ +AD4- +- smp+AF8-mb+AF8AXw-before+AF8-atomic()+ADs- +AD4- atomic+AF8-set(+ACY-ioc-+AD4-chain+AF8-lookup+AFs-st-+AD4-smid - 1+AF0-.chain+AF8-offset, 0)+ADs- +AD4- +- /+ACo- Adding memory barrier after atomic operation. +ACo-/ +AD4- +- smp+AF8-mb+AF8AXw-after+AF8-atomic()+ADs- +AD4- +- st-+AD4-smid +AD0- 0+ADs- 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+AF8-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.
