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