Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

[Adrian Hunter <adrian.hunter@intel.com>]

On 10/11/2021 20:56, Bart Van Assche wrote:
> On 11/10/21 12:57 AM, Adrian Hunter wrote:
>> On 10/11/2021 02:44, Bart Van Assche wrote:
>>> Make sure that aborted commands are completed once by clearing the
>>> corresponding tag bit from hba->outstanding_reqs. This patch is a
>>> follow-up for commit cd892096c940 ("scsi: ufs: core: Improve SCSI
>>> abort handling").
>>>
>>> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>   drivers/scsi/ufs/ufshcd.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index 8f5640647054..1e15ed1f639f 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -7090,6 +7090,15 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>>>           goto release;
>>>       }
>>>   +    /*
>>> +     * ufshcd_try_to_abort_task() cleared the 'tag' bit in the doorbell
>>> +     * register. Clear the corresponding bit from outstanding_reqs to
>>> +     * prevent early completion.
>>> +     */
>>> +    spin_lock_irqsave(&hba->outstanding_lock, flags);
>>> +    __clear_bit(tag, &hba->outstanding_reqs);
>>> +    spin_unlock_irqrestore(&hba->outstanding_lock, flags);
>>
>> Seems like something ufshcd_clear_cmd() should be doing instead?
> 
> Hi Adrian,
> 
> I'm concerned that would break ufshcd_eh_device_reset_handler() since that reset handler retries SCSI commands by calling __ufshcd_transfer_req_compl() after having called ufshcd_clear_cmd().

Whenever an outstanding_reqs bit transitions 1 -> 0, then
__ufshcd_transfer_req_compl() must be called.

In all cases, the correct logic must have the effect of:

	spin_lock_irqsave(&hba->outstanding_lock, flags);
	bit = __test_and_clear_bit(tag, &hba->outstanding_reqs);
	spin_unlock_irqrestore(&hba->outstanding_lock, flags);

	if (bit)
		__ufshcd_transfer_req_compl(hba, 1UL << tag);

To put it another way, how else does the driver know whether or
not __ufshcd_transfer_req_compl() has been called already.


As a separate issue, in ufshcd_abort() there is:

	/* If command is already aborted/completed, return FAILED. */
	if (!(test_bit(tag, &hba->outstanding_reqs))) {
		dev_err(hba->dev,
			"%s: cmd at tag %d already completed, outstanding=0x%lx, doorbell=0x%x\n",
			__func__, tag, hba->outstanding_reqs, reg);
		goto release;
	}

which seems wrong. FAILED should only be returned to escalate the
error handling, so if the slot has already successfully been
cleared, that is SUCCESS.  scsi_times_out() has already blocked
the scsi_done() path (by setting SCMD_STATE_COMPLETE), so any use
after free must be being caused by SCSI not the ufs driver.

path