* Re: [PATCH v4 1/1] scsi: ufs: core: Support Updating UIC Command Timeout
2024-07-18 0:17 ` [PATCH v4 1/1] scsi: ufs: core: Support Updating UIC Command Timeout Bao D. Nguyen
@ 2024-07-21 8:30 ` Manivannan Sadhasivam
2024-07-23 7:27 ` Peter Wang (王信友)
1 sibling, 0 replies; 4+ messages in thread
From: Manivannan Sadhasivam @ 2024-07-21 8:30 UTC (permalink / raw)
To: Bao D. Nguyen
Cc: quic_cang, quic_nitirawa, bvanassche, avri.altman, peter.wang,
minwoo.im, adrian.hunter, martin.petersen, linux-scsi,
Alim Akhtar, James E.J. Bottomley, Bean Huo, Maramaina Naresh,
open list
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
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v4 1/1] scsi: ufs: core: Support Updating UIC Command Timeout
2024-07-18 0:17 ` [PATCH v4 1/1] scsi: ufs: core: Support Updating UIC Command Timeout Bao D. Nguyen
2024-07-21 8:30 ` Manivannan Sadhasivam
@ 2024-07-23 7:27 ` Peter Wang (王信友)
2024-07-23 19:27 ` Bao D. Nguyen
1 sibling, 1 reply; 4+ messages in thread
From: Peter Wang (王信友) @ 2024-07-23 7:27 UTC (permalink / raw)
To: quic_nguyenb@quicinc.com, quic_nitirawa@quicinc.com,
avri.altman@wdc.com, bvanassche@acm.org,
martin.petersen@oracle.com, manivannan.sadhasivam@linaro.org,
minwoo.im@samsung.com, quic_cang@quicinc.com,
adrian.hunter@intel.com
Cc: linux-scsi@vger.kernel.org, alim.akhtar@samsung.com,
jejb@linux.ibm.com, beanhuo@micron.com,
linux-kernel@vger.kernel.org, quic_mnaresh@quicinc.com
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread