Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753502AbcKUHgn (ORCPT ); Mon, 21 Nov 2016 02:36:43 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:49379 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751983AbcKUHgm (ORCPT ); Mon, 21 Nov 2016 02:36:42 -0500 Date: Mon, 21 Nov 2016 08:36:39 +0100 From: Maxime Ripard To: Sean Paul Cc: Daniel Vetter , David Airlie , dri-devel , Laurent Pinchart , Boris Brezillon , Thomas Petazzoni , Linux ARM Kernel , Hans de Goede , Chen-Yu Tsai , Linux Kernel Mailing List Subject: Re: [PATCH 1/5] drm/modes: Rewrite the command line parser Message-ID: <20161121073639.pjikxfytzms6b6yk@lukather> References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="77qxllxkvuveuuyj" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6744 Lines: 197 --77qxllxkvuveuuyj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 > wrote: > > Rewrite the command line parser in order to get away from the state mac= hine > > parsing the video mode lines. > > > > Hopefully, this will allow to extend it more easily to support named mo= des > > and / or properties set directly on the command line. > > > > Signed-off-by: Maxime Ripard > > --- > > 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 > > #include > > #include > > #include > > @@ -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] !=3D '-') > > + return -EINVAL; > > + > > + mode->bpp =3D simple_strtol(str + 1, end_ptr, 10); > > + mode->bpp_specified =3D true; > > + > > + return 0; > > +} > > + > > +static int drm_mode_parse_cmdline_refresh(const char *str, char **end_= ptr, > > + struct drm_cmdline_mode *mode) > > +{ > > + if (str[0] !=3D '@') > > + return -EINVAL; > > + > > + mode->refresh =3D simple_strtol(str + 1, end_ptr, 10); > > + mode->refresh_specified =3D 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 =3D 0; i < length; i++) { > > + switch (str[i]) { > > + case 'i': > > + mode->interlace =3D true; > > + break; > > + case 'm': > > + mode->margins =3D true; > > + break; > > + case 'D': > > + if (mode->force !=3D DRM_FORCE_UNSPECIFIED) > > + return -EINVAL; > > + > > + if ((connector->connector_type !=3D DRM_MODE_CO= NNECTOR_DVII) && > > + (connector->connector_type !=3D DRM_MODE_CO= NNECTOR_HDMIB)) > > + mode->force =3D DRM_FORCE_ON; > > + else > > + mode->force =3D DRM_FORCE_ON_DIGITAL; > > + break; > > + case 'd': > > + if (mode->force !=3D DRM_FORCE_UNSPECIFIED) > > + return -EINVAL; > > + > > + mode->force =3D DRM_FORCE_OFF; > > + break; > > + case 'e': > > + if (mode->force !=3D DRM_FORCE_UNSPECIFIED) > > + return -EINVAL; > > + > > + mode->force =3D DRM_FORCE_ON; > > + break; > > + default: > > + return -EINVAL; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned i= nt length, > > + bool extras, > > + struct drm_connector *connec= tor, > > + struct drm_cmdline_mode *mod= e) > > +{ > > + bool rb =3D false, cvt =3D false; > > + int xres =3D 0, yres =3D 0; > > + int remaining, i; > > + char *end_ptr; > > + > > + xres =3D simple_strtol(str, &end_ptr, 10); > > + >=20 > checkpatch is telling me to use kstrtol instead, as simple_strtol is depr= ecated >=20 > > + if (end_ptr[0] !=3D 'x') >=20 > check that end_ptr !=3D NULL? you should probably also check that xres > isn't an error (ie: -ERANGE or -EINVAL) >=20 > > + return -EINVAL; > > + end_ptr++; > > + > > + yres =3D simple_strtol(end_ptr, &end_ptr, 10); >=20 > check end_ptr !=3D NULL and yres sane >=20 > > + > > + remaining =3D length - (end_ptr - str); > > + if (remaining < 0) >=20 > right, so if end_ptr is NULL here, we'll end up with a huge positive > value for remaining :) >=20 > > + return -EINVAL; > > + > > + for (i =3D 0; i < remaining; i++) { > > + switch (end_ptr[i]) { > > + case 'M': > > + cvt =3D true; >=20 > 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 --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --77qxllxkvuveuuyj Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYMqQCAAoJEBx+YmzsjxAg9V0P/RPeL+4rF82OhwgiKZGvFV6j UqRhd9U0ZJRp1DnxakVO2aAN7dE99V4+mscM9z2QoUy4BCJeu2gP7xbtJ/fhzrkR S3tSqbh76JuMhp3O+eTW5VjePKkkZaT700b0QfaDl0httTV41Ka4Hw8EWSQn5MTM oi0vhd9+X0PS7c4LpsAUmhZWW+dwC3tjP/U31DhreeSO/fPSpSLiUlnQtsjmL27p wupmOEZqOm51vkYkkTtO53Am7ri8Ucfh6mDF6vfsd40Y7z61BmtFSgnctHxxard1 5CPUDHR3+/0lyQfOg4ZgBLa6U3WAPmAj8sMXwyQ8nhLJpUXWdSfyyd9O8CnqD+UI S1XBP0zOawOJpesW8qO3LoIuvClzOkYIs3blFSKjhCfbw4UJuO9coJ3Adx9vf7x0 ESlKGSY+IGXWA5Bx5wyPgTXTnlL2koKRW2Al9uleWdjuZiPK0bQ/Ay3RiGKCiR6R HXIi0R8OcJRWSByuodU3qDs0EqutHFtFuDj3+DKNRcxlYe0HELlCM6FaGNrCZU+/ oMfAtMl4PkY3QngAj3sSA436nC5P3tnrEGcnkjF0qqHN0gZ9bYsTckEY5i9+kufR XWOmkwlX/aZEqV2VtjO8Jo4xsKqnxfcN7+jNO4+sdXOnXpP5CffykOCHQv0Ya4Rp xesnWhis79tOuvR4lxds =nCMK -----END PGP SIGNATURE----- --77qxllxkvuveuuyj--