| Message ID | 1611057183-6925-1-git-send-email-cang@codeaurora.org |
|---|---|
| Headers | show |
| Series | Three changes related with UFS clock scaling | expand |
Hi Can, On Tue, 2021-01-19 at 03:52 -0800, Can Guo wrote: > In contexts like suspend, shutdown and error handling, we need to suspend > devfreq to make sure these contexts won't be disturbed by clock scaling. > However, suspending devfreq is not enough since users can still trigger a > clock scaling by manipulating the devfreq sysfs nodes like min/max_freq and > governor even after devfreq is suspended. Moreover, mere suspending devfreq > cannot synchroinze a clock scaling which has already been invoked through > these sysfs nodes. Add one more flag in struct clk_scaling and wrap the > entire func ufshcd_devfreq_scale() with the clk_scaling_lock, so that we > can use this flag and clk_scaling_lock to control and synchronize clock > scaling invoked through devfreq sysfs nodes. > > Signed-off-by: Can Guo <cang@codeaurora.org> > --- > drivers/scsi/ufs/ufshcd.c | 83 +++++++++++++++++++++++++++++------------------ > drivers/scsi/ufs/ufshcd.h | 6 +++- > 2 files changed, 56 insertions(+), 33 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 53fd59c..ba62d84 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -1201,19 +1201,30 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba) > */ > ufshcd_scsi_block_requests(hba); > down_write(&hba->clk_scaling_lock); > - if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) { > + > + if (!hba->clk_scaling.is_allowed || > + ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) { > ret = -EBUSY; > up_write(&hba->clk_scaling_lock); > ufshcd_scsi_unblock_requests(hba); > + goto out; > } > > + /* let's not get into low power until clock scaling is completed */ > + ufshcd_hold(hba, false); > + > +out: > return ret; > } > > -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba) > +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock) > { > - up_write(&hba->clk_scaling_lock); > + if (writelock) > + up_write(&hba->clk_scaling_lock); > + else > + up_read(&hba->clk_scaling_lock); > ufshcd_scsi_unblock_requests(hba); > + ufshcd_release(hba); > } > > /** > @@ -1228,13 +1239,11 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba) > static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up) > { > int ret = 0; > - > - /* let's not get into low power until clock scaling is completed */ > - ufshcd_hold(hba, false); > + bool is_writelock = true; > > ret = ufshcd_clock_scaling_prepare(hba); > if (ret) > - goto out; > + return ret; > > /* scale down the gear before scaling down clocks */ > if (!scale_up) { > @@ -1260,14 +1269,12 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up) > } > > /* Enable Write Booster if we have scaled up else disable it */ > - up_write(&hba->clk_scaling_lock); > + downgrade_write(&hba->clk_scaling_lock); > + is_writelock = false; > ufshcd_wb_ctrl(hba, scale_up); > - down_write(&hba->clk_scaling_lock); > > out_unprepare: > - ufshcd_clock_scaling_unprepare(hba); > -out: > - ufshcd_release(hba); > + ufshcd_clock_scaling_unprepare(hba, is_writelock); > return ret; > } > > @@ -1541,7 +1548,7 @@ static ssize_t ufshcd_clkscale_enable_show(struct device *dev, > { > struct ufs_hba *hba = dev_get_drvdata(dev); > > - return snprintf(buf, PAGE_SIZE, "%d\n", hba->clk_scaling.is_allowed); > + return snprintf(buf, PAGE_SIZE, "%d\n", hba->clk_scaling.is_enabled); > } > > static ssize_t ufshcd_clkscale_enable_store(struct device *dev, > @@ -1555,7 +1562,7 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev, > return -EINVAL; > > value = !!value; > - if (value == hba->clk_scaling.is_allowed) > + if (value == hba->clk_scaling.is_enabled) > goto out; > > pm_runtime_get_sync(hba->dev); > @@ -1564,7 +1571,7 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev, > cancel_work_sync(&hba->clk_scaling.suspend_work); > cancel_work_sync(&hba->clk_scaling.resume_work); > > - hba->clk_scaling.is_allowed = value; > + hba->clk_scaling.is_enabled = value; > > if (value) { > ufshcd_resume_clkscaling(hba); > @@ -1902,8 +1909,6 @@ static void ufshcd_init_clk_scaling(struct ufs_hba *hba) > snprintf(wq_name, sizeof(wq_name), "ufs_clkscaling_%d", > hba->host->host_no); > hba->clk_scaling.workq = create_singlethread_workqueue(wq_name); > - > - ufshcd_clkscaling_init_sysfs(hba); > } > > static void ufshcd_exit_clk_scaling(struct ufs_hba *hba) > @@ -1911,6 +1916,8 @@ static void ufshcd_exit_clk_scaling(struct ufs_hba *hba) > if (!ufshcd_is_clkscaling_supported(hba)) > return; > > + if (hba->clk_scaling.enable_attr.attr.name) > + device_remove_file(hba->dev, &hba->clk_scaling.enable_attr); > destroy_workqueue(hba->clk_scaling.workq); > ufshcd_devfreq_remove(hba); > } > @@ -1975,7 +1982,7 @@ static void ufshcd_clk_scaling_start_busy(struct ufs_hba *hba) > if (!hba->clk_scaling.active_reqs++) > queue_resume_work = true; > > - if (!hba->clk_scaling.is_allowed || hba->pm_op_in_progress) > + if (!hba->clk_scaling.is_enabled || hba->pm_op_in_progress) > return; > > if (queue_resume_work) > @@ -5093,7 +5100,8 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba, > update_scaling = true; > } > } > - if (ufshcd_is_clkscaling_supported(hba) && update_scaling) > + if (ufshcd_is_clkscaling_supported(hba) && update_scaling && > + hba->clk_scaling.active_reqs > 0) Do we need to check hba->clk_scaling.active_reqs here? if hba->clk_scaling.active_reqs is possibly decreased to negative value here, then there may be something to be fixed? Otherwise this patch looks good to me. Thanks, Stanley Chu > hba->clk_scaling.active_reqs--; > } > > @@ -5759,18 +5767,24 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba) > ufshcd_vops_resume(hba, pm_op); > } else { > ufshcd_hold(hba, false); > - if (hba->clk_scaling.is_allowed) { > + if (hba->clk_scaling.is_enabled) { > cancel_work_sync(&hba->clk_scaling.suspend_work); > cancel_work_sync(&hba->clk_scaling.resume_work); > ufshcd_suspend_clkscaling(hba); > } > + down_write(&hba->clk_scaling_lock); > + hba->clk_scaling.is_allowed = false; > + up_write(&hba->clk_scaling_lock); > } > } > > static void ufshcd_err_handling_unprepare(struct ufs_hba *hba) > { > ufshcd_release(hba); > - if (hba->clk_scaling.is_allowed) > + down_write(&hba->clk_scaling_lock); > + hba->clk_scaling.is_allowed = true; > + up_write(&hba->clk_scaling_lock); > + if (hba->clk_scaling.is_enabled) > ufshcd_resume_clkscaling(hba); > pm_runtime_put(hba->dev); > } > @@ -7750,12 +7764,14 @@ static int ufshcd_add_lus(struct ufs_hba *hba) > sizeof(struct ufs_pa_layer_attr)); > hba->clk_scaling.saved_pwr_info.is_valid = true; > if (!hba->devfreq) { > + hba->clk_scaling.is_allowed = true; > ret = ufshcd_devfreq_init(hba); > if (ret) > goto out; > - } > > - hba->clk_scaling.is_allowed = true; > + hba->clk_scaling.is_enabled = true; > + ufshcd_clkscaling_init_sysfs(hba); > + } > } > > ufs_bsg_probe(hba); > @@ -8672,11 +8688,14 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > ufshcd_hold(hba, false); > hba->clk_gating.is_suspended = true; > > - if (hba->clk_scaling.is_allowed) { > + if (hba->clk_scaling.is_enabled) { > cancel_work_sync(&hba->clk_scaling.suspend_work); > cancel_work_sync(&hba->clk_scaling.resume_work); > ufshcd_suspend_clkscaling(hba); > } > + down_write(&hba->clk_scaling_lock); > + hba->clk_scaling.is_allowed = false; > + up_write(&hba->clk_scaling_lock); > > if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE && > req_link_state == UIC_LINK_ACTIVE_STATE) { > @@ -8773,8 +8792,6 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > goto out; > > set_link_active: > - if (hba->clk_scaling.is_allowed) > - ufshcd_resume_clkscaling(hba); > ufshcd_vreg_set_hpm(hba); > /* > * Device hardware reset is required to exit DeepSleep. Also, for > @@ -8798,7 +8815,10 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > if (!ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE)) > ufshcd_disable_auto_bkops(hba); > enable_gating: > - if (hba->clk_scaling.is_allowed) > + down_write(&hba->clk_scaling_lock); > + hba->clk_scaling.is_allowed = true; > + up_write(&hba->clk_scaling_lock); > + if (hba->clk_scaling.is_enabled) > ufshcd_resume_clkscaling(hba); > hba->clk_gating.is_suspended = false; > hba->dev_info.b_rpm_dev_flush_capable = false; > @@ -8901,7 +8921,10 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) > > hba->clk_gating.is_suspended = false; > > - if (hba->clk_scaling.is_allowed) > + down_write(&hba->clk_scaling_lock); > + hba->clk_scaling.is_allowed = true; > + up_write(&hba->clk_scaling_lock); > + if (hba->clk_scaling.is_enabled) > ufshcd_resume_clkscaling(hba); > > /* Enable Auto-Hibernate if configured */ > @@ -8925,8 +8948,6 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) > ufshcd_vreg_set_lpm(hba); > disable_irq_and_vops_clks: > ufshcd_disable_irq(hba); > - if (hba->clk_scaling.is_allowed) > - ufshcd_suspend_clkscaling(hba); > ufshcd_setup_clocks(hba, false); > if (ufshcd_is_clkgating_allowed(hba)) { > hba->clk_gating.state = CLKS_OFF; > @@ -9153,8 +9174,6 @@ void ufshcd_remove(struct ufs_hba *hba) > > ufshcd_exit_clk_scaling(hba); > ufshcd_exit_clk_gating(hba); > - if (ufshcd_is_clkscaling_supported(hba)) > - device_remove_file(hba->dev, &hba->clk_scaling.enable_attr); > ufshcd_hba_exit(hba); > } > EXPORT_SYMBOL_GPL(ufshcd_remove); > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index 85f9d0f..d553e46 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -419,7 +419,10 @@ struct ufs_saved_pwr_info { > * @suspend_work: worker to suspend devfreq > * @resume_work: worker to resume devfreq > * @min_gear: lowest HS gear to scale down to > - * @is_allowed: tracks if scaling is currently allowed or not > + * @is_enabled: tracks if scaling is currently enabled or not, controlled by > + clkscale_enable sysfs node > + * @is_allowed: tracks if scaling is currently allowed or not, used to block > + clock scaling which is not invoked from devfreq governor > * @is_busy_started: tracks if busy period has started or not > * @is_suspended: tracks if devfreq is suspended or not > */ > @@ -434,6 +437,7 @@ struct ufs_clk_scaling { > struct work_struct suspend_work; > struct work_struct resume_work; > u32 min_gear; > + bool is_enabled; > bool is_allowed; > bool is_busy_started; > bool is_suspended;
Hi Can, On Tue, 2021-01-19 at 03:53 -0800, Can Guo wrote: > ufshcd_hba_exit() is always called after ufshcd_exit_clk_scaling() and > ufshcd_exit_clk_gating(), so move ufshcd_exit_clk_scaling/gating() to > ufshcd_hba_exit(). Meanwhile, add dedicated funcs to init and remove > sysfs nodes of clock scaling/gating to make the code more readable. > Overall functionality remains same. > > Signed-off-by: Can Guo <cang@codeaurora.org> Looks good to me. BTW, FYI. this series has conflicts during the merge to the latest for-next branch in Martin's tree. Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
On 2021-01-20 12:54, Stanley Chu wrote: > Hi Can, > > On Tue, 2021-01-19 at 03:52 -0800, Can Guo wrote: >> In contexts like suspend, shutdown and error handling, we need to >> suspend >> devfreq to make sure these contexts won't be disturbed by clock >> scaling. >> However, suspending devfreq is not enough since users can still >> trigger a >> clock scaling by manipulating the devfreq sysfs nodes like >> min/max_freq and >> governor even after devfreq is suspended. Moreover, mere suspending >> devfreq >> cannot synchroinze a clock scaling which has already been invoked >> through >> these sysfs nodes. Add one more flag in struct clk_scaling and wrap >> the >> entire func ufshcd_devfreq_scale() with the clk_scaling_lock, so that >> we >> can use this flag and clk_scaling_lock to control and synchronize >> clock >> scaling invoked through devfreq sysfs nodes. >> >> Signed-off-by: Can Guo <cang@codeaurora.org> >> --- >> drivers/scsi/ufs/ufshcd.c | 83 >> +++++++++++++++++++++++++++++------------------ >> drivers/scsi/ufs/ufshcd.h | 6 +++- >> 2 files changed, 56 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index 53fd59c..ba62d84 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -1201,19 +1201,30 @@ static int ufshcd_clock_scaling_prepare(struct >> ufs_hba *hba) >> */ >> ufshcd_scsi_block_requests(hba); >> down_write(&hba->clk_scaling_lock); >> - if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) { >> + >> + if (!hba->clk_scaling.is_allowed || >> + ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) { >> ret = -EBUSY; >> up_write(&hba->clk_scaling_lock); >> ufshcd_scsi_unblock_requests(hba); >> + goto out; >> } >> >> + /* let's not get into low power until clock scaling is completed */ >> + ufshcd_hold(hba, false); >> + >> +out: >> return ret; >> } >> >> -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba) >> +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool >> writelock) >> { >> - up_write(&hba->clk_scaling_lock); >> + if (writelock) >> + up_write(&hba->clk_scaling_lock); >> + else >> + up_read(&hba->clk_scaling_lock); >> ufshcd_scsi_unblock_requests(hba); >> + ufshcd_release(hba); >> } >> >> /** >> @@ -1228,13 +1239,11 @@ static void >> ufshcd_clock_scaling_unprepare(struct ufs_hba *hba) >> static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up) >> { >> int ret = 0; >> - >> - /* let's not get into low power until clock scaling is completed */ >> - ufshcd_hold(hba, false); >> + bool is_writelock = true; >> >> ret = ufshcd_clock_scaling_prepare(hba); >> if (ret) >> - goto out; >> + return ret; >> >> /* scale down the gear before scaling down clocks */ >> if (!scale_up) { >> @@ -1260,14 +1269,12 @@ static int ufshcd_devfreq_scale(struct ufs_hba >> *hba, bool scale_up) >> } >> >> /* Enable Write Booster if we have scaled up else disable it */ >> - up_write(&hba->clk_scaling_lock); >> + downgrade_write(&hba->clk_scaling_lock); >> + is_writelock = false; >> ufshcd_wb_ctrl(hba, scale_up); >> - down_write(&hba->clk_scaling_lock); >> >> out_unprepare: >> - ufshcd_clock_scaling_unprepare(hba); >> -out: >> - ufshcd_release(hba); >> + ufshcd_clock_scaling_unprepare(hba, is_writelock); >> return ret; >> } >> >> @@ -1541,7 +1548,7 @@ static ssize_t >> ufshcd_clkscale_enable_show(struct device *dev, >> { >> struct ufs_hba *hba = dev_get_drvdata(dev); >> >> - return snprintf(buf, PAGE_SIZE, "%d\n", >> hba->clk_scaling.is_allowed); >> + return snprintf(buf, PAGE_SIZE, "%d\n", >> hba->clk_scaling.is_enabled); >> } >> >> static ssize_t ufshcd_clkscale_enable_store(struct device *dev, >> @@ -1555,7 +1562,7 @@ static ssize_t >> ufshcd_clkscale_enable_store(struct device *dev, >> return -EINVAL; >> >> value = !!value; >> - if (value == hba->clk_scaling.is_allowed) >> + if (value == hba->clk_scaling.is_enabled) >> goto out; >> >> pm_runtime_get_sync(hba->dev); >> @@ -1564,7 +1571,7 @@ static ssize_t >> ufshcd_clkscale_enable_store(struct device *dev, >> cancel_work_sync(&hba->clk_scaling.suspend_work); >> cancel_work_sync(&hba->clk_scaling.resume_work); >> >> - hba->clk_scaling.is_allowed = value; >> + hba->clk_scaling.is_enabled = value; >> >> if (value) { >> ufshcd_resume_clkscaling(hba); >> @@ -1902,8 +1909,6 @@ static void ufshcd_init_clk_scaling(struct >> ufs_hba *hba) >> snprintf(wq_name, sizeof(wq_name), "ufs_clkscaling_%d", >> hba->host->host_no); >> hba->clk_scaling.workq = create_singlethread_workqueue(wq_name); >> - >> - ufshcd_clkscaling_init_sysfs(hba); >> } >> >> static void ufshcd_exit_clk_scaling(struct ufs_hba *hba) >> @@ -1911,6 +1916,8 @@ static void ufshcd_exit_clk_scaling(struct >> ufs_hba *hba) >> if (!ufshcd_is_clkscaling_supported(hba)) >> return; >> >> + if (hba->clk_scaling.enable_attr.attr.name) >> + device_remove_file(hba->dev, &hba->clk_scaling.enable_attr); >> destroy_workqueue(hba->clk_scaling.workq); >> ufshcd_devfreq_remove(hba); >> } >> @@ -1975,7 +1982,7 @@ static void ufshcd_clk_scaling_start_busy(struct >> ufs_hba *hba) >> if (!hba->clk_scaling.active_reqs++) >> queue_resume_work = true; >> >> - if (!hba->clk_scaling.is_allowed || hba->pm_op_in_progress) >> + if (!hba->clk_scaling.is_enabled || hba->pm_op_in_progress) >> return; >> >> if (queue_resume_work) >> @@ -5093,7 +5100,8 @@ static void __ufshcd_transfer_req_compl(struct >> ufs_hba *hba, >> update_scaling = true; >> } >> } >> - if (ufshcd_is_clkscaling_supported(hba) && update_scaling) >> + if (ufshcd_is_clkscaling_supported(hba) && update_scaling && > >> + hba->clk_scaling.active_reqs > 0) > Do we need to check hba->clk_scaling.active_reqs here? > > if hba->clk_scaling.active_reqs is possibly decreased to negative value > here, then there may be something to be fixed? No actual issue here, just want to be on the safe side. Let me remove it in next version. Thanks, Can Guo. > > Otherwise this patch looks good to me. > > Thanks, > Stanley Chu > >> hba->clk_scaling.active_reqs--; >> } >> >> @@ -5759,18 +5767,24 @@ static void ufshcd_err_handling_prepare(struct >> ufs_hba *hba) >> ufshcd_vops_resume(hba, pm_op); >> } else { >> ufshcd_hold(hba, false); >> - if (hba->clk_scaling.is_allowed) { >> + if (hba->clk_scaling.is_enabled) { >> cancel_work_sync(&hba->clk_scaling.suspend_work); >> cancel_work_sync(&hba->clk_scaling.resume_work); >> ufshcd_suspend_clkscaling(hba); >> } >> + down_write(&hba->clk_scaling_lock); >> + hba->clk_scaling.is_allowed = false; >> + up_write(&hba->clk_scaling_lock); >> } >> } >> >> static void ufshcd_err_handling_unprepare(struct ufs_hba *hba) >> { >> ufshcd_release(hba); >> - if (hba->clk_scaling.is_allowed) >> + down_write(&hba->clk_scaling_lock); >> + hba->clk_scaling.is_allowed = true; >> + up_write(&hba->clk_scaling_lock); >> + if (hba->clk_scaling.is_enabled) >> ufshcd_resume_clkscaling(hba); >> pm_runtime_put(hba->dev); >> } >> @@ -7750,12 +7764,14 @@ static int ufshcd_add_lus(struct ufs_hba *hba) >> sizeof(struct ufs_pa_layer_attr)); >> hba->clk_scaling.saved_pwr_info.is_valid = true; >> if (!hba->devfreq) { >> + hba->clk_scaling.is_allowed = true; >> ret = ufshcd_devfreq_init(hba); >> if (ret) >> goto out; >> - } >> >> - hba->clk_scaling.is_allowed = true; >> + hba->clk_scaling.is_enabled = true; >> + ufshcd_clkscaling_init_sysfs(hba); >> + } >> } >> >> ufs_bsg_probe(hba); >> @@ -8672,11 +8688,14 @@ static int ufshcd_suspend(struct ufs_hba *hba, >> enum ufs_pm_op pm_op) >> ufshcd_hold(hba, false); >> hba->clk_gating.is_suspended = true; >> >> - if (hba->clk_scaling.is_allowed) { >> + if (hba->clk_scaling.is_enabled) { >> cancel_work_sync(&hba->clk_scaling.suspend_work); >> cancel_work_sync(&hba->clk_scaling.resume_work); >> ufshcd_suspend_clkscaling(hba); >> } >> + down_write(&hba->clk_scaling_lock); >> + hba->clk_scaling.is_allowed = false; >> + up_write(&hba->clk_scaling_lock); >> >> if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE && >> req_link_state == UIC_LINK_ACTIVE_STATE) { >> @@ -8773,8 +8792,6 @@ static int ufshcd_suspend(struct ufs_hba *hba, >> enum ufs_pm_op pm_op) >> goto out; >> >> set_link_active: >> - if (hba->clk_scaling.is_allowed) >> - ufshcd_resume_clkscaling(hba); >> ufshcd_vreg_set_hpm(hba); >> /* >> * Device hardware reset is required to exit DeepSleep. Also, for >> @@ -8798,7 +8815,10 @@ static int ufshcd_suspend(struct ufs_hba *hba, >> enum ufs_pm_op pm_op) >> if (!ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE)) >> ufshcd_disable_auto_bkops(hba); >> enable_gating: >> - if (hba->clk_scaling.is_allowed) >> + down_write(&hba->clk_scaling_lock); >> + hba->clk_scaling.is_allowed = true; >> + up_write(&hba->clk_scaling_lock); >> + if (hba->clk_scaling.is_enabled) >> ufshcd_resume_clkscaling(hba); >> hba->clk_gating.is_suspended = false; >> hba->dev_info.b_rpm_dev_flush_capable = false; >> @@ -8901,7 +8921,10 @@ static int ufshcd_resume(struct ufs_hba *hba, >> enum ufs_pm_op pm_op) >> >> hba->clk_gating.is_suspended = false; >> >> - if (hba->clk_scaling.is_allowed) >> + down_write(&hba->clk_scaling_lock); >> + hba->clk_scaling.is_allowed = true; >> + up_write(&hba->clk_scaling_lock); >> + if (hba->clk_scaling.is_enabled) >> ufshcd_resume_clkscaling(hba); >> >> /* Enable Auto-Hibernate if configured */ >> @@ -8925,8 +8948,6 @@ static int ufshcd_resume(struct ufs_hba *hba, >> enum ufs_pm_op pm_op) >> ufshcd_vreg_set_lpm(hba); >> disable_irq_and_vops_clks: >> ufshcd_disable_irq(hba); >> - if (hba->clk_scaling.is_allowed) >> - ufshcd_suspend_clkscaling(hba); >> ufshcd_setup_clocks(hba, false); >> if (ufshcd_is_clkgating_allowed(hba)) { >> hba->clk_gating.state = CLKS_OFF; >> @@ -9153,8 +9174,6 @@ void ufshcd_remove(struct ufs_hba *hba) >> >> ufshcd_exit_clk_scaling(hba); >> ufshcd_exit_clk_gating(hba); >> - if (ufshcd_is_clkscaling_supported(hba)) >> - device_remove_file(hba->dev, &hba->clk_scaling.enable_attr); >> ufshcd_hba_exit(hba); >> } >> EXPORT_SYMBOL_GPL(ufshcd_remove); >> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >> index 85f9d0f..d553e46 100644 >> --- a/drivers/scsi/ufs/ufshcd.h >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -419,7 +419,10 @@ struct ufs_saved_pwr_info { >> * @suspend_work: worker to suspend devfreq >> * @resume_work: worker to resume devfreq >> * @min_gear: lowest HS gear to scale down to >> - * @is_allowed: tracks if scaling is currently allowed or not >> + * @is_enabled: tracks if scaling is currently enabled or not, >> controlled by >> + clkscale_enable sysfs node >> + * @is_allowed: tracks if scaling is currently allowed or not, used >> to block >> + clock scaling which is not invoked from devfreq governor >> * @is_busy_started: tracks if busy period has started or not >> * @is_suspended: tracks if devfreq is suspended or not >> */ >> @@ -434,6 +437,7 @@ struct ufs_clk_scaling { >> struct work_struct suspend_work; >> struct work_struct resume_work; >> u32 min_gear; >> + bool is_enabled; >> bool is_allowed; >> bool is_busy_started; >> bool is_suspended;
This series is made based on 5.12/scsi-queue branch. Current devfreq framework allows sysfs nodes like governor, min_freq and max_freq to be changed even after devfreq device is suspended. Meanwhile, devfreq_suspend_device() cannot/wouldn't synchronize clock scaling which has already been invoked through devfreq sysfs nodes menitioned above. It means that clock scaling invoked through these devfreq sysfs nodes can happen at any time regardless of the state of UFS host and/or device. We need to control and synchronize clock scaling in this scenario. The 1st change allows contexts to prevent clock scaling from being invoked through devfreq sysfs nodes. The 2nd change is just a code cleanup for clk_scaling/gating initialization routine. The 3rd change reverts one old change which can be covered by the 1st change. For branches which do not have this change yet, it can be ignored. Change since v9: - Minor update in the 1st change. Change since v8: - Rebased on 5.12/scsi-queue Change since v7: - Slightly updated the 1st change: changed the up_write() before ufshcd_wb_ctrl() to downgrade_write() in ufshcd_devfreq_scale(), so that ufshcd_wb_ctrl() is called with clk_scale_lock held, this is to make sure race condition won't happen to ufshcd_wb_ctrl(). Change since v6: - Updated the 2nd change Change since v5: - Reomved the code change in ufshcd_shutdown() since it is not quite relevant with this fix Change since v4: - Updated some comment lines as requested by Stanley Change since v3: - Slightly updated the 1st change Change since v2: - Split the 1st change to two changes, which become the 1st change and the 3rd change Change since v1: - Updated the 2nd change *** BLURB HERE *** Can Guo (3): scsi: ufs: Protect some contexts from unexpected clock scaling scsi: ufs: Refactor ufshcd_init/exit_clk_scaling/gating() scsi: ufs: Revert "Make sure clk scaling happens only when HBA is runtime ACTIVE" drivers/scsi/ufs/ufshcd.c | 214 +++++++++++++++++++++++++--------------------- drivers/scsi/ufs/ufshcd.h | 10 ++- 2 files changed, 127 insertions(+), 97 deletions(-)