| Message ID | 1683620674-160173-1-git-send-email-kwmad.kim@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v1] ufs: poll HCS.UCRDY before issuing a UIC command | expand |
> 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> Is this replaces your previous suggestion - https://lore.kernel.org/lkml/1682385635-43601-1-git-send-email-kwmad.kim@samsung.com/ Or is it addressing another issue? Thanks, Avri > --- > 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 96ce6af..fc79539 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -2368,7 +2368,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; > + usleep_range(500, 1000); > + } while (ktime_before(ktime_get(), timeout)); > + > + return val ? true : false; > } > > /** > -- > 2.7.4
On 5/9/23 01:24, Kiwoong Kim wrote: > 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 96ce6af..fc79539 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -2368,7 +2368,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; > + usleep_range(500, 1000); > + } while (ktime_before(ktime_get(), timeout)); > + > + return val ? true : false; > } > > /** Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On 5/9/2023 1:24 AM, Kiwoong Kim wrote: > 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 96ce6af..fc79539 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -2368,7 +2368,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; > + usleep_range(500, 1000); Hi Kiwoong, It looks like you are sleeping while holding the spin_lock_irqsave(hba->host->host_lock, flags) in ufshcd_send_uic_cmd()? > + } while (ktime_before(ktime_get(), timeout)); > + > + return val ? true : false; > } > > /**
On 5/10/23 14:24, Bao D. Nguyen wrote: > It looks like you are sleeping while holding the > spin_lock_irqsave(hba->host->host_lock, flags) > in ufshcd_send_uic_cmd()? Right. Please drop my Reviewed-by. Thanks, Bart.
> On 5/9/2023 1:24 AM, Kiwoong Kim wrote: > > 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 96ce6af..fc79539 100644 > > --- a/drivers/ufs/core/ufshcd.c > > +++ b/drivers/ufs/core/ufshcd.c > > @@ -2368,7 +2368,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; > > + usleep_range(500, 1000); > Hi Kiwoong, > It looks like you are sleeping while holding the spin_lock_irqsave(hba- > >host->host_lock, flags) in ufshcd_send_uic_cmd()? Right. Let me fix it. Thanks. Kiwoong Kim > > > > + } while (ktime_before(ktime_get(), timeout)); > > + > > + return val ? true : false; > > } > > > > /**
On 9/05/23 11:24, Kiwoong Kim wrote: > 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 96ce6af..fc79539 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -2368,7 +2368,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; > + usleep_range(500, 1000); Pedantically, the sleep probably needs to be less than the auto-hibernate idle timer period? > + } while (ktime_before(ktime_get(), timeout)); read_poll_timeout() would be a better choice for I/O polling > + > + return val ? true : false; > } > > /**
> Pedantically, the sleep probably needs to be less than the auto-hibernate > idle timer period? > > > + } while (ktime_before(ktime_get(), timeout)); > > read_poll_timeout() would be a better choice for I/O polling It makes sense. Got it. Thanks.
> > + do { > > + val = ufshcd_readl(hba, REG_CONTROLLER_STATUS) & > > + UIC_COMMAND_READY; > > + if (val) > > + break; > > + usleep_range(500, 1000); > Hi Kiwoong, > It looks like you are sleeping while holding the spin_lock_irqsave(hba- > >host->host_lock, flags) in ufshcd_send_uic_cmd()? You're right. Let me fix it.
> > 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> > Is this replaces your previous suggestion - > https://lore.kernel.org/lkml/1682385635-43601-1-git-send-email- > kwmad.kim@samsung.com/ > Or is it addressing another issue? > > Thanks, > Avri > No, they are different. This is about reporting UIC ready and what you mentioned is about reporting UIC busy in a certain race condition.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 96ce6af..fc79539 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -2368,7 +2368,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; + usleep_range(500, 1000); + } while (ktime_before(ktime_get(), timeout)); + + return val ? true : false; } /**
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(-)