[PATCH v2 0/3] Fix abnormal clock scaling behaviors

[PATCH v2 0/3] Fix abnormal clock scaling behaviors

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

This patch fix abnormal clock scaling behaviors.

Peter Wang (3):
  ufs: core: only suspend clock scaling if scale down
  ufs: core: fix abnormal scale up after last cmd finish
  ufs: core: fix abnormal scale up after scale down

 drivers/ufs/core/ufshcd.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

-- 
2.18.0

[PATCH v2 1/3] ufs: core: only suspend clock scaling if scale down

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

If clock scale up and suspend clock scaling, ufs will keep high
performance/power mode but no read/write requests on going.
It is logic wrong and have power concern.

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 129446775796..e3672e55efae 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1458,7 +1458,7 @@ static int ufshcd_devfreq_target(struct device *dev,
 		ktime_to_us(ktime_sub(ktime_get(), start)), ret);
 
 out:
-	if (sched_clk_scaling_suspend_work)
+	if (sched_clk_scaling_suspend_work && !scale_up)
 		queue_work(hba->clk_scaling.workq,
 			   &hba->clk_scaling.suspend_work);
 
-- 
2.18.0

[PATCH v2 2/3] ufs: core: fix abnormal scale up after last cmd finish

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

When ufshcd_clk_scaling_suspend_work(Thread A) running and new command
coming, ufshcd_clk_scaling_start_busy(Thread B) may get host_lock
after Thread A first time release host_lock. Then Thread A second time
get host_lock will set clk_scaling.window_start_t = 0 which scale up
clock abnormal next polling_ms time.
Also inlines another __ufshcd_suspend_clkscaling calls.

Below is racing step:
1	hba->clk_scaling.suspend_work (Thread A)
	ufshcd_clk_scaling_suspend_work
2		spin_lock_irqsave(hba->host->host_lock, irq_flags);
3		hba->clk_scaling.is_suspended = true;
4		spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
		__ufshcd_suspend_clkscaling
7			spin_lock_irqsave(hba->host->host_lock, flags);
8			hba->clk_scaling.window_start_t = 0;
9			spin_unlock_irqrestore(hba->host->host_lock, flags);

	ufshcd_send_command (Thread B)
		ufshcd_clk_scaling_start_busy
5			spin_lock_irqsave(hba->host->host_lock, flags);
			....
6			spin_unlock_irqrestore(hba->host->host_lock, flags);

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index e3672e55efae..057549b0e586 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -275,7 +275,6 @@ static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba);
 static int ufshcd_host_reset_and_restore(struct ufs_hba *hba);
 static void ufshcd_resume_clkscaling(struct ufs_hba *hba);
 static void ufshcd_suspend_clkscaling(struct ufs_hba *hba);
-static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba);
 static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up);
 static irqreturn_t ufshcd_intr(int irq, void *__hba);
 static int ufshcd_change_power_mode(struct ufs_hba *hba,
@@ -1385,9 +1384,10 @@ static void ufshcd_clk_scaling_suspend_work(struct work_struct *work)
 		return;
 	}
 	hba->clk_scaling.is_suspended = true;
+	hba->clk_scaling.window_start_t = 0;
 	spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
 
-	__ufshcd_suspend_clkscaling(hba);
+	devfreq_suspend_device(hba->devfreq);
 }
 
 static void ufshcd_clk_scaling_resume_work(struct work_struct *work)
@@ -1564,16 +1564,6 @@ static void ufshcd_devfreq_remove(struct ufs_hba *hba)
 	dev_pm_opp_remove(hba->dev, clki->max_freq);
 }
 
-static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
-{
-	unsigned long flags;
-
-	devfreq_suspend_device(hba->devfreq);
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	hba->clk_scaling.window_start_t = 0;
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-}
-
 static void ufshcd_suspend_clkscaling(struct ufs_hba *hba)
 {
 	unsigned long flags;
@@ -1586,11 +1576,12 @@ static void ufshcd_suspend_clkscaling(struct ufs_hba *hba)
 	if (!hba->clk_scaling.is_suspended) {
 		suspend = true;
 		hba->clk_scaling.is_suspended = true;
+		hba->clk_scaling.window_start_t = 0;
 	}
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	if (suspend)
-		__ufshcd_suspend_clkscaling(hba);
+		devfreq_suspend_device(hba->devfreq);
 }
 
 static void ufshcd_resume_clkscaling(struct ufs_hba *hba)
-- 
2.18.0

Re: [PATCH v2 2/3] ufs: core: fix abnormal scale up after last cmd finish

[Peter Wang (王信友)]

Hi Bart,

This patch still need your review.

Thanks.
Peter


On Thu, 2023-08-31 at 21:08 +0800, peter.wang@mediatek.com wrote:
> From: Peter Wang <peter.wang@mediatek.com>
> 
> When ufshcd_clk_scaling_suspend_work(Thread A) running and new
> command
> coming, ufshcd_clk_scaling_start_busy(Thread B) may get host_lock
> after Thread A first time release host_lock. Then Thread A second
> time
> get host_lock will set clk_scaling.window_start_t = 0 which scale up
> clock abnormal next polling_ms time.
> Also inlines another __ufshcd_suspend_clkscaling calls.
> 
> Below is racing step:
> 1	hba->clk_scaling.suspend_work (Thread A)
> 	ufshcd_clk_scaling_suspend_work
> 2		spin_lock_irqsave(hba->host->host_lock, irq_flags);
> 3		hba->clk_scaling.is_suspended = true;
> 4		spin_unlock_irqrestore(hba->host->host_lock,
> irq_flags);
> 		__ufshcd_suspend_clkscaling
> 7			spin_lock_irqsave(hba->host->host_lock, flags);
> 8			hba->clk_scaling.window_start_t = 0;
> 9			spin_unlock_irqrestore(hba->host->host_lock,
> flags);
> 
> 	ufshcd_send_command (Thread B)
> 		ufshcd_clk_scaling_start_busy
> 5			spin_lock_irqsave(hba->host->host_lock, flags);
> 			....
> 6			spin_unlock_irqrestore(hba->host->host_lock,
> flags);
> 
> Signed-off-by: Peter Wang <peter.wang@mediatek.com>
> ---
>  drivers/ufs/core/ufshcd.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index e3672e55efae..057549b0e586 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -275,7 +275,6 @@ static inline void
> ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba);
>  static int ufshcd_host_reset_and_restore(struct ufs_hba *hba);
>  static void ufshcd_resume_clkscaling(struct ufs_hba *hba);
>  static void ufshcd_suspend_clkscaling(struct ufs_hba *hba);
> -static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba);
>  static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up);
>  static irqreturn_t ufshcd_intr(int irq, void *__hba);
>  static int ufshcd_change_power_mode(struct ufs_hba *hba,
> @@ -1385,9 +1384,10 @@ static void
> ufshcd_clk_scaling_suspend_work(struct work_struct *work)
>  		return;
>  	}
>  	hba->clk_scaling.is_suspended = true;
> +	hba->clk_scaling.window_start_t = 0;
>  	spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
>  
> -	__ufshcd_suspend_clkscaling(hba);
> +	devfreq_suspend_device(hba->devfreq);
>  }
>  
>  static void ufshcd_clk_scaling_resume_work(struct work_struct *work)
> @@ -1564,16 +1564,6 @@ static void ufshcd_devfreq_remove(struct
> ufs_hba *hba)
>  	dev_pm_opp_remove(hba->dev, clki->max_freq);
>  }
>  
> -static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
> -{
> -	unsigned long flags;
> -
> -	devfreq_suspend_device(hba->devfreq);
> -	spin_lock_irqsave(hba->host->host_lock, flags);
> -	hba->clk_scaling.window_start_t = 0;
> -	spin_unlock_irqrestore(hba->host->host_lock, flags);
> -}
> -
>  static void ufshcd_suspend_clkscaling(struct ufs_hba *hba)
>  {
>  	unsigned long flags;
> @@ -1586,11 +1576,12 @@ static void ufshcd_suspend_clkscaling(struct
> ufs_hba *hba)
>  	if (!hba->clk_scaling.is_suspended) {
>  		suspend = true;
>  		hba->clk_scaling.is_suspended = true;
> +		hba->clk_scaling.window_start_t = 0;
>  	}
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
>  
>  	if (suspend)
> -		__ufshcd_suspend_clkscaling(hba);
> +		devfreq_suspend_device(hba->devfreq);
>  }
>  
>  static void ufshcd_resume_clkscaling(struct ufs_hba *hba)

Re: [PATCH v2 2/3] ufs: core: fix abnormal scale up after last cmd finish

[Bart Van Assche]

On 10/13/23 02:42, Peter Wang (王信友) wrote:
> This patch still need your review.

Hi Peter,

Although I can see that patch 2/3 made it to lore.kernel.org, I did not receive
that patch in my mailbox. This probably was caused by an issue by the mailserver
I'm using. I will download the emails I'm missing from lore.kernel.org and review
these patches.

Thanks,

Bart.

Re: [PATCH v2 2/3] ufs: core: fix abnormal scale up after last cmd finish

[Bart Van Assche]

On 8/31/23 06:08, peter.wang@mediatek.com wrote:
> When ufshcd_clk_scaling_suspend_work(Thread A) running and new command
> coming, ufshcd_clk_scaling_start_busy(Thread B) may get host_lock
> after Thread A first time release host_lock. Then Thread A second time
> get host_lock will set clk_scaling.window_start_t = 0 which scale up
> clock abnormal next polling_ms time.
> Also inlines another __ufshcd_suspend_clkscaling calls.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 2/3] ufs: core: fix abnormal scale up after last cmd finish

[Martin K. Petersen]

> On 8/31/23 06:08, peter.wang@mediatek.com wrote:
>> When ufshcd_clk_scaling_suspend_work(Thread A) running and new command
>> coming, ufshcd_clk_scaling_start_busy(Thread B) may get host_lock
>> after Thread A first time release host_lock. Then Thread A second time
>> get host_lock will set clk_scaling.window_start_t = 0 which scale up
>> clock abnormal next polling_ms time.
>> Also inlines another __ufshcd_suspend_clkscaling calls.
>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Applied 1-3 to 6.7/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v2 3/3] ufs: core: fix abnormal scale up after scale down

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

When no active_reqs, devfreq_monitor(Thread A) will suspend clock scaling.
But it may have racing with clk_scaling.suspend_work(Thread B) and
actually not suspend clock scaling(requue after suspend).
Next time after polling_ms, devfreq_monitor read
clk_scaling.window_start_t = 0 then scale up clock abnormal.

Below is racing step:
	devfreq->work (Thread A)
	devfreq_monitor
		update_devfreq
		.....
			ufshcd_devfreq_target
			queue_work(hba->clk_scaling.workq,
1			   &hba->clk_scaling.suspend_work)
		.....
5	queue_delayed_work(devfreq_wq, &devfreq->work,
			msecs_to_jiffies(devfreq->profile->polling_ms));

2	hba->clk_scaling.suspend_work (Thread B)
	ufshcd_clk_scaling_suspend_work
		__ufshcd_suspend_clkscaling
			devfreq_suspend_device(hba->devfreq);
3				cancel_delayed_work_sync(&devfreq->work);
4			hba->clk_scaling.window_start_t = 0;
	.....

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 057549b0e586..d72fa2c1e316 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1430,6 +1430,13 @@ static int ufshcd_devfreq_target(struct device *dev,
 		return 0;
 	}
 
+	/* Skip scaling clock when clock scaling is suspended */
+	if (hba->clk_scaling.is_suspended) {
+		spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
+		dev_warn(hba->dev, "clock scaling is suspended, skip");
+		return 0;
+	}
+
 	if (!hba->clk_scaling.active_reqs)
 		sched_clk_scaling_suspend_work = true;
 
-- 
2.18.0

Re: [PATCH v2 0/3] Fix abnormal clock scaling behaviors

[Martin K. Petersen]

On Thu, 31 Aug 2023 21:08:23 +0800, peter.wang@mediatek.com wrote:

> This patch fix abnormal clock scaling behaviors.
> 
> Peter Wang (3):
>   ufs: core: only suspend clock scaling if scale down
>   ufs: core: fix abnormal scale up after last cmd finish
>   ufs: core: fix abnormal scale up after scale down
> 
> [...]

Applied to 6.7/scsi-queue, thanks!

[1/3] ufs: core: only suspend clock scaling if scale down
      https://git.kernel.org/mkp/scsi/c/1d969731b87f
[2/3] ufs: core: fix abnormal scale up after last cmd finish
      https://git.kernel.org/mkp/scsi/c/6fd53da45bbc
[3/3] ufs: core: fix abnormal scale up after scale down
      https://git.kernel.org/mkp/scsi/c/b50d9c27a31e

-- 
Martin K. Petersen	Oracle Linux Engineering