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

[Maxime Ripard]

Hi Sean,

Thanks for taking the time to review this.

On Wed, Nov 16, 2016 at 12:12:53PM -0500, Sean Paul wrote:
> 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.

Yes, I definitely overlooked the order of the parameters (and now I
get why the switch was so convoluted...)

I'll come up with a second version fixing that (and the other comments
you had).

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachment: signature.asc
Description: PGP signature