diff mbox series

[v4,02/10] scsi: ufs: Add flags pm_op_in_progress and is_sys_suspended

Message ID 1624433711-9339-3-git-send-email-cang@codeaurora.org
State New
Headers show
Series Complementary changes for error handling | expand

Commit Message

Can Guo June 23, 2021, 7:35 a.m. UTC
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(-)

Comments

Bart Van Assche June 23, 2021, 8:59 p.m. UTC | #1
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.
Can Guo June 24, 2021, 2:05 a.m. UTC | #2
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;

>>
Can Guo June 24, 2021, 2:07 a.m. UTC | #3
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.
Bart Van Assche June 24, 2021, 5:35 p.m. UTC | #4
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.
Can Guo June 28, 2021, 7:11 a.m. UTC | #5
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 mbox series

Patch

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;