diff mbox

[v3,05/15] scsi: ufs: increase fDeviceInit query response timeout

Message ID 1441188795-4600-6-git-send-email-ygardi@codeaurora.org (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show

Commit Message

Yaniv Gardi Sept. 2, 2015, 10:13 a.m. UTC 
fDeviceInit query response time for some devices is too long that default
query request timeout of 100ms may not be enough. Experiments show that
fDeviceInit response sometimes takes 500ms so to be on safer side this
change sets the timeout to 600ms. Without this change, we might
unnecessarily have to retry fDeviceInit query requests multiple times and
each query request timeout prints one error message.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Akinobu Mita Oct. 23, 2015, 11:18 a.m. UTC | #1 
2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
> fDeviceInit query response time for some devices is too long that default
> query request timeout of 100ms may not be enough. Experiments show that
> fDeviceInit response sometimes takes 500ms so to be on safer side this
> change sets the timeout to 600ms. Without this change, we might
> unnecessarily have to retry fDeviceInit query requests multiple times and
> each query request timeout prints one error message.
>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> ---
>  drivers/scsi/ufs/ufshcd.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e0b8755..573a8cb 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -58,6 +58,12 @@
>  #define QUERY_REQ_RETRIES 10
>  /* Query request timeout */
>  #define QUERY_REQ_TIMEOUT 30 /* msec */
> +/*
> + * Query request timeout for fDeviceInit flag
> + * fDeviceInit query response time for some devices is too large that default
> + * QUERY_REQ_TIMEOUT may not be enough for such devices.
> + */
> +#define QUERY_FDEVICEINIT_REQ_TIMEOUT 600 /* msec */

How about just increasing QUERY_REQ_TIMEOUT from 30ms to 600ms
instead of conditionally setting timeout depending on ufs flag?

>
>  /* Task management command timeout */
>  #define TM_CMD_TIMEOUT 100 /* msecs */
> @@ -1651,6 +1657,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
>         struct ufs_query_req *request = NULL;
>         struct ufs_query_res *response = NULL;
>         int err, index = 0, selector = 0;
> +       int timeout = QUERY_REQ_TIMEOUT;
>
>         BUG_ON(!hba);
>
> @@ -1683,7 +1690,10 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
>                 goto out_unlock;
>         }
>
> -       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, QUERY_REQ_TIMEOUT);
> +       if (idn == QUERY_FLAG_IDN_FDEVICEINIT)
> +               timeout = QUERY_FDEVICEINIT_REQ_TIMEOUT;
> +
> +       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, timeout);
>
>         if (err) {
>                 dev_err(hba->dev,
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yaniv Gardi Oct. 25, 2015, 1:38 p.m. UTC | #2 
> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>> fDeviceInit query response time for some devices is too long that
>> default
>> query request timeout of 100ms may not be enough. Experiments show that
>> fDeviceInit response sometimes takes 500ms so to be on safer side this
>> change sets the timeout to 600ms. Without this change, we might
>> unnecessarily have to retry fDeviceInit query requests multiple times
>> and
>> each query request timeout prints one error message.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index e0b8755..573a8cb 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -58,6 +58,12 @@
>>  #define QUERY_REQ_RETRIES 10
>>  /* Query request timeout */
>>  #define QUERY_REQ_TIMEOUT 30 /* msec */
>> +/*
>> + * Query request timeout for fDeviceInit flag
>> + * fDeviceInit query response time for some devices is too large that
>> default
>> + * QUERY_REQ_TIMEOUT may not be enough for such devices.
>> + */
>> +#define QUERY_FDEVICEINIT_REQ_TIMEOUT 600 /* msec */
>
> How about just increasing QUERY_REQ_TIMEOUT from 30ms to 600ms
> instead of conditionally setting timeout depending on ufs flag?

Your suggestion obviously could work (increasing the QUERY_REQ_TIMEOUT to
600ms), but in that case we bring extra delay of 570ms to error handling
of query timeout, and in such a case, instead of handling the error after
30ms we handle it after 600ms, which make the SW hang.
does it make sense ?


>
>>
>>  /* Task management command timeout */
>>  #define TM_CMD_TIMEOUT 100 /* msecs */
>> @@ -1651,6 +1657,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba,
>> enum query_opcode opcode,
>>         struct ufs_query_req *request = NULL;
>>         struct ufs_query_res *response = NULL;
>>         int err, index = 0, selector = 0;
>> +       int timeout = QUERY_REQ_TIMEOUT;
>>
>>         BUG_ON(!hba);
>>
>> @@ -1683,7 +1690,10 @@ static int ufshcd_query_flag(struct ufs_hba *hba,
>> enum query_opcode opcode,
>>                 goto out_unlock;
>>         }
>>
>> -       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY,
>> QUERY_REQ_TIMEOUT);
>> +       if (idn == QUERY_FLAG_IDN_FDEVICEINIT)
>> +               timeout = QUERY_FDEVICEINIT_REQ_TIMEOUT;
>> +
>> +       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, timeout);
>>
>>         if (err) {
>>                 dev_err(hba->dev,
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of Code Aurora Forum, hosted by The Linux Foundation
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Akinobu Mita Oct. 26, 2015, 12:59 p.m. UTC | #3 
2015-10-25 22:38 GMT+09:00  <ygardi@codeaurora.org>:
>> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>:
>>> fDeviceInit query response time for some devices is too long that
>>> default
>>> query request timeout of 100ms may not be enough. Experiments show that
>>> fDeviceInit response sometimes takes 500ms so to be on safer side this
>>> change sets the timeout to 600ms. Without this change, we might
>>> unnecessarily have to retry fDeviceInit query requests multiple times
>>> and
>>> each query request timeout prints one error message.
>>>
>>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>>
>>> ---
>>>  drivers/scsi/ufs/ufshcd.c | 12 +++++++++++-
>>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index e0b8755..573a8cb 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -58,6 +58,12 @@
>>>  #define QUERY_REQ_RETRIES 10
>>>  /* Query request timeout */
>>>  #define QUERY_REQ_TIMEOUT 30 /* msec */
>>> +/*
>>> + * Query request timeout for fDeviceInit flag
>>> + * fDeviceInit query response time for some devices is too large that
>>> default
>>> + * QUERY_REQ_TIMEOUT may not be enough for such devices.
>>> + */
>>> +#define QUERY_FDEVICEINIT_REQ_TIMEOUT 600 /* msec */
>>
>> How about just increasing QUERY_REQ_TIMEOUT from 30ms to 600ms
>> instead of conditionally setting timeout depending on ufs flag?
>
> Your suggestion obviously could work (increasing the QUERY_REQ_TIMEOUT to
> 600ms), but in that case we bring extra delay of 570ms to error handling
> of query timeout, and in such a case, instead of handling the error after
> 30ms we handle it after 600ms, which make the SW hang.
> does it make sense ?

Compared to default scsi disk timeout (30s), 600ms does not look very
long.  I was just worried that the code gets complicated if we need to
add yet another QUERY_XYZ_REQ_TIMEOUT macros when it turns out that
30ms timeout is not enough for other query requests under specific
conditions.  But I don't too much care about it for now.

>>
>>>
>>>  /* Task management command timeout */
>>>  #define TM_CMD_TIMEOUT 100 /* msecs */
>>> @@ -1651,6 +1657,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba,
>>> enum query_opcode opcode,
>>>         struct ufs_query_req *request = NULL;
>>>         struct ufs_query_res *response = NULL;
>>>         int err, index = 0, selector = 0;
>>> +       int timeout = QUERY_REQ_TIMEOUT;
>>>
>>>         BUG_ON(!hba);
>>>
>>> @@ -1683,7 +1690,10 @@ static int ufshcd_query_flag(struct ufs_hba *hba,
>>> enum query_opcode opcode,
>>>                 goto out_unlock;
>>>         }
>>>
>>> -       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY,
>>> QUERY_REQ_TIMEOUT);
>>> +       if (idn == QUERY_FLAG_IDN_FDEVICEINIT)
>>> +               timeout = QUERY_FDEVICEINIT_REQ_TIMEOUT;
>>> +
>>> +       err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, timeout);
>>>
>>>         if (err) {
>>>                 dev_err(hba->dev,
>>> --
>>> 1.8.5.2
>>>
>>> --
>>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>>> member of Code Aurora Forum, hosted by The Linux Foundation
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e0b8755..573a8cb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -58,6 +58,12 @@ 
 #define QUERY_REQ_RETRIES 10
 /* Query request timeout */
 #define QUERY_REQ_TIMEOUT 30 /* msec */
+/*
+ * Query request timeout for fDeviceInit flag
+ * fDeviceInit query response time for some devices is too large that default
+ * QUERY_REQ_TIMEOUT may not be enough for such devices.
+ */
+#define QUERY_FDEVICEINIT_REQ_TIMEOUT 600 /* msec */
 
 /* Task management command timeout */
 #define TM_CMD_TIMEOUT	100 /* msecs */
@@ -1651,6 +1657,7 @@  static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
 	struct ufs_query_req *request = NULL;
 	struct ufs_query_res *response = NULL;
 	int err, index = 0, selector = 0;
+	int timeout = QUERY_REQ_TIMEOUT;
 
 	BUG_ON(!hba);
 
@@ -1683,7 +1690,10 @@  static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
 		goto out_unlock;
 	}
 
-	err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, QUERY_REQ_TIMEOUT);
+	if (idn == QUERY_FLAG_IDN_FDEVICEINIT)
+		timeout = QUERY_FDEVICEINIT_REQ_TIMEOUT;
+
+	err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, timeout);
 
 	if (err) {
 		dev_err(hba->dev,