linux-kernel - Re: [PATCH] drm/edid: Dump the EDID when drm_edid_get_panel

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, 11 Nov 2022 12:45:58 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Abhinav Kumar <quic_abhinavk@...cinc.com>
Cc:     dri-devel@...ts.freedesktop.org,
        Kuogee Hsieh <quic_khsieh@...cinc.com>,
        Stephen Boyd <swboyd@...omium.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/edid: Dump the EDID when drm_edid_get_panel_id() has
 an error

Hi,

On Tue, Oct 25, 2022 at 1:39 PM Abhinav Kumar <quic_abhinavk@...cinc.com> wrote:
>
> Hi Doug
>
> On 10/24/2022 1:28 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Oct 21, 2022 at 2:18 PM Abhinav Kumar <quic_abhinavk@...cinc.com> wrote:
> >>
> >> Hi Doug
> >>
> >> On 10/21/2022 1:07 PM, Douglas Anderson wrote:
> >>> If we fail to get a valid panel ID in drm_edid_get_panel_id() we'd
> >>> like to see the EDID that was read so we have a chance of
> >>> understanding what's wrong. There's already a function for that, so
> >>> let's call it in the error case.
> >>>
> >>> NOTE: edid_block_read() has a retry loop in it, so actually we'll only
> >>> print the block read back from the final attempt. This still seems
> >>> better than nothing.
> >>>
> >>> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> >>
> >> Instead of checkinf for edid_block_status_valid() on the base_block, do
> >> you want to use drm_edid_block_valid() instead?
> >>
> >> That way you get the edid_block_dump() for free if it was invalid.
> >
> > I can... ...but it feels a bit awkward and maybe not quite how the
> > functions were intended to work together?
> >
> > One thing I notice is that if I call drm_edid_block_valid() I'm doing
> > a bunch of duplicate work that already happened in edid_block_read(),
> > which already calls edid_block_check() and handles fixing headers. I
> > guess also if I call drm_edid_block_valid() then I should ignore the
> > "status" return value of edid_block_read() because we don't need to
> > pass it anywhere (because the work is re-done in
> > drm_edid_block_valid()).
> >
> > So I guess I'm happy to do a v2 like that if everyone likes it better,
> > but to me it feels a little weird.
> >
> > -Doug
>
> Alright, agreed. There is some duplication of code happening if we use
> drm_edid_block_valid(). I had suggested that because it has inherent
> support for dumping the bad EDID.
>
> In that case, this change LGTM, because in principle you are doing the
> same thing as _drm_do_get_edid() (with the only difference being here we
> read only the base block as opposed to the full EDID there).
>
> Hence,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@...cinc.com>

I've given this patch a bunch of time because it wasn't urgent, but
seems like it could be about time to land. I'll plan to land it next
Monday or Tuesday unless anyone has any other comments.

Thanks!

-Doug