linux-kernel - Re: [PATCH v2 1/3] drm_edid: Support getting EDID through ddc without connector

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 for Android: free password hash cracker in your pocket

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

Date: Wed, 28 Feb 2024 16:21:11 -0800
From: Doug Anderson <dianders@...omium.org>
To: Hsin-Yi Wang <hsinyi@...omium.org>
Cc: Neil Armstrong <neil.armstrong@...aro.org>, Jessica Zhang <quic_jesszhan@...cinc.com>, 
	Sam Ravnborg <sam@...nborg.org>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] drm_edid: Support getting EDID through ddc without connector

Hi,

On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@...omium.org> wrote:
>
> Some panels are interested in the EDID during early probe when connector
> is still unknown.
>
> Add a function drm_get_edid_no_connector() to get edid without connector.
> No functional change for existing usage.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@...omium.org>
> ---
> v1->v2:
> add a function to return the entire edid without updating connector.
> ---
>  drivers/gpu/drm/drm_edid.c | 45 ++++++++++++++++++++++++++++----------
>  include/drm/drm_edid.h     |  1 +
>  2 files changed, 34 insertions(+), 12 deletions(-)

I'll respond in the discussion in v1 too, but overall I'm not a fan of
reading the whole EDID twice at bootup. Personally I'd love to see us
to back to just reading the base block like in v1, but I guess we can
see what Jani and others say.


> @@ -2385,18 +2385,20 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>         if (status == EDID_BLOCK_READ_FAIL)
>                 goto fail;
>
> -       /* FIXME: Clarify what a corrupt EDID actually means. */
> -       if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> -               connector->edid_corrupt = false;
> -       else
> -               connector->edid_corrupt = true;
> +       if (connector) {
> +               /* FIXME: Clarify what a corrupt EDID actually means. */
> +               if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> +                       connector->edid_corrupt = false;
> +               else
> +                       connector->edid_corrupt = true;
>
> -       if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> -               if (status == EDID_BLOCK_ZERO)
> -                       connector->null_edid_counter++;
> +               if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> +                       if (status == EDID_BLOCK_ZERO)
> +                               connector->null_edid_counter++;
>
> -               connector_bad_edid(connector, edid, 1);
> -               goto fail;
> +                       connector_bad_edid(connector, edid, 1);
> +                       goto fail;

This "goto fail" is now only run "if (connector)" which means that
you're not properly checking if the EDID is valid when "connector ==
NULL", right? That seems like a bug unless I missed something...