Re: [PATCH v9 29/36] media: subdev: add v4l2_subdev_set_routing helper()

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

Hi Tomi,

On Mon, Oct 11, 2021 at 11:13:40AM +0300, Tomi Valkeinen wrote:
> On 10/10/2021 13:21, Laurent Pinchart wrote:
> > On Tue, Oct 05, 2021 at 11:57:43AM +0300, Tomi Valkeinen wrote:
> >> Add a helper function to set the subdev routing. The helper can be used
> >> from subdev driver's set_routing op to store the routing table.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >>   drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++++++++++
> >>   include/media/v4l2-subdev.h           | 16 +++++++++++++++
> >>   2 files changed, 44 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >> index 14b8282fe45b..af53f827ec27 100644
> >> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >> @@ -1151,3 +1151,31 @@ void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
> >>   	mutex_unlock(&state->lock);
> >>   }
> >>   EXPORT_SYMBOL_GPL(v4l2_subdev_unlock_state);
> >> +
> >> +int v4l2_subdev_set_routing(struct v4l2_subdev *sd,
> >> +			    struct v4l2_subdev_state *state,
> >> +			    struct v4l2_subdev_krouting *routing)
> >> +{
> >> +	struct v4l2_subdev_krouting *dst = &state->routing;
> >> +	const struct v4l2_subdev_krouting *src = routing;
> >> +
> >> +	lockdep_assert_held(&state->lock);
> > 
> > Calling this function doesn't make much sense if the subdev doesn't have
> > the V4L2_SUBDEV_FL_MULTIPLEXED set, right ? If that's correct, should
> > this be documented below, and/or possibly checked here ?
> > 
> > 	if (WARN_ON(!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED)))
> > 		return -E???;
> 
> That is true, but it's true for all the functions introduced in the 
> series. Do we want to add the check everywhere? In many cases this 
> should be quite obvious: if you haven't ported the subdev driver to the 
> new API, you don't have routing table, and can't sensibly call this 
> function. Or you don't have stream ID, so you can't sensibly call 
> functions that take a stream number.

That's right. I was mostly concern about driver conversion, it's likely
easy to forget to add V4L2_SUBDEV_FL_MULTIPLEXED and then spend time
debugging issues.

I'm fine either way for the WARN_ON, but would still like to capture
this in the documentation.

> >> +
> >> +	kvfree(dst->routes);
> >> +	dst->routes = NULL;
> >> +	dst->num_routes = 0;
> >> +
> >> +	if (src->num_routes > 0) {
> >> +		dst->routes = kvmalloc_array(src->num_routes,
> >> +					     sizeof(*src->routes), GFP_KERNEL);
> > 
> > How many routes do we typically expect, is it worth a vmalloc ? If not,
> 
> I have to say I have no clue if it's worth it. vmalloc is used for the 
> pads, and was used in the original routing patches. I did wonder about 
> this but just went with what's already there.
> 
> It's not easy to guess the expected number of routes. I'd say... 4 pads 
> on each side and 8 streams per pad-pair sounds kind of a lot but still 
> sounds feasible. That would be 32 routes. But that would be on the 
> maximum side. On average, I guess we'll have... 1 or 2 routes? That's 
> wha the sensors will have.
> 
> > kmemdup() could be a candidate (as I think by the time we get here,
> > num_routes should have been validated to not overflow, but please tell
> > me if I got this part wrong).
> 
> I'm not sure what you mean here. We are allocating (enough) new memory, 
> it will never overflow.

I mean an integer overflow in the size * nelems calculation. The array
allocation functions pay special attention to this.

> >> +		if (!dst->routes)
> >> +			return -ENOMEM;
> >> +
> >> +		memcpy(dst->routes, src->routes,
> >> +		       src->num_routes * sizeof(*src->routes));
> >> +		dst->num_routes = src->num_routes;
> >> +	}
> >> +
> >> +	return 0;

-- 
Regards,

Laurent Pinchart