linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt
@ 2019-12-07 12:21 Stanley Chu
  2019-12-07 12:22 ` [PATCH v1 1/2] scsi: ufs: disable irq before disabling clocks Stanley Chu
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Stanley Chu @ 2019-12-07 12:21 UTC (permalink / raw)
  To: linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	pedrom.sousa, jejb, matthias.bgg
  Cc: linux-mediatek, linux-arm-kernel, linux-kernel, beanhuo,
	kuohong.wang, peter.wang, chun-hung.wu, andy.teng, Stanley Chu

This patchset fixes up active duration of ufshcd interrupt to avoid potential system hang issues.

Stanley Chu (2):
  scsi: ufs: disable irq before disabling clocks
  scsi: ufs: disable interrupt during clock-gating

 drivers/scsi/ufs/ufshcd.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

-- 
2.18.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v1 1/2] scsi: ufs: disable irq before disabling clocks
  2019-12-07 12:21 [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt Stanley Chu
@ 2019-12-07 12:22 ` Stanley Chu
  2019-12-07 12:22 ` [PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating Stanley Chu
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Stanley Chu @ 2019-12-07 12:22 UTC (permalink / raw)
  To: linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	pedrom.sousa, jejb, matthias.bgg
  Cc: linux-mediatek, linux-arm-kernel, linux-kernel, beanhuo,
	kuohong.wang, peter.wang, chun-hung.wu, andy.teng, Stanley Chu

During suspend flow, interrupt shall be disabled before disabling
clocks to avoid potential system hang due to accessing host registers
after host clocks are disabled.

For example, if an interrupt comes with IRQF_IRQPOLL flag configured
with the misrouted interrupt recovery feature enabled, ufshcd ISR may
be triggered even if nothing shall be done for UFS. In this case, system
hang may happen if UFS interrupt status register is accessed with host
clocks disabled.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b5966faf3e98..f80bd4e811cb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7908,6 +7908,11 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	ret = ufshcd_vops_suspend(hba, pm_op);
 	if (ret)
 		goto set_link_active;
+	/*
+	 * Disable the host irq as host controller as there won't be any
+	 * host controller transaction expected till resume.
+	 */
+	ufshcd_disable_irq(hba);
 
 	if (!ufshcd_is_link_active(hba))
 		ufshcd_setup_clocks(hba, false);
@@ -7917,11 +7922,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 
 	hba->clk_gating.state = CLKS_OFF;
 	trace_ufshcd_clk_gating(dev_name(hba->dev), hba->clk_gating.state);
-	/*
-	 * Disable the host irq as host controller as there won't be any
-	 * host controller transaction expected till resume.
-	 */
-	ufshcd_disable_irq(hba);
+
 	/* Put the host controller in low power mode if possible */
 	ufshcd_hba_vreg_set_lpm(hba);
 	goto out;
-- 
2.18.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating
  2019-12-07 12:21 [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt Stanley Chu
  2019-12-07 12:22 ` [PATCH v1 1/2] scsi: ufs: disable irq before disabling clocks Stanley Chu
@ 2019-12-07 12:22 ` Stanley Chu
  2019-12-17 23:25   ` Asutosh Das (asd)
  2019-12-13  2:48 ` [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt Stanley Chu
  2019-12-19 23:04 ` Martin K. Petersen
  3 siblings, 1 reply; 9+ messages in thread
From: Stanley Chu @ 2019-12-07 12:22 UTC (permalink / raw)
  To: linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	pedrom.sousa, jejb, matthias.bgg
  Cc: linux-mediatek, linux-arm-kernel, linux-kernel, beanhuo,
	kuohong.wang, peter.wang, chun-hung.wu, andy.teng, Stanley Chu

Similar to suspend, ufshcd interrupt can be disabled since
there won't be any host controller transaction expected till
clocks ungated.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f80bd4e811cb..5de105e82c32 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1490,6 +1490,8 @@ static void ufshcd_ungate_work(struct work_struct *work)
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	ufshcd_setup_clocks(hba, true);
 
+	ufshcd_enable_irq(hba);
+
 	/* Exit from hibern8 */
 	if (ufshcd_can_hibern8_during_gating(hba)) {
 		/* Prevent gating in this path */
@@ -1636,6 +1638,8 @@ static void ufshcd_gate_work(struct work_struct *work)
 		ufshcd_set_link_hibern8(hba);
 	}
 
+	ufshcd_disable_irq(hba);
+
 	if (!ufshcd_is_link_active(hba))
 		ufshcd_setup_clocks(hba, false);
 	else
-- 
2.18.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt
  2019-12-07 12:21 [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt Stanley Chu
  2019-12-07 12:22 ` [PATCH v1 1/2] scsi: ufs: disable irq before disabling clocks Stanley Chu
  2019-12-07 12:22 ` [PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating Stanley Chu
@ 2019-12-13  2:48 ` Stanley Chu
  2019-12-17  7:38   ` Avri Altman
  2019-12-19 23:04 ` Martin K. Petersen
  3 siblings, 1 reply; 9+ messages in thread
From: Stanley Chu @ 2019-12-13  2:48 UTC (permalink / raw)
  To: linux-scsi, alim.akhtar, pedrom.sousa, avri.altman
  Cc: martin.petersen, jejb, matthias.bgg, linux-mediatek,
	linux-arm-kernel, linux-kernel, beanhuo, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng

Dear reviewers,

	Gentle ping for this patch set.

On Sat, 2019-12-07 at 20:21 +0800, Stanley Chu wrote:
> This patchset fixes up active duration of ufshcd interrupt to avoid potential system hang issues.
> 
> Stanley Chu (2):
>   scsi: ufs: disable irq before disabling clocks
>   scsi: ufs: disable interrupt during clock-gating
> 
>  drivers/scsi/ufs/ufshcd.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 

Thanks,
Stanley


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt
  2019-12-13  2:48 ` [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt Stanley Chu
@ 2019-12-17  7:38   ` Avri Altman
  0 siblings, 0 replies; 9+ messages in thread
From: Avri Altman @ 2019-12-17  7:38 UTC (permalink / raw)
  To: Stanley Chu, linux-scsi, alim.akhtar, pedrom.sousa
  Cc: martin.petersen, jejb, matthias.bgg, linux-mediatek,
	linux-arm-kernel, linux-kernel, beanhuo, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng

Looks good to me.
Reviewed-by: Avri Altman <avri.altman@wdc.com>

> 
> On Sat, 2019-12-07 at 20:21 +0800, Stanley Chu wrote:
> > This patchset fixes up active duration of ufshcd interrupt to avoid potential
> system hang issues.
> >
> > Stanley Chu (2):
> >   scsi: ufs: disable irq before disabling clocks
> >   scsi: ufs: disable interrupt during clock-gating
> >
> >  drivers/scsi/ufs/ufshcd.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> 
> Thanks,
> Stanley


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating
  2019-12-07 12:22 ` [PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating Stanley Chu
@ 2019-12-17 23:25   ` Asutosh Das (asd)
  2019-12-18  3:52     ` Stanley Chu
  0 siblings, 1 reply; 9+ messages in thread
From: Asutosh Das (asd) @ 2019-12-17 23:25 UTC (permalink / raw)
  To: Stanley Chu, linux-scsi, martin.petersen, avri.altman,
	alim.akhtar, pedrom.sousa, jejb, matthias.bgg
  Cc: linux-mediatek, linux-arm-kernel, linux-kernel, beanhuo,
	kuohong.wang, peter.wang, chun-hung.wu, andy.teng

On 12/7/2019 4:22 AM, Stanley Chu wrote:
> Similar to suspend, ufshcd interrupt can be disabled since
> there won't be any host controller transaction expected till
> clocks ungated.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>   drivers/scsi/ufs/ufshcd.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f80bd4e811cb..5de105e82c32 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1490,6 +1490,8 @@ static void ufshcd_ungate_work(struct work_struct *work)
>   	spin_unlock_irqrestore(hba->host->host_lock, flags);
>   	ufshcd_setup_clocks(hba, true);
>   
> +	ufshcd_enable_irq(hba);
> +
>   	/* Exit from hibern8 */
>   	if (ufshcd_can_hibern8_during_gating(hba)) {
>   		/* Prevent gating in this path */
> @@ -1636,6 +1638,8 @@ static void ufshcd_gate_work(struct work_struct *work)
>   		ufshcd_set_link_hibern8(hba);
>   	}
>   
> +	ufshcd_disable_irq(hba);
> +
>   	if (!ufshcd_is_link_active(hba))
>   		ufshcd_setup_clocks(hba, false);
>   	else
> 

Hi,
Does this save significant power? I see that gate/ungate of clocks 
happen far too frequently than suspend/resume.

Have you considered how much latency this would add to the 
gating/ungating path?

-asd

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating
  2019-12-17 23:25   ` Asutosh Das (asd)
@ 2019-12-18  3:52     ` Stanley Chu
  2019-12-18 23:37       ` Asutosh Das (asd)
  0 siblings, 1 reply; 9+ messages in thread
From: Stanley Chu @ 2019-12-18  3:52 UTC (permalink / raw)
  To: Asutosh Das (asd)
  Cc: linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	pedrom.sousa, jejb, matthias.bgg, linux-mediatek,
	linux-arm-kernel, linux-kernel, beanhuo, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng

Hi Asutosh,

On Tue, 2019-12-17 at 15:25 -0800, Asutosh Das (asd) wrote:
> > 
> 
> Hi,
> Does this save significant power? I see that gate/ungate of clocks 
> happen far too frequently than suspend/resume.
> 
> Have you considered how much latency this would add to the 
> gating/ungating path?
> 
> -asd
> 

Yes, we have measured 200 times clk-gating/clk-ungating and latency data
is showed as below,

For clk-gating with interrupt disabling toggled,

	Average latency of each clk-gating: 55.117 us
	Average latency of irq-disabling during clk-gating: 4.2 us

For clk-ungating with interrupt enabling toggled,
	Average latency of each clk-ungating: 118.324 us
	Average latency of irq-enabling during clk-ungating: 2.9 us

The evaluation here is based on below Can's patch therefore the
interrupt control (enable_irq/disable_irq) latency is much shorter than
before (request_irq/free_irq).

scsi: ufs: Do not free irq in suspend
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/scsi/ufs/ufshcd.c?id=8709c1f68536e256668812788af5b2bb027f49c3

BTW, the main purpose of this patch is aimed to protect ufshcd register
from accessing while host clocks are disabled to fix potential system
hang issue. The possible scenario is mentioned in commit message of
patch "scsi: ufs: disable irq before disabling clocks" in the same
series.

Thanks
Stanley

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating
  2019-12-18  3:52     ` Stanley Chu
@ 2019-12-18 23:37       ` Asutosh Das (asd)
  0 siblings, 0 replies; 9+ messages in thread
From: Asutosh Das (asd) @ 2019-12-18 23:37 UTC (permalink / raw)
  To: Stanley Chu
  Cc: linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	pedrom.sousa, jejb, matthias.bgg, linux-mediatek,
	linux-arm-kernel, linux-kernel, beanhuo, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng

On 12/17/2019 7:52 PM, Stanley Chu wrote:
> Hi Asutosh,
> 
> On Tue, 2019-12-17 at 15:25 -0800, Asutosh Das (asd) wrote:
>>>
>>
>> Hi,
>> Does this save significant power? I see that gate/ungate of clocks
>> happen far too frequently than suspend/resume.
>>
>> Have you considered how much latency this would add to the
>> gating/ungating path?
>>
>> -asd
>>
> 
> Yes, we have measured 200 times clk-gating/clk-ungating and latency data
> is showed as below,
> 
> For clk-gating with interrupt disabling toggled,
> 
> 	Average latency of each clk-gating: 55.117 us
> 	Average latency of irq-disabling during clk-gating: 4.2 us
> 
> For clk-ungating with interrupt enabling toggled,
> 	Average latency of each clk-ungating: 118.324 us
> 	Average latency of irq-enabling during clk-ungating: 2.9 us
> 
> The evaluation here is based on below Can's patch therefore the
> interrupt control (enable_irq/disable_irq) latency is much shorter than
> before (request_irq/free_irq).
> 
> scsi: ufs: Do not free irq in suspend
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/scsi/ufs/ufshcd.c?id=8709c1f68536e256668812788af5b2bb027f49c3
> 
> BTW, the main purpose of this patch is aimed to protect ufshcd register
> from accessing while host clocks are disabled to fix potential system
> hang issue. The possible scenario is mentioned in commit message of
> patch "scsi: ufs: disable irq before disabling clocks" in the same
> series.
> 
> Thanks
> Stanley
> 

Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

Thanks for the data.
It'd be interesting to know more on the - misrouted interrupt recovery 
feature though.

-
Thanks
Asutosh

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt
  2019-12-07 12:21 [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt Stanley Chu
                   ` (2 preceding siblings ...)
  2019-12-13  2:48 ` [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt Stanley Chu
@ 2019-12-19 23:04 ` Martin K. Petersen
  3 siblings, 0 replies; 9+ messages in thread
From: Martin K. Petersen @ 2019-12-19 23:04 UTC (permalink / raw)
  To: Stanley Chu
  Cc: linux-scsi, martin.petersen, avri.altman, alim.akhtar,
	pedrom.sousa, jejb, matthias.bgg, linux-mediatek,
	linux-arm-kernel, linux-kernel, beanhuo, kuohong.wang,
	peter.wang, chun-hung.wu, andy.teng


Stanley,

> This patchset fixes up active duration of ufshcd interrupt to avoid
> potential system hang issues.

Applied to 5.6/scsi-queue, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-12-19 23:04 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-07 12:21 [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt Stanley Chu
2019-12-07 12:22 ` [PATCH v1 1/2] scsi: ufs: disable irq before disabling clocks Stanley Chu
2019-12-07 12:22 ` [PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating Stanley Chu
2019-12-17 23:25   ` Asutosh Das (asd)
2019-12-18  3:52     ` Stanley Chu
2019-12-18 23:37       ` Asutosh Das (asd)
2019-12-13  2:48 ` [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt Stanley Chu
2019-12-17  7:38   ` Avri Altman
2019-12-19 23:04 ` Martin K. Petersen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).