| Message ID | 20200428210609.6793-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On Tue, Apr 28, 2020 at 11:06:07PM +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. > > Remove checks from the v4l2-subdev file handle open/close functions and > ioctl handler as they are only accessible if a device node was registered > to user space in first place. Is there other motivation with this than clean up things a little? The change increases the binary size marginally if the Kconfig option is disabled. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 1dc263c2ca0a..f3fe515b8ccb 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -24,22 +24,19 @@ > > 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) > @@ -329,10 +326,8 @@ 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 +461,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 +640,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); > } > -- > 2.26.1 >
Hi Jacopo,
I love your patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on linus/master v5.7-rc3 next-20200428]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Jacopo-Mondi/media-Register-read-only-sub-dev-devnode/20200429-062133
base: git://linuxtv.org/media_tree.git master
config: arm-at91_dt_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_fh_init':
>> drivers/media/v4l2-core/v4l2-subdev.c:28:5: error: 'struct v4l2_subdev_fh' has no member named 'pad'
28 | fh->pad = v4l2_subdev_alloc_pad_config(sd);
| ^~
drivers/media/v4l2-core/v4l2-subdev.c:29:9: error: 'struct v4l2_subdev_fh' has no member named 'pad'
29 | if (fh->pad == NULL)
| ^~
drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_fh_free':
drivers/media/v4l2-core/v4l2-subdev.c:38:32: error: 'struct v4l2_subdev_fh' has no member named 'pad'
38 | v4l2_subdev_free_pad_config(fh->pad);
| ^~
drivers/media/v4l2-core/v4l2-subdev.c:39:4: error: 'struct v4l2_subdev_fh' has no member named 'pad'
39 | fh->pad = NULL;
| ^~
In file included from include/media/v4l2-device.h:13,
from drivers/media/v4l2-core/v4l2-subdev.c:20:
drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_do_ioctl':
drivers/media/v4l2-core/v4l2-subdev.c:469:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
469 | return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:469:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
469 | return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:480:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
480 | return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:480:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
480 | return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:494:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
494 | sd, pad, get_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:494:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
494 | sd, pad, get_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:516:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
516 | sd, pad, set_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:516:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
516 | sd, pad, set_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:527:61: error: 'struct v4l2_subdev_fh' has no member named 'pad'
527 | return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:527:61: error: 'struct v4l2_subdev_fh' has no member named 'pad'
527 | return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:535:62: error: 'struct v4l2_subdev_fh' has no member named 'pad'
535 | return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:535:62: error: 'struct v4l2_subdev_fh' has no member named 'pad'
535 | return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:560:66: error: 'struct v4l2_subdev_fh' has no member named 'pad'
560 | return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:560:66: error: 'struct v4l2_subdev_fh' has no member named 'pad'
560 | return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:569:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
vim +28 drivers/media/v4l2-core/v4l2-subdev.c
2096a5dcf9704f drivers/media/video/v4l2-subdev.c Laurent Pinchart 2009-12-09 24
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 25 static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 26 {
9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 27 if (sd->entity.num_pads) {
9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 @28 fh->pad = v4l2_subdev_alloc_pad_config(sd);
ae184cda8d0eeb drivers/media/video/v4l2-subdev.c Sakari Ailus 2011-10-14 29 if (fh->pad == NULL)
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 30 return -ENOMEM;
9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 31 }
b9e6aad3939a62 drivers/media/v4l2-core/v4l2-subdev.c Jacopo Mondi 2020-04-28 32
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 33 return 0;
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 34 }
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 35
:::::: The code at line 28 was first introduced by commit
:::::: 9b02cbb3ede89b5cd84bbe4ef493bd130d76b070 [media] v4l: subdev: Add pad config allocator and init
:::::: TO: Laurent Pinchart <laurent.pinchart@linaro.org>
:::::: CC: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Sakari, On Wed, Apr 29, 2020 at 12:26:43AM +0300, Sakari Ailus wrote: > Hi Jacopo, > > On Tue, Apr 28, 2020 at 11:06:07PM +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. > > > > Remove checks from the v4l2-subdev file handle open/close functions and > > ioctl handler as they are only accessible if a device node was registered > > to user space in first place. > > Is there other motivation with this than clean up things a little? > I had to add yet-another #if defined and I got fed up. If you don't have a device node registered you won't be able to access any of the functions where the existing #if defined() where placed. > The change increases the binary size marginally if the Kconfig option is > disabled. > I see. Should we instead guard the whole file handle operations and ioctl handler instead of having #if defined() spread inside them ? I assume they where there as leftover, as I'm still missing the point, give that, as said, without V4L2_SUBDEV_API, you can't register any device node to userspace.. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/media/v4l2-core/v4l2-subdev.c | 10 ++-------- > > 1 file changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 1dc263c2ca0a..f3fe515b8ccb 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -24,22 +24,19 @@ > > > > 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) > > @@ -329,10 +326,8 @@ 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 +461,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 +640,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); > > } > > -- > > 2.26.1 > > > > -- > Sakari Ailus
Here it goes -.-' I was hoping to have this going through kbuild soon Would guarding the whole file handle operations and the ioctl handler a best option then ? On Wed, Apr 29, 2020 at 07:44:30AM +0800, kbuild test robot wrote: > Hi Jacopo, > > I love your patch! Yet something to improve: > > [auto build test ERROR on linuxtv-media/master] > [also build test ERROR on linus/master v5.7-rc3 next-20200428] > [if your patch is applied to the wrong git tree, please drop us a note to help > improve the system. BTW, we also suggest to use '--base' option to specify the > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > url: https://github.com/0day-ci/linux/commits/Jacopo-Mondi/media-Register-read-only-sub-dev-devnode/20200429-062133 > base: git://linuxtv.org/media_tree.git master > config: arm-at91_dt_defconfig (attached as .config) > compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=arm > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kbuild test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_fh_init': > >> drivers/media/v4l2-core/v4l2-subdev.c:28:5: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 28 | fh->pad = v4l2_subdev_alloc_pad_config(sd); > | ^~ > drivers/media/v4l2-core/v4l2-subdev.c:29:9: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 29 | if (fh->pad == NULL) > | ^~ > drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_fh_free': > drivers/media/v4l2-core/v4l2-subdev.c:38:32: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 38 | v4l2_subdev_free_pad_config(fh->pad); > | ^~ > drivers/media/v4l2-core/v4l2-subdev.c:39:4: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 39 | fh->pad = NULL; > | ^~ > In file included from include/media/v4l2-device.h:13, > from drivers/media/v4l2-core/v4l2-subdev.c:20: > drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_do_ioctl': > drivers/media/v4l2-core/v4l2-subdev.c:469:54: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 469 | return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format); > | ^~ > include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call' > 1111 | __sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:469:54: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 469 | return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format); > | ^~ > include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call' > 1113 | __result = __sd->ops->o->f(__sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:480:54: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 480 | return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format); > | ^~ > include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call' > 1111 | __sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:480:54: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 480 | return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format); > | ^~ > include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call' > 1113 | __result = __sd->ops->o->f(__sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:494:37: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 494 | sd, pad, get_selection, subdev_fh->pad, &sel); > | ^~ > include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call' > 1111 | __sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:494:37: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 494 | sd, pad, get_selection, subdev_fh->pad, &sel); > | ^~ > include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call' > 1113 | __result = __sd->ops->o->f(__sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:516:37: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 516 | sd, pad, set_selection, subdev_fh->pad, &sel); > | ^~ > include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call' > 1111 | __sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:516:37: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 516 | sd, pad, set_selection, subdev_fh->pad, &sel); > | ^~ > include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call' > 1113 | __result = __sd->ops->o->f(__sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:527:61: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 527 | return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad, > | ^~ > include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call' > 1111 | __sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:527:61: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 527 | return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad, > | ^~ > include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call' > 1113 | __result = __sd->ops->o->f(__sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:535:62: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 535 | return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad, > | ^~ > include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call' > 1111 | __sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:535:62: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 535 | return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad, > | ^~ > include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call' > 1113 | __result = __sd->ops->o->f(__sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:560:66: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 560 | return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad, > | ^~ > include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call' > 1111 | __sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:560:66: error: 'struct v4l2_subdev_fh' has no member named 'pad' > 560 | return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad, > | ^~ > include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call' > 1113 | __result = __sd->ops->o->f(__sd, ##args); \ > | ^~~~ > drivers/media/v4l2-core/v4l2-subdev.c:569:37: error: 'struct v4l2_subdev_fh' has no member named 'pad' > > vim +28 drivers/media/v4l2-core/v4l2-subdev.c > > 2096a5dcf9704f drivers/media/video/v4l2-subdev.c Laurent Pinchart 2009-12-09 24 > 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 25 static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) > 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 26 { > 9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 27 if (sd->entity.num_pads) { > 9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 @28 fh->pad = v4l2_subdev_alloc_pad_config(sd); > ae184cda8d0eeb drivers/media/video/v4l2-subdev.c Sakari Ailus 2011-10-14 29 if (fh->pad == NULL) > 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 30 return -ENOMEM; > 9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 31 } > b9e6aad3939a62 drivers/media/v4l2-core/v4l2-subdev.c Jacopo Mondi 2020-04-28 32 > 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 33 return 0; > 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 34 } > 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 35 > > :::::: The code at line 28 was first introduced by commit > :::::: 9b02cbb3ede89b5cd84bbe4ef493bd130d76b070 [media] v4l: subdev: Add pad config allocator and init > > :::::: TO: Laurent Pinchart <laurent.pinchart@linaro.org> > :::::: CC: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Jacopo, On Wed, Apr 29, 2020 at 09:02:15AM +0200, Jacopo Mondi wrote: > Hi Sakari, > > On Wed, Apr 29, 2020 at 12:26:43AM +0300, Sakari Ailus wrote: > > Hi Jacopo, > > > > On Tue, Apr 28, 2020 at 11:06:07PM +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. > > > > > > Remove checks from the v4l2-subdev file handle open/close functions and > > > ioctl handler as they are only accessible if a device node was registered > > > to user space in first place. > > > > Is there other motivation with this than clean up things a little? > > > > I had to add yet-another #if defined and I got fed up. If you don't > have a device node registered you won't be able to access any of the > functions where the existing #if defined() where placed. > > > The change increases the binary size marginally if the Kconfig option is > > disabled. > > > > I see. Should we instead guard the whole file handle operations and > ioctl handler instead of having #if defined() spread inside them ? I > assume they where there as leftover, as I'm still missing the point, > give that, as said, without V4L2_SUBDEV_API, you can't register any > device node to userspace.. I think that's why those #ifdefs have been originally put there --- it's just dead code without the subdev nodes, and the compiler won't be able to figure this out. But it seems, later on, when people have added code that supports sub-device nodes, no #ifdefs have been added. I think I'd make sense to remove the current #ifdefs and add dummy ops for everything where needed that truly depends on that Kconfig option (i.e. sub-device nodes). Or just to remove these, as your patch does. It's not a lot of code.
Hi Sakari On Wed, Apr 29, 2020 at 11:27:37AM +0300, Sakari Ailus wrote: > Hi Jacopo, > > On Wed, Apr 29, 2020 at 09:02:15AM +0200, Jacopo Mondi wrote: > > Hi Sakari, > > > > On Wed, Apr 29, 2020 at 12:26:43AM +0300, Sakari Ailus wrote: > > > Hi Jacopo, > > > > > > On Tue, Apr 28, 2020 at 11:06:07PM +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. > > > > > > > > Remove checks from the v4l2-subdev file handle open/close functions and > > > > ioctl handler as they are only accessible if a device node was registered > > > > to user space in first place. > > > > > > Is there other motivation with this than clean up things a little? > > > > > > > I had to add yet-another #if defined and I got fed up. If you don't > > have a device node registered you won't be able to access any of the > > functions where the existing #if defined() where placed. > > > > > The change increases the binary size marginally if the Kconfig option is > > > disabled. > > > > > > > I see. Should we instead guard the whole file handle operations and > > ioctl handler instead of having #if defined() spread inside them ? I > > assume they where there as leftover, as I'm still missing the point, > > give that, as said, without V4L2_SUBDEV_API, you can't register any > > device node to userspace.. > > I think that's why those #ifdefs have been originally put there --- it's > just dead code without the subdev nodes, and the compiler won't be able to > figure this out. > > But it seems, later on, when people have added code that supports > sub-device nodes, no #ifdefs have been added. > > I think I'd make sense to remove the current #ifdefs and add dummy ops for > everything where needed that truly depends on that Kconfig option (i.e. > sub-device nodes). Or just to remove these, as your patch does. It's not a > lot of code. that's what I've done now, as soon as I've run a few compile test, I'll send a new version. Thanks j > > -- > Kind regards, > > Sakari Ailus
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 1dc263c2ca0a..f3fe515b8ccb 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -24,22 +24,19 @@ 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) @@ -329,10 +326,8 @@ 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 +461,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 +640,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); }
A sub-device device node can be registered in user space only if the CONFIG_V4L2_SUBDEV_API Kconfig option is selected. Remove checks from the v4l2-subdev file handle open/close functions and ioctl handler as they are only accessible if a device node was registered to user space in first place. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/media/v4l2-core/v4l2-subdev.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)