On Thu, Apr 11, 2024 at 08:13:31AM +0000, Sakari Ailus wrote:
> Hi Laurent,
>
> On Tue, Apr 02, 2024 at 02:41:05AM +0300, Laurent Pinchart wrote:
> > 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@xxxxxxxxxxxxxxx>
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxxx>
>
> 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.
> > - 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
