| Message ID | 1573627552-12615-8-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | UFS driver general fixes bundle 3 | expand |
> > > From: Subhash Jadavani <subhashj@codeaurora.org> > > During clock gating (ufshcd_gate_work()), we first put the link hibern8 by > calling ufshcd_uic_hibern8_enter() and if ufshcd_uic_hibern8_enter() returns > success (0) then we gate all the clocks. > Now let’s zoom in to what ufshcd_uic_hibern8_enter() does internally: > It calls __ufshcd_uic_hibern8_enter() which on detecting the LINERESET, > initiates the full recovery and puts the link back to highest HS gear and returns > success (0) to ufshcd_uic_hibern8_enter() which is the issue as link is still in > active state due to recovery! > Now ufshcd_uic_hibern8_enter() returns success to ufshcd_gate_work() and > hence it goes ahead with gating the UFS clock while link is still in active state > hence I believe controller would raise UIC error interrupts. But when we service > the interrupt, clocks might have already been disabled! > > This change fixes for this by returning failure from > __ufshcd_uic_hibern8_enter() if recovery succeeds as link is still not in hibern8, > upon receiving the error ufshcd_hibern8_enter() would initiate retry to put the > link state back into hibern8. > > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org> > Signed-off-by: Can Guo <cang@codeaurora.org> > --- > drivers/scsi/ufs/ufshcd.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index > 7a5a904..934c27a 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -3891,15 +3891,24 @@ static int __ufshcd_uic_hibern8_enter(struct > ufs_hba *hba) > ktime_to_us(ktime_sub(ktime_get(), start)), ret); > > if (ret) { > + int err; > + > dev_err(hba->dev, "%s: hibern8 enter failed. ret = %d\n", > __func__, ret); > > /* > - * If link recovery fails then return error so that caller > - * don't retry the hibern8 enter again. > + * If link recovery fails then return error code (-ENOLINK) > + * returned ufshcd_link_recovery(). > + * If link recovery succeeds then return -EAGAIN to attempt > + * hibern8 enter retry again. You no longer returning -ENOLINK, and either way retrying, regardless of the error code. Better check that the commit log is still telling the correct story, taking into consideration all those recent fixes and all. > */ > - if (ufshcd_link_recovery(hba)) > - ret = -ENOLINK; > + err = ufshcd_link_recovery(hba); > + if (err) { > + dev_err(hba->dev, "%s: link recovery failed", __func__); > + ret = err; > + } else { > + ret = -EAGAIN; > + } > } else > ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER, > POST_CHANGE); @@ -3913,7 +3922,7 @@ > static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba) > > for (retries = UIC_HIBERN8_ENTER_RETRIES; retries > 0; retries--) { > ret = __ufshcd_uic_hibern8_enter(hba); > - if (!ret || ret == -ENOLINK) > + if (!ret) > goto out; > } > out: > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project
On 2019-11-14 19:06, Avri Altman wrote: >> >> >> From: Subhash Jadavani <subhashj@codeaurora.org> >> >> During clock gating (ufshcd_gate_work()), we first put the link >> hibern8 by >> calling ufshcd_uic_hibern8_enter() and if ufshcd_uic_hibern8_enter() >> returns >> success (0) then we gate all the clocks. >> Now let’s zoom in to what ufshcd_uic_hibern8_enter() does internally: >> It calls __ufshcd_uic_hibern8_enter() which on detecting the >> LINERESET, >> initiates the full recovery and puts the link back to highest HS gear >> and returns >> success (0) to ufshcd_uic_hibern8_enter() which is the issue as link >> is still in >> active state due to recovery! >> Now ufshcd_uic_hibern8_enter() returns success to ufshcd_gate_work() >> and >> hence it goes ahead with gating the UFS clock while link is still in >> active state >> hence I believe controller would raise UIC error interrupts. But when >> we service >> the interrupt, clocks might have already been disabled! >> >> This change fixes for this by returning failure from >> __ufshcd_uic_hibern8_enter() if recovery succeeds as link is still not >> in hibern8, >> upon receiving the error ufshcd_hibern8_enter() would initiate retry >> to put the >> link state back into hibern8. >> >> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org> >> Signed-off-by: Can Guo <cang@codeaurora.org> >> --- >> drivers/scsi/ufs/ufshcd.c | 19 ++++++++++++++----- >> 1 file changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index >> 7a5a904..934c27a 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -3891,15 +3891,24 @@ static int __ufshcd_uic_hibern8_enter(struct >> ufs_hba *hba) >> ktime_to_us(ktime_sub(ktime_get(), >> start)), ret); >> >> if (ret) { >> + int err; >> + >> dev_err(hba->dev, "%s: hibern8 enter failed. ret = >> %d\n", >> __func__, ret); >> >> /* >> - * If link recovery fails then return error so that >> caller >> - * don't retry the hibern8 enter again. >> + * If link recovery fails then return error code >> (-ENOLINK) >> + * returned ufshcd_link_recovery(). >> + * If link recovery succeeds then return -EAGAIN to >> attempt >> + * hibern8 enter retry again. > You no longer returning -ENOLINK, and either way retrying, regardless > of the error code. > Better check that the commit log is still telling the correct story, > taking into consideration all those recent fixes and all. > Thanks for pointing this. Best Regards, Can Guo. >> */ >> - if (ufshcd_link_recovery(hba)) >> - ret = -ENOLINK; >> + err = ufshcd_link_recovery(hba); >> + if (err) { >> + dev_err(hba->dev, "%s: link recovery failed", >> __func__); >> + ret = err; >> + } else { >> + ret = -EAGAIN; >> + } >> } else >> ufshcd_vops_hibern8_notify(hba, >> UIC_CMD_DME_HIBER_ENTER, >> >> POST_CHANGE); @@ -3913,7 +3922,7 @@ >> static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba) >> >> for (retries = UIC_HIBERN8_ENTER_RETRIES; retries > 0; >> retries--) { >> ret = __ufshcd_uic_hibern8_enter(hba); >> - if (!ret || ret == -ENOLINK) >> + if (!ret) >> goto out; >> } >> out: >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >> Forum, >> a Linux Foundation Collaborative Project
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 7a5a904..934c27a 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -3891,15 +3891,24 @@ static int __ufshcd_uic_hibern8_enter(struct ufs_hba *hba) ktime_to_us(ktime_sub(ktime_get(), start)), ret); if (ret) { + int err; + dev_err(hba->dev, "%s: hibern8 enter failed. ret = %d\n", __func__, ret); /* - * If link recovery fails then return error so that caller - * don't retry the hibern8 enter again. + * If link recovery fails then return error code (-ENOLINK) + * returned ufshcd_link_recovery(). + * If link recovery succeeds then return -EAGAIN to attempt + * hibern8 enter retry again. */ - if (ufshcd_link_recovery(hba)) - ret = -ENOLINK; + err = ufshcd_link_recovery(hba); + if (err) { + dev_err(hba->dev, "%s: link recovery failed", __func__); + ret = err; + } else { + ret = -EAGAIN; + } } else ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER, POST_CHANGE); @@ -3913,7 +3922,7 @@ static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba) for (retries = UIC_HIBERN8_ENTER_RETRIES; retries > 0; retries--) { ret = __ufshcd_uic_hibern8_enter(hba); - if (!ret || ret == -ENOLINK) + if (!ret) goto out; } out: