Re: [PATCH v4 19/23] media: v4l: subdev: Add len_routes field to struct v4l2_subdev_routing

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

Thanks for the review.

On Tue, Sep 26, 2023 at 12:34:50AM +0300, Laurent Pinchart wrote:
> On Tue, Sep 26, 2023 at 12:01:02AM +0300, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Sep 22, 2023 at 05:22:35PM +0300, 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 changes the (now-disabled) UAPI, users must be updated.
> > 
> > This cause a regression in all upstream drivers that call
> > v4l2_subdev_set_routing(). You need to patch the following files:
> > 
> > drivers/media/i2c/ds90ub913.c
> > drivers/media/i2c/ds90ub953.c
> > drivers/media/i2c/ds90ub960.c
> > drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > 
> > It's a bit error-prone though. Consider the following change:
> > 
> > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > index 792f031e032a..c1c9e7018f24 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > @@ -201,6 +201,7 @@ static int mxc_isi_crossbar_init_cfg(struct v4l2_subdev *sd,
> >  		route->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE;
> >  	}
> > 
> > +	routing.len_routes = xbar->num_sources;
> >  	routing.num_routes = xbar->num_sources;
> >  	routing.routes = routes;
> > 
> > It's easy to forget to set len_routes :-S I wonder if we could do
> > better.
> 
> Actually, this isn't needed. My problem was caused by a bug in patch
> 21/23.
> 
> Still, not initializing len_routes in drivers feels wrong.

len_routes signifies the number of routes that can fit to the
user-allocated routes array. Drivers do need to ensure they don't write
more routes there, as the kernel allocates as much memory for the in-kernel
array.

-- 
Kind regards,

Sakari Ailus