[PATCH v2] scsi: ufs: Unbreak the reset handler

[PATCH v2] scsi: ufs: Unbreak the reset handler

[Bart Van Assche]

A command tag is passed as the second argument of the
__ufshcd_transfer_req_compl() call in ufshcd_eh_device_reset_handler()
instead of a bitmask. Fix this by passing a bitmask as argument instead
of a command tag.

Cc: Can Guo <cang@codeaurora.org>
Fixes: a45f937110fa ("scsi: ufs: Optimize host lock on transfer requests send/compl paths")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ufs/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Changes compared to v1: fixed patch description.

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3841ab49f556..d1dc52c76847 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6876,7 +6876,7 @@ static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
 			err = ufshcd_clear_cmd(hba, pos);
 			if (err)
 				break;
-			__ufshcd_transfer_req_compl(hba, pos, /*retry_requests=*/true);
+			__ufshcd_transfer_req_compl(hba, 1U << pos, false);
 		}
 	}
 

RE: [PATCH v2] scsi: ufs: Unbreak the reset handler

[Avri Altman]

> 
> A command tag is passed as the second argument of the
> __ufshcd_transfer_req_compl() call in ufshcd_eh_device_reset_handler()
> instead of a bitmask. Fix this by passing a bitmask as argument instead of a
> command tag.
> 
> Cc: Can Guo <cang@codeaurora.org>
> Fixes: a45f937110fa ("scsi: ufs: Optimize host lock on transfer requests
> send/compl paths")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Few nits below.
Thanks,
Avri

> ---
>  drivers/scsi/ufs/ufshcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Changes compared to v1: fixed patch description.
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 3841ab49f556..d1dc52c76847 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6876,7 +6876,7 @@ static int ufshcd_eh_device_reset_handler(struct
> scsi_cmnd *cmd)
>                         err = ufshcd_clear_cmd(hba, pos);
>                         if (err)
>                                 break;
> -                       __ufshcd_transfer_req_compl(hba, pos,
> /*retry_requests=*/true);
> +                       __ufshcd_transfer_req_compl(hba, 1U << pos,
> + false);
1) Maybe a word in the commit log about changing retry_requests from true to false.
2) Also while at it, maybe change u32 pos to u8 tag?
3) Add an unsigned long pending_reqs, add the tag inside the loop,
 And call __ufshcd_transfer_req_compl(hba, pending_reqs, false) one time outside the loop.

Thanks,
Avri

>                 }
>         }

Re: [PATCH v2] scsi: ufs: Unbreak the reset handler

[Bart Van Assche]

On 9/17/21 2:31 PM, Avri Altman wrote:
>> -                       __ufshcd_transfer_req_compl(hba, pos,
>> /*retry_requests=*/true);
>> +                       __ufshcd_transfer_req_compl(hba, 1U << pos,
>> + false);
> 1) Maybe a word in the commit log about changing retry_requests from true to false.
> 2) Also while at it, maybe change u32 pos to u8 tag?
> 3) Add an unsigned long pending_reqs, add the tag inside the loop,
>   And call __ufshcd_transfer_req_compl(hba, pending_reqs, false) one time outside the loop.

Hi Avri,

While working on this patch I realized that commit c11a1ae9b8f6
("scsi: ufs: Add fault injection support") is not necessary.
ufshcd_prepare_req_desc_hdr() initializes the command status to
OCS_INVALID_COMMAND_STATUS and ufshcd_transfer_rsp_status()
translates that status code into DID_REQUEUE. I'm considering to
revert commit c11a1ae9b8f6.

I prefer not to change "u32 pos" into "u8 tag", otherwise backporting
this patch would become harder than necessary.

Is (3) important to you? I don't think that calls to
ufshcd_eh_device_reset_handler() are that common so performance
probably is not that important in this function.

Thanks,

Bart.

RE: [PATCH v2] scsi: ufs: Unbreak the reset handler

[Avri Altman]

> 
> 
> On 9/17/21 2:31 PM, Avri Altman wrote:
> >> -                       __ufshcd_transfer_req_compl(hba, pos,
> >> /*retry_requests=*/true);
> >> +                       __ufshcd_transfer_req_compl(hba, 1U << pos,
> >> + false);
> > 1) Maybe a word in the commit log about changing retry_requests from
> true to false.
> > 2) Also while at it, maybe change u32 pos to u8 tag?
> > 3) Add an unsigned long pending_reqs, add the tag inside the loop,
> >   And call __ufshcd_transfer_req_compl(hba, pending_reqs, false) one time
> outside the loop.
> 
> Hi Avri,
> 
> While working on this patch I realized that commit c11a1ae9b8f6
> ("scsi: ufs: Add fault injection support") is not necessary.
> ufshcd_prepare_req_desc_hdr() initializes the command status to
> OCS_INVALID_COMMAND_STATUS and ufshcd_transfer_rsp_status()
> translates that status code into DID_REQUEUE. I'm considering to revert
> commit c11a1ae9b8f6.
> 
> I prefer not to change "u32 pos" into "u8 tag", otherwise backporting this
> patch would become harder than necessary.
> 
> Is (3) important to you? I don't think that calls to
> ufshcd_eh_device_reset_handler() are that common so performance
> probably is not that important in this function.
OK.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.

Re: [PATCH v2] scsi: ufs: Unbreak the reset handler

[Martin K. Petersen]

On Thu, 16 Sep 2021 10:54:04 -0700, Bart Van Assche wrote:

> A command tag is passed as the second argument of the
> __ufshcd_transfer_req_compl() call in ufshcd_eh_device_reset_handler()
> instead of a bitmask. Fix this by passing a bitmask as argument instead
> of a command tag.
> 
> 

Applied to 5.15/scsi-fixes, thanks!

[1/1] scsi: ufs: Unbreak the reset handler
      https://git.kernel.org/mkp/scsi/c/d04a968c3368

-- 
Martin K. Petersen	Oracle Linux Engineering