* [PATCH v2] scsi: ufs: Unbreak the reset handler
@ 2021-09-16 17:54 Bart Van Assche
2021-09-17 21:31 ` Avri Altman
2021-09-22 4:44 ` Martin K. Petersen
0 siblings, 2 replies; 5+ messages in thread
From: Bart Van Assche @ 2021-09-16 17:54 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Jaegeuk Kim, linux-scsi, Bart Van Assche, Can Guo,
James E.J. Bottomley, Bean Huo, Stanley Chu, Asutosh Das,
Adrian Hunter
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);
}
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH v2] scsi: ufs: Unbreak the reset handler
2021-09-16 17:54 [PATCH v2] scsi: ufs: Unbreak the reset handler Bart Van Assche
@ 2021-09-17 21:31 ` Avri Altman
2021-09-17 21:52 ` Bart Van Assche
2021-09-22 4:44 ` Martin K. Petersen
1 sibling, 1 reply; 5+ messages in thread
From: Avri Altman @ 2021-09-17 21:31 UTC (permalink / raw)
To: Bart Van Assche, Martin K . Petersen
Cc: Jaegeuk Kim, linux-scsi, Can Guo, James E.J. Bottomley, Bean Huo,
Stanley Chu, Asutosh Das, Adrian Hunter
>
> 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
> }
> }
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] scsi: ufs: Unbreak the reset handler
2021-09-17 21:31 ` Avri Altman
@ 2021-09-17 21:52 ` Bart Van Assche
2021-09-18 6:20 ` Avri Altman
0 siblings, 1 reply; 5+ messages in thread
From: Bart Van Assche @ 2021-09-17 21:52 UTC (permalink / raw)
To: Avri Altman, Martin K . Petersen
Cc: Jaegeuk Kim, linux-scsi, Can Guo, James E.J. Bottomley, Bean Huo,
Stanley Chu, Asutosh Das, Adrian Hunter
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v2] scsi: ufs: Unbreak the reset handler
2021-09-17 21:52 ` Bart Van Assche
@ 2021-09-18 6:20 ` Avri Altman
0 siblings, 0 replies; 5+ messages in thread
From: Avri Altman @ 2021-09-18 6:20 UTC (permalink / raw)
To: Bart Van Assche, Martin K . Petersen
Cc: Jaegeuk Kim, linux-scsi, Can Guo, James E.J. Bottomley, Bean Huo,
Stanley Chu, Asutosh Das, Adrian Hunter
>
>
> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] scsi: ufs: Unbreak the reset handler
2021-09-16 17:54 [PATCH v2] scsi: ufs: Unbreak the reset handler Bart Van Assche
2021-09-17 21:31 ` Avri Altman
@ 2021-09-22 4:44 ` Martin K. Petersen
1 sibling, 0 replies; 5+ messages in thread
From: Martin K. Petersen @ 2021-09-22 4:44 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, Adrian Hunter, James E.J. Bottomley,
Stanley Chu, Asutosh Das, Jaegeuk Kim, Bean Huo, linux-scsi,
Can Guo
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-22 4:45 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16 17:54 [PATCH v2] scsi: ufs: Unbreak the reset handler Bart Van Assche
2021-09-17 21:31 ` Avri Altman
2021-09-17 21:52 ` Bart Van Assche
2021-09-18 6:20 ` Avri Altman
2021-09-22 4:44 ` Martin K. Petersen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.