Received-SPF: pass (google.com: domain of linux-kernel+bounces-94784-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
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> <CAJMQK-i=0COuMGW+PGv3zT4+JgwJc_Qj9oQHva6EQys_n3xoHA@mail.gmail.com>
In-Reply-To: <CAJMQK-i=0COuMGW+PGv3zT4+JgwJc_Qj9oQHva6EQys_n3xoHA@mail.gmail.com>
From: Doug Anderson <dianders@chromium.org>
Date: Wed, 6 Mar 2024 16:37:43 -0800
Message-ID: <CAD=FV=VHaU4HZHGp6tSoVuJRbYD9nrMZfNdnOait=ApRcvcmug@mail.gmail.com>
Subject: Re: [PATCH v5 3/6] drm/edid: Add a function to match EDID with identity
To: Hsin-Yi Wang <hsinyi@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

Hi,

On Wed, Mar 6, 2024 at 4:20=E2=80=AFPM Hsin-Yi Wang <hsinyi@chromium.org> w=
rote:
>
> On Wed, Mar 6, 2024 at 3:30=E2=80=AFPM Doug Anderson <dianders@chromium.o=
rg> wrote:
> >
> > Hi,
> >
> > On Wed, Mar 6, 2024 at 12:04=E2=80=AFPM Hsin-Yi Wang <hsinyi@chromium.o=
rg> 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.=
data.str.str);
> > > +
> > > +       if (name_len > desc_len ||
> > > +           !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME)=
 ||
> > > +             is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRIN=
G)))
> > > +               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.

Ah, OK. I guess the EDID spec simply doesn't allow for '\0' in there.
I guess in that case I'd prefer simply removing the code to handle
'\0' instead of treating it like space until we see some actual need
for it. So just get rid of the "!desc[i]" case?

-Doug