linux-kernel - Re: [PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if nothing connected

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]
Message-ID: <CABXOdTfNaorDgyZT8owH6ypYpZMhVpUEcK8N4Mbw2UumA8Db2g@mail.gmail.com>
Date: Tue, 12 Mar 2024 17:46:44 -0700
From: Guenter Roeck <groeck@...gle.com>
To: Douglas Anderson <dianders@...omium.org>
Cc: Rob Clark <robdclark@...il.com>, Abhinav Kumar <quic_abhinavk@...cinc.com>, 
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, Bjorn Andersson <andersson@...nel.org>, 
	Daniel Vetter <daniel@...ll.ch>, David Airlie <airlied@...il.com>, Guenter Roeck <groeck@...omium.org>, 
	Kuogee Hsieh <quic_khsieh@...cinc.com>, Marijn Suijten <marijn.suijten@...ainline.org>, 
	Sean Paul <sean@...rly.run>, Tanmay Shah <tanmay@...eaurora.org>, 
	Vinod Polimera <quic_vpolimer@...cinc.com>, dri-devel@...ts.freedesktop.org, 
	freedreno@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] drm/msm/dp: Avoid a long timeout for AUX transfer if
 nothing connected

On Tue, Mar 12, 2024 at 5:14 PM Douglas Anderson <dianders@...omiumorg> wrote:
>
> As documented in the description of the transfer() function of
> "struct drm_dp_aux", the transfer() function can be called at any time
> regardless of the state of the DP port. Specifically if the kernel has
> the DP AUX character device enabled and userspace accesses
> "/dev/drm_dp_auxN" directly then the AUX transfer function will be
> called regardless of whether a DP device is connected.
>
> For eDP panels we have a special rule where we wait (with a 5 second
> timeout) for HPD to go high. This rule was important before all panels
> drivers were converted to call wait_hpd_asserted() and actually can be
> removed in a future commit.
>
> For external DP devices we never checked for HPD. That means that
> trying to access the DP AUX character device (AKA `hexdump -C
> /dev/drm_dp_auxN`) would very, very slowly timeout. Specifically on my
> system:
>   $ time hexdump -C /dev/drm_dp_aux0
>   hexdump: /dev/drm_dp_aux0: Connection timed out
>
>   real    0m8.200s
>
> Let's add a check for HPD to avoid the slow timeout. This matches
> what, for instance, the intel_dp_aux_xfer() function does when it
> calls intel_tc_port_connected_locked(). That call has a document by it
> explaining that it's important to avoid the long timeouts.
>
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
>
>  drivers/gpu/drm/msm/dp/dp_aux.c     |  8 +++++++-
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 10 ++++++++++
>  drivers/gpu/drm/msm/dp/dp_catalog.h |  1 +
>  3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 03f4951c49f4..de0b0eabced9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -307,7 +307,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>          * turned on the panel and then tried to do an AUX transfer. The panel
>          * driver has no way of knowing when the panel is ready, so it's up
>          * to us to wait. For DP we never get into this situation so let's
> -        * avoid ever doing the extra long wait for DP.
> +        * avoid ever doing the extra long wait for DP and just query HPD
> +        * directly.
>          */
>         if (aux->is_edp) {
>                 ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> @@ -315,6 +316,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>                         DRM_DEBUG_DP("Panel not ready for aux transactions\n");
>                         goto exit;
>                 }
> +       } else {
> +               if (!dp_catalog_aux_is_hpd_connected(aux->catalog)) {
> +                       ret = -ENXIO;
> +                       goto exit;
> +               }
>         }
>
>         dp_aux_update_offset_and_segment(aux, msg);
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 5142aeb705a4..93e2d413a1e7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -266,6 +266,16 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
>                                 2000, 500000);
>  }
>
> +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog)
> +{
> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
> +                               struct dp_catalog_private, dp_catalog);
> +
> +       /* poll for hpd connected status every 2ms and timeout after 500ms */

Maybe I am missing something, but the comment doesn't seem to match
the code below.

Guenter

> +       return readl(catalog->io->dp_controller.aux.base + REG_DP_DP_HPD_INT_STATUS) &
> +              DP_DP_HPD_STATE_STATUS_CONNECTED;
> +}
> +
>  static void dump_regs(void __iomem *base, int len)
>  {
>         int i;
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 38786e855b51..1694040c530f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -86,6 +86,7 @@ void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
>  void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
>  void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
>  int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
> +bool dp_catalog_aux_is_hpd_connected(struct dp_catalog *dp_catalog);
>  u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
>
>  /* DP Controller APIs */
> --
> 2.44.0.278.ge034bb2e1d-goog
>