On Wed, Apr 29, 2020 at 12:16:00PM +0200, Jacopo Mondi wrote:
> 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.
Um, as they're only used here, they indeed should remain here, not in
the header. I missed that while commenting.
>
> 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 ?
The fops struct is still there. It shouldn't be called, no, but if that
happens, then providing the right return value is helpful.
>
> 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;
Right, agreed.
--
Regards,
Sakari Ailus
