Re: [PATCH v5.1] media: v4l2-subdev: Guard whole fops and ioctl hdlr

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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@jmondi.org>
> > > ---
> > > 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