On 5/12/19 10:15 PM, Janusz Krzysztofik wrote:
> Correctness of format type (try or active) and pad ID parameters passed
> to subdevice operation callbacks is now verified only for IOCTL calls.
> However, those callbacks are also used by drivers, e.g., V4L2 host
> interfaces.
>
> Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call()
> macro while calling subdevice operations, move those parameter checks
> from subdev_do_ioctl() to v4l2_subdev_call() so we can avoid taking care
> of those checks inside drivers.
>
> Define a wrapper function for each operation callback in scope, then
> gather those wrappers in a static v4l2_subdev_ops structure so the
> v4l2_subdev_call() macro can find them easy if provided. Move reusable
> code to helpers.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@xxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 227 ++++++++++++++++----------
> include/media/v4l2-subdev.h | 6 +
> 2 files changed, 143 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index d75815ab0d7b..890916674d42 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -120,56 +120,168 @@ static int subdev_close(struct file *file)
> return 0;
> }
>
> -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> -static int check_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_format *format)
> +static int check_which(__u32 which)
> {
> - if (format->which != V4L2_SUBDEV_FORMAT_TRY &&
> - format->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> - return -EINVAL;
> -
> - if (format->pad >= sd->entity.num_pads)
> + if (which != V4L2_SUBDEV_FORMAT_TRY &&
> + which != V4L2_SUBDEV_FORMAT_ACTIVE)
> return -EINVAL;
>
> return 0;
> }
>
> -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop *crop)
> +static int check_pad(struct v4l2_subdev *sd, __u32 pad)
> {
> - if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
> - crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + if (sd->entity.num_pads && pad >= sd->entity.num_pads)
> return -EINVAL;
> +#endif
> + return 0;
> +}
>
> - if (crop->pad >= sd->entity.num_pads)
> - return -EINVAL;
> +static int check_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_format *format)
> +{
> + return check_which(format->which) ? : check_pad(sd, format->pad);
> +}
>
> - return 0;
> +static int check_get_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *format)
> +{
> + return check_format(sd, format) ? :
> + sd->ops->pad->get_fmt(sd, cfg, format);
> +}
> +
> +static int check_set_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *format)
> +{
> + return check_format(sd, format) ? :
> + sd->ops->pad->set_fmt(sd, cfg, format);
> +}
> +
> +static int check_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + return check_which(code->which) ? : check_pad(sd, code->pad) ? :
> + sd->ops->pad->enum_mbus_code(sd, cfg, code);
> +}
> +
> +static int check_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
> + sd->ops->pad->enum_frame_size(sd, cfg, fse);
> +}
> +
> +static int check_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_frame_interval *fi)
> +{
> + return check_pad(sd, fi->pad);
> +}
> +
> +static int check_g_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_frame_interval *fi)
> +{
> + return check_frame_interval(sd, fi) ? :
> + sd->ops->video->g_frame_interval(sd, fi);
> +}
> +
> +static int check_s_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_frame_interval *fi)
> +{
> + return check_frame_interval(sd, fi) ? :
> + sd->ops->video->s_frame_interval(sd, fi);
> +}
> +
> +static int check_enum_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_interval_enum *fie)
> +{
> + return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
> + sd->ops->pad->enum_frame_interval(sd, cfg, fie);
> }
>
> static int check_selection(struct v4l2_subdev *sd,
> struct v4l2_subdev_selection *sel)
> {
> - if (sel->which != V4L2_SUBDEV_FORMAT_TRY &&
> - sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> - return -EINVAL;
> + return check_which(sel->which) ? : check_pad(sd, sel->pad);
> +}
>
> - if (sel->pad >= sd->entity.num_pads)
> - return -EINVAL;
> +static int check_get_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_selection *sel)
> +{
> + return check_selection(sd, sel) ? :
> + sd->ops->pad->get_selection(sd, cfg, sel);
> +}
>
> - return 0;
> +static int check_set_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_selection *sel)
> +{
> + return check_selection(sd, sel) ? :
> + sd->ops->pad->set_selection(sd, cfg, sel);
> }
>
> static int check_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
> {
> - if (edid->pad >= sd->entity.num_pads)
> - return -EINVAL;
> -
> if (edid->blocks && edid->edid == NULL)
> return -EINVAL;
>
> - return 0;
> + return check_pad(sd, edid->pad);
> }
> -#endif
> +
> +static int check_get_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
> +{
> + return check_edid(sd, edid) ? : sd->ops->pad->get_edid(sd, edid);
> +}
> +
> +static int check_set_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
> +{
> + return check_edid(sd, edid) ? : sd->ops->pad->set_edid(sd, edid);
> +}
> +
> +static int check_dv_timings_cap(struct v4l2_subdev *sd,
> + struct v4l2_dv_timings_cap *cap)
> +{
> + return check_pad(sd, cap->pad) ? :
> + sd->ops->pad->dv_timings_cap(sd, cap);
> +}
> +
> +static int check_enum_dv_timings(struct v4l2_subdev *sd,
> + struct v4l2_enum_dv_timings *dvt)
> +{
> + return check_pad(sd, dvt->pad) ? :
> + sd->ops->pad->enum_dv_timings(sd, dvt);
> +}
> +
> +static const struct v4l2_subdev_pad_ops v4l2_subdev_pad_chk_args = {
> + .get_fmt = check_get_fmt,
> + .set_fmt = check_set_fmt,
> + .enum_mbus_code = check_enum_mbus_code,
> + .enum_frame_size = check_enum_frame_size,
> + .enum_frame_interval = check_enum_frame_interval,
> + .get_selection = check_get_selection,
> + .set_selection = check_set_selection,
> + .get_edid = check_get_edid,
> + .set_edid = check_set_edid,
> + .dv_timings_cap = check_dv_timings_cap,
> + .enum_dv_timings = check_enum_dv_timings,
> +};
> +
> +static const struct v4l2_subdev_video_ops v4l2_subdev_video_chk_args = {
> + .g_frame_interval = check_g_frame_interval,
> + .s_frame_interval = check_s_frame_interval,
> +};
> +
> +struct v4l2_subdev_ops v4l2_subdev_call_chk_args = {
Should be const.
BTW, I agree with Sakari regarding the naming of these ops.
Regards,
Hans
> + .pad = &v4l2_subdev_pad_chk_args,
> + .video = &v4l2_subdev_video_chk_args,
> +};
> +EXPORT_SYMBOL(v4l2_subdev_call_chk_args);
>
> static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> {
> @@ -292,10 +404,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> case VIDIOC_SUBDEV_G_FMT: {
> struct v4l2_subdev_format *format = arg;
>
> - rval = check_format(sd, format);
> - if (rval)
> - return rval;
> -
> memset(format->reserved, 0, sizeof(format->reserved));
> memset(format->format.reserved, 0, sizeof(format->format.reserved));
> return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
> @@ -304,10 +412,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> case VIDIOC_SUBDEV_S_FMT: {
> struct v4l2_subdev_format *format = arg;
>
> - rval = check_format(sd, format);
> - if (rval)
> - return rval;
> -
> memset(format->reserved, 0, sizeof(format->reserved));
> memset(format->format.reserved, 0, sizeof(format->format.reserved));
> return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
> @@ -317,10 +421,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> struct v4l2_subdev_crop *crop = arg;
> struct v4l2_subdev_selection sel;
>
> - rval = check_crop(sd, crop);
> - if (rval)
> - return rval;
> -
> memset(crop->reserved, 0, sizeof(crop->reserved));
> memset(&sel, 0, sizeof(sel));
> sel.which = crop->which;
> @@ -340,10 +440,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> struct v4l2_subdev_selection sel;
>
> memset(crop->reserved, 0, sizeof(crop->reserved));
> - rval = check_crop(sd, crop);
> - if (rval)
> - return rval;
> -
> memset(&sel, 0, sizeof(sel));
> sel.which = crop->which;
> sel.pad = crop->pad;
> @@ -361,13 +457,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> case VIDIOC_SUBDEV_ENUM_MBUS_CODE: {
> struct v4l2_subdev_mbus_code_enum *code = arg;
>
> - if (code->which != V4L2_SUBDEV_FORMAT_TRY &&
> - code->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> - return -EINVAL;
> -
> - if (code->pad >= sd->entity.num_pads)
> - return -EINVAL;
> -
> memset(code->reserved, 0, sizeof(code->reserved));
> return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
> code);
> @@ -376,13 +465,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> case VIDIOC_SUBDEV_ENUM_FRAME_SIZE: {
> struct v4l2_subdev_frame_size_enum *fse = arg;
>
> - if (fse->which != V4L2_SUBDEV_FORMAT_TRY &&
> - fse->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> - return -EINVAL;
> -
> - if (fse->pad >= sd->entity.num_pads)
> - return -EINVAL;
> -
> memset(fse->reserved, 0, sizeof(fse->reserved));
> return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
> fse);
> @@ -391,9 +473,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> case VIDIOC_SUBDEV_G_FRAME_INTERVAL: {
> struct v4l2_subdev_frame_interval *fi = arg;
>
> - if (fi->pad >= sd->entity.num_pads)
> - return -EINVAL;
> -
> memset(fi->reserved, 0, sizeof(fi->reserved));
> return v4l2_subdev_call(sd, video, g_frame_interval, arg);
> }
> @@ -401,9 +480,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> case VIDIOC_SUBDEV_S_FRAME_INTERVAL: {
> struct v4l2_subdev_frame_interval *fi = arg;
>
> - if (fi->pad >= sd->entity.num_pads)
> - return -EINVAL;
> -
> memset(fi->reserved, 0, sizeof(fi->reserved));
> return v4l2_subdev_call(sd, video, s_frame_interval, arg);
> }
> @@ -411,13 +487,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: {
> struct v4l2_subdev_frame_interval_enum *fie = arg;
>
> - if (fie->which != V4L2_SUBDEV_FORMAT_TRY &&
> - fie->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> - return -EINVAL;
> -
> - if (fie->pad >= sd->entity.num_pads)
> - return -EINVAL;
> -
> memset(fie->reserved, 0, sizeof(fie->reserved));
> return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
> fie);
> @@ -426,10 +495,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> case VIDIOC_SUBDEV_G_SELECTION: {
> struct v4l2_subdev_selection *sel = arg;
>
> - rval = check_selection(sd, sel);
> - if (rval)
> - return rval;
> -
> memset(sel->reserved, 0, sizeof(sel->reserved));
> return v4l2_subdev_call(
> sd, pad, get_selection, subdev_fh->pad, sel);
> @@ -438,10 +503,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> case VIDIOC_SUBDEV_S_SELECTION: {
> struct v4l2_subdev_selection *sel = arg;
>
> - rval = check_selection(sd, sel);
> - if (rval)
> - return rval;
> -
> memset(sel->reserved, 0, sizeof(sel->reserved));
> return v4l2_subdev_call(
> sd, pad, set_selection, subdev_fh->pad, sel);
> @@ -450,38 +511,24 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> case VIDIOC_G_EDID: {
> struct v4l2_subdev_edid *edid = arg;
>
> - rval = check_edid(sd, edid);
> - if (rval)
> - return rval;
> -
> return v4l2_subdev_call(sd, pad, get_edid, edid);
> }
>
> case VIDIOC_S_EDID: {
> struct v4l2_subdev_edid *edid = arg;
>
> - rval = check_edid(sd, edid);
> - if (rval)
> - return rval;
> -
> return v4l2_subdev_call(sd, pad, set_edid, edid);
> }
>
> case VIDIOC_SUBDEV_DV_TIMINGS_CAP: {
> struct v4l2_dv_timings_cap *cap = arg;
>
> - if (cap->pad >= sd->entity.num_pads)
> - return -EINVAL;
> -
> return v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
> }
>
> case VIDIOC_SUBDEV_ENUM_DV_TIMINGS: {
> struct v4l2_enum_dv_timings *dvt = arg;
>
> - if (dvt->pad >= sd->entity.num_pads)
> - return -EINVAL;
> -
> return v4l2_subdev_call(sd, pad, enum_dv_timings, dvt);
> }
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a7fa5b80915a..900f265fd950 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1091,6 +1091,8 @@ void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg);
> void v4l2_subdev_init(struct v4l2_subdev *sd,
> const struct v4l2_subdev_ops *ops);
>
> +extern struct v4l2_subdev_ops v4l2_subdev_call_chk_args;
> +
> /**
> * v4l2_subdev_call - call an operation of a v4l2_subdev.
> *
> @@ -1112,6 +1114,10 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
> __result = -ENODEV; \
> else if (!(__sd->ops->o && __sd->ops->o->f)) \
> __result = -ENOIOCTLCMD; \
> + else if (v4l2_subdev_call_chk_args.o && \
> + v4l2_subdev_call_chk_args.o->f) \
> + __result = v4l2_subdev_call_chk_args.o->f( \
> + __sd, ##args); \
> else \
> __result = __sd->ops->o->f(__sd, ##args); \
> __result; \
>
