* [PATCH 1/1] v4l: subdev: Allow 32-bit compat IOCTLs @ 2014-01-31 15:28 Sakari Ailus 2014-01-31 15:37 ` Hans Verkuil 0 siblings, 1 reply; 11+ messages in thread From: Sakari Ailus @ 2014-01-31 15:28 UTC (permalink / raw) To: linux-media I thought this was already working but apparently not. Allow 32-bit compat IOCTLs on 64-bit systems. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 996c248..99c54f4 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -389,6 +389,9 @@ const struct v4l2_file_operations v4l2_subdev_fops = { .owner = THIS_MODULE, .open = subdev_open, .unlocked_ioctl = subdev_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl32 = subdev_ioctl, +#endif /* CONFIG_COMPAT */ .release = subdev_close, .poll = subdev_poll, }; -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] v4l: subdev: Allow 32-bit compat IOCTLs 2014-01-31 15:28 [PATCH 1/1] v4l: subdev: Allow 32-bit compat IOCTLs Sakari Ailus @ 2014-01-31 15:37 ` Hans Verkuil 2014-01-31 15:51 ` Sakari Ailus 0 siblings, 1 reply; 11+ messages in thread From: Hans Verkuil @ 2014-01-31 15:37 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media Hi Sakari, Sorry, this isn't right. It should go through v4l2_compat_ioctl32, otherwise ioctls for e.g. extended controls won't be converted correctly. In addition, v4l2_compat_ioctl32 needs to list all the subdev-specific ioctls. I'd have sworn I did that once, but I've no idea what happened to that patch... Regards, Hans On 01/31/2014 04:28 PM, Sakari Ailus wrote: > I thought this was already working but apparently not. Allow 32-bit compat > IOCTLs on 64-bit systems. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 996c248..99c54f4 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -389,6 +389,9 @@ const struct v4l2_file_operations v4l2_subdev_fops = { > .owner = THIS_MODULE, > .open = subdev_open, > .unlocked_ioctl = subdev_ioctl, > +#ifdef CONFIG_COMPAT > + .compat_ioctl32 = subdev_ioctl, > +#endif /* CONFIG_COMPAT */ > .release = subdev_close, > .poll = subdev_poll, > }; > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] v4l: subdev: Allow 32-bit compat IOCTLs 2014-01-31 15:37 ` Hans Verkuil @ 2014-01-31 15:51 ` Sakari Ailus 2014-01-31 16:05 ` Hans Verkuil 2014-01-31 16:06 ` Sakari Ailus 0 siblings, 2 replies; 11+ messages in thread From: Sakari Ailus @ 2014-01-31 15:51 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media Hi Hans, Thanks for the comments. Hans Verkuil wrote: > Hi Sakari, > > Sorry, this isn't right. > > It should go through v4l2_compat_ioctl32, otherwise ioctls for e.g. extended controls > won't be converted correctly. Now that you mention it, indeed the state back when I thought this was already implemented, the IOCTLs were exactly the same. Now that struct v4l2_subdev_edid is used on VIDIOC_SUBDEV_G_EDID and VIDIOC_SUBDEV_S_EDID32, this no longer holds. The two IOCTLs are already handled by v4l2_compat_ioctl32 explicitly. Perhaps that's what you remember? :-) -- Kind regards, Sakari Ailus sakari.ailus@linux.intel.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] v4l: subdev: Allow 32-bit compat IOCTLs 2014-01-31 15:51 ` Sakari Ailus @ 2014-01-31 16:05 ` Hans Verkuil 2014-01-31 16:06 ` Sakari Ailus 1 sibling, 0 replies; 11+ messages in thread From: Hans Verkuil @ 2014-01-31 16:05 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media On 01/31/2014 04:51 PM, Sakari Ailus wrote: > Hi Hans, > > Thanks for the comments. > > Hans Verkuil wrote: >> Hi Sakari, >> >> Sorry, this isn't right. >> >> It should go through v4l2_compat_ioctl32, otherwise ioctls for e.g. extended controls >> won't be converted correctly. > > Now that you mention it, indeed the state back when I thought this was already implemented, the IOCTLs were exactly the same. Now that struct v4l2_subdev_edid is used on VIDIOC_SUBDEV_G_EDID and VIDIOC_SUBDEV_S_EDID32, this no longer holds. > > The two IOCTLs are already handled by v4l2_compat_ioctl32 explicitly. Perhaps that's what you remember? :-) > No, someone recently mentioned similar problems with compat32 and subdev nodes. I did some work on it then, but I've no idea where it is :-( I did add support for subdev ioctl32 tests to v4l-utils.git recently, so I know I worked on it... It could be on one other test server that I can't access from here, I'll check on Tuesday. Regards, Hans ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] v4l: subdev: Allow 32-bit compat IOCTLs 2014-01-31 15:51 ` Sakari Ailus 2014-01-31 16:05 ` Hans Verkuil @ 2014-01-31 16:06 ` Sakari Ailus 2014-01-31 16:07 ` Hans Verkuil 1 sibling, 1 reply; 11+ messages in thread From: Sakari Ailus @ 2014-01-31 16:06 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media Sakari Ailus wrote: > Hi Hans, > > Thanks for the comments. > > Hans Verkuil wrote: >> Hi Sakari, >> >> Sorry, this isn't right. >> >> It should go through v4l2_compat_ioctl32, otherwise ioctls for e.g. >> extended controls >> won't be converted correctly. > > Now that you mention it, indeed the state back when I thought this was > already implemented, the IOCTLs were exactly the same. Now that struct > v4l2_subdev_edid is used on VIDIOC_SUBDEV_G_EDID and > VIDIOC_SUBDEV_S_EDID32, this no longer holds. Well, indeed, with the patch, the compat_ioctl32 handler wrongly would handle the non-compat IOCTL as well. To fix this properly, the sub-device IOCTL numbers that require no conversion should be added to v4l2_compat_ioctl32() list of IOCTLs. Currently they're not there. Is this what you meant? -- Regards, Sakari Ailus sakari.ailus@linux.intel.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] v4l: subdev: Allow 32-bit compat IOCTLs 2014-01-31 16:06 ` Sakari Ailus @ 2014-01-31 16:07 ` Hans Verkuil 2014-01-31 16:15 ` [PATCH v2 " Sakari Ailus 0 siblings, 1 reply; 11+ messages in thread From: Hans Verkuil @ 2014-01-31 16:07 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media On 01/31/2014 05:06 PM, Sakari Ailus wrote: > Sakari Ailus wrote: >> Hi Hans, >> >> Thanks for the comments. >> >> Hans Verkuil wrote: >>> Hi Sakari, >>> >>> Sorry, this isn't right. >>> >>> It should go through v4l2_compat_ioctl32, otherwise ioctls for e.g. >>> extended controls >>> won't be converted correctly. >> >> Now that you mention it, indeed the state back when I thought this was >> already implemented, the IOCTLs were exactly the same. Now that struct >> v4l2_subdev_edid is used on VIDIOC_SUBDEV_G_EDID and >> VIDIOC_SUBDEV_S_EDID32, this no longer holds. > > Well, indeed, with the patch, the compat_ioctl32 handler wrongly would handle the non-compat IOCTL as well. > > To fix this properly, the sub-device IOCTL numbers that require no conversion should be added to v4l2_compat_ioctl32() list of IOCTLs. Currently they're not there. Is this what you meant? > Yes. Hans ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/1] v4l: subdev: Allow 32-bit compat IOCTLs 2014-01-31 16:07 ` Hans Verkuil @ 2014-01-31 16:15 ` Sakari Ailus 2014-01-31 17:35 ` Hans Verkuil 0 siblings, 1 reply; 11+ messages in thread From: Sakari Ailus @ 2014-01-31 16:15 UTC (permalink / raw) To: linux-media; +Cc: hverkuil I thought this was already working but apparently not. Allow 32-bit compat IOCTLs on 64-bit systems. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 8f7a6a4..1fce944 100644 --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -1087,6 +1087,18 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg) case VIDIOC_QUERY_DV_TIMINGS: case VIDIOC_DV_TIMINGS_CAP: case VIDIOC_ENUM_FREQ_BANDS: + /* Sub-device IOCTLs */ + case VIDIOC_SUBDEV_G_FMT: + case VIDIOC_SUBDEV_S_FMT: + case VIDIOC_SUBDEV_G_FRAME_INTERVAL: + case VIDIOC_SUBDEV_S_FRAME_INTERVAL: + case VIDIOC_SUBDEV_ENUM_MBUS_CODE: + case VIDIOC_SUBDEV_ENUM_FRAME_SIZE: + case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: + case VIDIOC_SUBDEV_G_CROP: + case VIDIOC_SUBDEV_S_CROP: + case VIDIOC_SUBDEV_G_SELECTION: + case VIDIOC_SUBDEV_S_SELECTION: case VIDIOC_SUBDEV_G_EDID32: case VIDIOC_SUBDEV_S_EDID32: ret = do_video_ioctl(file, cmd, arg); -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/1] v4l: subdev: Allow 32-bit compat IOCTLs 2014-01-31 16:15 ` [PATCH v2 " Sakari Ailus @ 2014-01-31 17:35 ` Hans Verkuil 2014-01-31 17:52 ` Sakari Ailus 2014-02-04 20:10 ` Mauro Carvalho Chehab 0 siblings, 2 replies; 11+ messages in thread From: Hans Verkuil @ 2014-01-31 17:35 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media Hi Sakari, On 01/31/2014 05:15 PM, Sakari Ailus wrote: > I thought this was already working but apparently not. Allow 32-bit compat > IOCTLs on 64-bit systems. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > index 8f7a6a4..1fce944 100644 > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > @@ -1087,6 +1087,18 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg) > case VIDIOC_QUERY_DV_TIMINGS: > case VIDIOC_DV_TIMINGS_CAP: > case VIDIOC_ENUM_FREQ_BANDS: > + /* Sub-device IOCTLs */ > + case VIDIOC_SUBDEV_G_FMT: > + case VIDIOC_SUBDEV_S_FMT: > + case VIDIOC_SUBDEV_G_FRAME_INTERVAL: > + case VIDIOC_SUBDEV_S_FRAME_INTERVAL: > + case VIDIOC_SUBDEV_ENUM_MBUS_CODE: > + case VIDIOC_SUBDEV_ENUM_FRAME_SIZE: > + case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: > + case VIDIOC_SUBDEV_G_CROP: > + case VIDIOC_SUBDEV_S_CROP: > + case VIDIOC_SUBDEV_G_SELECTION: > + case VIDIOC_SUBDEV_S_SELECTION: > case VIDIOC_SUBDEV_G_EDID32: > case VIDIOC_SUBDEV_S_EDID32: > ret = do_video_ioctl(file, cmd, arg); > Can you test with contrib/test/ioctl-test? Compile with: gcc -o ioctl-test -m32 -I ../../include/ ioctl-test.c Make sure you use the latest v4l-utils version and run autoreconf -vfi and configure first. BTW, I noticed that VIDIOC_DBG_G_CHIP_INFO is missing as well. Hmm, this is just asking for problems. How about this patch: Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 8f7a6a4..cd9da4ce 100644 --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -1001,108 +1001,19 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg) { struct video_device *vdev = video_devdata(file); - long ret = -ENOIOCTLCMD; + long ret = -ENOTTY; if (!file->f_op->unlocked_ioctl) return ret; - switch (cmd) { - case VIDIOC_QUERYCAP: - case VIDIOC_RESERVED: - case VIDIOC_ENUM_FMT: - case VIDIOC_G_FMT32: - case VIDIOC_S_FMT32: - case VIDIOC_REQBUFS: - case VIDIOC_QUERYBUF32: - case VIDIOC_G_FBUF32: - case VIDIOC_S_FBUF32: - case VIDIOC_OVERLAY32: - case VIDIOC_QBUF32: - case VIDIOC_EXPBUF: - case VIDIOC_DQBUF32: - case VIDIOC_STREAMON32: - case VIDIOC_STREAMOFF32: - case VIDIOC_G_PARM: - case VIDIOC_S_PARM: - case VIDIOC_G_STD: - case VIDIOC_S_STD: - case VIDIOC_ENUMSTD32: - case VIDIOC_ENUMINPUT32: - case VIDIOC_G_CTRL: - case VIDIOC_S_CTRL: - case VIDIOC_G_TUNER: - case VIDIOC_S_TUNER: - case VIDIOC_G_AUDIO: - case VIDIOC_S_AUDIO: - case VIDIOC_QUERYCTRL: - case VIDIOC_QUERYMENU: - case VIDIOC_G_INPUT32: - case VIDIOC_S_INPUT32: - case VIDIOC_G_OUTPUT32: - case VIDIOC_S_OUTPUT32: - case VIDIOC_ENUMOUTPUT: - case VIDIOC_G_AUDOUT: - case VIDIOC_S_AUDOUT: - case VIDIOC_G_MODULATOR: - case VIDIOC_S_MODULATOR: - case VIDIOC_S_FREQUENCY: - case VIDIOC_G_FREQUENCY: - case VIDIOC_CROPCAP: - case VIDIOC_G_CROP: - case VIDIOC_S_CROP: - case VIDIOC_G_SELECTION: - case VIDIOC_S_SELECTION: - case VIDIOC_G_JPEGCOMP: - case VIDIOC_S_JPEGCOMP: - case VIDIOC_QUERYSTD: - case VIDIOC_TRY_FMT32: - case VIDIOC_ENUMAUDIO: - case VIDIOC_ENUMAUDOUT: - case VIDIOC_G_PRIORITY: - case VIDIOC_S_PRIORITY: - case VIDIOC_G_SLICED_VBI_CAP: - case VIDIOC_LOG_STATUS: - case VIDIOC_G_EXT_CTRLS32: - case VIDIOC_S_EXT_CTRLS32: - case VIDIOC_TRY_EXT_CTRLS32: - case VIDIOC_ENUM_FRAMESIZES: - case VIDIOC_ENUM_FRAMEINTERVALS: - case VIDIOC_G_ENC_INDEX: - case VIDIOC_ENCODER_CMD: - case VIDIOC_TRY_ENCODER_CMD: - case VIDIOC_DECODER_CMD: - case VIDIOC_TRY_DECODER_CMD: - case VIDIOC_DBG_S_REGISTER: - case VIDIOC_DBG_G_REGISTER: - case VIDIOC_S_HW_FREQ_SEEK: - case VIDIOC_S_DV_TIMINGS: - case VIDIOC_G_DV_TIMINGS: - case VIDIOC_DQEVENT: - case VIDIOC_DQEVENT32: - case VIDIOC_SUBSCRIBE_EVENT: - case VIDIOC_UNSUBSCRIBE_EVENT: - case VIDIOC_CREATE_BUFS32: - case VIDIOC_PREPARE_BUF32: - case VIDIOC_ENUM_DV_TIMINGS: - case VIDIOC_QUERY_DV_TIMINGS: - case VIDIOC_DV_TIMINGS_CAP: - case VIDIOC_ENUM_FREQ_BANDS: - case VIDIOC_SUBDEV_G_EDID32: - case VIDIOC_SUBDEV_S_EDID32: + if (_IOC_NR(cmd) < BASE_VIDIOC_PRIVATE) ret = do_video_ioctl(file, cmd, arg); - break; + else if (vdev->fops->compat_ioctl32) + ret = vdev->fops->compat_ioctl32(file, cmd, arg); - default: - if (vdev->fops->compat_ioctl32) - ret = vdev->fops->compat_ioctl32(file, cmd, arg); - - if (ret == -ENOIOCTLCMD) - printk(KERN_WARNING "compat_ioctl32: " - "unknown ioctl '%c', dir=%d, #%d (0x%08x)\n", - _IOC_TYPE(cmd), _IOC_DIR(cmd), _IOC_NR(cmd), - cmd); - break; - } + if (ret == -ENOTTY) + pr_warn("compat_ioctl32: unknown ioctl '%c', dir=%d, #%d (0x%08x)\n", + _IOC_TYPE(cmd), _IOC_DIR(cmd), _IOC_NR(cmd), cmd); return ret; } EXPORT_SYMBOL_GPL(v4l2_compat_ioctl32); Note the ENOIOCTLCMD to ENOTTY changes: ENOTTY should be returned if the ioctl is not supported. Although v4l2-subdev seems to return ENOIOCTLCMD as well :-( Regards, Hans ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/1] v4l: subdev: Allow 32-bit compat IOCTLs 2014-01-31 17:35 ` Hans Verkuil @ 2014-01-31 17:52 ` Sakari Ailus 2014-02-04 20:10 ` Mauro Carvalho Chehab 1 sibling, 0 replies; 11+ messages in thread From: Sakari Ailus @ 2014-01-31 17:52 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media Hi Hans, Hans Verkuil wrote: > On 01/31/2014 05:15 PM, Sakari Ailus wrote: >> I thought this was already working but apparently not. Allow 32-bit compat >> IOCTLs on 64-bit systems. >> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> --- >> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> index 8f7a6a4..1fce944 100644 >> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> @@ -1087,6 +1087,18 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg) >> case VIDIOC_QUERY_DV_TIMINGS: >> case VIDIOC_DV_TIMINGS_CAP: >> case VIDIOC_ENUM_FREQ_BANDS: >> + /* Sub-device IOCTLs */ >> + case VIDIOC_SUBDEV_G_FMT: >> + case VIDIOC_SUBDEV_S_FMT: >> + case VIDIOC_SUBDEV_G_FRAME_INTERVAL: >> + case VIDIOC_SUBDEV_S_FRAME_INTERVAL: >> + case VIDIOC_SUBDEV_ENUM_MBUS_CODE: >> + case VIDIOC_SUBDEV_ENUM_FRAME_SIZE: >> + case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: >> + case VIDIOC_SUBDEV_G_CROP: >> + case VIDIOC_SUBDEV_S_CROP: >> + case VIDIOC_SUBDEV_G_SELECTION: >> + case VIDIOC_SUBDEV_S_SELECTION: >> case VIDIOC_SUBDEV_G_EDID32: >> case VIDIOC_SUBDEV_S_EDID32: >> ret = do_video_ioctl(file, cmd, arg); >> > > Can you test with contrib/test/ioctl-test? Compile with: > > gcc -o ioctl-test -m32 -I ../../include/ ioctl-test.c > > Make sure you use the latest v4l-utils version and run autoreconf -vfi > and configure first. > > BTW, I noticed that VIDIOC_DBG_G_CHIP_INFO is missing as well. > > Hmm, this is just asking for problems. > > How about this patch: > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > index 8f7a6a4..cd9da4ce 100644 > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > @@ -1001,108 +1001,19 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar > long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg) > { > struct video_device *vdev = video_devdata(file); > - long ret = -ENOIOCTLCMD; > + long ret = -ENOTTY; I don't think we should return -ENOTTY here. The conversion is performed in compat_sys_ioctl() (in fs/compat_ioctl.c) which, if I understand correctly, expressly expects -ENOIOCTLCMD instead. Otherwise this looks good to me. -- Regards, Sakari Ailus sakari.ailus@linux.intel.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/1] v4l: subdev: Allow 32-bit compat IOCTLs 2014-01-31 17:35 ` Hans Verkuil 2014-01-31 17:52 ` Sakari Ailus @ 2014-02-04 20:10 ` Mauro Carvalho Chehab 2014-02-05 7:03 ` Hans Verkuil 1 sibling, 1 reply; 11+ messages in thread From: Mauro Carvalho Chehab @ 2014-02-04 20:10 UTC (permalink / raw) To: Hans Verkuil; +Cc: Sakari Ailus, linux-media Em Fri, 31 Jan 2014 18:35:12 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > Hi Sakari, > > On 01/31/2014 05:15 PM, Sakari Ailus wrote: > > I thought this was already working but apparently not. Allow 32-bit compat > > IOCTLs on 64-bit systems. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > > index 8f7a6a4..1fce944 100644 > > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > > @@ -1087,6 +1087,18 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg) > > case VIDIOC_QUERY_DV_TIMINGS: > > case VIDIOC_DV_TIMINGS_CAP: > > case VIDIOC_ENUM_FREQ_BANDS: > > + /* Sub-device IOCTLs */ > > + case VIDIOC_SUBDEV_G_FMT: > > + case VIDIOC_SUBDEV_S_FMT: > > + case VIDIOC_SUBDEV_G_FRAME_INTERVAL: > > + case VIDIOC_SUBDEV_S_FRAME_INTERVAL: > > + case VIDIOC_SUBDEV_ENUM_MBUS_CODE: > > + case VIDIOC_SUBDEV_ENUM_FRAME_SIZE: > > + case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: > > + case VIDIOC_SUBDEV_G_CROP: > > + case VIDIOC_SUBDEV_S_CROP: > > + case VIDIOC_SUBDEV_G_SELECTION: > > + case VIDIOC_SUBDEV_S_SELECTION: > > case VIDIOC_SUBDEV_G_EDID32: > > case VIDIOC_SUBDEV_S_EDID32: > > ret = do_video_ioctl(file, cmd, arg); > > > > Can you test with contrib/test/ioctl-test? Compile with: > > gcc -o ioctl-test -m32 -I ../../include/ ioctl-test.c > > Make sure you use the latest v4l-utils version and run autoreconf -vfi > and configure first. > > BTW, I noticed that VIDIOC_DBG_G_CHIP_INFO is missing as well. > > Hmm, this is just asking for problems. > > How about this patch: > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > index 8f7a6a4..cd9da4ce 100644 > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > @@ -1001,108 +1001,19 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar > long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg) > { > struct video_device *vdev = video_devdata(file); > - long ret = -ENOIOCTLCMD; > + long ret = -ENOTTY; > > if (!file->f_op->unlocked_ioctl) > return ret; > > - switch (cmd) { > - case VIDIOC_QUERYCAP: > - case VIDIOC_RESERVED: > - case VIDIOC_ENUM_FMT: > - case VIDIOC_G_FMT32: > - case VIDIOC_S_FMT32: > - case VIDIOC_REQBUFS: > - case VIDIOC_QUERYBUF32: > - case VIDIOC_G_FBUF32: > - case VIDIOC_S_FBUF32: > - case VIDIOC_OVERLAY32: > - case VIDIOC_QBUF32: > - case VIDIOC_EXPBUF: > - case VIDIOC_DQBUF32: > - case VIDIOC_STREAMON32: > - case VIDIOC_STREAMOFF32: > - case VIDIOC_G_PARM: > - case VIDIOC_S_PARM: > - case VIDIOC_G_STD: > - case VIDIOC_S_STD: > - case VIDIOC_ENUMSTD32: > - case VIDIOC_ENUMINPUT32: > - case VIDIOC_G_CTRL: > - case VIDIOC_S_CTRL: > - case VIDIOC_G_TUNER: > - case VIDIOC_S_TUNER: > - case VIDIOC_G_AUDIO: > - case VIDIOC_S_AUDIO: > - case VIDIOC_QUERYCTRL: > - case VIDIOC_QUERYMENU: > - case VIDIOC_G_INPUT32: > - case VIDIOC_S_INPUT32: > - case VIDIOC_G_OUTPUT32: > - case VIDIOC_S_OUTPUT32: > - case VIDIOC_ENUMOUTPUT: > - case VIDIOC_G_AUDOUT: > - case VIDIOC_S_AUDOUT: > - case VIDIOC_G_MODULATOR: > - case VIDIOC_S_MODULATOR: > - case VIDIOC_S_FREQUENCY: > - case VIDIOC_G_FREQUENCY: > - case VIDIOC_CROPCAP: > - case VIDIOC_G_CROP: > - case VIDIOC_S_CROP: > - case VIDIOC_G_SELECTION: > - case VIDIOC_S_SELECTION: > - case VIDIOC_G_JPEGCOMP: > - case VIDIOC_S_JPEGCOMP: > - case VIDIOC_QUERYSTD: > - case VIDIOC_TRY_FMT32: > - case VIDIOC_ENUMAUDIO: > - case VIDIOC_ENUMAUDOUT: > - case VIDIOC_G_PRIORITY: > - case VIDIOC_S_PRIORITY: > - case VIDIOC_G_SLICED_VBI_CAP: > - case VIDIOC_LOG_STATUS: > - case VIDIOC_G_EXT_CTRLS32: > - case VIDIOC_S_EXT_CTRLS32: > - case VIDIOC_TRY_EXT_CTRLS32: > - case VIDIOC_ENUM_FRAMESIZES: > - case VIDIOC_ENUM_FRAMEINTERVALS: > - case VIDIOC_G_ENC_INDEX: > - case VIDIOC_ENCODER_CMD: > - case VIDIOC_TRY_ENCODER_CMD: > - case VIDIOC_DECODER_CMD: > - case VIDIOC_TRY_DECODER_CMD: > - case VIDIOC_DBG_S_REGISTER: > - case VIDIOC_DBG_G_REGISTER: > - case VIDIOC_S_HW_FREQ_SEEK: > - case VIDIOC_S_DV_TIMINGS: > - case VIDIOC_G_DV_TIMINGS: > - case VIDIOC_DQEVENT: > - case VIDIOC_DQEVENT32: > - case VIDIOC_SUBSCRIBE_EVENT: > - case VIDIOC_UNSUBSCRIBE_EVENT: > - case VIDIOC_CREATE_BUFS32: > - case VIDIOC_PREPARE_BUF32: > - case VIDIOC_ENUM_DV_TIMINGS: > - case VIDIOC_QUERY_DV_TIMINGS: > - case VIDIOC_DV_TIMINGS_CAP: > - case VIDIOC_ENUM_FREQ_BANDS: > - case VIDIOC_SUBDEV_G_EDID32: > - case VIDIOC_SUBDEV_S_EDID32: > + if (_IOC_NR(cmd) < BASE_VIDIOC_PRIVATE) > ret = do_video_ioctl(file, cmd, arg); I liked this approach. > - break; > + else if (vdev->fops->compat_ioctl32) > + ret = vdev->fops->compat_ioctl32(file, cmd, arg); > > - default: > - if (vdev->fops->compat_ioctl32) > - ret = vdev->fops->compat_ioctl32(file, cmd, arg); > - > - if (ret == -ENOIOCTLCMD) > - printk(KERN_WARNING "compat_ioctl32: " > - "unknown ioctl '%c', dir=%d, #%d (0x%08x)\n", > - _IOC_TYPE(cmd), _IOC_DIR(cmd), _IOC_NR(cmd), > - cmd); > - break; > - } > + if (ret == -ENOTTY) > + pr_warn("compat_ioctl32: unknown ioctl '%c', dir=%d, #%d (0x%08x)\n", > + _IOC_TYPE(cmd), _IOC_DIR(cmd), _IOC_NR(cmd), cmd); I would use, instead, pr_dbg(). > return ret; > } > EXPORT_SYMBOL_GPL(v4l2_compat_ioctl32); > > Note the ENOIOCTLCMD to ENOTTY changes: ENOTTY should be returned if the ioctl is > not supported. Although v4l2-subdev seems to return ENOIOCTLCMD as well :-( > > Regards, > > Hans > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Cheers, Mauro ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/1] v4l: subdev: Allow 32-bit compat IOCTLs 2014-02-04 20:10 ` Mauro Carvalho Chehab @ 2014-02-05 7:03 ` Hans Verkuil 0 siblings, 0 replies; 11+ messages in thread From: Hans Verkuil @ 2014-02-05 7:03 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Sakari Ailus, linux-media On 02/04/2014 09:10 PM, Mauro Carvalho Chehab wrote: > Em Fri, 31 Jan 2014 18:35:12 +0100 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> Hi Sakari, >> >> On 01/31/2014 05:15 PM, Sakari Ailus wrote: >>> I thought this was already working but apparently not. Allow 32-bit compat >>> IOCTLs on 64-bit systems. >>> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> --- >>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >>> index 8f7a6a4..1fce944 100644 >>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >>> @@ -1087,6 +1087,18 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg) >>> case VIDIOC_QUERY_DV_TIMINGS: >>> case VIDIOC_DV_TIMINGS_CAP: >>> case VIDIOC_ENUM_FREQ_BANDS: >>> + /* Sub-device IOCTLs */ >>> + case VIDIOC_SUBDEV_G_FMT: >>> + case VIDIOC_SUBDEV_S_FMT: >>> + case VIDIOC_SUBDEV_G_FRAME_INTERVAL: >>> + case VIDIOC_SUBDEV_S_FRAME_INTERVAL: >>> + case VIDIOC_SUBDEV_ENUM_MBUS_CODE: >>> + case VIDIOC_SUBDEV_ENUM_FRAME_SIZE: >>> + case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: >>> + case VIDIOC_SUBDEV_G_CROP: >>> + case VIDIOC_SUBDEV_S_CROP: >>> + case VIDIOC_SUBDEV_G_SELECTION: >>> + case VIDIOC_SUBDEV_S_SELECTION: >>> case VIDIOC_SUBDEV_G_EDID32: >>> case VIDIOC_SUBDEV_S_EDID32: >>> ret = do_video_ioctl(file, cmd, arg); >>> >> >> Can you test with contrib/test/ioctl-test? Compile with: >> >> gcc -o ioctl-test -m32 -I ../../include/ ioctl-test.c >> >> Make sure you use the latest v4l-utils version and run autoreconf -vfi >> and configure first. >> >> BTW, I noticed that VIDIOC_DBG_G_CHIP_INFO is missing as well. >> >> Hmm, this is just asking for problems. >> >> How about this patch: >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >> >> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> index 8f7a6a4..cd9da4ce 100644 >> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c >> @@ -1001,108 +1001,19 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar >> long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg) >> { >> struct video_device *vdev = video_devdata(file); >> - long ret = -ENOIOCTLCMD; >> + long ret = -ENOTTY; >> >> if (!file->f_op->unlocked_ioctl) >> return ret; >> >> - switch (cmd) { >> - case VIDIOC_QUERYCAP: >> - case VIDIOC_RESERVED: >> - case VIDIOC_ENUM_FMT: >> - case VIDIOC_G_FMT32: >> - case VIDIOC_S_FMT32: >> - case VIDIOC_REQBUFS: >> - case VIDIOC_QUERYBUF32: >> - case VIDIOC_G_FBUF32: >> - case VIDIOC_S_FBUF32: >> - case VIDIOC_OVERLAY32: >> - case VIDIOC_QBUF32: >> - case VIDIOC_EXPBUF: >> - case VIDIOC_DQBUF32: >> - case VIDIOC_STREAMON32: >> - case VIDIOC_STREAMOFF32: >> - case VIDIOC_G_PARM: >> - case VIDIOC_S_PARM: >> - case VIDIOC_G_STD: >> - case VIDIOC_S_STD: >> - case VIDIOC_ENUMSTD32: >> - case VIDIOC_ENUMINPUT32: >> - case VIDIOC_G_CTRL: >> - case VIDIOC_S_CTRL: >> - case VIDIOC_G_TUNER: >> - case VIDIOC_S_TUNER: >> - case VIDIOC_G_AUDIO: >> - case VIDIOC_S_AUDIO: >> - case VIDIOC_QUERYCTRL: >> - case VIDIOC_QUERYMENU: >> - case VIDIOC_G_INPUT32: >> - case VIDIOC_S_INPUT32: >> - case VIDIOC_G_OUTPUT32: >> - case VIDIOC_S_OUTPUT32: >> - case VIDIOC_ENUMOUTPUT: >> - case VIDIOC_G_AUDOUT: >> - case VIDIOC_S_AUDOUT: >> - case VIDIOC_G_MODULATOR: >> - case VIDIOC_S_MODULATOR: >> - case VIDIOC_S_FREQUENCY: >> - case VIDIOC_G_FREQUENCY: >> - case VIDIOC_CROPCAP: >> - case VIDIOC_G_CROP: >> - case VIDIOC_S_CROP: >> - case VIDIOC_G_SELECTION: >> - case VIDIOC_S_SELECTION: >> - case VIDIOC_G_JPEGCOMP: >> - case VIDIOC_S_JPEGCOMP: >> - case VIDIOC_QUERYSTD: >> - case VIDIOC_TRY_FMT32: >> - case VIDIOC_ENUMAUDIO: >> - case VIDIOC_ENUMAUDOUT: >> - case VIDIOC_G_PRIORITY: >> - case VIDIOC_S_PRIORITY: >> - case VIDIOC_G_SLICED_VBI_CAP: >> - case VIDIOC_LOG_STATUS: >> - case VIDIOC_G_EXT_CTRLS32: >> - case VIDIOC_S_EXT_CTRLS32: >> - case VIDIOC_TRY_EXT_CTRLS32: >> - case VIDIOC_ENUM_FRAMESIZES: >> - case VIDIOC_ENUM_FRAMEINTERVALS: >> - case VIDIOC_G_ENC_INDEX: >> - case VIDIOC_ENCODER_CMD: >> - case VIDIOC_TRY_ENCODER_CMD: >> - case VIDIOC_DECODER_CMD: >> - case VIDIOC_TRY_DECODER_CMD: >> - case VIDIOC_DBG_S_REGISTER: >> - case VIDIOC_DBG_G_REGISTER: >> - case VIDIOC_S_HW_FREQ_SEEK: >> - case VIDIOC_S_DV_TIMINGS: >> - case VIDIOC_G_DV_TIMINGS: >> - case VIDIOC_DQEVENT: >> - case VIDIOC_DQEVENT32: >> - case VIDIOC_SUBSCRIBE_EVENT: >> - case VIDIOC_UNSUBSCRIBE_EVENT: >> - case VIDIOC_CREATE_BUFS32: >> - case VIDIOC_PREPARE_BUF32: >> - case VIDIOC_ENUM_DV_TIMINGS: >> - case VIDIOC_QUERY_DV_TIMINGS: >> - case VIDIOC_DV_TIMINGS_CAP: >> - case VIDIOC_ENUM_FREQ_BANDS: >> - case VIDIOC_SUBDEV_G_EDID32: >> - case VIDIOC_SUBDEV_S_EDID32: >> + if (_IOC_NR(cmd) < BASE_VIDIOC_PRIVATE) >> ret = do_video_ioctl(file, cmd, arg); > > I liked this approach. > >> - break; >> + else if (vdev->fops->compat_ioctl32) >> + ret = vdev->fops->compat_ioctl32(file, cmd, arg); >> >> - default: >> - if (vdev->fops->compat_ioctl32) >> - ret = vdev->fops->compat_ioctl32(file, cmd, arg); >> - >> - if (ret == -ENOIOCTLCMD) >> - printk(KERN_WARNING "compat_ioctl32: " >> - "unknown ioctl '%c', dir=%d, #%d (0x%08x)\n", >> - _IOC_TYPE(cmd), _IOC_DIR(cmd), _IOC_NR(cmd), >> - cmd); >> - break; >> - } >> + if (ret == -ENOTTY) >> + pr_warn("compat_ioctl32: unknown ioctl '%c', dir=%d, #%d (0x%08x)\n", >> + _IOC_TYPE(cmd), _IOC_DIR(cmd), _IOC_NR(cmd), cmd); > > I would use, instead, pr_dbg(). I plan to take another careful look at this over the weekend. Regards, Hans > >> return ret; >> } >> EXPORT_SYMBOL_GPL(v4l2_compat_ioctl32); >> >> Note the ENOIOCTLCMD to ENOTTY changes: ENOTTY should be returned if the ioctl is >> not supported. Although v4l2-subdev seems to return ENOIOCTLCMD as well :-( >> >> Regards, >> >> Hans >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-media" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-02-05 7:04 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-31 15:28 [PATCH 1/1] v4l: subdev: Allow 32-bit compat IOCTLs Sakari Ailus 2014-01-31 15:37 ` Hans Verkuil 2014-01-31 15:51 ` Sakari Ailus 2014-01-31 16:05 ` Hans Verkuil 2014-01-31 16:06 ` Sakari Ailus 2014-01-31 16:07 ` Hans Verkuil 2014-01-31 16:15 ` [PATCH v2 " Sakari Ailus 2014-01-31 17:35 ` Hans Verkuil 2014-01-31 17:52 ` Sakari Ailus 2014-02-04 20:10 ` Mauro Carvalho Chehab 2014-02-05 7:03 ` Hans Verkuil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox