| Message ID | 20240313072516.241106-18-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v8,01/38] media: mc: Add INTERNAL pad flag | expand |
On 3/13/24 08:24, 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: Julien Massot <julien.massot@collabora.com> > --- > 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; > + } > + > for (i = 0; i < routing->num_routes; ++i) { > const struct v4l2_subdev_route *route = &routes[i]; > const struct media_pad *pads = sd->entity.pads;
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> > --- > 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; > + } > + > for (i = 0; i < routing->num_routes; ++i) { > const struct v4l2_subdev_route *route = &routes[i]; > const struct media_pad *pads = sd->entity.pads;
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;
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@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> 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. > */ > - 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; >
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@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> > > 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;
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.
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; + } + for (i = 0; i < routing->num_routes; ++i) { const struct v4l2_subdev_route *route = &routes[i]; const struct media_pad *pads = sd->entity.pads;
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> --- drivers/media/v4l2-core/v4l2-subdev.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)