[v3,4/9] scsi: ufs: Complete the cmd before returning in queuecommand

Message ID 1623300218-9454-5-git-send-email-cang@codeaurora.org
State Superseded
Headers show
Series
  • [v3,1/9] scsi: ufs: Differentiate status between hba pm ops and wl pm ops
Related show

Commit Message

Can Guo June 10, 2021, 4:43 a.m.
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 <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Comments

Bart Van Assche June 11, 2021, 8:52 p.m. | #1
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.
Can Guo June 12, 2021, 7:38 a.m. | #2
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.
Bart Van Assche June 12, 2021, 3:50 p.m. | #3
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.
Can Guo June 13, 2021, 1:30 p.m. | #4
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.

Patch

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;