diff mbox series

scsi: ufs: Improve SCSI abort handling

Message ID 1891546521.01636066202065.JavaMail.epsvc@epcpadp3 (mailing list archive)
State Not Applicable
Headers show
Series scsi: ufs: Improve SCSI abort handling | expand

Commit Message

Daejun Park Nov. 4, 2021, 10:39 p.m. UTC 
Hi Bart,


>diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>index 3b4a714e2f68..d8a59258b1dc 100644
>--- a/drivers/scsi/ufs/ufshcd.c
>+++ b/drivers/scsi/ufs/ufshcd.c
>@@ -7069,6 +7069,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>                 goto release;
>         }
> 
>+        lrbp->cmd = NULL;
>         err = SUCCESS;
> 
> release:

I found similar code in the ufshcd_err_handler(). I think the following
patch will required to fix another warning.



Thanks,
Daejun

Comments

Bart Van Assche Nov. 4, 2021, 11:14 p.m. UTC | #1 
On 11/4/21 3:39 PM, Daejun Park wrote:
> I found similar code in the ufshcd_err_handler(). I think the following
> patch will required to fix another warning.
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f5ba8f953b87..cce9abc6b879 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6190,6 +6190,7 @@ static void ufshcd_err_handler(struct work_struct *work)
>                  }
>                  dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag,
>                          hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1);
> +               hba->lrb[tag].cmd = NULL;
>          }
> 
>          /* Clear pending task management requests */

Hmm ... since the error handler calls ufshcd_complete_requests(), 
shouldn't the completion function clear the 'cmd' member? I'm concerned 
that the above change would break the completion handler.

Thanks,

Bart.
Daejun Park Nov. 4, 2021, 11:39 p.m. UTC | #2 
>On 11/4/21 3:39 PM, Daejun Park wrote:
>> I found similar code in the ufshcd_err_handler(). I think the following
>> patch will required to fix another warning.
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index f5ba8f953b87..cce9abc6b879 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -6190,6 +6190,7 @@ static void ufshcd_err_handler(struct work_struct *work)
>>                  }
>>                  dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag,
>>                          hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1);
>> +               hba->lrb[tag].cmd = NULL;
>>          }
>> 
>>          /* Clear pending task management requests */
> 
>Hmm ... since the error handler calls ufshcd_complete_requests(), 
>shouldn't the completion function clear the 'cmd' member? I'm concerned 
>that the above change would break the completion handler.

I missed that the error handler calls ufshcd_complete_requests(). Please
ignore my suggestion.

By the way, I give my reviewed-by tag.

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun
Bart Van Assche Nov. 4, 2021, 11:54 p.m. UTC | #3 
On 11/4/21 4:39 PM, Daejun Park wrote:
>> On 11/4/21 3:39 PM, Daejun Park wrote:
>>> I found similar code in the ufshcd_err_handler(). I think the following
>>> patch will required to fix another warning.
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index f5ba8f953b87..cce9abc6b879 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -6190,6 +6190,7 @@ static void ufshcd_err_handler(struct work_struct *work)
>>>                   }
>>>                   dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag,
>>>                           hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1);
>>> +               hba->lrb[tag].cmd = NULL;
>>>           }
>>>
>>>           /* Clear pending task management requests */
>>
>> Hmm ... since the error handler calls ufshcd_complete_requests(),
>> shouldn't the completion function clear the 'cmd' member? I'm concerned
>> that the above change would break the completion handler.
> 
> I missed that the error handler calls ufshcd_complete_requests(). Please
> ignore my suggestion.
> 
> By the way, I give my reviewed-by tag.
> 
> Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks Daejun! However, your question made me wonder whether ufshcd_abort()
should clear the 'tag' bit from hba->outstanding_reqs. Although the SCSI
standard requires that a command that is aborted is not completed, the UFSHCI
specification requires that writing into the UTRLCLR register clears the
corresponding bit(s) in the UTRLDBR register. I think bit 'tag' will have to
be cleared from hba->outstanding_reqs to prevent that the aborted request is
completed while the SCSI core is resubmitting it.

Thanks,

Bart.
Daejun Park Nov. 5, 2021, 1:53 a.m. UTC | #4 
>On 11/4/21 4:39 PM, Daejun Park wrote:
>>> On 11/4/21 3:39 PM, Daejun Park wrote:
>>>> I found similar code in the ufshcd_err_handler(). I think the following
>>>> patch will required to fix another warning.
>>>>
>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>> index f5ba8f953b87..cce9abc6b879 100644
>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>> @@ -6190,6 +6190,7 @@ static void ufshcd_err_handler(struct work_struct *work)
>>>>                   }
>>>>                   dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag,
>>>>                           hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1);
>>>> +               hba->lrb[tag].cmd = NULL;
>>>>           }
>>>>
>>>>           /* Clear pending task management requests */
>>>
>>> Hmm ... since the error handler calls ufshcd_complete_requests(),
>>> shouldn't the completion function clear the 'cmd' member? I'm concerned
>>> that the above change would break the completion handler.
>> 
>> I missed that the error handler calls ufshcd_complete_requests(). Please
>> ignore my suggestion.
>> 
>> By the way, I give my reviewed-by tag.
>> 
>> Reviewed-by: Daejun Park <daejun7.park@samsung.com>
> 
>Thanks Daejun! However, your question made me wonder whether ufshcd_abort()
>should clear the 'tag' bit from hba->outstanding_reqs. Although the SCSI
>standard requires that a command that is aborted is not completed, the UFSHCI
>specification requires that writing into the UTRLCLR register clears the
>corresponding bit(s) in the UTRLDBR register. I think bit 'tag' will have to
>be cleared from hba->outstanding_reqs to prevent that the aborted request is
>completed while the SCSI core is resubmitting it.

My understanding is the completion of aborted request can be duplicated
because SCSI core will resubmit the request. Therefore the clearing of the 
bit can avoid duplicated completion of the request.

Thanks,
Daejun
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f5ba8f953b87..cce9abc6b879 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6190,6 +6190,7 @@  static void ufshcd_err_handler(struct work_struct *work)
                }
                dev_err(hba->dev, "Aborted tag %d / CDB %#02x\n", tag,
                        hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1);
+               hba->lrb[tag].cmd = NULL;
        }

        /* Clear pending task management requests */