Hi Sakari,
On Wed, Apr 29, 2020 at 12:49:49PM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> Thanks for the update.
>
> On Wed, Apr 29, 2020 at 10:58:55AM +0200, Jacopo Mondi wrote:
> > A sub-device device node can be registered in user space only if the
> > CONFIG_V4L2_SUBDEV_API Kconfig option is selected. Currently the
> > open/close file operations and the ioctl handler have some parts of their
> > implementations guarded by #if defined(CONFIG_V4L2_SUBDEV_API), while
> > they are actually not accessible without a video device node registered
> > to user space.
> >
> > Guard the whole open, close and ioctl handler and provide stubs if the
> > V4L2_SUBDEV_API Kconfig option is not selected.
> >
> > This slightly reduces the kernel size when the option is not selected
> > and simplifies the file ops and ioctl implementations.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > ---
> > A different approach compared to v5, which was anyway buggy and not a proper
> > solution.
> >
> > Sending out for comments, while waiting for consensus on v5 [5/6] (reserved
> > space in the ioctl argument vs versioning based on structure size)
> >
> > Compile tested with and without V4L2_SUBDEV_API Kconfig option enabled and
> > with drivers that depends on it built-in or as modules.
> >
> > ---
> > drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++------
> > 1 file changed, 31 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 1dc263c2ca0a..6fef52880c99 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -22,24 +22,22 @@
> > #include <media/v4l2-fh.h>
> > #include <media/v4l2-event.h>
> >
> > +#if defined(CONFIG_V4L2_SUBDEV_API)
> > static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
> > {
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > if (sd->entity.num_pads) {
> > fh->pad = v4l2_subdev_alloc_pad_config(sd);
> > if (fh->pad == NULL)
> > return -ENOMEM;
> > }
> > -#endif
> > +
> > return 0;
> > }
> >
> > static void subdev_fh_free(struct v4l2_subdev_fh *fh)
> > {
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > v4l2_subdev_free_pad_config(fh->pad);
> > fh->pad = NULL;
> > -#endif
> > }
> >
> > static int subdev_open(struct file *file)
> > @@ -111,6 +109,17 @@ static int subdev_close(struct file *file)
> >
> > return 0;
> > }
> > +#else /* CONFIG_V4L2_SUBDEV_API */
> > +static int subdev_open(struct file *file)
> > +{
> > + return 0;
>
> Perhaps:
>
> return -ENODEV;
>
> And I'd use inline functions in the header.
There is no way this functions gets called if no device node is
registered to userspace, I think. These are only here to please the
compiler. Am I mistaken ?
Also, these function are not exported by any headers, but only the
file_operations structure that contains them is:
include/media/v4l2-subdev.h
extern const struct v4l2_file_operations v4l2_subdev_fops;
>
> > +}
> > +
> > +static int subdev_close(struct file *file)
> > +{
> > + return 0;
> > +}
> > +#endif /* CONFIG_V4L2_SUBDEV_API */
> >
> > static inline int check_which(u32 which)
> > {
> > @@ -324,15 +333,14 @@ const struct v4l2_subdev_ops v4l2_subdev_call_wrappers = {
> > };
> > EXPORT_SYMBOL(v4l2_subdev_call_wrappers);
> >
> > +#if defined(CONFIG_V4L2_SUBDEV_API)
> > static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > {
> > struct video_device *vdev = video_devdata(file);
> > struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> > struct v4l2_fh *vfh = file->private_data;
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
> > bool ro_subdev = test_bit(V4L2_FL_SUBDEV_RO_DEVNODE, &vdev->flags);
> > -#endif
> > int rval;
> >
> > switch (cmd) {
> > @@ -466,7 +474,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > return ret;
> > }
> >
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > case VIDIOC_SUBDEV_G_FMT: {
> > struct v4l2_subdev_format *format = arg;
> >
> > @@ -646,7 +653,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >
> > case VIDIOC_SUBDEV_QUERYSTD:
> > return v4l2_subdev_call(sd, video, querystd, arg);
> > -#endif
> > +
> > default:
> > return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
> > }
> > @@ -686,6 +693,22 @@ static long subdev_compat_ioctl32(struct file *file, unsigned int cmd,
> > }
> > #endif
> >
> > +#else /* CONFIG_V4L2_SUBDEV_API */
> > +static long subdev_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return 0;
>
> return -ENOTTY;
>
> > +}
> > +
> > +#ifdef CONFIG_COMPAT
> > +static long subdev_compat_ioctl32(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return 0;
>
> Ditto.
>
> > +}
> > +#endif
> > +#endif /* CONFIG_V4L2_SUBDEV_API */
> > +
> > static __poll_t subdev_poll(struct file *file, poll_table *wait)
> > {
> > struct video_device *vdev = video_devdata(file);
>
> --
> Kind regards,
>
> Sakari Ailus
