Re: [PATCH v4 8/9] media: v4l: subdev: Return NULL from pad access functions on error

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

Hi Sakari,

Thank you for the patch.

On Thu, Oct 26, 2023 at 10:03:28AM +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@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 9d4ff9b4fcec..bd0d89c2996f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1684,12 +1684,8 @@ __v4l2_subdev_state_get_format_stream(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.
> -		 */
>  		if (WARN_ON(pad >= state->sd->entity.num_pads))
> -			pad = 0;
> +			return NULL;
>  
>  		return &state->pads[pad].try_fmt;
>  	}
> @@ -1722,12 +1718,8 @@ __v4l2_subdev_state_get_crop_stream(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.
> -		 */
>  		if (WARN_ON(pad >= state->sd->entity.num_pads))
> -			pad = 0;
> +			return NULL;
>  
>  		return &state->pads[pad].try_crop;
>  	}
> @@ -1760,12 +1752,8 @@ __v4l2_subdev_state_get_compose_stream(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.
> -		 */
>  		if (WARN_ON(pad >= state->sd->entity.num_pads))
> -			pad = 0;
> +			return NULL;
>  
>  		return &state->pads[pad].try_compose;
>  	}

-- 
Regards,

Laurent Pinchart