* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.