Received-SPF: pass (google.com: domain of linux-kernel+bounces-95586-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Jani Nikula <jani.nikula@linux.intel.com>
To: Doug Anderson <dianders@chromium.org>, Hsin-Yi Wang <hsinyi@chromium.org>
Cc: 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
Subject: Re: [PATCH v5 3/6] drm/edid: Add a function to match EDID with
 identity
In-Reply-To: <CAD=FV=VHaU4HZHGp6tSoVuJRbYD9nrMZfNdnOait=ApRcvcmug@mail.gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
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>
 <CAD=FV=VHaU4HZHGp6tSoVuJRbYD9nrMZfNdnOait=ApRcvcmug@mail.gmail.com>
Date: Thu, 07 Mar 2024 15:20:27 +0200
Message-ID: <87r0gmw544.fsf@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On Wed, 06 Mar 2024, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Wed, Mar 6, 2024 at 4:20=E2=80=AFPM Hsin-Yi Wang <hsinyi@chromium.org>=
 wrote:
>>
>> 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=
data.str.str);
>> > > +
>> > > +       if (name_len > desc_len ||
>> > > +           !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME=
) ||
>> > > +             is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRI=
NG)))
>> > > +               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?

The spec text, similar for both EDID_DETAIL_MONITOR_NAME and
EDID_DETAIL_MONITOR_STRING:

	Up to 13 alphanumeric characters (using ASCII codes) may be used
	to define the model name of the display product. The data shall
	be sequenced such that the 1st byte (ASCII code) =3D the 1st
	character, the 2nd byte (ASCII code) =3D the 2nd character,
	etc. If there are less than 13 characters in the string, then
	terminate the display product name string with ASCII code =E2=80=980Ah=E2=
=80=99
	(line feed) and pad the unused bytes in the field with ASCII
	code =E2=80=9820h=E2=80=99 (space).

In theory, only checking for '\n' for termination should be enough, and
this is what drm_edid_get_monitor_name() does. If there's a space
*before* that, it should be considered part of the name, and not
ignored. (So my suggestion in reply to the previous version is wrong.)

However, since the match name uses NUL termination, maybe we should
ignore NULs *before* '\n'? Like so:

for (i =3D name_len; i < desc_len; i++) {
        if (desc[i] =3D=3D '\n')
                break;
        if (!desc[i])
                return;
}


BR,
Jani.


>
> -Doug

--=20
Jani Nikula, Intel