Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1526423

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

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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@free-electrons.com> 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@free-electrons.com>
> > ---
> >  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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

Re: [PATCH 1/5] drm/modes: Rewrite the command line parser Sean Paul <seanpaul@chromium.org> - 2016-11-16 18:20 +0100
  Re: [PATCH 1/5] drm/modes: Rewrite the command line parser Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-11-21 08:40 +0100

csiph-web