Hi Sakari,
Thank you for the patch.
On Fri, Oct 27, 2023 at 12:59:13PM +0300, Sakari Ailus wrote:
> Return NULL from sub-device pad state access functions
> (v4l2_subdev_state_get_{format,crop,compose}) for non-existent pads. While
> this behaviour differs from older set of pad state information access
> functions, we've had a WARN_ON() there for a long time and callers also do
> validate the pad index nowadays. Therefore problems are not expected.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 36 +++++----------------------
> 1 file changed, 6 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index bae932134cfc..923a221955a9 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1684,16 +1684,8 @@ __v4l2_subdev_state_get_format(struct v4l2_subdev_state *state,
> 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. The
> - * purpose of setting pad to 0 here is to avoid accessing memory
> - * outside the pads array, but still issuing warning of the
> - * invalid access while making the caller's error handling
> - * easier.
> - */
> - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads))
> - pad = 0;
> + if (pad >= state->sd->entity.num_pads)
> + return NULL;
>
> return &state->pads[pad].try_fmt;
> }
> @@ -1726,16 +1718,8 @@ __v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad,
> 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. The
> - * purpose of setting pad to 0 here is to avoid accessing memory
> - * outside the pads array, but still issuing warning of the
> - * invalid access while making the caller's error handling
> - * easier.
> - */
> - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads))
> - pad = 0;
> + if (pad >= state->sd->entity.num_pads)
> + return NULL;
>
> return &state->pads[pad].try_crop;
> }
> @@ -1768,16 +1752,8 @@ __v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state,
> 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. The
> - * purpose of setting pad to 0 here is to avoid accessing memory
> - * outside the pads array, but still issuing warning of the
> - * invalid access while making the caller's error handling
> - * easier.
> - */
> - if (WARN_ON_ONCE(pad >= state->sd->entity.num_pads))
> - pad = 0;
> + if (pad >= state->sd->entity.num_pads)
> + return NULL;
>
> return &state->pads[pad].try_compose;
> }
--
Regards,
Laurent Pinchart
