Re: [PATCH v5 08/22] drm/modes: Move named modes parsing to a separate function

[Noralf Trønnes]

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 <maxime@xxxxxxxxxx>
> 

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 */
>