Re: [PATCH v5 8/8] 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 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@linux.intel.com>

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

> ---
>  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