On 24/04/2024 14:22, Sakari Ailus wrote:
> The len_routes field is used to tell the size of the routes array in
> struct v4l2_subdev_routing. This way the number of routes returned from
> S_ROUTING IOCTL may be larger than the number of routes provided, in case
> there are more routes returned by the driver.
>
> Note that this uAPI is still disabled in the code, so this change can
> safely be done. Anyone who manually patched the code to enable this uAPI
> must update their code.
I see that the 'reserved' array is also enlarged by this patch. That should
be mentioned in the commit log.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> .../media/v4l/vidioc-subdev-g-routing.rst | 46 +++++++++++++------
> drivers/media/v4l2-core/v4l2-ioctl.c | 4 +-
> drivers/media/v4l2-core/v4l2-subdev.c | 12 ++---
> include/media/v4l2-subdev.h | 2 +
> include/uapi/linux/v4l2-subdev.h | 9 ++--
> 5 files changed, 48 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> index 26b5004bfe6d..cbd9370006b6 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst
> @@ -43,23 +43,38 @@ The routing configuration determines the flows of data inside an entity.
> Drivers report their current routing tables using the
> ``VIDIOC_SUBDEV_G_ROUTING`` ioctl and application may enable or disable routes
> with the ``VIDIOC_SUBDEV_S_ROUTING`` ioctl, by adding or removing routes and
> -setting or clearing flags of the ``flags`` field of a
> -struct :c:type:`v4l2_subdev_route`.
> +setting or clearing flags of the ``flags`` field of a struct
> +:c:type:`v4l2_subdev_route`.
>
> -All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This
> -means that the userspace must reconfigure all streams after calling the ioctl
> -with e.g. ``VIDIOC_SUBDEV_S_FMT``.
> +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called.
> +This means that the userspace must reconfigure all stream formats and selections
> +after calling the ioctl with e.g. ``VIDIOC_SUBDEV_S_FMT``.
>
> Only subdevices which have both sink and source pads can support routing.
>
> -When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application
> -provided ``num_routes`` is not big enough to contain all the available routes
> -the subdevice exposes, drivers return the ENOSPC error code and adjust the
> -value of the ``num_routes`` field. Application should then reserve enough memory
> -for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again.
> -
> -On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the
> -``num_routes`` field to reflect the actual number of routes returned.
> +The ``len_routes`` field indicates the number of routes that can fit in the
> +``routes`` array allocated by userspace. It is set by applications for both
> +ioctls to indicate how many routes the kernel can return, and is never modified
> +by the kernel.
> +
> +The ``num_routes`` field indicates the number of routes in the routing
> +table. For ``VIDIOC_SUBDEV_S_ROUTING``, it is set by userspace to the number of
> +routes that the application stored in the ``routes`` array. For both ioctls, it
> +is returned by the kernel and indicates how many routes are stored in the
> +subdevice routing table. This may be smaller or larger than the value of
> +``num_routes`` set by the application for ``VIDIOC_SUBDEV_S_ROUTING``, as
> +drivers may adjust the requested routing table.
> +
> +The kernel can return a ``num_routes`` value larger than ``len_routes`` from
> +both ioctls. This indicates thare are more routes in the routing table than fits
> +the ``routes`` array. In this case, the ``routes`` array is filled by the kernel
> +with the first ``len_routes`` entries of the subdevice routing table. This is
> +not considered to be an error, and the ioctl call succeeds. If the applications
> +wants to retrieve the missing routes, it can issue a new
> +``VIDIOC_SUBDEV_G_ROUTING`` call with a large enough ``routes`` array.
> +
> +``VIDIOC_SUBDEV_S_ROUTING`` may return more routes than the user provided in
> +``num_routes`` field due to e.g. hardware properties.
>
> .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
>
> @@ -74,6 +89,9 @@ On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the
> - ``which``
> - Routing table to be accessed, from enum
> :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`.
> + * - __u32
> + - ``len_routes``
> + - The length of the array (as in memory reserved for the array)
> * - struct :c:type:`v4l2_subdev_route`
> - ``routes[]``
> - Array of struct :c:type:`v4l2_subdev_route` entries
> @@ -81,7 +99,7 @@ On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the
> - ``num_routes``
> - Number of entries of the routes array
> * - __u32
> - - ``reserved``\ [5]
> + - ``reserved``\ [11]
> - Reserved for future extensions. Applications and drivers must set
> the array to zero.
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 0260acef97d2..aab671fae45b 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -3201,13 +3201,13 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
> case VIDIOC_SUBDEV_S_ROUTING: {
> struct v4l2_subdev_routing *routing = parg;
>
> - if (routing->num_routes > 256)
> + if (routing->len_routes > 256)
> return -E2BIG;
>
> *user_ptr = u64_to_user_ptr(routing->routes);
> *kernel_ptr = (void **)&routing->routes;
> *array_size = sizeof(struct v4l2_subdev_route)
> - * routing->num_routes;
> + * routing->len_routes;
> ret = 1;
> break;
> }
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 779583447eac..b565f202df67 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -960,14 +960,10 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>
> krouting = &state->routing;
>
> - if (routing->num_routes < krouting->num_routes) {
> - routing->num_routes = krouting->num_routes;
> - return -ENOSPC;
> - }
> -
> memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
> krouting->routes,
> - krouting->num_routes * sizeof(*krouting->routes));
> + min(krouting->num_routes, routing->len_routes) *
> + sizeof(*krouting->routes));
> routing->num_routes = krouting->num_routes;
>
> return 0;
> @@ -989,6 +985,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> if (routing->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
> return -EPERM;
>
> + if (routing->num_routes > routing->len_routes)
> + return -EINVAL;
> +
> memset(routing->reserved, 0, sizeof(routing->reserved));
>
> for (i = 0; i < routing->num_routes; ++i) {
> @@ -1015,6 +1014,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> }
>
> krouting.num_routes = routing->num_routes;
> + krouting.len_routes = routing->len_routes;
> krouting.routes = routes;
>
> return v4l2_subdev_call(sd, pad, set_routing, state,
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index e22c50ce7e05..e30c463d90e5 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -714,12 +714,14 @@ struct v4l2_subdev_stream_configs {
> /**
> * struct v4l2_subdev_krouting - subdev routing table
> *
> + * @len_routes: length of routes array, in routes
> * @num_routes: number of routes
> * @routes: &struct v4l2_subdev_route
> *
> * This structure contains the routing table for a subdev.
> */
> struct v4l2_subdev_krouting {
> + unsigned int len_routes;
> unsigned int num_routes;
> struct v4l2_subdev_route *routes;
> };
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index 81a24bd38003..6a39128d0606 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -228,15 +228,18 @@ struct v4l2_subdev_route {
> * struct v4l2_subdev_routing - Subdev routing information
> *
> * @which: configuration type (from enum v4l2_subdev_format_whence)
> - * @num_routes: the total number of routes in the routes array
> + * @len_routes: the length of the routes array, in routes
I recommend adding: "Set by userspace, untouched by kernelspace." or something
similar.
> * @routes: pointer to the routes array
> + * @num_routes: the total number of routes, possibly more than fits in the
> + * routes array
> * @reserved: drivers and applications must zero this array
> */
> struct v4l2_subdev_routing {
> __u32 which;
> - __u32 num_routes;
> + __u32 len_routes;
> __u64 routes;
> - __u32 reserved[6];
> + __u32 num_routes;
> + __u32 reserved[11];
> };
>
> /*
Regards,
Hans
