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.
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?
> + if (WARN_ON(pad >= state->sd->entity.num_pads))
> + pad = 0;
> +
> + return &state->pads[pad].try_fmt;
> + }
> +
Regards,
Hans
