On Fri, Aug 3, 2018 at 10:02 PM, Bart Van Assche <Bart.VanAssche@xxxxxxx> wrote: > On Fri, 2018-08-03 at 12:16 -0400, Sreekanth Reddy wrote: >> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c >> index 902610d..2c5a5b4 100644 >> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c >> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c >> @@ -1702,6 +1702,13 @@ 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(); >> atomic_inc(&ioc->chain_lookup[smid - 1].chain_offset); >> return chain_req; >> } >> @@ -3283,8 +3290,15 @@ 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(); >> + st->smid = 0; >> } > > Thanks for having addressed previous review comments. Hence: > > Reviewed-by: Bart Van Assche <bart.vanassche@xxxxxxx> > > However, I'm not yet convinced that this patch is sufficient to fix the race > between _base_get_chain_buffer_tracker() and mpt3sas_base_clear_st(). Can e.g. > the atomic_set() that resets chain_offset occur after it has been read by > _base_get_chain_buffer_tracker() and before that function increments the > chain_offset member variable? > No Bart, their is no race condition here. Since chain lookup table entry is uniquely accessed using smid value. And this smid (which is scmd->request->tag +1) is unique for each IO request. And _base_get_chain_buffer_tracker() function is called only during IO submission time (i.e. before submitting the IO to the IOC firmware) and mpt3sas_base_clear_st() function is called in the ISR (i.e during IO completion) path. So for a particular IO these two functions called at two different instances of time. Thanks, Sreekanth > Thanks, > > Bart. >
