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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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 <[email protected]>
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
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