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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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
> 

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