Re: [PATCH v5 3/6] drm/edid: Add a function to match EDID with identity

[Hsin-Yi Wang]

On Thu, Mar 7, 2024 at 5:20 AM Jani Nikula <jani.nikula@linux.intelcom> wrote:
>
> On Wed, 06 Mar 2024, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> > Hi,
> >
> > On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> wrote:
> >>
> >> On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> >> >
> >> > Hi,
> >> >
> >> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> wrote:
> >> > >
> >> > > +static void
> >> > > +match_identity(const struct detailed_timing *timing, void *data)
> >> > > +{
> >> > > +       struct drm_edid_match_closure *closure = data;
> >> > > +       unsigned int i;
> >> > > +       const char *name = closure->ident->name;
> >> > > +       unsigned int name_len = strlen(name);
> >> > > +       const char *desc = timing->data.other_data.data.str.str;
> >> > > +       unsigned int desc_len = 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_STRING)))
> >> > > +               return;
> >> > > +
> >> > > +       if (strncmp(name, desc, name_len))
> >> > > +               return;
> >> > > +
> >> > > +       /* Allow trailing white spaces and \0. */
> >> > > +       for (i = name_len; i < desc_len; i++) {
> >> > > +               if (desc[i] == '\n')
> >> > > +                       break;
> >> > > +               if (!isspace(desc[i]) && !desc[i])
> >> > > +                       return;
> >> > > +       }
> >> >
> >> > If my code analysis is correct, I think you'll reject the case where:
> >> >
> >> > name = "foo"
> >> > desc[13] = "foo \0zzzzzzzz"
> >> >
> >> > ...but you'll accept these cases:
> >> >
> >> > desc[13] = "foo \nzzzzzzzz"
> >> > desc[13] = "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) = the 1st
>         character, the 2nd byte (ASCII code) = the 2nd character,
>         etc. If there are less than 13 characters in the string, then
>         terminate the display product name string with ASCII code ‘0Ah’
>         (line feed) and pad the unused bytes in the field with ASCII
>         code ‘20h’ (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 = name_len; i < desc_len; i++) {
>         if (desc[i] == '\n')
>                 break;
>         if (!desc[i])
>                 return;
> }
>
Allow trailing white spaces so we don't need to add the trailing white
space in edp_panel_entry.

https://lore.kernel.org/lkml/CAA8EJpr7LHvqeGXhbFQ8KNn0LGDuv19cw0i04qVUz51TJeSQrA@xxxxxxxxxxxxxx/

>
> BR,
> Jani.
>
>
> >
> > -Doug
>
> --
> Jani Nikula, Intel