Den 13.10.2022 15.18, skrev Maxime Ripard:
> The current construction of the named mode parsing doesn't allow to extend
> it easily. Let's move it to a separate function so we can add more
> parameters and modes.
>
> In order for the tests to still pass, some extra checks are needed, so
> it's not a 1:1 move.
>
> Signed-off-by: Maxime Ripard <[email protected]>
>
I was hoping that someone else would step up and review these parser
patches since the parser code is rather difficult to read, for me at
least. I have studied it now, so I'll give it a try.
> ---
> Changes in v4:
> - Fold down all the named mode patches that were split into a single
> patch again to maintain bisectability
> ---
> drivers/gpu/drm/drm_modes.c | 73 ++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index c0dceff51cac..2f020ef2ddf2 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2229,6 +2229,55 @@ static const char * const drm_named_modes_whitelist[] = {
> "PAL",
> };
>
> +static int drm_mode_parse_cmdline_named_mode(const char *name,
> + unsigned int name_end,
> + struct drm_cmdline_mode *cmdline_mode)
> +{
> + unsigned int i;
> +
> + if (!name_end)
> + return 0;
name_end can't be zero since the argument is checked before calling this
function.
> +
> + /* If the name starts with a digit, it's not a named mode */
> + if (isdigit(name[0]))
> + return 0;
> +
> + /*
> + * If there's an equal sign in the name, the command-line
> + * contains only an option and no mode.
> + */
> + if (strnchr(name, name_end, '='))
> + return 0;
I think this check actually belongs in
drm_mode_parse_command_line_for_connector() after options_off is set. If
theres an equal sign it should skip all mode parsing and skip down to
drm_mode_parse_cmdline_options(). Which probably means that the mode
parsing should have been moved out to separate function to avoid using a
goto.
But that's probably beyond the scope of this patchset :)
> +
> +#define STR_STRICT_EQ(str, len, cmp) \
> + (str_has_prefix(str, cmp) == len)
> +
> + /* The connection status extras can be set without a mode. */
> + if (STR_STRICT_EQ(name, name_end, "d") ||
> + STR_STRICT_EQ(name, name_end, "D") ||
> + STR_STRICT_EQ(name, name_end, "e"))
> + return 0;
It took me a while to understand what is going on here.
If str_has_prefix() finds a match it returns strlen(prefix). Since
prefix is always of length 1, name_end has to always be 1 for the
statement to be true.
I would have written it like this:
/* The connection status extras can be set without a mode. */
if (name_end == 1) {
if (name[0] == "d" || name[0] == "D" || name[0] == "e")
return 0;
}
> +
> + /*
> + * We're sure we're a named mode at that point, iterate over the
that -> this ?
> + * list of modes we're aware of.
> + */
> + for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> + int ret;
> +
> + ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> + if (ret != name_end)
> + continue;
> +
> + strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
> + cmdline_mode->specified = true;
> +
> + return 1;
> + }
> +
> + return -EINVAL;
> +}
> +
> /**
> * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
> * @mode_option: optional per connector mode option
> @@ -2265,7 +2314,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> const char *options_ptr = NULL;
> char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
> - int i, len, ret;
> + int len, ret;
>
> memset(mode, 0, sizeof(*mode));
> mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> @@ -2306,17 +2355,19 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> parse_extras = true;
> }
>
> - /* First check for a named mode */
> - for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> - ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> - if (ret == mode_end) {
> - if (refresh_ptr)
> - return false; /* named + refresh is invalid */
>
> - strcpy(mode->name, drm_named_modes_whitelist[i]);
> - mode->specified = true;
> - break;
> - }
> + if (mode_end) {
Shouldn't this be:
if (!mode_end)
return false;
I can't see how mode_end == 0 can produce anything valid.
Scenarios for mode_option that gives mode_end == 0:
- zero length
- starts with a comma
- starts with -bpp
- starts with @refresh
Maybe it's a separate patch with matching tests ;)
Noralf.
> + ret = drm_mode_parse_cmdline_named_mode(name, mode_end, mode);
> + if (ret < 0)
> + return false;
> +
> + /*
> + * Having a mode that starts by a letter (and thus is named)
> + * and an at-sign (used to specify a refresh rate) is
> + * disallowed.
> + */
> + if (ret && refresh_ptr)
> + return false;
> }
>
> /* No named mode? Check for a normal mode argument, e.g. 1024x768 */
>
On Sun, Oct 16, 2022 at 06:11:21PM +0200, Noralf Tr?nnes wrote:
> Den 13.10.2022 15.18, skrev Maxime Ripard:
> > The current construction of the named mode parsing doesn't allow to extend
> > it easily. Let's move it to a separate function so we can add more
> > parameters and modes.
> >
> > In order for the tests to still pass, some extra checks are needed, so
> > it's not a 1:1 move.
> >
> > Signed-off-by: Maxime Ripard <[email protected]>
> >
>
> I was hoping that someone else would step up and review these parser
> patches since the parser code is rather difficult to read, for me at
> least. I have studied it now, so I'll give it a try.
>
> > ---
> > Changes in v4:
> > - Fold down all the named mode patches that were split into a single
> > patch again to maintain bisectability
> > ---
> > drivers/gpu/drm/drm_modes.c | 73 ++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 62 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index c0dceff51cac..2f020ef2ddf2 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -2229,6 +2229,55 @@ static const char * const drm_named_modes_whitelist[] = {
> > "PAL",
> > };
> >
> > +static int drm_mode_parse_cmdline_named_mode(const char *name,
> > + unsigned int name_end,
> > + struct drm_cmdline_mode *cmdline_mode)
> > +{
> > + unsigned int i;
> > +
> > + if (!name_end)
> > + return 0;
>
> name_end can't be zero since the argument is checked before calling this
> function.
I'd really like to keep it in though. At least, we know by looking at
this small function that we're going to be safe all the time, no matter
what the caller does.
And if the caller wants to check it as well, fine, it's only a simple
comparison ran once or twice at boot, it's not like it's in a hot-path.
I've addressed your other comments, thanks!
Maxime