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

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(-)

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)

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.

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>

> 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!