Re: [PATCH 1/5] drm/modes: Rewrite the command line parser

[Sean Paul]

On Tue, Oct 18, 2016 at 4:29 AM, Maxime Ripard
<maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> Rewrite the command line parser in order to get away from the state machine
> parsing the video mode lines.
>
> Hopefully, this will allow to extend it more easily to support named modes
> and / or properties set directly on the command line.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_modes.c | 305 +++++++++++++++++++++++--------------
>  1 file changed, 190 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 53f07ac7c174..7d5bdca276f2 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -30,6 +30,7 @@
>   * authorization from the copyright holder(s) and author(s).
>   */
>
> +#include <linux/ctype.h>
>  #include <linux/list.h>
>  #include <linux/list_sort.h>
>  #include <linux/export.h>
> @@ -1261,6 +1262,131 @@ void drm_mode_connector_list_update(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_mode_connector_list_update);
>
> +static int drm_mode_parse_cmdline_bpp(const char *str, char **end_ptr,
> +                                     struct drm_cmdline_mode *mode)
> +{
> +       if (str[0] != '-')
> +               return -EINVAL;
> +
> +       mode->bpp = simple_strtol(str + 1, end_ptr, 10);
> +       mode->bpp_specified = true;
> +
> +       return 0;
> +}
> +
> +static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr,
> +                                         struct drm_cmdline_mode *mode)
> +{
> +       if (str[0] != '@')
> +               return -EINVAL;
> +
> +       mode->refresh = simple_strtol(str + 1, end_ptr, 10);
> +       mode->refresh_specified = true;
> +
> +       return 0;
> +}
> +
> +static int drm_mode_parse_cmdline_extra(const char *str, int length,
> +                                       struct drm_connector *connector,
> +                                       struct drm_cmdline_mode *mode)
> +{
> +       int i;
> +
> +       for (i = 0; i < length; i++) {
> +               switch (str[i]) {
> +               case 'i':
> +                       mode->interlace = true;
> +                       break;
> +               case 'm':
> +                       mode->margins = true;
> +                       break;
> +               case 'D':
> +                       if (mode->force != DRM_FORCE_UNSPECIFIED)
> +                               return -EINVAL;
> +
> +                       if ((connector->connector_type != DRM_MODE_CONNECTOR_DVII) &&
> +                           (connector->connector_type != DRM_MODE_CONNECTOR_HDMIB))
> +                               mode->force = DRM_FORCE_ON;
> +                       else
> +                               mode->force = DRM_FORCE_ON_DIGITAL;
> +                       break;
> +               case 'd':
> +                       if (mode->force != DRM_FORCE_UNSPECIFIED)
> +                               return -EINVAL;
> +
> +                       mode->force = DRM_FORCE_OFF;
> +                       break;
> +               case 'e':
> +                       if (mode->force != DRM_FORCE_UNSPECIFIED)
> +                               return -EINVAL;
> +
> +                       mode->force = DRM_FORCE_ON;
> +                       break;
> +               default:
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
> +                                          bool extras,
> +                                          struct drm_connector *connector,
> +                                          struct drm_cmdline_mode *mode)
> +{
> +       bool rb = false, cvt = false;
> +       int xres = 0, yres = 0;
> +       int remaining, i;
> +       char *end_ptr;
> +
> +       xres = simple_strtol(str, &end_ptr, 10);
> +

checkpatch is telling me to use kstrtol instead, as simple_strtol is deprecated

> +       if (end_ptr[0] != 'x')

check that end_ptr != NULL? you should probably also check that xres
isn't an error (ie: -ERANGE or -EINVAL)

> +               return -EINVAL;
> +       end_ptr++;
> +
> +       yres = simple_strtol(end_ptr, &end_ptr, 10);

check end_ptr != NULL and yres sane

> +
> +       remaining = length - (end_ptr - str);
> +       if (remaining < 0)

right, so if end_ptr is NULL here, we'll end up with a huge positive
value for remaining :)

> +               return -EINVAL;
> +
> +       for (i = 0; i < remaining; i++) {
> +               switch (end_ptr[i]) {
> +               case 'M':
> +                       cvt = true;

the previous code ensured proper ordering as well as parsing, whereas
yours will take these in any order (and accepts duplicates). i don't
think this should cause any issues, but perhaps something to verify.

> +                       break;
> +               case 'R':
> +                       rb = true;
> +                       break;
> +               default:
> +                       /*
> +                        * Try to pass that to our extras parsing
> +                        * function to handle the case where the
> +                        * extras are directly after the resolution
> +                        */
> +                       if (extras) {
> +                               int ret = drm_mode_parse_cmdline_extra(end_ptr + i,
> +                                                                      1,
> +                                                                      connector,
> +                                                                      mode);
> +                               if (ret)
> +                                       return ret;
> +                       } else {
> +                               return -EINVAL;
> +                       }
> +               }
> +       }
> +
> +       mode->xres = xres;
> +       mode->yres = yres;
> +       mode->cvt = cvt;
> +       mode->rb = rb;
> +
> +       return 0;
> +}
> +
>  /**
>   * drm_mode_parse_command_line_for_connector - parse command line modeline for connector
>   * @mode_option: optional per connector mode option
> @@ -1287,13 +1413,12 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>                                                struct drm_cmdline_mode *mode)
>  {
>         const char *name;
> -       unsigned int namelen;
> -       bool res_specified = false, bpp_specified = false, refresh_specified = false;
> -       unsigned int xres = 0, yres = 0, bpp = 32, refresh = 0;
> -       bool yres_specified = false, cvt = false, rb = false;
> -       bool interlace = false, margins = false, was_digit = false;
> -       int i;
> -       enum drm_connector_force force = DRM_FORCE_UNSPECIFIED;
> +       bool parse_extras = false;
> +       unsigned int bpp_off = 0, refresh_off = 0;
> +       unsigned int mode_end = 0;
> +       char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> +       char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
> +       int ret;
>
>  #ifdef CONFIG_FB
>         if (!mode_option)
> @@ -1306,127 +1431,77 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>         }
>
>         name = mode_option;
> -       namelen = strlen(name);
> -       for (i = namelen-1; i >= 0; i--) {
> -               switch (name[i]) {
> -               case '@':
> -                       if (!refresh_specified && !bpp_specified &&
> -                           !yres_specified && !cvt && !rb && was_digit) {
> -                               refresh = simple_strtol(&name[i+1], NULL, 10);
> -                               refresh_specified = true;
> -                               was_digit = false;
> -                       } else
> -                               goto done;
> -                       break;
> -               case '-':
> -                       if (!bpp_specified && !yres_specified && !cvt &&
> -                           !rb && was_digit) {
> -                               bpp = simple_strtol(&name[i+1], NULL, 10);
> -                               bpp_specified = true;
> -                               was_digit = false;
> -                       } else
> -                               goto done;
> -                       break;
> -               case 'x':
> -                       if (!yres_specified && was_digit) {
> -                               yres = simple_strtol(&name[i+1], NULL, 10);
> -                               yres_specified = true;
> -                               was_digit = false;
> -                       } else
> -                               goto done;
> -                       break;
> -               case '0' ... '9':
> -                       was_digit = true;
> -                       break;
> -               case 'M':
> -                       if (yres_specified || cvt || was_digit)
> -                               goto done;
> -                       cvt = true;
> -                       break;
> -               case 'R':
> -                       if (yres_specified || cvt || rb || was_digit)
> -                               goto done;
> -                       rb = true;
> -                       break;
> -               case 'm':
> -                       if (cvt || yres_specified || was_digit)
> -                               goto done;
> -                       margins = true;
> -                       break;
> -               case 'i':
> -                       if (cvt || yres_specified || was_digit)
> -                               goto done;
> -                       interlace = true;
> -                       break;
> -               case 'e':
> -                       if (yres_specified || bpp_specified || refresh_specified ||
> -                           was_digit || (force != DRM_FORCE_UNSPECIFIED))
> -                               goto done;
>
> -                       force = DRM_FORCE_ON;
> -                       break;
> -               case 'D':
> -                       if (yres_specified || bpp_specified || refresh_specified ||
> -                           was_digit || (force != DRM_FORCE_UNSPECIFIED))
> -                               goto done;
> +       if (!isdigit(name[0]))
> +               return false;
>
> -                       if ((connector->connector_type != DRM_MODE_CONNECTOR_DVII) &&
> -                           (connector->connector_type != DRM_MODE_CONNECTOR_HDMIB))
> -                               force = DRM_FORCE_ON;
> -                       else
> -                               force = DRM_FORCE_ON_DIGITAL;
> -                       break;
> -               case 'd':
> -                       if (yres_specified || bpp_specified || refresh_specified ||
> -                           was_digit || (force != DRM_FORCE_UNSPECIFIED))
> -                               goto done;
> +       /* Try to locate the bpp and refresh specifiers, if any */
> +       bpp_ptr = strchr(name, '-');
> +       if (bpp_ptr) {
> +               bpp_off = bpp_ptr - name;
> +               mode->bpp_specified = true;
> +       }
>
> -                       force = DRM_FORCE_OFF;
> -                       break;
> -               default:
> -                       goto done;
> -               }
> +       refresh_ptr = strchr(name, '@');
> +       if (refresh_ptr) {
> +               refresh_off = refresh_ptr - name;
> +               mode->refresh_specified = true;
>         }
>
> -       if (i < 0 && yres_specified) {
> -               char *ch;
> -               xres = simple_strtol(name, &ch, 10);
> -               if ((ch != NULL) && (*ch == 'x'))
> -                       res_specified = true;
> -               else
> -                       i = ch - name;
> -       } else if (!yres_specified && was_digit) {
> -               /* catch mode that begins with digits but has no 'x' */
> -               i = 0;
> +       /* Locate the end of the name / resolution, and parse it */
> +       if (bpp_ptr && refresh_ptr) {
> +               mode_end = min(bpp_off, refresh_off);
> +       } else if (bpp_ptr) {
> +               mode_end = bpp_off;
> +       } else if (refresh_ptr) {
> +               mode_end = refresh_off;
> +       } else {
> +               mode_end = strlen(name);
> +               parse_extras = true;
>         }
> -done:
> -       if (i >= 0) {
> -               pr_warn("[drm] parse error at position %i in video mode '%s'\n",
> -                       i, name);
> -               mode->specified = false;
> +
> +       ret = drm_mode_parse_cmdline_res_mode(name, mode_end,
> +                                             parse_extras,
> +                                             connector,
> +                                             mode);
> +       if (ret)
>                 return false;
> -       }
> +       mode->specified = true;
>
> -       if (res_specified) {
> -               mode->specified = true;
> -               mode->xres = xres;
> -               mode->yres = yres;
> +       if (bpp_ptr) {
> +               ret = drm_mode_parse_cmdline_bpp(bpp_ptr, &bpp_end_ptr, mode);
> +               if (ret)
> +                       return false;
>         }
>
> -       if (refresh_specified) {
> -               mode->refresh_specified = true;
> -               mode->refresh = refresh;
> +       if (refresh_ptr) {
> +               ret = drm_mode_parse_cmdline_refresh(refresh_ptr,
> +                                                    &refresh_end_ptr, mode);
> +               if (ret)
> +                       return false;
>         }
>
> -       if (bpp_specified) {
> -               mode->bpp_specified = true;
> -               mode->bpp = bpp;
> +       /*
> +        * Locate the end of the bpp / refresh, and parse the extras
> +        * if relevant
> +        */
> +       if (bpp_ptr && refresh_ptr)

Perhaps I'm paranoid, but I think it'd be better to check bpp_end_ptr
&& refresh_end_ptr in this conditional.

Sean

> +               extra_ptr = max(bpp_end_ptr, refresh_end_ptr);
> +       else if (bpp_ptr)
> +               extra_ptr = bpp_end_ptr;
> +       else if (refresh_ptr)
> +               extra_ptr = refresh_end_ptr;
> +
> +       if (extra_ptr) {
> +               int remaining = strlen(name) - (extra_ptr - name);
> +
> +               /*
> +                * We still have characters to process, while
> +                * we shouldn't have any
> +                */
> +               if (remaining > 0)
> +                       return false;
>         }
> -       mode->rb = rb;
> -       mode->cvt = cvt;
> -       mode->interlace = interlace;
> -       mode->margins = margins;
> -       mode->force = force;
>
>         return true;
>  }
> --
> git-series 0.8.10