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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Mar 20, 2024 at 03:55:58AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wed, Mar 13, 2024 at 09:24:55AM +0200, Sakari Ailus wrote:
> > Add trivial S_ROUTING IOCTL support for drivers where routing is static.
> > Essentially this means returning the same information G_ROUTING call would
> > have done.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Actually...

> > ---
> >  drivers/media/v4l2-core/v4l2-subdev.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index a6107e440ef0..c8c435df92c8 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -930,6 +930,20 @@ 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.
> > +		 */
> > +		if (!v4l2_subdev_has_op(sd, pad, set_routing)) {
> > +			memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
> > +			       state->routing.routes,
> > +			       min(state->routing.num_routes, routing->len_routes) *
> > +			       sizeof(*state->routing.routes));
> > +			routing->num_routes = state->routing.num_routes;
> > +
> > +			return 0;
> > +		}
> > +

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>

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.
 		 */
-		if (!v4l2_subdev_has_op(sd, pad, set_routing)) {
-			memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
-			       state->routing.routes,
-			       min(state->routing.num_routes, routing->len_routes) *
-			       sizeof(*state->routing.routes));
-			routing->num_routes = state->routing.num_routes;
-
-			return 0;
-		}
-
 		for (i = 0; i < routing->num_routes; ++i) {
 			const struct v4l2_subdev_route *route = &routes[i];
 			const struct media_pad *pads = sd->entity.pads;
@@ -969,6 +962,20 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
 				return -EINVAL;
 		}

+		/*
+		 * If the driver doesn't support setting routing, just return
+		 * the routing table here.
+		 */
+		if (!v4l2_subdev_has_op(sd, pad, set_routing)) {
+			memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
+			       state->routing.routes,
+			       min(state->routing.num_routes, routing->len_routes) *
+			       sizeof(*state->routing.routes));
+			routing->num_routes = state->routing.num_routes;
+
+			return 0;
+		}
+
 		krouting.num_routes = routing->num_routes;
 		krouting.len_routes = routing->len_routes;
 		krouting.routes = routes;


> >  		for (i = 0; i < routing->num_routes; ++i) {
> >  			const struct v4l2_subdev_route *route = &routes[i];
> >  			const struct media_pad *pads = sd->entity.pads;

-- 
Regards,

Laurent Pinchart