Re: [PATCH v2 15/20] scsi: ufs: Improve SCSI abort handling

Bart Van Assche
2021-11-30T04:13:07Z

On 11/24/21 04:28, Adrian Hunter wrote:
> On 19/11/2021 21:57, Bart Van Assche wrote:
>> Release resources when aborting a command. Make sure that aborted commands
>> are completed once by clearing the corresponding tag bit from
>> hba->outstanding_reqs.
>>
>> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 18 ++++++++++++++++--
>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 39dcf997a638..7e27d6436889 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -7042,8 +7042,12 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>>
>> ufshcd_hold(hba, false);
>> reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>> - /* If command is already aborted/completed, return FAILED. */
>> - if (!(test_bit(tag, &hba->outstanding_reqs))) {
>> + /*
>> + * If the command is already aborted/completed, return FAILED. This
>> + * should never happen since the SCSI core serializes error handling
>> + * and scsi_done() calls.
>
> I don't think that is correct. ufshcd_transfer_req_compl() gets called directly
> by the interrupt handler.

I will revert this change.

>> + /*
>> + * Clear the corresponding bit from outstanding_reqs since the command
>> + * has been aborted successfully.
>> + */
>> + spin_lock_irqsave(&hba->outstanding_lock, flags);
>> + __clear_bit(tag, &hba->outstanding_reqs);
>> + spin_unlock_irqrestore(&hba->outstanding_lock, flags);
>> +
>> + ufshcd_release_scsi_cmd(hba, lrbp);
>
> ufshcd_release_scsi_cmd() must not be called if the bit was already clear
> i.e.
>
> spin_lock_irqsave(&hba->outstanding_lock, flags);
> rel = __test_and_clear_bit(tag, &hba->outstanding_reqs);
> spin_unlock_irqrestore(&hba->outstanding_lock, flags);
>
> if (rel)
> ufshcd_release_scsi_cmd(hba, lrbp);

Will look into this.

Thanks,

Bart.