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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



--
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



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux