linux-kernel - Re: [PATCH v4 1/2] drm/msm/dp: do not complete dp_aux_cmd_fifo

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:   Fri, 16 Dec 2022 01:04:40 +0200
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Kuogee Hsieh <quic_khsieh@...cinc.com>
Cc:     dri-devel@...ts.freedesktop.org, robdclark@...il.com,
        sean@...rly.run, swboyd@...omium.org, dianders@...omium.org,
        vkoul@...nel.org, daniel@...ll.ch, airlied@...il.com,
        agross@...nel.org, andersson@...nel.org, quic_abhinavk@...cinc.com,
        quic_sbillaka@...cinc.com, freedreno@...ts.freedesktop.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/2] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx()
 if irq is not for aux transfer

On Fri, 16 Dec 2022 at 00:53, Kuogee Hsieh <quic_khsieh@...cinc.com> wrote:
>
> There are 3 possible interrupt sources are handled by DP controller,
> HPDstatus, Controller state changes and Aux read/write transaction.
> At every irq, DP controller have to check isr status of every interrupt
> sources and service the interrupt if its isr status bits shows interrupts
> are pending. There is potential race condition may happen at current aux
> isr handler implementation since it is always complete dp_aux_cmd_fifo_tx()
> even irq is not for aux read or write transaction. This may cause aux read
> transaction return premature if host aux data read is in the middle of
> waiting for sink to complete transferring data to host while irq happen.
> This will cause host's receiving buffer contains unexpected data. This
> patch fixes this problem by checking aux isr and return immediately at
> aux isr handler if there are no any isr status bits set.
>
> Current there is a bug report regrading eDP edid corruption happen during
> system booting up. After lengthy debugging to found that that VIDEO_READY
> interrupt was continuously firing during system booting up which cause
> dp_aux_isr() to complete dp_aux_cmd_fifo_tx() prematurely to retrieve data
> from aux hardware buffer which is not yet contains complete data transfer
> from sink. This cause edid corruption.
>
> Follows are the signature at kernel logs when problem happen,
> EDID has corrupt header
> panel-simple-dp-aux aux-aea0000.edp: Couldn't identify panel via EDID
> panel-simple-dp-aux aux-aea0000.edp: error -EIO: Couldn't detect panel nor find a fallback
>
> Changes in v2:
> -- do complete if (ret == IRQ_HANDLED) ay dp-aux_isr()
> -- add more commit text
>
> Changes in v3:
> -- add Stephen suggested
> -- dp_aux_isr() return IRQ_XXX back to caller
> -- dp_ctrl_isr() return IRQ_XXX back to caller
>
> Changes in v4:
> -- split into two patches
>
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
>

The same feedback as the one I gave for v2 or v3:

There should be no empty lines between tags. And you still have one
empty line here.

With that fixed:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>

> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
> Tested-by: Douglas Anderson <dianders@...omium.org>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@...cinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index d030a93..cc3efed 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -423,6 +423,10 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
>
>         isr = dp_catalog_aux_get_irq(aux->catalog);
>
> +       /* no interrupts pending, return immediately */
> +       if (!isr)
> +               return;
> +
>         if (!aux->cmd_busy)
>                 return;
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>


-- 
With best wishes
Dmitry