Yep, there is a need for barrier when setting MCC_TAG_STATE_TIMEOUT. I am sorry, was under the assumption that lock prefix would be used for that atomic operation as well. Thanks, JB -----Original Message----- From: Mike Christie [mailto:michaelc@xxxxxxxxxxx] Sent: Sunday, December 20, 2015 3:44 PM To: Jitendra Bhivare; linux-scsi@xxxxxxxxxxxxxxx Subject: Re: [PATCH 03/15] be2iscsi: Fix to use atomic operations for tag_state On 12/20/15 3:01 AM, Mike Christie wrote: > On 12/20/2015 01:44 AM, Mike Christie wrote: > >>> diff --git a/drivers/scsi/be2iscsi/be_cmds.c >>> b/drivers/scsi/be2iscsi/be_cmds.c index 6fabded..21c806f 100644 >>> --- a/drivers/scsi/be2iscsi/be_cmds.c >>> +++ b/drivers/scsi/be2iscsi/be_cmds.c >>> @@ -164,9 +164,8 @@ int beiscsi_mccq_compl(struct beiscsi_hba *phba, >>> } >>> >>> /* Set MBX Tag state to Active */ >>> - mutex_lock(&phba->ctrl.mbox_lock); >>> - phba->ctrl.ptag_state[tag].tag_state = MCC_TAG_STATE_RUNNING; >>> - mutex_unlock(&phba->ctrl.mbox_lock); >>> + atomic_set(&phba->ctrl.ptag_state[tag].tag_state, >>> + MCC_TAG_STATE_RUNNING); >>> > > > Is it possible for be_mcc_compl_process_isr to run before we have set > the state to MCC_TAG_STATE_RUNNING, so the > wait_event_interruptible_timeout call timesout? > > >>> /* wait for the mccq completion */ >>> rc = wait_event_interruptible_timeout( @@ -178,9 +177,8 @@ int >>> beiscsi_mccq_compl(struct beiscsi_hba *phba, >>> if (rc <= 0) { >>> struct be_dma_mem *tag_mem; >>> /* Set MBX Tag state to timeout */ >>> - mutex_lock(&phba->ctrl.mbox_lock); >>> - phba->ctrl.ptag_state[tag].tag_state = MCC_TAG_STATE_TIMEOUT; >>> - mutex_unlock(&phba->ctrl.mbox_lock); >>> + atomic_set(&phba->ctrl.ptag_state[tag].tag_state, >>> + MCC_TAG_STATE_TIMEOUT); >>> >>> /* Store resource addr to be freed later */ >>> tag_mem = &phba->ctrl.ptag_state[tag].tag_mem_state; >>> @@ -199,9 +197,8 @@ int beiscsi_mccq_compl(struct beiscsi_hba *phba, >>> } else { >>> rc = 0; >>> /* Set MBX Tag state to completed */ >>> - mutex_lock(&phba->ctrl.mbox_lock); >>> - phba->ctrl.ptag_state[tag].tag_state = MCC_TAG_STATE_COMPLETED; >>> - mutex_unlock(&phba->ctrl.mbox_lock); >>> + atomic_set(&phba->ctrl.ptag_state[tag].tag_state, >>> + MCC_TAG_STATE_COMPLETED); >>> } >>> >>> mcc_tag_response = phba->ctrl.mcc_numtag[tag]; @@ -373,9 +370,11 >>> @@ int be_mcc_compl_process_isr(struct be_ctrl_info *ctrl, >>> ctrl->mcc_numtag[tag] |= (extd_status & 0x000000FF) << 8; >>> ctrl->mcc_numtag[tag] |= (compl_status & 0x000000FF); >>> >>> - if (ctrl->ptag_state[tag].tag_state == MCC_TAG_STATE_RUNNING) { >>> + if (atomic_read(&ctrl->ptag_state[tag].tag_state) == >>> + MCC_TAG_STATE_RUNNING) { >>> wake_up_interruptible(&ctrl->mcc_wait[tag]); >>> - } else if (ctrl->ptag_state[tag].tag_state == MCC_TAG_STATE_TIMEOUT) { >>> + } else if (atomic_read(&ctrl->ptag_state[tag].tag_state) == >>> + MCC_TAG_STATE_TIMEOUT) { >>> struct be_dma_mem *tag_mem; >>> tag_mem = &ctrl->ptag_state[tag].tag_mem_state; >>> >>> @@ -390,9 +389,8 @@ int be_mcc_compl_process_isr(struct be_ctrl_info *ctrl, >>> tag_mem->va, tag_mem->dma); >>> >>> /* Change tag state */ >>> - mutex_lock(&phba->ctrl.mbox_lock); >>> - ctrl->ptag_state[tag].tag_state = MCC_TAG_STATE_COMPLETED; >>> - mutex_unlock(&phba->ctrl.mbox_lock); >>> + atomic_set(&ctrl->ptag_state[tag].tag_state, >>> + MCC_TAG_STATE_COMPLETED); >>> >>> /* Free MCC Tag */ >>> free_mcc_tag(ctrl, tag); >>> >> >> I think if you only need to get and set a u8 then you can just use a >> u8 since the operation will be atomic. No need for a atomic_t. > > I think you can ignore this. I think you would need some barriers in > there too and it might be more complicated for no gain. > Ughhh. Sorry. The atomic_ops.txt doc says atomic_read/atomic_set use still needs barriers, so I guess they do not do anything for you and a u8 is ok. The memory-barrier.txt doc says wake_up/wait calls have barriers if the wake_up path is hit, so you are ok there. However, besides the timeout issue in the previous mail, can beiscsi_mccq_compl set the tag_mem_state to MCC_TAG_STATE_TIMEOUT and be_mcc_compl_process_isr does not see the tag_mem values updated? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
