linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl()
@ 2022-05-06 21:41 Kuogee Hsieh
  2022-05-07  0:29 ` Stephen Boyd
  0 siblings, 1 reply; 3+ messages in thread
From: Kuogee Hsieh @ 2022-05-06 21:41 UTC (permalink / raw)
  To: robdclark, sean, swboyd, dianders, vkoul, daniel, airlied,
	agross, dmitry.baryshkov, bjorn.andersson
  Cc: quic_abhinavk, quic_aravindh, quic_khsieh, quic_sbillaka,
	freedreno, dri-devel, linux-arm-msm, linux-kernel

dp_catalog_ctrl_reset() will software reset DP controller. But it will
not reset programmable registers to default value. DP driver still have
to clear mask bits to interrupt status registers to disable interrupts
after software reset of controller. This patch removes the enable flag
condition checking to always clear mask bits of interrupt status
registers to disable interrupts if enable flag is false.

Fixes: ba0a422be723 ("drm/msm/dp: do not initialize phy until plugin interrupt received")
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 38026f2..cbf3399 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1379,8 +1379,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
 
 	dp_catalog_ctrl_reset(ctrl->catalog);
 
-	if (enable)
-		dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
+	/*
+	 * all dp controller programmable registers will not
+	 * be reset to default value after DP_SW_RESET
+	 * therefore interrupt mask bits have to be updated
+	 * to enable/disable interrupts
+	 */
+	dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
 }
 
 void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

* Re: [PATCH] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl()
  2022-05-06 21:41 [PATCH] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl() Kuogee Hsieh
@ 2022-05-07  0:29 ` Stephen Boyd
  2022-05-09 21:56   ` Kuogee Hsieh
  0 siblings, 1 reply; 3+ messages in thread
From: Stephen Boyd @ 2022-05-07  0:29 UTC (permalink / raw)
  To: Kuogee Hsieh, agross, airlied, bjorn.andersson, daniel, dianders,
	dmitry.baryshkov, robdclark, sean, vkoul
  Cc: quic_abhinavk, quic_aravindh, quic_sbillaka, freedreno,
	dri-devel, linux-arm-msm, linux-kernel

Quoting Kuogee Hsieh (2022-05-06 14:41:07)
> dp_catalog_ctrl_reset() will software reset DP controller. But it will
> not reset programmable registers to default value. DP driver still have
> to clear mask bits to interrupt status registers to disable interrupts
> after software reset of controller. This patch removes the enable flag
> condition checking to always clear mask bits of interrupt status
> registers to disable interrupts if enable flag is false.

Another paragraph is needed which is that this (partially?) fixes the
suspend path where we call dp_catalog_ctrl_reset() but the irq is still
unmasked and can come in while we're suspending. This leads to bus hangs
if the irq is handled after we power down the DP hardware because we run
the irq handler and access a device register assuming that no irq could
ever come in if we powered down the device. We don't know when the irq
will be handled though, so it's possible the irq is pending from before
we disable the irq in the hardware. Don't we need some irq synchronize
to make sure it doesn't run?

>
> Fixes: ba0a422be723 ("drm/msm/dp: do not initialize phy until plugin interrupt received")
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 38026f2..cbf3399 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1379,8 +1379,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
>
>         dp_catalog_ctrl_reset(ctrl->catalog);
>
> -       if (enable)
> -               dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
> +       /*
> +        * all dp controller programmable registers will not
> +        * be reset to default value after DP_SW_RESET
> +        * therefore interrupt mask bits have to be updated
> +        * to enable/disable interrupts
> +        */
> +       dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
>  }
>
>  void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

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

* Re: [PATCH] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl()
  2022-05-07  0:29 ` Stephen Boyd
@ 2022-05-09 21:56   ` Kuogee Hsieh
  0 siblings, 0 replies; 3+ messages in thread
From: Kuogee Hsieh @ 2022-05-09 21:56 UTC (permalink / raw)
  To: Stephen Boyd, agross, airlied, bjorn.andersson, daniel, dianders,
	dmitry.baryshkov, robdclark, sean, vkoul
  Cc: quic_abhinavk, quic_aravindh, quic_sbillaka, freedreno,
	dri-devel, linux-arm-msm, linux-kernel


On 5/6/2022 5:29 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-05-06 14:41:07)
>> dp_catalog_ctrl_reset() will software reset DP controller. But it will
>> not reset programmable registers to default value. DP driver still have
>> to clear mask bits to interrupt status registers to disable interrupts
>> after software reset of controller. This patch removes the enable flag
>> condition checking to always clear mask bits of interrupt status
>> registers to disable interrupts if enable flag is false.
> Another paragraph is needed which is that this (partially?) fixes the
> suspend path where we call dp_catalog_ctrl_reset() but the irq is still
> unmasked and can come in while we're suspending. This leads to bus hangs
> if the irq is handled after we power down the DP hardware because we run
> the irq handler and access a device register assuming that no irq could
> ever come in if we powered down the device. We don't know when the irq
> will be handled though, so it's possible the irq is pending from before
> we disable the irq in the hardware. Don't we need some irq synchronize
> to make sure it doesn't run?

Since irqs are converted into events and executed in event thread 
context, as long as no irq disabled then event q will be emptied 
accordingly.

Also, i think synchronize_irq() have higher risk of deadlocking if 
resources are hold by others.

>> Fixes: ba0a422be723 ("drm/msm/dp: do not initialize phy until plugin interrupt received")
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_ctrl.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 38026f2..cbf3399 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1379,8 +1379,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
>>
>>          dp_catalog_ctrl_reset(ctrl->catalog);
>>
>> -       if (enable)
>> -               dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
>> +       /*
>> +        * all dp controller programmable registers will not
>> +        * be reset to default value after DP_SW_RESET
>> +        * therefore interrupt mask bits have to be updated
>> +        * to enable/disable interrupts
>> +        */
>> +       dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
>>   }
>>
>>   void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>

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

end of thread, other threads:[~2022-05-09 21:56 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-06 21:41 [PATCH] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl() Kuogee Hsieh
2022-05-07  0:29 ` Stephen Boyd
2022-05-09 21:56   ` Kuogee Hsieh

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