Commit 7a7e66c65d4148fc3f23b058405bc9f102414fcb ("scsi: ufs: Fix a race
condition between ufshcd_abort() and eh_work()") forgot to complete the
cmd, which takes an occupied lrb, before returning in queuecommand. This
change adds the missing codes.
Fixes: 7a7e66c65d414 ("scsi: ufs: Fix a race condition between ufshcd_abort() and eh_work()")
Signed-off-by: Can Guo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0c9d2ee..7dc0fda 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2758,6 +2758,16 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
goto out;
}
+ if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
+ if (hba->wl_pm_op_in_progress) {
+ set_host_byte(cmd, DID_BAD_TARGET);
+ cmd->scsi_done(cmd);
+ } else {
+ err = SCSI_MLQUEUE_HOST_BUSY;
+ }
+ goto out;
+ }
+
hba->req_abort_count = 0;
err = ufshcd_hold(hba, true);
@@ -2768,15 +2778,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
WARN_ON(ufshcd_is_clkgating_allowed(hba) &&
(hba->clk_gating.state != CLKS_ON));
- if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
- if (hba->wl_pm_op_in_progress)
- set_host_byte(cmd, DID_BAD_TARGET);
- else
- err = SCSI_MLQUEUE_HOST_BUSY;
- ufshcd_release(hba);
- goto out;
- }
-
lrbp = &hba->lrb[tag];
WARN_ON(lrbp->cmd);
lrbp->cmd = cmd;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
On 6/9/21 9:43 PM, Can Guo wrote:
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0c9d2ee..7dc0fda 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2758,6 +2758,16 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
> goto out;
> }
>
> + if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
> + if (hba->wl_pm_op_in_progress) {
> + set_host_byte(cmd, DID_BAD_TARGET);
> + cmd->scsi_done(cmd);
> + } else {
> + err = SCSI_MLQUEUE_HOST_BUSY;
> + }
> + goto out;
> + }
> +
> hba->req_abort_count = 0;
>
> err = ufshcd_hold(hba, true);
> @@ -2768,15 +2778,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
> WARN_ON(ufshcd_is_clkgating_allowed(hba) &&
> (hba->clk_gating.state != CLKS_ON));
>
> - if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
> - if (hba->wl_pm_op_in_progress)
> - set_host_byte(cmd, DID_BAD_TARGET);
> - else
> - err = SCSI_MLQUEUE_HOST_BUSY;
> - ufshcd_release(hba);
> - goto out;
> - }
> -
> lrbp = &hba->lrb[tag];
> WARN_ON(lrbp->cmd);
> lrbp->cmd = cmd;
Can the code under "if (unlikely(test_bit(tag,
&hba->outstanding_reqs)))" be deleted instead of moving it? I don't
think that it is useful to verify whether the block layer tag allocator
works correctly. Additionally, I'm not aware of any similar code in any
other SCSI LLD.
Thanks,
Bart.
On 2021-06-12 04:52, Bart Van Assche wrote:
> On 6/9/21 9:43 PM, Can Guo wrote:
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 0c9d2ee..7dc0fda 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -2758,6 +2758,16 @@ static int ufshcd_queuecommand(struct Scsi_Host
>> *host, struct scsi_cmnd *cmd)
>> goto out;
>> }
>>
>> + if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
>> + if (hba->wl_pm_op_in_progress) {
>> + set_host_byte(cmd, DID_BAD_TARGET);
>> + cmd->scsi_done(cmd);
>> + } else {
>> + err = SCSI_MLQUEUE_HOST_BUSY;
>> + }
>> + goto out;
>> + }
>> +
>> hba->req_abort_count = 0;
>>
>> err = ufshcd_hold(hba, true);
>> @@ -2768,15 +2778,6 @@ static int ufshcd_queuecommand(struct Scsi_Host
>> *host, struct scsi_cmnd *cmd)
>> WARN_ON(ufshcd_is_clkgating_allowed(hba) &&
>> (hba->clk_gating.state != CLKS_ON));
>>
>> - if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
>> - if (hba->wl_pm_op_in_progress)
>> - set_host_byte(cmd, DID_BAD_TARGET);
>> - else
>> - err = SCSI_MLQUEUE_HOST_BUSY;
>> - ufshcd_release(hba);
>> - goto out;
>> - }
>> -
>> lrbp = &hba->lrb[tag];
>> WARN_ON(lrbp->cmd);
>> lrbp->cmd = cmd;
>
> Can the code under "if (unlikely(test_bit(tag,
> &hba->outstanding_reqs)))" be deleted instead of moving it? I don't
> think that it is useful to verify whether the block layer tag allocator
> works correctly. Additionally, I'm not aware of any similar code in any
> other SCSI LLD.
>
ufshcd_abort() aborts PM requests differently from other requests -
it simply evicts the cmd from lrbp [1], schedules error handler and
returns SUCCESS (the reason why I am doing it this way is in patch #8).
After ufshcd_abort() returns, the tag shall be released, the logic
here is to prevent subsequent cmds re-use the lrbp [1] before error
handler recovers the device and host.
Thanks,
Can Guo.
> Thanks,
>
> Bart.
On 6/12/21 12:38 AM, Can Guo wrote:
> On 2021-06-12 04:52, Bart Van Assche wrote:
>> On 6/9/21 9:43 PM, Can Guo wrote:
>>> @@ -2768,15 +2778,6 @@ static int ufshcd_queuecommand(struct
>>> Scsi_Host *host, struct scsi_cmnd *cmd)
>>> WARN_ON(ufshcd_is_clkgating_allowed(hba) &&
>>> (hba->clk_gating.state != CLKS_ON));
>>>
>>> - if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
>>> - if (hba->wl_pm_op_in_progress)
>>> - set_host_byte(cmd, DID_BAD_TARGET);
>>> - else
>>> - err = SCSI_MLQUEUE_HOST_BUSY;
>>> - ufshcd_release(hba);
>>> - goto out;
>>> - }
>>> -
>>> lrbp = &hba->lrb[tag];
>>> WARN_ON(lrbp->cmd);
>>> lrbp->cmd = cmd;
>>
>> Can the code under "if (unlikely(test_bit(tag,
>> &hba->outstanding_reqs)))" be deleted instead of moving it? I don't
>> think that it is useful to verify whether the block layer tag allocator
>> works correctly. Additionally, I'm not aware of any similar code in any
>> other SCSI LLD.
>
> ufshcd_abort() aborts PM requests differently from other requests -
> it simply evicts the cmd from lrbp [1], schedules error handler and
> returns SUCCESS (the reason why I am doing it this way is in patch #8).
>
> After ufshcd_abort() returns, the tag shall be released, the logic
> here is to prevent subsequent cmds re-use the lrbp [1] before error
> handler recovers the device and host.
Thanks for the background information. However, this approach sounds
cumbersome to me. For PM requests, please change the UFS driver such
that calling scsi_done() for aborted requests is postponed until error
handling has finished and delete the code shown above from
ufshcd_queuecommand().
Thanks,
Bart.
On 2021-06-12 23:50, Bart Van Assche wrote:
> On 6/12/21 12:38 AM, Can Guo wrote:
>> On 2021-06-12 04:52, Bart Van Assche wrote:
>>> On 6/9/21 9:43 PM, Can Guo wrote:
>>>> @@ -2768,15 +2778,6 @@ static int ufshcd_queuecommand(struct
>>>> Scsi_Host *host, struct scsi_cmnd *cmd)
>>>> WARN_ON(ufshcd_is_clkgating_allowed(hba) &&
>>>> (hba->clk_gating.state != CLKS_ON));
>>>>
>>>> - if (unlikely(test_bit(tag, &hba->outstanding_reqs))) {
>>>> - if (hba->wl_pm_op_in_progress)
>>>> - set_host_byte(cmd, DID_BAD_TARGET);
>>>> - else
>>>> - err = SCSI_MLQUEUE_HOST_BUSY;
>>>> - ufshcd_release(hba);
>>>> - goto out;
>>>> - }
>>>> -
>>>> lrbp = &hba->lrb[tag];
>>>> WARN_ON(lrbp->cmd);
>>>> lrbp->cmd = cmd;
>>>
>>> Can the code under "if (unlikely(test_bit(tag,
>>> &hba->outstanding_reqs)))" be deleted instead of moving it? I don't
>>> think that it is useful to verify whether the block layer tag
>>> allocator
>>> works correctly. Additionally, I'm not aware of any similar code in
>>> any
>>> other SCSI LLD.
>>
>> ufshcd_abort() aborts PM requests differently from other requests -
>> it simply evicts the cmd from lrbp [1], schedules error handler and
>> returns SUCCESS (the reason why I am doing it this way is in patch
>> #8).
>>
>> After ufshcd_abort() returns, the tag shall be released, the logic
>> here is to prevent subsequent cmds re-use the lrbp [1] before error
>> handler recovers the device and host.
>
> Thanks for the background information. However, this approach sounds
> cumbersome to me. For PM requests, please change the UFS driver such
> that calling scsi_done() for aborted requests is postponed until error
> handling has finished and delete the code shown above from
> ufshcd_queuecommand().
I will delete the code in next version, since I believe the hba_state
checks before the code is enough to achieve the same purpose, so this
code becomes redundant.
Thanks,
Can Guo.
>
> Thanks,
>
> Bart.