| Message ID | 1684208012-114324-1-git-send-email-kwmad.kim@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] ufs: poll HCS.UCRDY before issuing a UIC command | expand |
On 5/15/23 20:33, Kiwoong Kim wrote: > v1 -> v2: replace usleep_range with udelay > because it's a sleepable period. > > With auto hibern8 enabled, UIC could be working > for a while to process a hibern8 operation and HCI > reports UIC not ready for a short term through HCS.UCRDY. > And UFS driver can't recognize the operation. > UFSHCI spec specifies UCRDY like this: > whether the host controller is ready to process UIC COMMAND > > The 'ready' could be seen as many different meanings. If the meaning > includes not processing any request from HCI, processing a hibern8 > operation can be 'not ready'. In this situation, the driver needs to > wait until the operations is completed. > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > --- > drivers/ufs/core/ufshcd.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 9434328..5f6819a 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -2365,7 +2365,18 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba) > */ > static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba) > { > - return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY; > + ktime_t timeout = ktime_add_ms(ktime_get(), UIC_CMD_TIMEOUT); > + u32 val = 0; > + > + do { > + val = ufshcd_readl(hba, REG_CONTROLLER_STATUS) & > + UIC_COMMAND_READY; > + if (val) > + break; > + udelay(500); > + } while (ktime_before(ktime_get(), timeout)); > + > + return val ? true : false; > } Sleeping during up to 500 ms while holding a spin lock is not acceptable. Has it been considered to modify the UFS core such that the host_lock is not held around calls of the above function, e.g. via the (untested) patch below? Thanks, Bart. diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 9736b2b4120e..394283b04d7c 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -2416,7 +2416,6 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd, bool completion) { lockdep_assert_held(&hba->uic_cmd_mutex); - lockdep_assert_held(hba->host->host_lock); if (!ufshcd_ready_for_uic_cmd(hba)) { dev_err(hba->dev, @@ -2452,9 +2451,7 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd) mutex_lock(&hba->uic_cmd_mutex); ufshcd_add_delay_before_dme_cmd(hba); - spin_lock_irqsave(hba->host->host_lock, flags); ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true); - spin_unlock_irqrestore(hba->host->host_lock, flags); if (!ret) ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd); @@ -4122,8 +4119,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd) wmb(); reenable_intr = true; } - ret = __ufshcd_send_uic_cmd(hba, cmd, false); spin_unlock_irqrestore(hba->host->host_lock, flags); + ret = __ufshcd_send_uic_cmd(hba, cmd, false); if (ret) { dev_err(hba->dev, "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
> > static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba) > > { > > - return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY; > > + ktime_t timeout = ktime_add_ms(ktime_get(), UIC_CMD_TIMEOUT); > > + u32 val = 0; > > + > > + do { > > + val = ufshcd_readl(hba, REG_CONTROLLER_STATUS) & > > + UIC_COMMAND_READY; > > + if (val) > > + break; > > + udelay(500); > > + } while (ktime_before(ktime_get(), timeout)); > > + > > + return val ? true : false; > > } > > Sleeping during up to 500 ms while holding a spin lock is not acceptable. > Has it been considered to modify the UFS core such that the host_lock is > not held around calls of the above function, e.g. via the (untested) patch > below? > > Thanks, > > Bart. Let me consider it. > > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index > 9736b2b4120e..394283b04d7c 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -2416,7 +2416,6 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct > uic_command *uic_cmd, > bool completion) > { > lockdep_assert_held(&hba->uic_cmd_mutex); > - lockdep_assert_held(hba->host->host_lock); > > if (!ufshcd_ready_for_uic_cmd(hba)) { > dev_err(hba->dev, > @@ -2452,9 +2451,7 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct > uic_command *uic_cmd) > mutex_lock(&hba->uic_cmd_mutex); > ufshcd_add_delay_before_dme_cmd(hba); > > - spin_lock_irqsave(hba->host->host_lock, flags); > ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true); > - spin_unlock_irqrestore(hba->host->host_lock, flags); > if (!ret) > ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd); > > @@ -4122,8 +4119,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, > struct uic_command *cmd) > wmb(); > reenable_intr = true; > } > - ret = __ufshcd_send_uic_cmd(hba, cmd, false); > spin_unlock_irqrestore(hba->host->host_lock, flags); > + ret = __ufshcd_send_uic_cmd(hba, cmd, false); > if (ret) { > dev_err(hba->dev, > "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 9434328..5f6819a 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -2365,7 +2365,18 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba) */ static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba) { - return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY; + ktime_t timeout = ktime_add_ms(ktime_get(), UIC_CMD_TIMEOUT); + u32 val = 0; + + do { + val = ufshcd_readl(hba, REG_CONTROLLER_STATUS) & + UIC_COMMAND_READY; + if (val) + break; + udelay(500); + } while (ktime_before(ktime_get(), timeout)); + + return val ? true : false; } /**
v1 -> v2: replace usleep_range with udelay because it's a sleepable period. With auto hibern8 enabled, UIC could be working for a while to process a hibern8 operation and HCI reports UIC not ready for a short term through HCS.UCRDY. And UFS driver can't recognize the operation. UFSHCI spec specifies UCRDY like this: whether the host controller is ready to process UIC COMMAND The 'ready' could be seen as many different meanings. If the meaning includes not processing any request from HCI, processing a hibern8 operation can be 'not ready'. In this situation, the driver needs to wait until the operations is completed. Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> --- drivers/ufs/core/ufshcd.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)