Re: [PATCH 03/15] be2iscsi: Fix to use atomic operations for tag_state

[Mike Christie <michaelc@cs.wisc.edu>]

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?