Re: [PATCH v8 17/38] media: v4l: subdev: Add trivial set_routing support

From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org, tomi.valkeinen@ideasonboard.com,
	bingbu.cao@intel.com, hongju.wang@intel.com, hverkuil@xs4all.nl,
	Andrey Konovalov <andrey.konovalov@linaro.org>,
	Jacopo Mondi <jacopo.mondi@ideasonboard.com>,
	Dmitry Perchanov <dmitry.perchanov@intel.com>,
	"Ng, Khai Wen" <khai.wen.ng@intel.com>,
	Alain Volmat <alain.volmat@foss.st.com>
Subject: Re: [PATCH v8 17/38] media: v4l: subdev: Add trivial set_routing support
Date: Mon, 15 Apr 2024 08:10:53 +0000	[thread overview]
Message-ID: <ZhzhDXEOVvBLqLqL@kekkonen.localdomain> (raw)
In-Reply-To: <20240412191035.GP31122@pendragon.ideasonboard.com>

Hi Laurent,

On Fri, Apr 12, 2024 at 10:14:26PM +0300, Laurent Pinchart wrote:
> > > I think this should be done after the next code block that validates the
> > > arguments. Otherwise the S_ROUTING ioctl will behave differently with
> > > regard to blatantly invalid arguments, depending on whether or not the
> > > subdev implements the .set_routing() operation. This can confuse
> > > userspace, and does confuse v4l2-compliance.
> > > 
> > > I have the following change in my tree that fixes the problem, feel free
> > > to squash it with this patch in v9.
> > > 
> > > commit 1e1f03eb8bc118c53a9deab05063d71a2fe7aa5f
> > > Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Date:   Tue Apr 2 02:06:01 2024 +0300
> > > 
> > >     fixup! media: v4l: subdev: Add trivial set_routing support
> > > 
> > >     Validate arguments before handling the trivial set_routing support to
> > >     expose a consistent behaviour to userspace. This fixes an issue with
> > >     v4l2-compliance.
> > > 
> > >     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Thanks, I'll squash this into the patch.
> > 
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index def91ab32c07..129867ed2bad 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -933,19 +933,12 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> > >  		memset(routing->reserved, 0, sizeof(routing->reserved));
> > > 
> > >  		/*
> > > -		 * If the driver doesn't support setting routing, just return
> > > -		 * the routing table here.
> > > +		 * Perform argument validation first, or subdevs that don't
> > > +		 * support setting routing will not return an error when
> > > +		 * arguments are blatantly wrong. The difference in behaviour
> > > +		 * could be confusing for userspace, and in particular for API
> > > +		 * compliance checkers.
> > 
> > This is more fit for development time discussion, not something that should
> > be left in the code IMO.
> 
> I added that comment to make sure that the next developer who refactors
> the code will not end up changing the order and introducing subtle
> breakages without realizing it. There's a high chance we wouldn't catch
> the problem during review if this happens after our brain caches get
> flushed.

We don't have other comments like that in the code either. Input validation
is generally needed, in this case it wasn't needed to carry out the task
but to align the API behaviour with the drivers that do and don't support
setting routing. It's not worth a comment here: it's the same for other
IOCTLs as well. So if a comment were to be added, I'd add it before the
switch.

-- 
Regards,

Sakari Ailus