diff mbox series

[v3,1/1] scsi: ufs: core: Support Updating UIC Command Timeout

Message ID 6513429b6d3b10829263bf33ace5c5128f106e59.1720503791.git.quic_nguyenb@quicinc.com
State Superseded
Headers show
Series [v3,1/1] scsi: ufs: core: Support Updating UIC Command Timeout | expand

Commit Message

Bao D. Nguyen July 9, 2024, 6:06 a.m. UTC 
The default UIC command timeout still remains 500ms.
Allow platform drivers to override the UIC command
timeout if desired.

In a real product, the 500ms timeout value is probably good enough.
However, during the product development where there are a lot of
logging and debug messages being printed to the uart console,
interrupt starvations happen occasionally because the uart may
print long debug messages from different modules in the system.
While printing, the uart may have interrupts disabled for more
than 500ms, causing UIC command timeout.
The UIC command timeout would trigger more printing from
the UFS driver, and eventually a watchdog timeout may
occur unnecessarily.

Add support for overriding the UIC command timeout value
with the newly created uic_cmd_timeout kernel module parameter.
Default value is 500ms. Supported values range from 500ms
to 2 seconds.

Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

Comments

Bao D. Nguyen July 11, 2024, 5:46 a.m. UTC | #1 
On 7/9/2024 11:10 AM, Bart Van Assche wrote:
> On 7/8/24 11:06 PM, Bao D. Nguyen wrote:
>> +static int uic_cmd_timeout_set(const char *val, const struct 
>> kernel_param *kp)
>> +{
>> +    unsigned int n;
>> +    int ret;
>> +
>> +    ret = kstrtou32(val, 0, &n);
>> +    if (ret != 0 || n < UIC_CMD_TIMEOUT_DEFAULT || n > 
>> UIC_CMD_TIMEOUT_MAX)
>> +        return -EINVAL;
>> +
>> +    return param_set_int(val, kp);
>> +}
> 
> The above code converts 'val' twice to an integer: a first time by
> calling kstrtou32() and a second time by calling param_set_int().
> Please remove one of the two string-to-integer conversions, e.g. by
> changing "param_set_int(val, kp)" into "uic_cmd_timeout = n" or
> *(unsigned int *)kp->arg = n".
Hi Bart,
My understanding is that in the kstrtou32() function, the  the result of 
the conversion is written to the third parameter only which is '&n' in 
this case. 'val' does not get updated (and it cannot be updated because 
of its 'const' type). Please correct me if I am wrong.

Thanks, Bao

> 
> Thanks,
> 
> Bart.
Bao D. Nguyen July 12, 2024, 1:20 a.m. UTC | #2 
On 7/10/2024 10:46 PM, Bao D. Nguyen wrote:
> On 7/9/2024 11:10 AM, Bart Van Assche wrote:
>> On 7/8/24 11:06 PM, Bao D. Nguyen wrote:
>>> +static int uic_cmd_timeout_set(const char *val, const struct 
>>> kernel_param *kp)
>>> +{
>>> +    unsigned int n;
>>> +    int ret;
>>> +
>>> +    ret = kstrtou32(val, 0, &n);
>>> +    if (ret != 0 || n < UIC_CMD_TIMEOUT_DEFAULT || n > 
>>> UIC_CMD_TIMEOUT_MAX)
>>> +        return -EINVAL;
>>> +
>>> +    return param_set_int(val, kp);
>>> +}
>>
>> The above code converts 'val' twice to an integer: a first time by
>> calling kstrtou32() and a second time by calling param_set_int().
>> Please remove one of the two string-to-integer conversions, e.g. by
>> changing "param_set_int(val, kp)" into "uic_cmd_timeout = n" or
>> *(unsigned int *)kp->arg = n".
> Hi Bart,
> My understanding is that in the kstrtou32() function, the  the result of 
> the conversion is written to the third parameter only which is '&n' in 
> this case. 'val' does not get updated (and it cannot be updated because 
> of its 'const' type). Please correct me if I am wrong.
Hi Bart,
Maybe I misunderstood your comment. You probably was concerned about the 
execution time of param_set_int(val, kp) vs a single assignment 
instruction 'uic_cmd_timeout = n;', right? If that's the case, I'll 
update the patch.

Thanks, Bao

> 
> Thanks, Bao
> 
>>
>> Thanks,
>>
>> Bart.
>
Bart Van Assche July 12, 2024, 5:51 p.m. UTC | #3 
On 7/11/24 6:20 PM, Bao D. Nguyen wrote:
> Maybe I misunderstood your comment. You probably was concerned about the 
> execution time of param_set_int(val, kp) vs a single assignment 
> instruction 'uic_cmd_timeout = n;', right? If that's the case, I'll 
> update the patch.

I'm concerned about the two conversions yielding different results, e.g.
because of different handling of base prefixes (e.g. 0x) or different
handling of overflows. This patch would be easier to review if the
conversion from string to int only happens once.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 21429ee..421e295 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -51,8 +51,10 @@ 
 
 
 /* UIC command timeout, unit: ms */
-#define UIC_CMD_TIMEOUT	500
-
+enum {
+	UIC_CMD_TIMEOUT_DEFAULT	= 500,
+	UIC_CMD_TIMEOUT_MAX	= 2000,
+};
 /* NOP OUT retries waiting for NOP IN response */
 #define NOP_OUT_RETRIES    10
 /* Timeout after 50 msecs if NOP OUT hangs without response */
@@ -113,6 +115,28 @@  static bool is_mcq_supported(struct ufs_hba *hba)
 module_param(use_mcq_mode, bool, 0644);
 MODULE_PARM_DESC(use_mcq_mode, "Control MCQ mode for controllers starting from UFSHCI 4.0. 1 - enable MCQ, 0 - disable MCQ. MCQ is enabled by default");
 
+static int uic_cmd_timeout_set(const char *val, const struct kernel_param *kp)
+{
+	unsigned int n;
+	int ret;
+
+	ret = kstrtou32(val, 0, &n);
+	if (ret != 0 || n < UIC_CMD_TIMEOUT_DEFAULT || n > UIC_CMD_TIMEOUT_MAX)
+		return -EINVAL;
+
+	return param_set_int(val, kp);
+}
+
+static const struct kernel_param_ops uic_cmd_timeout_ops = {
+	.set = uic_cmd_timeout_set,
+	.get = param_get_uint,
+};
+
+static unsigned int uic_cmd_timeout = UIC_CMD_TIMEOUT_DEFAULT;
+module_param_cb(uic_cmd_timeout, &uic_cmd_timeout_ops, &uic_cmd_timeout, 0644);
+MODULE_PARM_DESC(uic_cmd_timeout,
+		"UFS UIC command timeout in milliseconds. Defaults to 500ms. Supported values range from 500ms to 2 seconds inclusively");
+
 #define ufshcd_toggle_vreg(_dev, _vreg, _on)				\
 	({                                                              \
 		int _ret;                                               \
@@ -2460,7 +2484,7 @@  static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba)
 {
 	u32 val;
 	int ret = read_poll_timeout(ufshcd_readl, val, val & UIC_COMMAND_READY,
-				    500, UIC_CMD_TIMEOUT * 1000, false, hba,
+				    500, uic_cmd_timeout * 1000, false, hba,
 				    REG_CONTROLLER_STATUS);
 	return ret == 0;
 }
@@ -2520,7 +2544,7 @@  ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	lockdep_assert_held(&hba->uic_cmd_mutex);
 
 	if (wait_for_completion_timeout(&uic_cmd->done,
-					msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
+					msecs_to_jiffies(uic_cmd_timeout))) {
 		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
 	} else {
 		ret = -ETIMEDOUT;
@@ -4298,7 +4322,7 @@  static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 	}
 
 	if (!wait_for_completion_timeout(hba->uic_async_done,
-					 msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
+					 msecs_to_jiffies(uic_cmd_timeout))) {
 		dev_err(hba->dev,
 			"pwr ctrl cmd 0x%x with mode 0x%x completion timeout\n",
 			cmd->command, cmd->argument3);