| Message ID | 1624433711-9339-2-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v4,01/10] scsi: ufs: Rename flags pm_op_in_progress and is_sys_suspended | expand |
On 6/23/21 12:35 AM, Can Guo wrote: > Rename pm_op_in_progress and is_sys_suspended to wlu_pm_op_in_progress and > is_wlu_sys_suspended accordingly. Hi Can, My understanding is that power management operations must be submitted to one particular UFS WLUN (hba->sdev_ufs_device). That makes the "wlu_" part of the new names redundant. In other words, I like the current names better than the new names. Unless if I missed something, consider dropping this patch. Thanks, Bart.
On Wed 23 Jun 02:35 CDT 2021, Can Guo wrote: > Rename pm_op_in_progress and is_sys_suspended to wlu_pm_op_in_progress and > is_wlu_sys_suspended accordingly. > This reflects what the change does, but the commit message is supposed to capture "why". Regards, Bjorn > Signed-off-by: Can Guo <cang@codeaurora.org> > --- > drivers/scsi/ufs/ufs-qcom.c | 2 +- > drivers/scsi/ufs/ufshcd.c | 30 +++++++++++++++--------------- > drivers/scsi/ufs/ufshcd.h | 6 ++++-- > 3 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c > index 9b1d18d..fbe21e0 100644 > --- a/drivers/scsi/ufs/ufs-qcom.c > +++ b/drivers/scsi/ufs/ufs-qcom.c > @@ -641,7 +641,7 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) > if (err) > return err; > > - hba->is_sys_suspended = false; > + hba->is_wlu_sys_suspended = false; > return 0; > } > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 25fe18a..c40ba1d 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -549,8 +549,8 @@ static void ufshcd_print_host_state(struct ufs_hba *hba) > hba->saved_err, hba->saved_uic_err); > dev_err(hba->dev, "Device power mode=%d, UIC link state=%d\n", > hba->curr_dev_pwr_mode, hba->uic_link_state); > - dev_err(hba->dev, "PM in progress=%d, sys. suspended=%d\n", > - hba->pm_op_in_progress, hba->is_sys_suspended); > + dev_err(hba->dev, "wlu_pm_op_in_progress=%d, is_wlu_sys_suspended=%d\n", > + hba->wlu_pm_op_in_progress, hba->is_wlu_sys_suspended); > dev_err(hba->dev, "Auto BKOPS=%d, Host self-block=%d\n", > hba->auto_bkops_enabled, hba->host->host_self_blocked); > dev_err(hba->dev, "Clk gate=%d\n", hba->clk_gating.state); > @@ -1999,7 +1999,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_enabled || hba->pm_op_in_progress) { > + if (!hba->clk_scaling.is_enabled || hba->wlu_pm_op_in_progress) { > spin_unlock_irqrestore(hba->host->host_lock, flags); > return; > } > @@ -2734,7 +2734,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) > * err handler blocked for too long. So, just fail the scsi cmd > * sent from PM ops, err handler can recover PM error anyways. > */ > - if (hba->pm_op_in_progress) { > + if (hba->wlu_pm_op_in_progress) { > hba->force_reset = true; > set_host_byte(cmd, DID_BAD_TARGET); > cmd->scsi_done(cmd); > @@ -2767,7 +2767,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) > (hba->clk_gating.state != CLKS_ON)); > > if (unlikely(test_bit(tag, &hba->outstanding_reqs))) { > - if (hba->pm_op_in_progress) > + if (hba->wlu_pm_op_in_progress) > set_host_byte(cmd, DID_BAD_TARGET); > else > err = SCSI_MLQUEUE_HOST_BUSY; > @@ -5116,7 +5116,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) > * solution could be to abort the system suspend if > * UFS device needs urgent BKOPs. > */ > - if (!hba->pm_op_in_progress && > + if (!hba->wlu_pm_op_in_progress && > !ufshcd_eh_in_progress(hba) && > ufshcd_is_exception_event(lrbp->ucd_rsp_ptr)) > /* Flushed in suspend */ > @@ -5916,7 +5916,7 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba) > { > ufshcd_rpm_get_sync(hba); > if (pm_runtime_status_suspended(&hba->sdev_ufs_device->sdev_gendev) || > - hba->is_sys_suspended) { > + hba->is_wlu_sys_suspended) { > enum ufs_pm_op pm_op; > > /* > @@ -5933,7 +5933,7 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba) > if (!ufshcd_is_clkgating_allowed(hba)) > ufshcd_setup_clocks(hba, true); > ufshcd_release(hba); > - pm_op = hba->is_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM; > + pm_op = hba->is_wlu_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM; > ufshcd_vops_resume(hba, pm_op); > } else { > ufshcd_hold(hba, false); > @@ -5976,7 +5976,7 @@ static void ufshcd_recover_pm_error(struct ufs_hba *hba) > struct request_queue *q; > int ret; > > - hba->is_sys_suspended = false; > + hba->is_wlu_sys_suspended = false; > /* > * Set RPM status of wlun device to RPM_ACTIVE, > * this also clears its runtime error. > @@ -8784,7 +8784,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > enum ufs_dev_pwr_mode req_dev_pwr_mode; > enum uic_link_state req_link_state; > > - hba->pm_op_in_progress = true; > + hba->wlu_pm_op_in_progress = true; > if (pm_op != UFS_SHUTDOWN_PM) { > pm_lvl = pm_op == UFS_RUNTIME_PM ? > hba->rpm_lvl : hba->spm_lvl; > @@ -8919,7 +8919,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > hba->clk_gating.is_suspended = false; > ufshcd_release(hba); > } > - hba->pm_op_in_progress = false; > + hba->wlu_pm_op_in_progress = false; > return ret; > } > > @@ -8928,7 +8928,7 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) > int ret; > enum uic_link_state old_link_state = hba->uic_link_state; > > - hba->pm_op_in_progress = true; > + hba->wlu_pm_op_in_progress = true; > > /* > * Call vendor specific resume callback. As these callbacks may access > @@ -9006,7 +9006,7 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) > ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret); > hba->clk_gating.is_suspended = false; > ufshcd_release(hba); > - hba->pm_op_in_progress = false; > + hba->wlu_pm_op_in_progress = false; > return ret; > } > > @@ -9072,7 +9072,7 @@ static int ufshcd_wl_suspend(struct device *dev) > > out: > if (!ret) > - hba->is_sys_suspended = true; > + hba->is_wlu_sys_suspended = true; > trace_ufshcd_wl_suspend(dev_name(dev), ret, > ktime_to_us(ktime_sub(ktime_get(), start)), > hba->curr_dev_pwr_mode, hba->uic_link_state); > @@ -9100,7 +9100,7 @@ static int ufshcd_wl_resume(struct device *dev) > ktime_to_us(ktime_sub(ktime_get(), start)), > hba->curr_dev_pwr_mode, hba->uic_link_state); > if (!ret) > - hba->is_sys_suspended = false; > + hba->is_wlu_sys_suspended = false; > up(&hba->host_sem); > return ret; > } > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index c98d540..93aeeb3 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -752,7 +752,8 @@ struct ufs_hba { > enum ufs_pm_level spm_lvl; > struct device_attribute rpm_lvl_attr; > struct device_attribute spm_lvl_attr; > - int pm_op_in_progress; > + /* A flag to tell whether __ufshcd_wl_suspend/resume() is in progress */ > + bool wlu_pm_op_in_progress; > > /* Auto-Hibernate Idle Timer register value */ > u32 ahit; > @@ -838,7 +839,8 @@ struct ufs_hba { > > struct devfreq *devfreq; > struct ufs_clk_scaling clk_scaling; > - bool is_sys_suspended; > + /* A flag to tell whether the UFS device W-LU is system suspended */ > + bool is_wlu_sys_suspended; > > enum bkops_status urgent_bkops_lvl; > bool is_urgent_bkops_lvl_checked; > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. >
On 6/23/21 1:05 PM, Bart Van Assche wrote: > On 6/23/21 12:35 AM, Can Guo wrote: >> Rename pm_op_in_progress and is_sys_suspended to wlu_pm_op_in_progress and >> is_wlu_sys_suspended accordingly. > > My understanding is that power management operations must be submitted > to one particular UFS WLUN (hba->sdev_ufs_device). That makes the "wlu_" > part of the new names redundant. In other words, I like the current > names better than the new names. Unless if I missed something, consider > dropping this patch. Hi Can, Reviewing later patches in this series made me realize that there are two families of suspend/resume functions. One family of functions operates at the platform level while the other family operates at the SCSI LUN level. My comments about the suspend/resume functions are as follows: - It seems redundant to me to have system suspend support at the SCSI LUN level (__ufshcd_wl_suspend(hba, UFS_SYSTEM_PM)) and also at the platform level. Since the platform device is a parent of the SCSI WLUN, can system suspend/resume support be left out from ufshcd_wl_pm_ops (or in other words, remove the .freeze and .thaw callbacks)? Do we really need two calls from the power management subsystem into the UFS driver for every system suspend and every system resume? - Because of the device links (device_link_add()), the ufschd_wl_*() RPM callbacks are invoked after all LUNs have been suspended. I would appreciate it if the "ufshcd_wl_" prefix would be changed into "ufshcd_lun_" since that would make it more clear that these callbacks are associated with all LUNs and not only with the WLUN through which power management commands are submitted. Thanks, Bart.
On 6/23/21 1:42 PM, Bjorn Andersson wrote: > On Wed 23 Jun 02:35 CDT 2021, Can Guo wrote: >> Rename pm_op_in_progress and is_sys_suspended to wlu_pm_op_in_progress and >> is_wlu_sys_suspended accordingly. > > This reflects what the change does, but the commit message is supposed > to capture "why". What's even better is to describe both: what has been changed and also why a change has been made. See also https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes. Thanks, Bart.
Hi Bart, On 2021-06-24 04:57, Bart Van Assche wrote: > On 6/23/21 1:05 PM, Bart Van Assche wrote: >> On 6/23/21 12:35 AM, Can Guo wrote: >>> Rename pm_op_in_progress and is_sys_suspended to >>> wlu_pm_op_in_progress and >>> is_wlu_sys_suspended accordingly. >> >> My understanding is that power management operations must be submitted >> to one particular UFS WLUN (hba->sdev_ufs_device). That makes the >> "wlu_" >> part of the new names redundant. In other words, I like the current >> names better than the new names. Unless if I missed something, >> consider >> dropping this patch. > > Hi Can, > > Reviewing later patches in this series made me realize that there are > two families of suspend/resume functions. One family of functions > operates at the platform level while the other family operates at the > SCSI LUN level. My comments about the suspend/resume functions are as > follows: > - It seems redundant to me to have system suspend support at the SCSI > LUN level (__ufshcd_wl_suspend(hba, UFS_SYSTEM_PM)) and also at the > platform level. Since the platform device is a parent of the SCSI > WLUN, can system suspend/resume support be left out from > ufshcd_wl_pm_ops (or in other words, remove the .freeze and .thaw > callbacks)? Do we really need two calls from the power management > subsystem into the UFS driver for every system suspend and every > system resume? Asutosh and Adrian should be the right persons to answer this, since they've been working together on that huge change for 4 months - https://lore.kernel.org/patchwork/patch/1417696/ In short, we need a dedicated suspend/resume ops for the UFS device W-LU because one cannot send requests (not even PM requests) after the device is runtime suspended - sending SSU cmds in hba suspend/resume cannot pass through blk_queue_enter() as SSU cmd is sent to the UFS device W-LU scsi device (by now it is runtime suspended) but not the hba device. Of course we can keep the old way and send the SSU cmd through a request queue without block layer PM initialized (hba->cmd_queue for example, by pointing cmd_queue->dev to the UFS device W-LU scsi device), but that would look like a hack. > - Because of the device links (device_link_add()), the ufschd_wl_*() > RPM callbacks are invoked after all LUNs have been suspended. I would > appreciate it if the "ufshcd_wl_" prefix would be changed into > "ufshcd_lun_" since that would make it more clear that these > callbacks > are associated with all LUNs and not only with the WLUN through which > power management commands are submitted. > Sure, we will do that later. Thanks, Can Guo. > Thanks, > > Bart.
Hi Bjorn, On 2021-06-24 04:42, Bjorn Andersson wrote: > On Wed 23 Jun 02:35 CDT 2021, Can Guo wrote: > >> Rename pm_op_in_progress and is_sys_suspended to wlu_pm_op_in_progress >> and >> is_wlu_sys_suspended accordingly. >> > > This reflects what the change does, but the commit message is supposed > to capture "why". > Sure, I will add the "why" in next ver. Thanks, Can Guo. > Regards, > Bjorn > >> Signed-off-by: Can Guo <cang@codeaurora.org> >> --- >> drivers/scsi/ufs/ufs-qcom.c | 2 +- >> drivers/scsi/ufs/ufshcd.c | 30 +++++++++++++++--------------- >> drivers/scsi/ufs/ufshcd.h | 6 ++++-- >> 3 files changed, 20 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c >> index 9b1d18d..fbe21e0 100644 >> --- a/drivers/scsi/ufs/ufs-qcom.c >> +++ b/drivers/scsi/ufs/ufs-qcom.c >> @@ -641,7 +641,7 @@ static int ufs_qcom_resume(struct ufs_hba *hba, >> enum ufs_pm_op pm_op) >> if (err) >> return err; >> >> - hba->is_sys_suspended = false; >> + hba->is_wlu_sys_suspended = false; >> return 0; >> } >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index 25fe18a..c40ba1d 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -549,8 +549,8 @@ static void ufshcd_print_host_state(struct ufs_hba >> *hba) >> hba->saved_err, hba->saved_uic_err); >> dev_err(hba->dev, "Device power mode=%d, UIC link state=%d\n", >> hba->curr_dev_pwr_mode, hba->uic_link_state); >> - dev_err(hba->dev, "PM in progress=%d, sys. suspended=%d\n", >> - hba->pm_op_in_progress, hba->is_sys_suspended); >> + dev_err(hba->dev, "wlu_pm_op_in_progress=%d, >> is_wlu_sys_suspended=%d\n", >> + hba->wlu_pm_op_in_progress, hba->is_wlu_sys_suspended); >> dev_err(hba->dev, "Auto BKOPS=%d, Host self-block=%d\n", >> hba->auto_bkops_enabled, hba->host->host_self_blocked); >> dev_err(hba->dev, "Clk gate=%d\n", hba->clk_gating.state); >> @@ -1999,7 +1999,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_enabled || hba->pm_op_in_progress) { >> + if (!hba->clk_scaling.is_enabled || hba->wlu_pm_op_in_progress) { >> spin_unlock_irqrestore(hba->host->host_lock, flags); >> return; >> } >> @@ -2734,7 +2734,7 @@ static int ufshcd_queuecommand(struct Scsi_Host >> *host, struct scsi_cmnd *cmd) >> * err handler blocked for too long. So, just fail the scsi cmd >> * sent from PM ops, err handler can recover PM error anyways. >> */ >> - if (hba->pm_op_in_progress) { >> + if (hba->wlu_pm_op_in_progress) { >> hba->force_reset = true; >> set_host_byte(cmd, DID_BAD_TARGET); >> cmd->scsi_done(cmd); >> @@ -2767,7 +2767,7 @@ static int ufshcd_queuecommand(struct Scsi_Host >> *host, struct scsi_cmnd *cmd) >> (hba->clk_gating.state != CLKS_ON)); >> >> if (unlikely(test_bit(tag, &hba->outstanding_reqs))) { >> - if (hba->pm_op_in_progress) >> + if (hba->wlu_pm_op_in_progress) >> set_host_byte(cmd, DID_BAD_TARGET); >> else >> err = SCSI_MLQUEUE_HOST_BUSY; >> @@ -5116,7 +5116,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, >> struct ufshcd_lrb *lrbp) >> * solution could be to abort the system suspend if >> * UFS device needs urgent BKOPs. >> */ >> - if (!hba->pm_op_in_progress && >> + if (!hba->wlu_pm_op_in_progress && >> !ufshcd_eh_in_progress(hba) && >> ufshcd_is_exception_event(lrbp->ucd_rsp_ptr)) >> /* Flushed in suspend */ >> @@ -5916,7 +5916,7 @@ static void ufshcd_err_handling_prepare(struct >> ufs_hba *hba) >> { >> ufshcd_rpm_get_sync(hba); >> if (pm_runtime_status_suspended(&hba->sdev_ufs_device->sdev_gendev) >> || >> - hba->is_sys_suspended) { >> + hba->is_wlu_sys_suspended) { >> enum ufs_pm_op pm_op; >> >> /* >> @@ -5933,7 +5933,7 @@ static void ufshcd_err_handling_prepare(struct >> ufs_hba *hba) >> if (!ufshcd_is_clkgating_allowed(hba)) >> ufshcd_setup_clocks(hba, true); >> ufshcd_release(hba); >> - pm_op = hba->is_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM; >> + pm_op = hba->is_wlu_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM; >> ufshcd_vops_resume(hba, pm_op); >> } else { >> ufshcd_hold(hba, false); >> @@ -5976,7 +5976,7 @@ static void ufshcd_recover_pm_error(struct >> ufs_hba *hba) >> struct request_queue *q; >> int ret; >> >> - hba->is_sys_suspended = false; >> + hba->is_wlu_sys_suspended = false; >> /* >> * Set RPM status of wlun device to RPM_ACTIVE, >> * this also clears its runtime error. >> @@ -8784,7 +8784,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba >> *hba, enum ufs_pm_op pm_op) >> enum ufs_dev_pwr_mode req_dev_pwr_mode; >> enum uic_link_state req_link_state; >> >> - hba->pm_op_in_progress = true; >> + hba->wlu_pm_op_in_progress = true; >> if (pm_op != UFS_SHUTDOWN_PM) { >> pm_lvl = pm_op == UFS_RUNTIME_PM ? >> hba->rpm_lvl : hba->spm_lvl; >> @@ -8919,7 +8919,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba >> *hba, enum ufs_pm_op pm_op) >> hba->clk_gating.is_suspended = false; >> ufshcd_release(hba); >> } >> - hba->pm_op_in_progress = false; >> + hba->wlu_pm_op_in_progress = false; >> return ret; >> } >> >> @@ -8928,7 +8928,7 @@ static int __ufshcd_wl_resume(struct ufs_hba >> *hba, enum ufs_pm_op pm_op) >> int ret; >> enum uic_link_state old_link_state = hba->uic_link_state; >> >> - hba->pm_op_in_progress = true; >> + hba->wlu_pm_op_in_progress = true; >> >> /* >> * Call vendor specific resume callback. As these callbacks may >> access >> @@ -9006,7 +9006,7 @@ static int __ufshcd_wl_resume(struct ufs_hba >> *hba, enum ufs_pm_op pm_op) >> ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret); >> hba->clk_gating.is_suspended = false; >> ufshcd_release(hba); >> - hba->pm_op_in_progress = false; >> + hba->wlu_pm_op_in_progress = false; >> return ret; >> } >> >> @@ -9072,7 +9072,7 @@ static int ufshcd_wl_suspend(struct device *dev) >> >> out: >> if (!ret) >> - hba->is_sys_suspended = true; >> + hba->is_wlu_sys_suspended = true; >> trace_ufshcd_wl_suspend(dev_name(dev), ret, >> ktime_to_us(ktime_sub(ktime_get(), start)), >> hba->curr_dev_pwr_mode, hba->uic_link_state); >> @@ -9100,7 +9100,7 @@ static int ufshcd_wl_resume(struct device *dev) >> ktime_to_us(ktime_sub(ktime_get(), start)), >> hba->curr_dev_pwr_mode, hba->uic_link_state); >> if (!ret) >> - hba->is_sys_suspended = false; >> + hba->is_wlu_sys_suspended = false; >> up(&hba->host_sem); >> return ret; >> } >> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >> index c98d540..93aeeb3 100644 >> --- a/drivers/scsi/ufs/ufshcd.h >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -752,7 +752,8 @@ struct ufs_hba { >> enum ufs_pm_level spm_lvl; >> struct device_attribute rpm_lvl_attr; >> struct device_attribute spm_lvl_attr; >> - int pm_op_in_progress; >> + /* A flag to tell whether __ufshcd_wl_suspend/resume() is in >> progress */ >> + bool wlu_pm_op_in_progress; >> >> /* Auto-Hibernate Idle Timer register value */ >> u32 ahit; >> @@ -838,7 +839,8 @@ struct ufs_hba { >> >> struct devfreq *devfreq; >> struct ufs_clk_scaling clk_scaling; >> - bool is_sys_suspended; >> + /* A flag to tell whether the UFS device W-LU is system suspended */ >> + bool is_wlu_sys_suspended; >> >> enum bkops_status urgent_bkops_lvl; >> bool is_urgent_bkops_lvl_checked; >> -- >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a >> Linux Foundation Collaborative Project. >>
Typo fixed. On 2021-06-24 10:02, Can Guo wrote: > Hi Bart, > > On 2021-06-24 04:57, Bart Van Assche wrote: >> On 6/23/21 1:05 PM, Bart Van Assche wrote: >>> On 6/23/21 12:35 AM, Can Guo wrote: >>>> Rename pm_op_in_progress and is_sys_suspended to >>>> wlu_pm_op_in_progress and >>>> is_wlu_sys_suspended accordingly. >>> >>> My understanding is that power management operations must be >>> submitted >>> to one particular UFS WLUN (hba->sdev_ufs_device). That makes the >>> "wlu_" >>> part of the new names redundant. In other words, I like the current >>> names better than the new names. Unless if I missed something, >>> consider >>> dropping this patch. >> >> Hi Can, >> >> Reviewing later patches in this series made me realize that there are >> two families of suspend/resume functions. One family of functions >> operates at the platform level while the other family operates at the >> SCSI LUN level. My comments about the suspend/resume functions are as >> follows: >> - It seems redundant to me to have system suspend support at the SCSI >> LUN level (__ufshcd_wl_suspend(hba, UFS_SYSTEM_PM)) and also at the >> platform level. Since the platform device is a parent of the SCSI >> WLUN, can system suspend/resume support be left out from >> ufshcd_wl_pm_ops (or in other words, remove the .freeze and .thaw >> callbacks)? Do we really need two calls from the power management >> subsystem into the UFS driver for every system suspend and every >> system resume? > > Asutosh and Adrian should be the right persons to answer this, since > they've been working together on that huge change for 4 months - > > https://lore.kernel.org/patchwork/patch/1417696/ > > In short, we need a dedicated suspend/resume ops for the UFS device > W-LU because one cannot send requests (not even PM requests) after the > device is runtime suspended - sending SSU cmds in hba suspend/resume > cannot pass through blk_queue_enter() as SSU cmd is sent to the UFS > device W-LU scsi device (by now it is runtime suspended) but not the > hba device. > > Of course we can keep the old way and send the SSU cmd through a > request queue without block layer PM initialized (hba->cmd_queue for > example, by pointing cmd_queue->queuedata to the UFS device W-LU scsi > device), but that would look like a hack. > >> - Because of the device links (device_link_add()), the ufschd_wl_*() >> RPM callbacks are invoked after all LUNs have been suspended. I >> would >> appreciate it if the "ufshcd_wl_" prefix would be changed into >> "ufshcd_lun_" since that would make it more clear that these >> callbacks >> are associated with all LUNs and not only with the WLUN through >> which >> power management commands are submitted. >> > > Sure, we will do that later. > > Thanks, > > Can Guo. > >> Thanks, >> >> Bart.
On 24/06/21 5:02 am, Can Guo wrote: > Hi Bart, > > On 2021-06-24 04:57, Bart Van Assche wrote: >> On 6/23/21 1:05 PM, Bart Van Assche wrote: >>> On 6/23/21 12:35 AM, Can Guo wrote: >>>> Rename pm_op_in_progress and is_sys_suspended to wlu_pm_op_in_progress and >>>> is_wlu_sys_suspended accordingly. >>> >>> My understanding is that power management operations must be submitted >>> to one particular UFS WLUN (hba->sdev_ufs_device). That makes the "wlu_" >>> part of the new names redundant. In other words, I like the current >>> names better than the new names. Unless if I missed something, consider >>> dropping this patch. >> >> Hi Can, >> >> Reviewing later patches in this series made me realize that there are >> two families of suspend/resume functions. One family of functions >> operates at the platform level while the other family operates at the >> SCSI LUN level. My comments about the suspend/resume functions are as >> follows: >> - It seems redundant to me to have system suspend support at the SCSI >> LUN level (__ufshcd_wl_suspend(hba, UFS_SYSTEM_PM)) and also at the >> platform level. Since the platform device is a parent of the SCSI >> WLUN, can system suspend/resume support be left out from >> ufshcd_wl_pm_ops (or in other words, remove the .freeze and .thaw >> callbacks)? Do we really need two calls from the power management >> subsystem into the UFS driver for every system suspend and every >> system resume? > > Asutosh and Adrian should be the right persons to answer this, since > they've been working together on that huge change for 4 months - > > https://lore.kernel.org/patchwork/patch/1417696/ > > In short, we need a dedicated suspend/resume ops for the UFS device > W-LU because one cannot send requests (not even PM requests) after the > device is runtime suspended - sending SSU cmds in hba suspend/resume > cannot pass through blk_queue_enter() as SSU cmd is sent to the UFS > device W-LU scsi device (by now it is runtime suspended) but not the > hba device. Yes it is quite normal to have different PM callbacks for different devices (e.g. host controller and device controlled by host controller), and SCSI effectively expects it that way now. .freeze and .thaw are necessary for suspend-to-disk > > Of course we can keep the old way and send the SSU cmd through a > request queue without block layer PM initialized (hba->cmd_queue for > example, by pointing cmd_queue->dev to the UFS device W-LU scsi device), > but that would look like a hack. > >> - Because of the device links (device_link_add()), the ufschd_wl_*() >> RPM callbacks are invoked after all LUNs have been suspended. I would >> appreciate it if the "ufshcd_wl_" prefix would be changed into >> "ufshcd_lun_" since that would make it more clear that these callbacks >> are associated with all LUNs and not only with the WLUN through which >> power management commands are submitted. >> > > Sure, we will do that later. > > Thanks, > > Can Guo. > >> Thanks, >> >> Bart.
On 6/23/21 12:35 AM, Can Guo wrote: > Rename pm_op_in_progress and is_sys_suspended to wlu_pm_op_in_progress and > is_wlu_sys_suspended accordingly. Can the pm_op_in_progress variable be removed if the UFS driver checks whether q->rpm_status == RPM_SUSPENDING || q->rpm_status == RPM_RESUMING instead of using pm_op_in_progress? The fewer state variables we maintain the lower the chance that these are inconsistent or incorrect. See also block/blk-pm.c for the code that sets q->rpm_status. Thanks, Bart.
On 6/23/21 12:35 AM, Can Guo wrote: > Rename pm_op_in_progress and is_sys_suspended to wlu_pm_op_in_progress and > is_wlu_sys_suspended accordingly. Can the is_wlu_sys_suspended member variable be removed by checking dev->power.is_suspended where dev represents the WLUN? Thanks, Bart.
On 2021-06-25 07:42, Bart Van Assche wrote: > On 6/23/21 12:35 AM, Can Guo wrote: >> Rename pm_op_in_progress and is_sys_suspended to wlu_pm_op_in_progress >> and >> is_wlu_sys_suspended accordingly. > > Can the is_wlu_sys_suspended member variable be removed by checking > dev->power.is_suspended where dev represents the WLUN? > No, PM set dev->power.is_suspended to "false" even the device failed resuming, while is_wlu_sys_suspended can be used to tell that. Thanks, Can Guo. > Thanks, > > Bart.
On 2021-06-28 15:01, Can Guo wrote: > On 2021-06-25 07:42, Bart Van Assche wrote: >> On 6/23/21 12:35 AM, Can Guo wrote: >>> Rename pm_op_in_progress and is_sys_suspended to >>> wlu_pm_op_in_progress and >>> is_wlu_sys_suspended accordingly. >> >> Can the is_wlu_sys_suspended member variable be removed by checking >> dev->power.is_suspended where dev represents the WLUN? >> > > No, PM set dev->power.is_suspended to "false" even the device failed > resuming, > while is_wlu_sys_suspended can be used to tell that. FYI, 892 /** 893 * device_resume - Execute "resume" callbacks for given device. 894 * @dev: Device to handle. 895 * @state: PM transition of the system being carried out. 896 * @async: If true, the device is being resumed asynchronously. 897 */ 898 static int device_resume(struct device *dev, pm_message_t state, bool async) ... 967 End: 968 error = dpm_run_callback(callback, dev, state, info); 969 dev->power.is_suspended = false; ... Can Guo. > > Thanks, > > Can Guo. > >> Thanks, >> >> Bart.
On 6/28/21 12:01 AM, Can Guo wrote: > On 2021-06-25 07:42, Bart Van Assche wrote: >> On 6/23/21 12:35 AM, Can Guo wrote: >>> Rename pm_op_in_progress and is_sys_suspended to >>> wlu_pm_op_in_progress and >>> is_wlu_sys_suspended accordingly. >> >> Can the is_wlu_sys_suspended member variable be removed by checking >> dev->power.is_suspended where dev represents the WLUN? >> > > No, PM set dev->power.is_suspended to "false" even the device failed > resuming, > while is_wlu_sys_suspended can be used to tell that. (+Rafael) Hi Rafael, In drivers/base/power/main.c we found the following code: End: error = dpm_run_callback(callback, dev, state, info); dev->power.is_suspended = false; Is it a bug or a feature that dev->power.is_suspended is set to false if dpm_run_callback() fails? I'm asking this because only clearing dev->power.is_suspended if dpm_run_callback() returns 0 would allow to simplify the UFS driver. It can happen for UFS devices that runtime resume fails and if this fails we need to track this. Thanks, Bart.
diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c index 9b1d18d..fbe21e0 100644 --- a/drivers/scsi/ufs/ufs-qcom.c +++ b/drivers/scsi/ufs/ufs-qcom.c @@ -641,7 +641,7 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) if (err) return err; - hba->is_sys_suspended = false; + hba->is_wlu_sys_suspended = false; return 0; } diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 25fe18a..c40ba1d 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -549,8 +549,8 @@ static void ufshcd_print_host_state(struct ufs_hba *hba) hba->saved_err, hba->saved_uic_err); dev_err(hba->dev, "Device power mode=%d, UIC link state=%d\n", hba->curr_dev_pwr_mode, hba->uic_link_state); - dev_err(hba->dev, "PM in progress=%d, sys. suspended=%d\n", - hba->pm_op_in_progress, hba->is_sys_suspended); + dev_err(hba->dev, "wlu_pm_op_in_progress=%d, is_wlu_sys_suspended=%d\n", + hba->wlu_pm_op_in_progress, hba->is_wlu_sys_suspended); dev_err(hba->dev, "Auto BKOPS=%d, Host self-block=%d\n", hba->auto_bkops_enabled, hba->host->host_self_blocked); dev_err(hba->dev, "Clk gate=%d\n", hba->clk_gating.state); @@ -1999,7 +1999,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_enabled || hba->pm_op_in_progress) { + if (!hba->clk_scaling.is_enabled || hba->wlu_pm_op_in_progress) { spin_unlock_irqrestore(hba->host->host_lock, flags); return; } @@ -2734,7 +2734,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) * err handler blocked for too long. So, just fail the scsi cmd * sent from PM ops, err handler can recover PM error anyways. */ - if (hba->pm_op_in_progress) { + if (hba->wlu_pm_op_in_progress) { hba->force_reset = true; set_host_byte(cmd, DID_BAD_TARGET); cmd->scsi_done(cmd); @@ -2767,7 +2767,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) (hba->clk_gating.state != CLKS_ON)); if (unlikely(test_bit(tag, &hba->outstanding_reqs))) { - if (hba->pm_op_in_progress) + if (hba->wlu_pm_op_in_progress) set_host_byte(cmd, DID_BAD_TARGET); else err = SCSI_MLQUEUE_HOST_BUSY; @@ -5116,7 +5116,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) * solution could be to abort the system suspend if * UFS device needs urgent BKOPs. */ - if (!hba->pm_op_in_progress && + if (!hba->wlu_pm_op_in_progress && !ufshcd_eh_in_progress(hba) && ufshcd_is_exception_event(lrbp->ucd_rsp_ptr)) /* Flushed in suspend */ @@ -5916,7 +5916,7 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba) { ufshcd_rpm_get_sync(hba); if (pm_runtime_status_suspended(&hba->sdev_ufs_device->sdev_gendev) || - hba->is_sys_suspended) { + hba->is_wlu_sys_suspended) { enum ufs_pm_op pm_op; /* @@ -5933,7 +5933,7 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba) if (!ufshcd_is_clkgating_allowed(hba)) ufshcd_setup_clocks(hba, true); ufshcd_release(hba); - pm_op = hba->is_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM; + pm_op = hba->is_wlu_sys_suspended ? UFS_SYSTEM_PM : UFS_RUNTIME_PM; ufshcd_vops_resume(hba, pm_op); } else { ufshcd_hold(hba, false); @@ -5976,7 +5976,7 @@ static void ufshcd_recover_pm_error(struct ufs_hba *hba) struct request_queue *q; int ret; - hba->is_sys_suspended = false; + hba->is_wlu_sys_suspended = false; /* * Set RPM status of wlun device to RPM_ACTIVE, * this also clears its runtime error. @@ -8784,7 +8784,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) enum ufs_dev_pwr_mode req_dev_pwr_mode; enum uic_link_state req_link_state; - hba->pm_op_in_progress = true; + hba->wlu_pm_op_in_progress = true; if (pm_op != UFS_SHUTDOWN_PM) { pm_lvl = pm_op == UFS_RUNTIME_PM ? hba->rpm_lvl : hba->spm_lvl; @@ -8919,7 +8919,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) hba->clk_gating.is_suspended = false; ufshcd_release(hba); } - hba->pm_op_in_progress = false; + hba->wlu_pm_op_in_progress = false; return ret; } @@ -8928,7 +8928,7 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) int ret; enum uic_link_state old_link_state = hba->uic_link_state; - hba->pm_op_in_progress = true; + hba->wlu_pm_op_in_progress = true; /* * Call vendor specific resume callback. As these callbacks may access @@ -9006,7 +9006,7 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret); hba->clk_gating.is_suspended = false; ufshcd_release(hba); - hba->pm_op_in_progress = false; + hba->wlu_pm_op_in_progress = false; return ret; } @@ -9072,7 +9072,7 @@ static int ufshcd_wl_suspend(struct device *dev) out: if (!ret) - hba->is_sys_suspended = true; + hba->is_wlu_sys_suspended = true; trace_ufshcd_wl_suspend(dev_name(dev), ret, ktime_to_us(ktime_sub(ktime_get(), start)), hba->curr_dev_pwr_mode, hba->uic_link_state); @@ -9100,7 +9100,7 @@ static int ufshcd_wl_resume(struct device *dev) ktime_to_us(ktime_sub(ktime_get(), start)), hba->curr_dev_pwr_mode, hba->uic_link_state); if (!ret) - hba->is_sys_suspended = false; + hba->is_wlu_sys_suspended = false; up(&hba->host_sem); return ret; } diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index c98d540..93aeeb3 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -752,7 +752,8 @@ struct ufs_hba { enum ufs_pm_level spm_lvl; struct device_attribute rpm_lvl_attr; struct device_attribute spm_lvl_attr; - int pm_op_in_progress; + /* A flag to tell whether __ufshcd_wl_suspend/resume() is in progress */ + bool wlu_pm_op_in_progress; /* Auto-Hibernate Idle Timer register value */ u32 ahit; @@ -838,7 +839,8 @@ struct ufs_hba { struct devfreq *devfreq; struct ufs_clk_scaling clk_scaling; - bool is_sys_suspended; + /* A flag to tell whether the UFS device W-LU is system suspended */ + bool is_wlu_sys_suspended; enum bkops_status urgent_bkops_lvl; bool is_urgent_bkops_lvl_checked;
Rename pm_op_in_progress and is_sys_suspended to wlu_pm_op_in_progress and is_wlu_sys_suspended accordingly. Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/ufs/ufs-qcom.c | 2 +- drivers/scsi/ufs/ufshcd.c | 30 +++++++++++++++--------------- drivers/scsi/ufs/ufshcd.h | 6 ++++-- 3 files changed, 20 insertions(+), 18 deletions(-)