diff mbox series

[v4,3/7] scsi: ufs: Fix up auto hibern8 enablement

Message ID 1573627552-12615-4-git-send-email-cang@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series UFS driver general fixes bundle 3 | expand

Commit Message

Can Guo Nov. 13, 2019, 6:45 a.m. UTC 
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 | 17 +++++++++++------
 drivers/scsi/ufs/ufshcd.c    | 12 ++++++------
 2 files changed, 17 insertions(+), 12 deletions(-)

Comments

Avri Altman Nov. 14, 2019, 9:23 a.m. UTC | #1 
> 
> 
> 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>
Acked-by: Avri Altman <avri.altman@wdc.com>

> ---
>  drivers/scsi/ufs/ufs-sysfs.c | 17 +++++++++++------
>  drivers/scsi/ufs/ufshcd.c    | 12 ++++++------
>  2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c index
> 969a36b..6c2f19e 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -126,13 +126,18 @@ 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);
> +               spin_lock_irqsave(hba->host->host_lock, flags);
> +               ufshcd_writel(hba, hba->ahit, REG_AUTO_HIBERNATE_IDLE_TIMER);
> +               spin_unlock_irqrestore(hba->host->host_lock, flags);
Call ufshcd_auto_hibern8_enable instead of open coding? 

> +               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..f12f5a7
> 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -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:
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
Can Guo Nov. 15, 2019, 3:33 a.m. UTC | #2 
On 2019-11-14 17:23, Avri Altman wrote:
>> 
>> 
>> 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>
> Acked-by: Avri Altman <avri.altman@wdc.com>
> 
>> ---
>>  drivers/scsi/ufs/ufs-sysfs.c | 17 +++++++++++------
>>  drivers/scsi/ufs/ufshcd.c    | 12 ++++++------
>>  2 files changed, 17 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufs-sysfs.c 
>> b/drivers/scsi/ufs/ufs-sysfs.c index
>> 969a36b..6c2f19e 100644
>> --- a/drivers/scsi/ufs/ufs-sysfs.c
>> +++ b/drivers/scsi/ufs/ufs-sysfs.c
>> @@ -126,13 +126,18 @@ 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);
>> +               spin_lock_irqsave(hba->host->host_lock, flags);
>> +               ufshcd_writel(hba, hba->ahit, 
>> REG_AUTO_HIBERNATE_IDLE_TIMER);
>> +               spin_unlock_irqrestore(hba->host->host_lock, flags);
> Call ufshcd_auto_hibern8_enable instead of open coding?
> 

In that case, we would have to export ufshcd_auto_hibern8_enable() as it 
is static for now.
Shall do that.

Thanks,

Can Guo.

>> +               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..f12f5a7
>> 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -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:
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 969a36b..6c2f19e 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -126,13 +126,18 @@  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);
+		spin_lock_irqsave(hba->host->host_lock, flags);
+		ufshcd_writel(hba, hba->ahit, REG_AUTO_HIBERNATE_IDLE_TIMER);
+		spin_unlock_irqrestore(hba->host->host_lock, flags);
+		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..f12f5a7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -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: