[PATCH] scsi: ufs: Increase the START STOP UNIT timeout from 1 s to 10 s

[PATCH] scsi: ufs: Increase the START STOP UNIT timeout from 1 s to 10 s

[Bart Van Assche]

One UFS vendor asked to increase the UFS timeout from 1 s to 3 s.
Another UFS vendor asked to increase the UFS timeout from 1 s to 10 s.
Hence this patch that increases the UFS timeout to 10 s. This patch can
cause the total timeout to exceed 20 s, the Android shutdown timeout.
This is fine since the loop around ufshcd_execute_start_stop() exists to
deal with unit attentions and because unit attentions are reported
quickly.

Fixes: dcd5b7637c6d ("scsi: ufs: Reduce the START STOP UNIT timeout")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 03c47f9a2750..8363a1667feb 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9181,7 +9181,8 @@ static int ufshcd_execute_start_stop(struct scsi_device *sdev,
 	};
 
 	return scsi_execute_cmd(sdev, cdb, REQ_OP_DRV_IN, /*buffer=*/NULL,
-			/*bufflen=*/0, /*timeout=*/HZ, /*retries=*/0, &args);
+			/*bufflen=*/0, /*timeout=*/10 * HZ, /*retries=*/0,
+			&args);
 }
 
 /**

Re: [PATCH] scsi: ufs: Increase the START STOP UNIT timeout from 1 s to 10 s

[Adrian Hunter]

On 11/04/23 03:11, Bart Van Assche wrote:
> One UFS vendor asked to increase the UFS timeout from 1 s to 3 s.
> Another UFS vendor asked to increase the UFS timeout from 1 s to 10 s.
> Hence this patch that increases the UFS timeout to 10 s. This patch can
> cause the total timeout to exceed 20 s, the Android shutdown timeout.
> This is fine since the loop around ufshcd_execute_start_stop() exists to
> deal with unit attentions and because unit attentions are reported
> quickly.
> 
> Fixes: dcd5b7637c6d ("scsi: ufs: Reduce the START STOP UNIT timeout")

Did that commit (shown below) actually increase the timeout
because the previous commit (8f2c96420c6e) had put
"remaining / HZ" when it should have been just "remaining"?
Or am I misreading?

So maybe it also needs a fixes tag for 8f2c96420c6e.

commit dcd5b7637c6d442d957f73780a03047413ed3a10
Author: Bart Van Assche <bvanassche@acm.org>
Date:   Tue Oct 18 13:29:54 2022 -0700

    scsi: ufs: Reduce the START STOP UNIT timeout
    
    Reduce the START STOP UNIT command timeout to one second since on Android
    devices a kernel panic is triggered if an attempt to suspend the system
    takes more than 20 seconds. One second should be enough for the START STOP
    UNIT command since this command completes in less than a millisecond for
    the UFS devices I have access to.
    
    Signed-off-by: Bart Van Assche <bvanassche@acm.org>
    Link: https://lore.kernel.org/r/20221018202958.1902564-7-bvanassche@acm.org
    Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index db1997e99da2..f83a0045a129 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8746,8 +8746,6 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	struct scsi_device *sdp;
 	unsigned long flags;
 	int ret, retries;
-	unsigned long deadline;
-	int32_t remaining;
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	sdp = hba->ufs_device_wlun;
@@ -8775,14 +8773,9 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 	 * callbacks hence set the RQF_PM flag so that it doesn't resume the
 	 * already suspended childs.
 	 */
-	deadline = jiffies + 10 * HZ;
 	for (retries = 3; retries > 0; --retries) {
-		ret = -ETIMEDOUT;
-		remaining = deadline - jiffies;
-		if (remaining <= 0)
-			break;
 		ret = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
-				   remaining / HZ, 0, 0, RQF_PM, NULL);
+				   HZ, 0, 0, RQF_PM, NULL);
 		if (!scsi_status_is_check_condition(ret) ||
 				!scsi_sense_valid(&sshdr) ||
 				sshdr.sense_key != UNIT_ATTENTION)



> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 03c47f9a2750..8363a1667feb 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9181,7 +9181,8 @@ static int ufshcd_execute_start_stop(struct scsi_device *sdev,
>  	};
>  
>  	return scsi_execute_cmd(sdev, cdb, REQ_OP_DRV_IN, /*buffer=*/NULL,
> -			/*bufflen=*/0, /*timeout=*/HZ, /*retries=*/0, &args);
> +			/*bufflen=*/0, /*timeout=*/10 * HZ, /*retries=*/0,
> +			&args);
>  }
>  
>  /**

Re: [PATCH] scsi: ufs: Increase the START STOP UNIT timeout from 1 s to 10 s

[Bart Van Assche]

On 4/11/23 01:39, Adrian Hunter wrote:
> On 11/04/23 03:11, Bart Van Assche wrote:
>> One UFS vendor asked to increase the UFS timeout from 1 s to 3 s.
>> Another UFS vendor asked to increase the UFS timeout from 1 s to 10 s.
>> Hence this patch that increases the UFS timeout to 10 s. This patch can
>> cause the total timeout to exceed 20 s, the Android shutdown timeout.
>> This is fine since the loop around ufshcd_execute_start_stop() exists to
>> deal with unit attentions and because unit attentions are reported
>> quickly.
>>
>> Fixes: dcd5b7637c6d ("scsi: ufs: Reduce the START STOP UNIT timeout")
> 
> Did that commit (shown below) actually increase the timeout
> because the previous commit (8f2c96420c6e) had put
> "remaining / HZ" when it should have been just "remaining"?
> Or am I misreading?
> 
> So maybe it also needs a fixes tag for 8f2c96420c6e.

Commit 8f2c96420c6e ("scsi: ufs: core: Reduce the power mode change 
timeout") changed the START STOP UNIT timeout from START_STOP_TIMEOUT 
into "remaining / HZ" (should have been "remaining") and hence passed a 
smaller value than intended to scsi_execute(). Commit dcd5b7637c6d 
changed the timeout from remaining / HZ into one second. Both values are 
too small. I'm not sure a second Fixes: tag would help since the above 
Fixes: tag should be sufficient to make this patch land in all relevant 
stable trees.

Thanks,

Bart.

Re: [PATCH] scsi: ufs: Increase the START STOP UNIT timeout from 1 s to 10 s

[Adrian Hunter]

On 11/04/23 20:31, Bart Van Assche wrote:
> On 4/11/23 01:39, Adrian Hunter wrote:
>> On 11/04/23 03:11, Bart Van Assche wrote:
>>> One UFS vendor asked to increase the UFS timeout from 1 s to 3 s.
>>> Another UFS vendor asked to increase the UFS timeout from 1 s to 10 s.
>>> Hence this patch that increases the UFS timeout to 10 s. This patch can
>>> cause the total timeout to exceed 20 s, the Android shutdown timeout.
>>> This is fine since the loop around ufshcd_execute_start_stop() exists to
>>> deal with unit attentions and because unit attentions are reported
>>> quickly.
>>>
>>> Fixes: dcd5b7637c6d ("scsi: ufs: Reduce the START STOP UNIT timeout")
>>
>> Did that commit (shown below) actually increase the timeout
>> because the previous commit (8f2c96420c6e) had put
>> "remaining / HZ" when it should have been just "remaining"?
>> Or am I misreading?
>>
>> So maybe it also needs a fixes tag for 8f2c96420c6e.
> 
> Commit 8f2c96420c6e ("scsi: ufs: core: Reduce the power mode change timeout") changed the START STOP UNIT timeout from START_STOP_TIMEOUT into "remaining / HZ" (should have been "remaining") and hence passed a smaller value than intended to scsi_execute(). Commit dcd5b7637c6d changed the timeout from remaining / HZ into one second. Both values are too small. I'm not sure a second Fixes: tag would help since the above Fixes: tag should be sufficient to make this patch land in all relevant stable trees.

It would be better not to assume current stable trees are the only consumers of fixes.  Presumably adding the extra Fixes tag does no harm.

Re: [PATCH] scsi: ufs: Increase the START STOP UNIT timeout from 1 s to 10 s

[Bart Van Assche]

On 4/11/23 11:31, Adrian Hunter wrote:
> It would be better not to assume current stable trees are the only
> consumers of fixes.  Presumably adding the extra Fixes tag does no
> harm.

Hi Adrian,

The convention is to add a reference to the most recent patch that got 
fixed in the patch description. Anyone who backports fixes is assumed to 
follow the chain of patches transitively that is created by Fixes: tags.

Thanks,

Bart.

Re: [PATCH] scsi: ufs: Increase the START STOP UNIT timeout from 1 s to 10 s

[Adrian Hunter]

On 12/04/23 19:34, Bart Van Assche wrote:
> On 4/11/23 11:31, Adrian Hunter wrote:
>> It would be better not to assume current stable trees are the only
>> consumers of fixes.  Presumably adding the extra Fixes tag does no
>> harm.
> 
> Hi Adrian,
> 
> The convention is to add a reference to the most recent patch that got fixed in the patch description. Anyone who backports fixes is assumed to follow the chain of patches transitively that is created by Fixes: tags.

Wouldn't that only work if commit dcd5b7637c6d had a Fixes tag too.

Re: [PATCH] scsi: ufs: Increase the START STOP UNIT timeout from 1 s to 10 s

[Bart Van Assche]

On 4/12/23 10:24, Adrian Hunter wrote:
> Wouldn't that only work if commit dcd5b7637c6d had a Fixes tag too.

Hi Adrian,

I will add a second Fixes tag and repost this patch.

Thanks,

Bart.