linux-kernel - Re: [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 20 Apr 2021 15:01:02 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Kuogee Hsieh <khsieh@...eaurora.org>, robdclark@...il.com,
        sean@...rly.run
Cc:     abhinavk@...eaurora.org, aravindh@...eaurora.org,
        khsieh@...eaurora.org, airlied@...ux.ie, daniel@...ll.ch,
        linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending

Quoting Kuogee Hsieh (2021-04-16 13:27:57)
> Some dongle may generate more than one irq_hpd events in a short period of
> time. This patch will treat those irq_hpd events as single one and service
> only one irq_hpd event.

Why is it bad to get multiple irq_hpd events in a short period of time?
Please tell us here in the commit text.

> 
> Signed-off-by: Kuogee Hsieh <khsieh@...eaurora.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5a39da6..0a7d383 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -707,6 +707,9 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
>                 return 0;
>         }
>  
> +       /* only handle first irq_hpd in case of multiple irs_hpd pending */
> +       dp_del_event(dp, EV_IRQ_HPD_INT);
> +
>         ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
>         if (ret == -ECONNRESET) { /* cable unplugged */
>                 dp->core_initialized = false;
> @@ -1300,6 +1303,9 @@ static int dp_pm_suspend(struct device *dev)
>         /* host_init will be called at pm_resume */
>         dp->core_initialized = false;
>  
> +       /* system suspended, delete pending irq_hdps */
> +       dp_del_event(dp, EV_IRQ_HPD_INT);

What happens if I suspend my device and when this function is running I
toggle my monitor to use the HDMI input that is connected instead of some
other input, maybe the second HDMI input? Wouldn't that generate an HPD
interrupt to grab the attention of this device?

> +
>         mutex_unlock(&dp->event_mutex);
>  
>         return 0;
> @@ -1496,6 +1502,9 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
>         /* stop sentinel checking */
>         dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
>  
> +       /* link is down, delete pending irq_hdps */
> +       dp_del_event(dp_display, EV_IRQ_HPD_INT);
> +

I'm becoming convinced that the whole kthread design and event queue is
broken. These sorts of patches are working around the larger problem
that the kthread is running independently of the driver and irqs can
come in at any time but the event queue is not checked from the irq
handler to debounce the irq event. Is the event queue necessary at all?
I wonder if it would be simpler to just use an irq thread and process
the hpd signal from there. Then we're guaranteed to not get an irq again
until the irq thread is done processing the event. This would naturally
debounce the irq hpd event that way.

>         dp_display_disable(dp_display, 0);
>  
>         rc = dp_display_unprepare(dp);