On Thu, 07 Mar 2024, Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx> wrote:
> 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.
Just so it's clear here too: Agreed.
>
> https://lore.kernel.org/lkml/CAA8EJpr7LHvqeGXhbFQ8KNn0LGDuv19cw0i04qVUz51TJeSQrA@xxxxxxxxxxxxxx/
>
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > -Doug
>>
>> --
>> Jani Nikula, Intel
--
Jani Nikula, Intel
