[PATCH] scsi: ufs: Improve SCSI abort handling

[PATCH] scsi: ufs: Improve SCSI abort handling

[Bart Van Assche]

The following has been observed on a test setup:

WARNING: CPU: 4 PID: 250 at drivers/scsi/ufs/ufshcd.c:2737 ufshcd_queuecommand+0x468/0x65c
Call trace:
 ufshcd_queuecommand+0x468/0x65c
 scsi_send_eh_cmnd+0x224/0x6a0
 scsi_eh_test_devices+0x248/0x418
 scsi_eh_ready_devs+0xc34/0xe58
 scsi_error_handler+0x204/0x80c
 kthread+0x150/0x1b4
 ret_from_fork+0x10/0x30

That warning is triggered by the following statement:

	WARN_ON(lrbp->cmd);

Fix this warning by clearing lrbp->cmd from the abort handler.

Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)

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:

Re: [PATCH] scsi: ufs: Improve SCSI abort handling

[Bean Huo]

On Thu, 2021-11-04 at 11:10 -0700, Bart Van Assche wrote:
> The following has been observed on a test setup:
> 
> WARNING: CPU: 4 PID: 250 at drivers/scsi/ufs/ufshcd.c:2737
> ufshcd_queuecommand+0x468/0x65c
> Call trace:
>  ufshcd_queuecommand+0x468/0x65c
>  scsi_send_eh_cmnd+0x224/0x6a0
>  scsi_eh_test_devices+0x248/0x418
>  scsi_eh_ready_devs+0xc34/0xe58
>  scsi_error_handler+0x204/0x80c
>  kthread+0x150/0x1b4
>  ret_from_fork+0x10/0x30
> 
> That warning is triggered by the following statement:
> 
> 	WARN_ON(lrbp->cmd);
> 
> Fix this warning by clearing lrbp->cmd from the abort handler.
> 
> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Bean Huo <beanhuo@micron.com>

> ---
>  drivers/scsi/ufs/ufshcd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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:

[PATCH] scsi: ufs: Improve SCSI abort handling

[Daejun Park]

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.

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


Thanks,
Daejun

Re: [PATCH] scsi: ufs: Improve SCSI abort handling

[Bart Van Assche]

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.

RE: [PATCH] scsi: ufs: Improve SCSI abort handling

[Daejun Park]

>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

Re: [PATCH] scsi: ufs: Improve SCSI abort handling

[Bart Van Assche]

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.

RE: [PATCH] scsi: ufs: Improve SCSI abort handling

[Daejun Park]

>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

Re: [PATCH] scsi: ufs: Improve SCSI abort handling

[Stanley Chu]

Hi Bart,

On Thu, 2021-11-04 at 11:10 -0700, Bart Van Assche wrote:
> The following has been observed on a test setup:
> 
> WARNING: CPU: 4 PID: 250 at drivers/scsi/ufs/ufshcd.c:2737
> ufshcd_queuecommand+0x468/0x65c
> Call trace:
>  ufshcd_queuecommand+0x468/0x65c
>  scsi_send_eh_cmnd+0x224/0x6a0
>  scsi_eh_test_devices+0x248/0x418
>  scsi_eh_ready_devs+0xc34/0xe58
>  scsi_error_handler+0x204/0x80c
>  kthread+0x150/0x1b4
>  ret_from_fork+0x10/0x30
> 
> That warning is triggered by the following statement:
> 
> 	WARN_ON(lrbp->cmd);
> 
> Fix this warning by clearing lrbp->cmd from the abort handler.
> 
> Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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:

Thanks for this patch! We are looking at the same issue as well
recently.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

Re: [PATCH] scsi: ufs: Improve SCSI abort handling

[Martin K. Petersen]

Bart,

> That warning is triggered by the following statement:
>
> 	WARN_ON(lrbp->cmd);
>
> Fix this warning by clearing lrbp->cmd from the abort handler.

Applied to 5.16/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: ufs: Improve SCSI abort handling

[Martin K. Petersen]

On Thu, 4 Nov 2021 11:10:53 -0700, Bart Van Assche wrote:

> The following has been observed on a test setup:
> 
> WARNING: CPU: 4 PID: 250 at drivers/scsi/ufs/ufshcd.c:2737 ufshcd_queuecommand+0x468/0x65c
> Call trace:
>  ufshcd_queuecommand+0x468/0x65c
>  scsi_send_eh_cmnd+0x224/0x6a0
>  scsi_eh_test_devices+0x248/0x418
>  scsi_eh_ready_devs+0xc34/0xe58
>  scsi_error_handler+0x204/0x80c
>  kthread+0x150/0x1b4
>  ret_from_fork+0x10/0x30
> 
> [...]

Applied to 5.16/scsi-fixes, thanks!

[1/1] scsi: ufs: Improve SCSI abort handling
      https://git.kernel.org/mkp/scsi/c/3ff1f6b6ba6f

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: ufs: Improve SCSI abort handling

[James Bottomley]

On Thu, 2021-11-18 at 23:16 -0500, Martin K. Petersen wrote:
> On Thu, 4 Nov 2021 11:10:53 -0700, Bart Van Assche wrote:
> 
> > The following has been observed on a test setup:
> > 
> > WARNING: CPU: 4 PID: 250 at drivers/scsi/ufs/ufshcd.c:2737
> > ufshcd_queuecommand+0x468/0x65c
> > Call trace:
> >  ufshcd_queuecommand+0x468/0x65c
> >  scsi_send_eh_cmnd+0x224/0x6a0
> >  scsi_eh_test_devices+0x248/0x418
> >  scsi_eh_ready_devs+0xc34/0xe58
> >  scsi_error_handler+0x204/0x80c
> >  kthread+0x150/0x1b4
> >  ret_from_fork+0x10/0x30
> > 
> > [...]
> 
> Applied to 5.16/scsi-fixes, thanks!
> 
> [1/1] scsi: ufs: Improve SCSI abort handling
>       https://git.kernel.org/mkp/scsi/c/3ff1f6b6ba6f

OK, so now we have a conflict between fixes and queue.  My impression
is that the patch causing the conflict:

https://lore.kernel.org/all/20211203231950.193369-14-bvanassche@acm.org/

Actually supersedes this one, so I can simply drop the entirety of this
patch in fixes, is that correct?

James

Re: [PATCH] scsi: ufs: Improve SCSI abort handling

[Bart Van Assche]

On 12/14/21 8:35 AM, James Bottomley wrote:
> On Thu, 2021-11-18 at 23:16 -0500, Martin K. Petersen wrote:
>> Applied to 5.16/scsi-fixes, thanks!
>>
>> [1/1] scsi: ufs: Improve SCSI abort handling
>>        https://git.kernel.org/mkp/scsi/c/3ff1f6b6ba6f
> 
> OK, so now we have a conflict between fixes and queue.  My impression
> is that the patch causing the conflict:
> 
> https://lore.kernel.org/all/20211203231950.193369-14-bvanassche@acm.org/
> 
> Actually supersedes this one, so I can simply drop the entirety of this
> patch in fixes, is that correct?

Hi James,

Commit 1fbaa02dfd05 ("scsi: ufs: Improve SCSI abort handling further") is
intended as an improvement for commit 3ff1f6b6ba6f ("scsi: ufs: core:
Improve SCSI abort handling"). Since commit 3ff1f6b6ba6f is already in
Linus' tree I don't think that it can be dropped? A possible approach is
to revert commit 3ff1f6b6ba6f before merging the mkp-scsi/for-next branch.

Thanks,

Bart.

Re: [PATCH] scsi: ufs: Improve SCSI abort handling

[James Bottomley]

On Tue, 2021-12-14 at 09:37 -0800, Bart Van Assche wrote:
> On 12/14/21 8:35 AM, James Bottomley wrote:
> > On Thu, 2021-11-18 at 23:16 -0500, Martin K. Petersen wrote:
> > > Applied to 5.16/scsi-fixes, thanks!
> > > 
> > > [1/1] scsi: ufs: Improve SCSI abort handling
> > >        https://git.kernel.org/mkp/scsi/c/3ff1f6b6ba6f
> > 
> > OK, so now we have a conflict between fixes and queue.  My
> > impression
> > is that the patch causing the conflict:
> > 
> > https://lore.kernel.org/all/20211203231950.193369-14-bvanassche@acm.org/
> > 
> > Actually supersedes this one, so I can simply drop the entirety of
> > this patch in fixes, is that correct?
> 
> Hi James,
> 
> Commit 1fbaa02dfd05 ("scsi: ufs: Improve SCSI abort handling
> further") is intended as an improvement for commit 3ff1f6b6ba6f
> ("scsi: ufs: core: Improve SCSI abort handling"). Since commit
> 3ff1f6b6ba6f is already in Linus' tree I don't think that it can be
> dropped? A possible approach is to revert commit 3ff1f6b6ba6f before
> merging the mkp-scsi/for-next branch.

I meant the effect of the fixes patch can be dropped in the merge
commit.  So the sole surviving code is from the misc tree.  Like what I
did at the top of the for-next branch:

https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git/commit/?h=for-next&id=014adbc9a838772b265834a55cd7b13eb2665d7e

James

Re: [PATCH] scsi: ufs: Improve SCSI abort handling

[Bart Van Assche]

On 12/14/21 9:43 AM, James Bottomley wrote:
> I meant the effect of the fixes patch can be dropped in the merge
> commit.  So the sole surviving code is from the misc tree.  Like what I
> did at the top of the for-next branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git/commit/?h=for-next&id=014adbc9a838772b265834a55cd7b13eb2665d7e

Thanks for the clarification. The ufshcd_abort() code at the above URL
looks good to me.

Thanks,

Bart.