linux-kernel - Re: [PATCH v2 2/3] drm/panel: panel-edp: Match edp

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: <CAD=FV=VMVr+eJ7eyuLGa671fMgH6ZX9zPOkbKzYJ0H79MZ2k9A@mail.gmail.com>
Date: Wed, 28 Feb 2024 16:21:37 -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 2/3] drm/panel: panel-edp: Match edp_panels with panel name

Hi,

On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@...omium.org> wrote:
>
> @@ -2107,13 +2113,41 @@ static const struct edp_panel_entry edp_panels[] = {
>         { /* sentinal */ }
>  };
>
> -static const struct edp_panel_entry *find_edp_panel(u32 panel_id)
> +static bool edid_has_name(struct edid *edid, const char *panel_name)
> +{
> +       int i, j;
> +       char buf[13];
> +

Should have some comment about why this can't use
drm_edid_get_monitor_name(). AKA because panels seem to be storing the
monitor name tagged as raw strings instead of as the name. Should
probably also have some of the other checks from
is_display_descriptor() like checking for clock of 0 and pad1 of 0.


> +       for (i = 0; i < 4; i++) {

Instead of 4, I think this can be ARRAY_SIZE(edid->detailed_timings), right?


> +               strncpy(buf, edid->detailed_timings[i].data.other_data.data.str.str,
> +                       sizeof(buf));

I can never quite remember which of the strXcpy() routines are frowned
upon and which are the golden children at the moment. ...but I don't
think we really need the copy nor the local buffer anyway, right?
You're already going through this thing 1 byte at a time so just
compare it straight away.


> +               for (j = 0; j < 13; j++) {
> +                       if (buf[j] == 0x0a) {

Instead of hardcoding 0x0a, I think you want '\n', no?


> +                               buf[j] = '\0';
> +                               break;
> +                       }
> +               }
> +               buf[12] = '\0';
> +               if (strncmp(panel_name, buf, strlen(panel_name)) == 0)
> +                       return true;

Untested, but I think with my suggestions above the function becomes this:

static bool edid_has_name(struct edid *edid, const char *panel_name)
{
    int i, j;

    /*
     * We can't use drm_edid_get_monitor_name() since many eDP panels store
     * their name as a raw string. We'll accept either a string or name
     * match as long as the panel ID also matches.
     */
    for (i = 0; i < ARRAY_SIZE(edid->detailed_timings); i++) {
        struct detailed_timing *timing = &edid->detailed_timings[i];

        if (timing->pixel_clock != 0 ||
            timing->data.other_data.pad1 != 0 ||
            (timing->data.other_data.type != EDID_DETAIL_MONITOR_NAME &&
             timing->data.other_data.type != EDID_DETAIL_MONITOR_STRING))
                continue;

        for (j = 0; j < ARRAY_SIZE(timing->data.other_data.data.str.str); j++) {
            const char *str = timing->data.other_data.data.str.str;

            if (panel_name[j] == '\0') {
                if (str[j] == '\0' || str[j] == '\n')
                    return true;
                break;
            }
        }
        if (j == ARRAY_SIZE(timing->data.other_data.data.str.str) &&
            panel_name[j] == '\0')
            return true;
    }

    return false;
}