Hi Mauro,
On Mon, Oct 09, 2017 at 07:19:21AM -0300, Mauro Carvalho Chehab wrote:
> The __V4L2_SUBDEV_MK_GET_TRY() macro is used to define
> 3 functions that have the same arguments. The code of those
> functions is simple enough to just declare them, de-obfuscating
> the code.
>
> While here, replace BUG_ON() by WARN_ON() as there's no reason
> why to panic the Kernel if this fails.
BUG_ON() might actually be a better idea as this will lead to memory
corruption. I presume it's not been hit often.
That said, I, too, favour WARN_ON() in this case. In case pad exceeds the
number of pads, then zero could be used, for instance. The only real
problem comes if there were no pads to begin with. The callers of these
functions also don't expect to receive NULL. Another option would be to
define a static, dummy variable for the purpose that would be at least safe
to access. Or we could just use the dummy entry whenever the pad isn't
valid.
This will make the functions more complex and I might just keep the
original macro. Even grep works on it nowadays.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> include/media/v4l2-subdev.h | 37 +++++++++++++++++++++++++------------
> 1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1f34045f07ce..35c4476c56ee 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -897,19 +897,32 @@ struct v4l2_subdev_fh {
> container_of(fh, struct v4l2_subdev_fh, vfh)
>
> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> -#define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)
> \
> - static inline struct rtype * \
> - fun_name(struct v4l2_subdev *sd, \
> - struct v4l2_subdev_pad_config *cfg, \
> - unsigned int pad) \
> - { \
> - BUG_ON(pad >= sd->entity.num_pads); \
> - return &cfg[pad].field_name; \
> - }
> +static inline struct v4l2_mbus_framefmt
> +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad)
> +{
> + WARN_ON(pad >= sd->entity.num_pads);
> + return &cfg[pad].try_fmt;
> +}
>
> -__V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, v4l2_subdev_get_try_format,
> try_fmt)
> -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_crop, try_crop)
> -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_compose, try_compose)
> +static inline struct v4l2_rect
> +*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad)
> +{
> + WARN_ON(pad >= sd->entity.num_pads);
> + return &cfg[pad].try_crop;
> +}
> +
> +static inline struct v4l2_rect
> +*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad)
> +{
> + WARN_ON(pad >= sd->entity.num_pads);
> + return &cfg[pad].try_compose;
> +}
> #endif
>
> extern const struct v4l2_file_operations v4l2_subdev_fops;
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
