2019-05-14 22:52:38

by Janusz Krzysztofik

[permalink] [raw]
Subject: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

Correctness of format type (try or active) and pad number 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.

Signed-off-by: Janusz Krzysztofik <[email protected]>
---
drivers/media/v4l2-core/v4l2-subdev.c | 232 ++++++++++++++++----------
include/media/v4l2-subdev.h | 6 +
2 files changed, 146 insertions(+), 92 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index d75815ab0d7b..c61c95007d89 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -120,56 +120,169 @@ 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 inline 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 inline 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 inline 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 call_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_selection(struct v4l2_subdev *sd,
- struct v4l2_subdev_selection *sel)
+static int call_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *format)
{
- if (sel->which != V4L2_SUBDEV_FORMAT_TRY &&
- sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
- return -EINVAL;
+ return check_format(sd, format) ? :
+ sd->ops->pad->set_fmt(sd, cfg, format);
+}

- if (sel->pad >= sd->entity.num_pads)
- return -EINVAL;
+static int call_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);
+}

- return 0;
+static int call_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_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
+static inline int check_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fi)
{
- if (edid->pad >= sd->entity.num_pads)
- return -EINVAL;
+ return check_pad(sd, fi->pad);
+}
+
+static int call_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 call_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 call_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 inline int check_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_selection *sel)
+{
+ return check_which(sel->which) ? : check_pad(sd, sel->pad);
+}
+
+static int call_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);
+}
+
+static int call_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 inline int check_edid(struct v4l2_subdev *sd,
+ struct v4l2_subdev_edid *edid)
+{
if (edid->blocks && edid->edid == NULL)
return -EINVAL;

- return 0;
+ return check_pad(sd, edid->pad);
}
-#endif
+
+static int call_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 call_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 call_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 call_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_call_pad_wrappers = {
+ .get_fmt = call_get_fmt,
+ .set_fmt = call_set_fmt,
+ .enum_mbus_code = call_enum_mbus_code,
+ .enum_frame_size = call_enum_frame_size,
+ .enum_frame_interval = call_enum_frame_interval,
+ .get_selection = call_get_selection,
+ .set_selection = call_set_selection,
+ .get_edid = call_get_edid,
+ .set_edid = call_set_edid,
+ .dv_timings_cap = call_dv_timings_cap,
+ .enum_dv_timings = call_enum_dv_timings,
+};
+
+static const struct v4l2_subdev_video_ops v4l2_subdev_call_video_wrappers = {
+ .g_frame_interval = call_g_frame_interval,
+ .s_frame_interval = call_s_frame_interval,
+};
+
+const struct v4l2_subdev_ops v4l2_subdev_call_wrappers = {
+ .pad = &v4l2_subdev_call_pad_wrappers,
+ .video = &v4l2_subdev_call_video_wrappers,
+};
+EXPORT_SYMBOL(v4l2_subdev_call_wrappers);

static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
{
@@ -292,10 +405,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 +413,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 +422,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 +441,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 +458,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 +466,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 +474,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 +481,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 +488,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 +496,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 +504,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 +512,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..e1e3c18c3fd6 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 const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
+
/**
* 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_wrappers.o && \
+ v4l2_subdev_call_wrappers.o->f) \
+ __result = v4l2_subdev_call_wrappers.o->f( \
+ __sd, ##args); \
else \
__result = __sd->ops->o->f(__sd, ##args); \
__result; \
--
2.21.0


2019-05-15 07:17:28

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

Hi Janusz,

On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop *crop)
> +static inline 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)

One more comment.

The num_pads doesn't really tell whether a given op is valid for a device.
Well, in this case it would have to be a bug in the driver, but those do
happen. How about checking for sd->entity.graph_obj.mdev instead? It's
non-NULL if the entity is registered with a media device, i.e. when these
callback functions are supposed to be called.

--
Sakari Ailus
[email protected]

2019-05-15 20:59:10

by Janusz Krzysztofik

[permalink] [raw]
Subject: Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

Hi Sakari,

On Wednesday, May 15, 2019 9:16:02 AM CEST Sakari Ailus wrote:
> Hi Janusz,
>
> On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> > -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop
*crop)
> > +static inline 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)
>
> One more comment.
>
> The num_pads doesn't really tell whether a given op is valid for a device.
> Well, in this case it would have to be a bug in the driver, but those do
> happen. How about checking for sd->entity.graph_obj.mdev instead? It's
> non-NULL if the entity is registered with a media device, i.e. when these
> callback functions are supposed to be called.

Before I do that, let me undestand your point better.

My intentions were:
1) to provide a check for validity of a pad ID passed to an operation, not ann
eligibility of a driver to support the operation,
2) to not break drivers which don't set pad_num, especially when building them
with CONFIG_MEDIA_CONTROLLER turned on for whatever reason.

Since pad IDs are verified against pad_num which may be not set, we should
obviously check validity of pad_num before comparing against it. Since media
controller compatible subdevices need at least one pad, I think the check for
non-zero pad_num is quite reasonable.

Moreover, old drivers are actually using those pad operations you describe as
not supposed to be called. They are using them because they were converted to
use them in place of former video ops. Already dealing with pad IDs, they may
decide to turn on CONFIG_MEDIA_CONTROLLER and use selected functionality, for
example register pads, without implementing fulll media controller support.
Why should we refuse to perform pad ID verification for them?

Thanks,
Janusz




2019-05-17 17:15:41

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

Hi Janusz,

On Wed, May 15, 2019 at 10:56:36PM +0200, Janusz Krzysztofik wrote:
> Hi Sakari,
>
> On Wednesday, May 15, 2019 9:16:02 AM CEST Sakari Ailus wrote:
> > Hi Janusz,
> >
> > On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> > > -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop
> *crop)
> > > +static inline 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)
> >
> > One more comment.
> >
> > The num_pads doesn't really tell whether a given op is valid for a device.
> > Well, in this case it would have to be a bug in the driver, but those do
> > happen. How about checking for sd->entity.graph_obj.mdev instead? It's
> > non-NULL if the entity is registered with a media device, i.e. when these
> > callback functions are supposed to be called.
>
> Before I do that, let me undestand your point better.
>
> My intentions were:
> 1) to provide a check for validity of a pad ID passed to an operation, not ann
> eligibility of a driver to support the operation,
> 2) to not break drivers which don't set pad_num, especially when building them
> with CONFIG_MEDIA_CONTROLLER turned on for whatever reason.

Indeed.

But these checks still allow calling the pad operations on sub-devices that
have no pads. That should not be allowed. Pads are a Media controller
concept, they do not exist outside it; therefore checking for pads only if
the subdev is a part of the media device would be entirely correct.

It should probably accompany a check that requires the pad number is zero
if the subdev doesn't have a graph object, even if the pad field isn't
supposedly used for any purpose. Would that address your concern?

>
> Since pad IDs are verified against pad_num which may be not set, we should
> obviously check validity of pad_num before comparing against it. Since media
> controller compatible subdevices need at least one pad, I think the check for
> non-zero pad_num is quite reasonable.
>
> Moreover, old drivers are actually using those pad operations you describe as
> not supposed to be called. They are using them because they were converted to
> use them in place of former video ops. Already dealing with pad IDs, they may
> decide to turn on CONFIG_MEDIA_CONTROLLER and use selected functionality, for
> example register pads, without implementing fulll media controller support.
> Why should we refuse to perform pad ID verification for them?

--
Kind regards,

Sakari Ailus
[email protected]

2019-05-17 23:06:25

by Janusz Krzysztofik

[permalink] [raw]
Subject: Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

On Friday, May 17, 2019 5:58:40 PM CEST Sakari Ailus wrote:
> Hi Janusz,
>
> On Wed, May 15, 2019 at 10:56:36PM +0200, Janusz Krzysztofik wrote:
> > Hi Sakari,
> >
> > On Wednesday, May 15, 2019 9:16:02 AM CEST Sakari Ailus wrote:
> > > Hi Janusz,
> > >
> > > On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> > > > -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop
> > *crop)
> > > > +static inline 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)
> > >
> > > One more comment.
> > >
> > > The num_pads doesn't really tell whether a given op is valid for a
device.
> > > Well, in this case it would have to be a bug in the driver, but those do
> > > happen. How about checking for sd->entity.graph_obj.mdev instead? It's
> > > non-NULL if the entity is registered with a media device, i.e. when
these
> > > callback functions are supposed to be called.
> >
> > Before I do that, let me undestand your point better.
> >
> > My intentions were:
> > 1) to provide a check for validity of a pad ID passed to an operation, not
ann
> > eligibility of a driver to support the operation,
> > 2) to not break drivers which don't set pad_num, especially when building
them
> > with CONFIG_MEDIA_CONTROLLER turned on for whatever reason.
>
> Indeed.
>
> But these checks still allow calling the pad operations on sub-devices that
> have no pads. That should not be allowed. Pads are a Media controller
> concept, they do not exist outside it; therefore checking for pads only if
> the subdev is a part of the media device would be entirely correct.

OK, now I see your point. You don't want the check to succeed if a media
entity has num_pads == 0.

> It should probably accompany a check that requires the pad number is zero
> if the subdev doesn't have a graph object, even if the pad field isn't
> supposedly used for any purpose. Would that address your concern?

Yes, that's acceptable. Let's require subdevice drivers to register as media
entities if they want to use pads > 0.

I'll update the patches and submit as v7 soon.

Thanks,
Janusz

> > Since pad IDs are verified against pad_num which may be not set, we should
> > obviously check validity of pad_num before comparing against it. Since
media
> > controller compatible subdevices need at least one pad, I think the check
for
> > non-zero pad_num is quite reasonable.
> >
> > Moreover, old drivers are actually using those pad operations you describe
as
> > not supposed to be called. They are using them because they were
converted to
> > use them in place of former video ops. Already dealing with pad IDs, they
may
> > decide to turn on CONFIG_MEDIA_CONTROLLER and use selected functionality,
for
> > example register pads, without implementing fulll media controller
support.
> > Why should we refuse to perform pad ID verification for them?
>
>