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

From: Maxime Ripard <maxime@cerno.tech>
To: "Noralf Trønnes" <noralf@tronnes.org>
Cc: Karol Herbst <kherbst@redhat.com>,
	Jani Nikula <jani.nikula@linux.intel.com>,
	Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	David Airlie <airlied@linux.ie>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Lyude Paul <lyude@redhat.com>, Emma Anholt <emma@anholt.net>,
	Chen-Yu Tsai <wens@csie.org>,
	Samuel Holland <samuel@sholland.org>,
	Ben Skeggs <bskeggs@redhat.com>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Dom Cobley <dom@raspberrypi.com>,
	linux-sunxi@lists.linux.dev,
	Dave Stevenson <dave.stevenson@raspberrypi.com>,
	intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	nouveau@lists.freedesktop.org,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	linux-arm-kernel@lists.infradead.org,
	Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>,
	dri-devel@lists.freedesktop.org,
	Hans de Goede <hdegoede@redhat.com>,
	Phil Elwell <phil@raspberrypi.com>
Subject: Re: [PATCH v5 08/22] drm/modes: Move named modes parsing to a separate function
Date: Tue, 18 Oct 2022 09:57:42 +0200	[thread overview]
Message-ID: <20221018075742.7g3sfay3o7wk54kv@houat> (raw)
In-Reply-To: <729f849a-0287-a2fb-125f-892b4dd6053f@tronnes.org>

[-- Attachment #1.1: Type: text/plain, Size: 1995 bytes --]

On Sun, Oct 16, 2022 at 06:11:21PM +0200, Noralf Trønnes wrote:
> 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@cerno.tech>
> > 
> 
> 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.

I'd really like to keep it in though. At least, we know by looking at
this small function that we're going to be safe all the time, no matter
what the caller does.

And if the caller wants to check it as well, fine, it's only a simple
comparison ran once or twice at boot, it's not like it's in a hot-path.

I've addressed your other comments, thanks!
Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel