Hi Hans,
On Thu, Oct 26, 2023 at 09:11:04AM +0200, Hans Verkuil wrote:
> On 26/10/2023 09:03, Sakari Ailus wrote:
> > There are two sets of functions that return information from sub-device
> > state, one for stream-unaware users and another for stream-aware users.
> > Add support for stream-aware functions to return format, crop and compose
> > information from pad-based array that are functionally equivalent to the
> > old, stream-unaware ones.
> >
> > Also check state is non-NULL, in order to guard against old drivers
> > potentially calling this with NULL state for active formats or selection
> > rectangles.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/v4l2-core/v4l2-subdev.c | 51 +++++++++++++++++++++++++++
> > include/media/v4l2-subdev.h | 9 +++--
> > 2 files changed, 57 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index ee4fe8f33a41..7feaea6b04ad 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -1684,6 +1684,23 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state,
> > struct v4l2_subdev_stream_configs *stream_configs;
> > unsigned int i;
> >
> > + if (WARN_ON(!state))
> > + return NULL;
> > +
> > + if (state->pads) {
> > + if (stream)
> > + return NULL;
> > +
> > + /*
> > + * Set the pad to 0 on error as this is aligned with the
> > + * behaviour of the pad state information access functions.
> > + */
>
> This comment does not explain why pad can be an invalid value or why we
> map that to 0 instead of returning an error.
>
> The phrase "as this is aligned with the behaviour of..." makes no sense:
> if it was aligned with that, then you wouldn't need a WARN_ON.
Nevertheless this is true: the pad state information access functions have
that same WARN_ON().
The original reason for this is to avoid accessing memory outside the
array. Primarily the source of this was previously state being NULL (for
state-unaware drivers), but we have a check for this already earlier on. I
can add this.
Note that the comment is removed later on in the set as the function will
return NULL in this case as well, so polishing it will not have any long
term benefits. :-)
>
> If this WARN_ON is triggered, and someone investigates, then this comment
> should explain why you got it and how to fix it.
>
> I'm also wondering if this should be a WARN_ON_ONCE. If this is wrong,
> won't you get this warning a lot?
Probably so, indeed. I can switch it to WARN_ON_ONCE().
>
> > + if (WARN_ON(pad >= state->sd->entity.num_pads))
> > + pad = 0;
> > +
> > + return &state->pads[pad].try_fmt;
> > + }
> > +
>
--
Regards,
Sakari Ailus
