[PATCH] scsi: ufs: Reduce the power mode change timeout

[PATCH] scsi: ufs: Reduce the power mode change timeout

[Bart Van Assche]

The current power mode change timeout (180 s) is so large that it can
cause a watchdog timer to fire. Reduce the power mode change timeout to
10 seconds.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index e32b6b834b7f..2dd355a5da54 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8776,6 +8776,8 @@ 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;
@@ -8808,9 +8810,14 @@ 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,
-				START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL);
+				   remaining / HZ, 0, 0, RQF_PM, NULL);
 		if (!scsi_status_is_check_condition(ret) ||
 				!scsi_sense_valid(&sshdr) ||
 				sshdr.sense_key != UNIT_ATTENTION)

Re: [PATCH] scsi: ufs: Reduce the power mode change timeout

[Stanley Chu]

On Fri, Aug 12, 2022 at 7:52 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> The current power mode change timeout (180 s) is so large that it can
> cause a watchdog timer to fire. Reduce the power mode change timeout to
> 10 seconds.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

RE: [PATCH] scsi: ufs: Reduce the power mode change timeout

[Avri Altman]

> The current power mode change timeout (180 s) is so large that it can
> cause a watchdog timer to fire. Reduce the power mode change timeout to
> 10 seconds.
Maybe also worth noting that it was invented 20 years ago for scsi ioctl,
And is less applicable for nowadays ufs devices

> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index e32b6b834b7f..2dd355a5da54 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8776,6 +8776,8 @@ 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;
Maybe use ktime api, like its done throughout the driver?

Thanks,
Avri

> 
>         spin_lock_irqsave(hba->host->host_lock, flags);
>         sdp = hba->ufs_device_wlun;
> @@ -8808,9 +8810,14 @@ 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,
> -                               START_STOP_TIMEOUT, 0, 0, RQF_PM, NULL);
> +                                  remaining / HZ, 0, 0, RQF_PM, NULL);
>                 if (!scsi_status_is_check_condition(ret) ||
>                                 !scsi_sense_valid(&sshdr) ||
>                                 sshdr.sense_key != UNIT_ATTENTION)

Re: [PATCH] scsi: ufs: Reduce the power mode change timeout

[Bart Van Assche]

On 8/14/22 00:45, Avri Altman wrote:
>> The current power mode change timeout (180 s) is so large that it can
>> cause a watchdog timer to fire. Reduce the power mode change timeout to
>> 10 seconds.
> Maybe also worth noting that it was invented 20 years ago for scsi ioctl,
> And is less applicable for nowadays ufs devices
> 
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/ufs/core/ufshcd.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index e32b6b834b7f..2dd355a5da54 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -8776,6 +8776,8 @@ 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;
 >
> Maybe use ktime api, like its done throughout the driver?

Hi Avri,

Calling ktime_get() is much more expensive than reading the jiffies 
counter. That's why I prefer to use the jiffies counter if the timeout 
is significantly larger than 1 / HZ and if the accuracy of 1 / HZ is 
sufficient.

Thanks,

Bart.