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@xxxxxxxxxxx>
> Suggested-by: Bart Van Assche <bvanassche@xxxxxxx>
> Reviewed-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
>  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
>