| Message ID | 1624433711-9339-3-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Complementary changes for error handling | expand |
On 23/06/21 10:35 am, Can Guo wrote: > Add flags pm_op_in_progress and is_sys_suspended to track the status of hba > runtime and system suspend/resume operations. > > Signed-off-by: Can Guo <cang@codeaurora.org> > --- > drivers/scsi/ufs/ufshcd.c | 12 +++++++++++- > drivers/scsi/ufs/ufshcd.h | 4 ++++ > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index c40ba1d..abe5f2d 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -551,6 +551,8 @@ static void ufshcd_print_host_state(struct ufs_hba *hba) > hba->curr_dev_pwr_mode, hba->uic_link_state); > 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, "pm_op_in_progress=%d, is_sys_suspended=%d\n", > + hba->pm_op_in_progress, hba->is_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); > @@ -9141,6 +9143,8 @@ static int ufshcd_suspend(struct ufs_hba *hba) > > if (!hba->is_powered) > return 0; > + > + hba->pm_op_in_progress = true; > /* > * Disable the host irq as host controller as there won't be any > * host controller transaction expected till resume. */ ufshcd_disable_irq(hba); ret = ufshcd_setup_clocks(hba, false); if (ret) { ufshcd_enable_irq(hba); Is "hba->pm_op_in_progress = false" needed here? return ret; } > @@ -9160,6 +9164,7 @@ static int ufshcd_suspend(struct ufs_hba *hba) > ufshcd_vreg_set_lpm(hba); > /* Put the host controller in low power mode if possible */ > ufshcd_hba_vreg_set_lpm(hba); > + hba->pm_op_in_progress = false; > return ret; > } > > @@ -9179,6 +9184,7 @@ static int ufshcd_resume(struct ufs_hba *hba) > if (!hba->is_powered) > return 0; > > + hba->pm_op_in_progress = true; > ufshcd_hba_vreg_set_hpm(hba); > ret = ufshcd_vreg_set_hpm(hba); > if (ret) > @@ -9198,6 +9204,7 @@ static int ufshcd_resume(struct ufs_hba *hba) > out: > if (ret) > ufshcd_update_evt_hist(hba, UFS_EVT_RESUME_ERR, (u32)ret); > + hba->pm_op_in_progress = false; > return ret; > } > > @@ -9222,6 +9229,8 @@ int ufshcd_system_suspend(struct ufs_hba *hba) > trace_ufshcd_system_suspend(dev_name(hba->dev), ret, > ktime_to_us(ktime_sub(ktime_get(), start)), > hba->curr_dev_pwr_mode, hba->uic_link_state); > + if (!ret) > + hba->is_sys_suspended = true; > return ret; > } > EXPORT_SYMBOL(ufshcd_system_suspend); > @@ -9247,7 +9256,8 @@ int ufshcd_system_resume(struct ufs_hba *hba) > trace_ufshcd_system_resume(dev_name(hba->dev), ret, > ktime_to_us(ktime_sub(ktime_get(), start)), > hba->curr_dev_pwr_mode, hba->uic_link_state); > - > + if (!ret) > + hba->is_sys_suspended = false; > return ret; > } > EXPORT_SYMBOL(ufshcd_system_resume); > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index 93aeeb3..1e7fe73 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -754,6 +754,8 @@ struct ufs_hba { > struct device_attribute spm_lvl_attr; > /* A flag to tell whether __ufshcd_wl_suspend/resume() is in progress */ > bool wlu_pm_op_in_progress; > + /* A flag to tell whether ufshcd_suspend/resume() is in progress */ > + bool pm_op_in_progress; > > /* Auto-Hibernate Idle Timer register value */ > u32 ahit; > @@ -841,6 +843,8 @@ struct ufs_hba { > struct ufs_clk_scaling clk_scaling; > /* A flag to tell whether the UFS device W-LU is system suspended */ > bool is_wlu_sys_suspended; > + /* A flag to tell whether hba is system suspended */ > + bool is_sys_suspended; > > enum bkops_status urgent_bkops_lvl; > bool is_urgent_bkops_lvl_checked; >
On 6/23/21 12:35 AM, Can Guo wrote: > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index 93aeeb3..1e7fe73 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -754,6 +754,8 @@ struct ufs_hba { > struct device_attribute spm_lvl_attr; > /* A flag to tell whether __ufshcd_wl_suspend/resume() is in progress */ > bool wlu_pm_op_in_progress; > + /* A flag to tell whether ufshcd_suspend/resume() is in progress */ > + bool pm_op_in_progress; > > /* Auto-Hibernate Idle Timer register value */ > u32 ahit; > @@ -841,6 +843,8 @@ struct ufs_hba { > struct ufs_clk_scaling clk_scaling; > /* A flag to tell whether the UFS device W-LU is system suspended */ > bool is_wlu_sys_suspended; > + /* A flag to tell whether hba is system suspended */ > + bool is_sys_suspended; > > enum bkops_status urgent_bkops_lvl; > bool is_urgent_bkops_lvl_checked; It is not yet clear to me whether we really need these new member variables. If these are retained, please rename pm_op_in_progress into platform_pm_op_in_progress and is_sys_suspended into platform_is_sys_suspended. Thanks, Bart.
Hi Adrian, On 2021-06-23 20:33, Adrian Hunter wrote: > On 23/06/21 10:35 am, Can Guo wrote: >> Add flags pm_op_in_progress and is_sys_suspended to track the status >> of hba >> runtime and system suspend/resume operations. >> >> Signed-off-by: Can Guo <cang@codeaurora.org> >> --- >> drivers/scsi/ufs/ufshcd.c | 12 +++++++++++- >> drivers/scsi/ufs/ufshcd.h | 4 ++++ >> 2 files changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index c40ba1d..abe5f2d 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -551,6 +551,8 @@ static void ufshcd_print_host_state(struct ufs_hba >> *hba) >> hba->curr_dev_pwr_mode, hba->uic_link_state); >> 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, "pm_op_in_progress=%d, is_sys_suspended=%d\n", >> + hba->pm_op_in_progress, hba->is_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); >> @@ -9141,6 +9143,8 @@ static int ufshcd_suspend(struct ufs_hba *hba) >> >> if (!hba->is_powered) >> return 0; >> + >> + hba->pm_op_in_progress = true; >> /* >> * Disable the host irq as host controller as there won't be any >> * host controller transaction expected till resume. > */ > ufshcd_disable_irq(hba); > ret = ufshcd_setup_clocks(hba, false); > if (ret) { > ufshcd_enable_irq(hba); > > Is "hba->pm_op_in_progress = false" needed here? > > return ret; > } > > You are right, I missed it. Will add it in next ver. Thanks! Regards, Can Guo. > >> @@ -9160,6 +9164,7 @@ static int ufshcd_suspend(struct ufs_hba *hba) >> ufshcd_vreg_set_lpm(hba); >> /* Put the host controller in low power mode if possible */ >> ufshcd_hba_vreg_set_lpm(hba); >> + hba->pm_op_in_progress = false; >> return ret; >> } >> >> @@ -9179,6 +9184,7 @@ static int ufshcd_resume(struct ufs_hba *hba) >> if (!hba->is_powered) >> return 0; >> >> + hba->pm_op_in_progress = true; >> ufshcd_hba_vreg_set_hpm(hba); >> ret = ufshcd_vreg_set_hpm(hba); >> if (ret) >> @@ -9198,6 +9204,7 @@ static int ufshcd_resume(struct ufs_hba *hba) >> out: >> if (ret) >> ufshcd_update_evt_hist(hba, UFS_EVT_RESUME_ERR, (u32)ret); >> + hba->pm_op_in_progress = false; >> return ret; >> } >> >> @@ -9222,6 +9229,8 @@ int ufshcd_system_suspend(struct ufs_hba *hba) >> trace_ufshcd_system_suspend(dev_name(hba->dev), ret, >> ktime_to_us(ktime_sub(ktime_get(), start)), >> hba->curr_dev_pwr_mode, hba->uic_link_state); >> + if (!ret) >> + hba->is_sys_suspended = true; >> return ret; >> } >> EXPORT_SYMBOL(ufshcd_system_suspend); >> @@ -9247,7 +9256,8 @@ int ufshcd_system_resume(struct ufs_hba *hba) >> trace_ufshcd_system_resume(dev_name(hba->dev), ret, >> ktime_to_us(ktime_sub(ktime_get(), start)), >> hba->curr_dev_pwr_mode, hba->uic_link_state); >> - >> + if (!ret) >> + hba->is_sys_suspended = false; >> return ret; >> } >> EXPORT_SYMBOL(ufshcd_system_resume); >> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >> index 93aeeb3..1e7fe73 100644 >> --- a/drivers/scsi/ufs/ufshcd.h >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -754,6 +754,8 @@ struct ufs_hba { >> struct device_attribute spm_lvl_attr; >> /* A flag to tell whether __ufshcd_wl_suspend/resume() is in >> progress */ >> bool wlu_pm_op_in_progress; >> + /* A flag to tell whether ufshcd_suspend/resume() is in progress */ >> + bool pm_op_in_progress; >> >> /* Auto-Hibernate Idle Timer register value */ >> u32 ahit; >> @@ -841,6 +843,8 @@ struct ufs_hba { >> struct ufs_clk_scaling clk_scaling; >> /* A flag to tell whether the UFS device W-LU is system suspended */ >> bool is_wlu_sys_suspended; >> + /* A flag to tell whether hba is system suspended */ >> + bool is_sys_suspended; >> >> enum bkops_status urgent_bkops_lvl; >> bool is_urgent_bkops_lvl_checked; >>
On 2021-06-24 04:59, Bart Van Assche wrote: > On 6/23/21 12:35 AM, Can Guo wrote: >> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >> index 93aeeb3..1e7fe73 100644 >> --- a/drivers/scsi/ufs/ufshcd.h >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -754,6 +754,8 @@ struct ufs_hba { >> struct device_attribute spm_lvl_attr; >> /* A flag to tell whether __ufshcd_wl_suspend/resume() is in >> progress */ >> bool wlu_pm_op_in_progress; >> + /* A flag to tell whether ufshcd_suspend/resume() is in progress */ >> + bool pm_op_in_progress; >> >> /* Auto-Hibernate Idle Timer register value */ >> u32 ahit; >> @@ -841,6 +843,8 @@ struct ufs_hba { >> struct ufs_clk_scaling clk_scaling; >> /* A flag to tell whether the UFS device W-LU is system suspended */ >> bool is_wlu_sys_suspended; >> + /* A flag to tell whether hba is system suspended */ >> + bool is_sys_suspended; >> >> enum bkops_status urgent_bkops_lvl; >> bool is_urgent_bkops_lvl_checked; > > It is not yet clear to me whether we really need these new member > variables. If these are retained, please rename pm_op_in_progress into > platform_pm_op_in_progress and is_sys_suspended into > platform_is_sys_suspended. Hi Bart, These flags are informative when we debug some UFS issues and they are used by later changes. Sure, I will modify the namings. Thanks, Can Guo. > > Thanks, > > Bart.
On 6/23/21 12:35 AM, Can Guo wrote: > @@ -9141,6 +9143,8 @@ static int ufshcd_suspend(struct ufs_hba *hba) > > if (!hba->is_powered) > return 0; > + > + hba->pm_op_in_progress = true; > /* > * Disable the host irq as host controller as there won't be any > * host controller transaction expected till resume. > @@ -9160,6 +9164,7 @@ static int ufshcd_suspend(struct ufs_hba *hba) > ufshcd_vreg_set_lpm(hba); > /* Put the host controller in low power mode if possible */ > ufshcd_hba_vreg_set_lpm(hba); > + hba->pm_op_in_progress = false; > return ret; > } > > @@ -9179,6 +9184,7 @@ static int ufshcd_resume(struct ufs_hba *hba) > if (!hba->is_powered) > return 0; > > + hba->pm_op_in_progress = true; > ufshcd_hba_vreg_set_hpm(hba); > ret = ufshcd_vreg_set_hpm(hba); > if (ret) > @@ -9198,6 +9204,7 @@ static int ufshcd_resume(struct ufs_hba *hba) > out: > if (ret) > ufshcd_update_evt_hist(hba, UFS_EVT_RESUME_ERR, (u32)ret); > + hba->pm_op_in_progress = false; > return ret; > } Has it been considered to check dev->power.runtime_status instead of introducing the pm_op_in_progress variable? Thanks, Bart.
On 2021-06-25 01:35, Bart Van Assche wrote: > On 6/23/21 12:35 AM, Can Guo wrote: >> @@ -9141,6 +9143,8 @@ static int ufshcd_suspend(struct ufs_hba *hba) >> >> if (!hba->is_powered) >> return 0; >> + >> + hba->pm_op_in_progress = true; >> /* >> * Disable the host irq as host controller as there won't be any >> * host controller transaction expected till resume. >> @@ -9160,6 +9164,7 @@ static int ufshcd_suspend(struct ufs_hba *hba) >> ufshcd_vreg_set_lpm(hba); >> /* Put the host controller in low power mode if possible */ >> ufshcd_hba_vreg_set_lpm(hba); >> + hba->pm_op_in_progress = false; >> return ret; >> } >> >> @@ -9179,6 +9184,7 @@ static int ufshcd_resume(struct ufs_hba *hba) >> if (!hba->is_powered) >> return 0; >> >> + hba->pm_op_in_progress = true; >> ufshcd_hba_vreg_set_hpm(hba); >> ret = ufshcd_vreg_set_hpm(hba); >> if (ret) >> @@ -9198,6 +9204,7 @@ static int ufshcd_resume(struct ufs_hba *hba) >> out: >> if (ret) >> ufshcd_update_evt_hist(hba, UFS_EVT_RESUME_ERR, (u32)ret); >> + hba->pm_op_in_progress = false; >> return ret; >> } > > Has it been considered to check dev->power.runtime_status instead of > introducing the pm_op_in_progress variable? ufshcd_resume() is also used by system resume, while runtime_status only tells about runtime resume. So does ufshcd_suspend(). Thanks, Can Guo. > > Thanks, > > Bart.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index c40ba1d..abe5f2d 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -551,6 +551,8 @@ static void ufshcd_print_host_state(struct ufs_hba *hba) hba->curr_dev_pwr_mode, hba->uic_link_state); 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, "pm_op_in_progress=%d, is_sys_suspended=%d\n", + hba->pm_op_in_progress, hba->is_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); @@ -9141,6 +9143,8 @@ static int ufshcd_suspend(struct ufs_hba *hba) if (!hba->is_powered) return 0; + + hba->pm_op_in_progress = true; /* * Disable the host irq as host controller as there won't be any * host controller transaction expected till resume. @@ -9160,6 +9164,7 @@ static int ufshcd_suspend(struct ufs_hba *hba) ufshcd_vreg_set_lpm(hba); /* Put the host controller in low power mode if possible */ ufshcd_hba_vreg_set_lpm(hba); + hba->pm_op_in_progress = false; return ret; } @@ -9179,6 +9184,7 @@ static int ufshcd_resume(struct ufs_hba *hba) if (!hba->is_powered) return 0; + hba->pm_op_in_progress = true; ufshcd_hba_vreg_set_hpm(hba); ret = ufshcd_vreg_set_hpm(hba); if (ret) @@ -9198,6 +9204,7 @@ static int ufshcd_resume(struct ufs_hba *hba) out: if (ret) ufshcd_update_evt_hist(hba, UFS_EVT_RESUME_ERR, (u32)ret); + hba->pm_op_in_progress = false; return ret; } @@ -9222,6 +9229,8 @@ int ufshcd_system_suspend(struct ufs_hba *hba) trace_ufshcd_system_suspend(dev_name(hba->dev), ret, ktime_to_us(ktime_sub(ktime_get(), start)), hba->curr_dev_pwr_mode, hba->uic_link_state); + if (!ret) + hba->is_sys_suspended = true; return ret; } EXPORT_SYMBOL(ufshcd_system_suspend); @@ -9247,7 +9256,8 @@ int ufshcd_system_resume(struct ufs_hba *hba) trace_ufshcd_system_resume(dev_name(hba->dev), ret, ktime_to_us(ktime_sub(ktime_get(), start)), hba->curr_dev_pwr_mode, hba->uic_link_state); - + if (!ret) + hba->is_sys_suspended = false; return ret; } EXPORT_SYMBOL(ufshcd_system_resume); diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 93aeeb3..1e7fe73 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -754,6 +754,8 @@ struct ufs_hba { struct device_attribute spm_lvl_attr; /* A flag to tell whether __ufshcd_wl_suspend/resume() is in progress */ bool wlu_pm_op_in_progress; + /* A flag to tell whether ufshcd_suspend/resume() is in progress */ + bool pm_op_in_progress; /* Auto-Hibernate Idle Timer register value */ u32 ahit; @@ -841,6 +843,8 @@ struct ufs_hba { struct ufs_clk_scaling clk_scaling; /* A flag to tell whether the UFS device W-LU is system suspended */ bool is_wlu_sys_suspended; + /* A flag to tell whether hba is system suspended */ + bool is_sys_suspended; enum bkops_status urgent_bkops_lvl; bool is_urgent_bkops_lvl_checked;
Add flags pm_op_in_progress and is_sys_suspended to track the status of hba runtime and system suspend/resume operations. Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/ufs/ufshcd.c | 12 +++++++++++- drivers/scsi/ufs/ufshcd.h | 4 ++++ 2 files changed, 15 insertions(+), 1 deletion(-)