| Message ID | 44dc4790b53e2f8aa92568a9e13785e3bedd617d.1721261491.git.quic_nguyenb@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow platform drivers to update UIC command timeout | expand |
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 >
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 >
> > 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
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);