[PATCH v4 0/1] Allow platform drivers to update UIC command timeout

[PATCH v4 0/1] Allow platform drivers to update UIC command timeout

[Bao D. Nguyen]

The UIC command timeout default value remains as 500ms.

Allow platform drivers to change the UIC command timeout as they wish.
During product development where a lot of debug logging/printing can
occur, the uart may print from different modules with interrupt disabled
for more than 500ms, causing interrupt starvation and UIC command timeout
as a result. The UIC command timeout may eventually cause a watchdog
timeout unnecessarily. With this change, the platform drivers can set a
different UIC command timeout as desired. The supported values range
from 500ms to 2 seconds.

v3 -> v4: - Addressed Bart's concern about the two string to int conversions
	    would yield different results.
v2 -> v3: - Addressed Bart's comments. Renamed UIC_CMD_TIMEOUT to
            UIC_CMD_TIMEOUT_DEFAULT. Changed "Default.." to "Defaults..".
            Removed an extra line in the module description.
v1 -> v2: - Created kernel module parameter namely uic_cmd_timeout
            as recommended by Bart. Addressed some other comments.
          - Un-do the change in the include/ufs/ufshcd.h file
            which added the uic_cmd_timeout field to the hba struct.
	  - Removed the patch 2 in the series where the UIC command
	    timeout value was overridden by the platform driver.

Bao D. Nguyen (1):
  scsi: ufs: core: Support Updating UIC Command Timeout

 drivers/ufs/core/ufshcd.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH v4 1/1] scsi: ufs: core: Support Updating UIC Command Timeout

[Bao D. Nguyen]

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 | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 21429ee..d66da13 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,31 @@ 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 unsigned int uic_cmd_timeout = UIC_CMD_TIMEOUT_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;
+
+	uic_cmd_timeout = n;
+
+	return 0;
+}
+
+static const struct kernel_param_ops uic_cmd_timeout_ops = {
+	.set = uic_cmd_timeout_set,
+	.get = param_get_uint,
+};
+
+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 +2487,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 +2547,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 +4325,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);
-- 
2.7.4

Re: [PATCH v4 1/1] scsi: ufs: core: Support Updating UIC Command Timeout

[Manivannan Sadhasivam]

On Wed, Jul 17, 2024 at 05:17:34PM -0700, Bao D. Nguyen wrote:
> 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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Suggested-by: Bart Van Assche <bvanassche@acm.org>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 21429ee..d66da13 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,31 @@ 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 unsigned int uic_cmd_timeout = UIC_CMD_TIMEOUT_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;
> +
> +	uic_cmd_timeout = n;
> +
> +	return 0;
> +}
> +
> +static const struct kernel_param_ops uic_cmd_timeout_ops = {
> +	.set = uic_cmd_timeout_set,
> +	.get = param_get_uint,
> +};
> +
> +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 +2487,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 +2547,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 +4325,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);
> -- 
> 2.7.4
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 1/1] scsi: ufs: core: Support Updating UIC Command Timeout

[Peter Wang (王信友)]

On Wed, 2024-07-17 at 17:17 -0700, Bao D. Nguyen wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  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 | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 21429ee..d66da13 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,31 @@ 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 unsigned int uic_cmd_timeout = UIC_CMD_TIMEOUT_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);
> 
Hi Bao,

n type is unsigned int, so it should be kstrtouint? 
Although they should be the same.


> +	if (ret != 0 || n < UIC_CMD_TIMEOUT_DEFAULT || n >
> UIC_CMD_TIMEOUT_MAX)
> +		return -EINVAL;
> +
> +	uic_cmd_timeout = n;
> +
> +	return 0;
> 

Could be just use this line instead?
	return
param_set_uint_minmax(val, kp, UIC_CMD_TIMEOUT_DEFAULT,
		
		     UIC_CMD_TIMEOUT_MAX);

It should be more simple.

> +}

> +
> +static const struct kernel_param_ops uic_cmd_timeout_ops = {
> +	.set = uic_cmd_timeout_set,
> +	.get = param_get_uint,
> +};
> +
> +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 +2487,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 +2547,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_TIMEOU
> T))) {
> +					msecs_to_jiffies(uic_cmd_timeou
> t))) {
>  		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
>  	} else {
>  		ret = -ETIMEDOUT;
> @@ -4298,7 +4325,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_TIMEO
> UT))) {
> +					 msecs_to_jiffies(uic_cmd_timeo
> ut))) {
>  		dev_err(hba->dev,
>  			"pwr ctrl cmd 0x%x with mode 0x%x completion
> timeout\n",
>  			cmd->command, cmd->argument3);
> -- 
> 2.7.4
> 

Re: [PATCH v4 1/1] scsi: ufs: core: Support Updating UIC Command Timeout

[Bao D. Nguyen]

> 
> Could be just use this line instead?
> 	return
> param_set_uint_minmax(val, kp, UIC_CMD_TIMEOUT_DEFAULT,
> 		
> 		     UIC_CMD_TIMEOUT_MAX);
> 
> It should be more simple.
Thank you Peter. Yes it would be cleaner. I will update the patch.

Thanks, Bao