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

[Maxime Ripard]

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@xxxxxxxxxx>
> > 
> 
> 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: signature.asc
Description: PGP signature