Received-SPF: pass (google.com: domain of linux-kernel+bounces-94775-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240306200353.1436198-1-hsinyi@chromium.org> <20240306200353.1436198-4-hsinyi@chromium.org>
 <CAD=FV=W2CKoOyhN49RBU0FdzcRC6SEwvVQYdJRnBBK16Lp-=FQ@mail.gmail.com>
In-Reply-To: <CAD=FV=W2CKoOyhN49RBU0FdzcRC6SEwvVQYdJRnBBK16Lp-=FQ@mail.gmail.com>
From: Hsin-Yi Wang <hsinyi@chromium.org>
Date: Wed, 6 Mar 2024 16:20:08 -0800
Message-ID: <CAJMQK-i=0COuMGW+PGv3zT4+JgwJc_Qj9oQHva6EQys_n3xoHA@mail.gmail.com>
Subject: Re: [PATCH v5 3/6] drm/edid: Add a function to match EDID with identity
To: Doug Anderson <dianders@chromium.org>
Cc: Jani Nikula <jani.nikula@linux.intel.com>, 
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Neil Armstrong <neil.armstrong@linaro.org>, 
	Jessica Zhang <quic_jesszhan@quicinc.com>, Sam Ravnborg <sam@ravnborg.org>, 
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, 
	Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, 
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 6, 2024 at 3:30=E2=80=AFPM Doug Anderson <dianders@chromium.org=
> wrote:
>
> Hi,
>
> On Wed, Mar 6, 2024 at 12:04=E2=80=AFPM Hsin-Yi Wang <hsinyi@chromium.org=
> wrote:
> >
> > +static void
> > +match_identity(const struct detailed_timing *timing, void *data)
> > +{
> > +       struct drm_edid_match_closure *closure =3D data;
> > +       unsigned int i;
> > +       const char *name =3D closure->ident->name;
> > +       unsigned int name_len =3D strlen(name);
> > +       const char *desc =3D timing->data.other_data.data.str.str;
> > +       unsigned int desc_len =3D ARRAY_SIZE(timing->data.other_data.da=
ta.str.str);
> > +
> > +       if (name_len > desc_len ||
> > +           !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) |=
|
> > +             is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING)=
))
> > +               return;
> > +
> > +       if (strncmp(name, desc, name_len))
> > +               return;
> > +
> > +       /* Allow trailing white spaces and \0. */
> > +       for (i =3D name_len; i < desc_len; i++) {
> > +               if (desc[i] =3D=3D '\n')
> > +                       break;
> > +               if (!isspace(desc[i]) && !desc[i])
> > +                       return;
> > +       }
>
> If my code analysis is correct, I think you'll reject the case where:
>
> name =3D "foo"
> desc[13] =3D "foo \0zzzzzzzz"
>
> ...but you'll accept these cases:
>
> desc[13] =3D "foo \nzzzzzzzz"
> desc[13] =3D "foo \0\0\0\0\0\0\0\0\0"
>
> It somehow seems weird to me that a '\n' terminates the string but not a =
'\0'.

I'm also not sure about \0... based on
https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493,
they use \n as terminator. Maybe we should also reject \0 before\n?
Since it's not printable.

>
> I would have done:
>
>       for (i =3D name_len; i < desc_len; i++) {
>               /* Consider \n or \0 to terminate the string */
>               if (desc[i] =3D=3D '\n' || desc[i] =3D=3D '\0')
>                       break;
>               /* OK for spaces at the end, but non-space is a fail */
>               if (!isspace(desc[i]))
>                       return;
>       }
>
>
> > @@ -367,6 +367,12 @@ struct edid {
> >         u8 checksum;
> >  } __attribute__((packed));
> >
> > +/* EDID matching */
> > +struct drm_edid_ident {
> > +       u32 panel_id;
> > +       const char *name;
>
> Might not hurt to have a comment for panel_id saying that it's encoded
> by drm_edid_encode_panel_id() so it's obvious what this random u32 is.
>
>
> -Doug