On Thu, Mar 7, 2024 at 5:20 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> 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
