| Message ID | 1573798172-20534-4-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 71d848b8d97ec0f8e993d63cf9de6ac8b3f7c43d |
| Headers | show |
| Series | UFS driver general fixes bundle 3 | expand |
Hi Can, On Thu, 2019-11-14 at 22:09 -0800, Can Guo wrote: > + if (hba->ahit != ahit) > + hba->ahit = ahit; > spin_unlock_irqrestore(hba->host->host_lock, flags); > + if (!pm_runtime_suspended(hba->dev)) { Always do pm_runtime_get_sync() here could avoid possible racing? And thus AH8 could be enabled regardless of runtime status. > + pm_runtime_get_sync(hba->dev); > + ufshcd_hold(hba, false); > + ufshcd_auto_hibern8_enable(hba); > + ufshcd_release(hba); > + pm_runtime_put(hba->dev); > + } > } Thanks, Stanley
On 2019-11-15 14:35, Stanley Chu wrote: > Hi Can, > > On Thu, 2019-11-14 at 22:09 -0800, Can Guo wrote: >> + if (hba->ahit != ahit) >> + hba->ahit = ahit; >> spin_unlock_irqrestore(hba->host->host_lock, flags); >> + if (!pm_runtime_suspended(hba->dev)) { > > Always do pm_runtime_get_sync() here could avoid possible racing? > > And thus AH8 could be enabled regardless of runtime status. > >> + pm_runtime_get_sync(hba->dev); >> + ufshcd_hold(hba, false); >> + ufshcd_auto_hibern8_enable(hba); >> + ufshcd_release(hba); >> + pm_runtime_put(hba->dev); >> + } >> } > > Thanks, > Stanley Hi Stanley, if !pm_runtime_suspended() is true, hba->dev's runtime status, other than RPM_ACTIVE, may be RPM_SUSPENDING or RPM_RESUMING. So, here for safty, do pm_runtime_get_sync() once before access registers, in case we hit corner cases in which powers and/or clocks are OFF. Thanks, Can Guo.
Hi Can, On Fri, 2019-11-15 at 15:03 +0800, Can Guo wrote: > On 2019-11-15 14:35, Stanley Chu wrote: > > Hi Can, > > > > On Thu, 2019-11-14 at 22:09 -0800, Can Guo wrote: > >> + if (hba->ahit != ahit) > >> + hba->ahit = ahit; > >> spin_unlock_irqrestore(hba->host->host_lock, flags); > >> + if (!pm_runtime_suspended(hba->dev)) { > > > > Always do pm_runtime_get_sync() here could avoid possible racing? > > > > And thus AH8 could be enabled regardless of runtime status. > > > >> + pm_runtime_get_sync(hba->dev); > >> + ufshcd_hold(hba, false); > >> + ufshcd_auto_hibern8_enable(hba); > >> + ufshcd_release(hba); > >> + pm_runtime_put(hba->dev); > >> + } > >> } > > > > Thanks, > > Stanley > > Hi Stanley, > > if !pm_runtime_suspended() is true, hba->dev's runtime status, other > than RPM_ACTIVE, > may be RPM_SUSPENDING or RPM_RESUMING. So, here for safty, do > pm_runtime_get_sync() once > before access registers, in case we hit corner cases in which powers > and/or clocks are OFF. > Thanks for explanation. I fully understand the intention of this patch. Just wonder if "if (!pm_runtime_suspended(hba->dev))" could be removed and then always do pm_runtime_get_sync() here. This could avoid possible racing and enable AH8 regardless of current runtime status. > Thanks, > Can Guo. Thanks, Stanley
On 2019-11-15 15:18, Stanley Chu wrote: > Hi Can, > > On Fri, 2019-11-15 at 15:03 +0800, Can Guo wrote: >> On 2019-11-15 14:35, Stanley Chu wrote: >> > Hi Can, >> > >> > On Thu, 2019-11-14 at 22:09 -0800, Can Guo wrote: >> >> + if (hba->ahit != ahit) >> >> + hba->ahit = ahit; >> >> spin_unlock_irqrestore(hba->host->host_lock, flags); >> >> + if (!pm_runtime_suspended(hba->dev)) { >> > >> > Always do pm_runtime_get_sync() here could avoid possible racing? >> > >> > And thus AH8 could be enabled regardless of runtime status. >> > >> >> + pm_runtime_get_sync(hba->dev); >> >> + ufshcd_hold(hba, false); >> >> + ufshcd_auto_hibern8_enable(hba); >> >> + ufshcd_release(hba); >> >> + pm_runtime_put(hba->dev); >> >> + } >> >> } >> > >> > Thanks, >> > Stanley >> >> Hi Stanley, >> >> if !pm_runtime_suspended() is true, hba->dev's runtime status, other >> than RPM_ACTIVE, >> may be RPM_SUSPENDING or RPM_RESUMING. So, here for safty, do >> pm_runtime_get_sync() once >> before access registers, in case we hit corner cases in which powers >> and/or clocks are OFF. >> > > Thanks for explanation. > > I fully understand the intention of this patch. > > Just wonder if "if (!pm_runtime_suspended(hba->dev))" could be removed > and then always do pm_runtime_get_sync() here. > > This could avoid possible racing and enable AH8 regardless of current > runtime status. > >> Thanks, >> Can Guo. > > Thanks, > Stanley Hi Stanley, Actually, I thought about the way you reommand. But I guess the author's intention here is to update the AH8 timer only when current runtime status is RPM_ACTIVE. If it is not RPM_ACTIVE, we just update the hba->ahit and bail out, because the AH8 timer will be updated in ufshcd_reusme() eventually when hba is resumed. This can avoid frequently waking up hba just for accessing a single register. How do you think? Thanks, Can Guo.
Hi Can, On Fri, 2019-11-15 at 20:27 +0800, Can Guo wrote: > Hi Stanley, > > Actually, I thought about the way you reommand. > > But I guess the author's intention here is to update the AH8 timer > only when current runtime status is RPM_ACTIVE. If it is not RPM_ACTIVE, > we just update the hba->ahit and bail out, because the AH8 timer will be > updated in ufshcd_reusme() eventually when hba is resumed. This can > avoid > frequently waking up hba just for accessing a single register. > How do you think? > > Thanks, > Can Guo. > > Agree with you. Thanks :) Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c index 969a36b..ad2abc9 100644 --- a/drivers/scsi/ufs/ufs-sysfs.c +++ b/drivers/scsi/ufs/ufs-sysfs.c @@ -126,13 +126,16 @@ static void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit) return; spin_lock_irqsave(hba->host->host_lock, flags); - if (hba->ahit == ahit) - goto out_unlock; - hba->ahit = ahit; - if (!pm_runtime_suspended(hba->dev)) - ufshcd_writel(hba, hba->ahit, REG_AUTO_HIBERNATE_IDLE_TIMER); -out_unlock: + if (hba->ahit != ahit) + hba->ahit = ahit; spin_unlock_irqrestore(hba->host->host_lock, flags); + if (!pm_runtime_suspended(hba->dev)) { + pm_runtime_get_sync(hba->dev); + ufshcd_hold(hba, false); + ufshcd_auto_hibern8_enable(hba); + ufshcd_release(hba); + pm_runtime_put(hba->dev); + } } /* Convert Auto-Hibernate Idle Timer register value to microseconds */ diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 525f8e6..9bc2cad 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -3947,7 +3947,7 @@ static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba) return ret; } -static void ufshcd_auto_hibern8_enable(struct ufs_hba *hba) +void ufshcd_auto_hibern8_enable(struct ufs_hba *hba) { unsigned long flags; @@ -6892,9 +6892,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba) /* UniPro link is active now */ ufshcd_set_link_active(hba); - /* Enable Auto-Hibernate if configured */ - ufshcd_auto_hibern8_enable(hba); - ret = ufshcd_verify_dev_init(hba); if (ret) goto out; @@ -6945,6 +6942,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba) /* set the state as operational after switching to desired gear */ hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL; + /* Enable Auto-Hibernate if configured */ + ufshcd_auto_hibern8_enable(hba); + /* * If we are in error handling context or in power management callbacks * context, no need to scan the host @@ -7962,12 +7962,12 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) if (hba->clk_scaling.is_allowed) ufshcd_resume_clkscaling(hba); - /* Schedule clock gating in case of no access to UFS device yet */ - ufshcd_release(hba); - /* Enable Auto-Hibernate if configured */ ufshcd_auto_hibern8_enable(hba); + /* Schedule clock gating in case of no access to UFS device yet */ + ufshcd_release(hba); + goto out; set_old_link_state: diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index e0fe247..2740f69 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -926,6 +926,8 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode, int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode, enum flag_idn idn, bool *flag_res); +void ufshcd_auto_hibern8_enable(struct ufs_hba *hba); + #define SD_ASCII_STD true #define SD_RAW false int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index,
Fix up possible unclocked register access to auto hibern8 register in resume path and through sysfs entry. Meanwhile, enable auto hibern8 only after device is fully initialized in probe path. Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/ufs/ufs-sysfs.c | 15 +++++++++------ drivers/scsi/ufs/ufshcd.c | 14 +++++++------- drivers/scsi/ufs/ufshcd.h | 2 ++ 3 files changed, 18 insertions(+), 13 deletions(-)