[PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt

[PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt

[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

[PATCH v1 1/2] scsi: ufs: disable irq before disabling clocks

[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

[PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating

[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

Re: [PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating

[Asutosh Das (asd)]

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

Re: [PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating

[Stanley Chu]

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

Re: [PATCH v1 2/2] scsi: ufs: disable interrupt during clock-gating

[Asutosh Das (asd)]

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

Re: [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt

[Stanley Chu]

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

RE: [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt

[Avri Altman]

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

Re: [PATCH v1 0/2] scsi: ufs: fixup active period of ufshcd interrupt

[Martin K. Petersen]

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