On 02/09/2021 10:24, Hans Verkuil wrote:
> Hi Dafna,
>
> On 28/07/2021 12:06, Dafna Hirschfeld wrote:
>> Currently, if the ioctl VIDIOC_DQEVENT_TIME32 is called on e.g. Arm-64
>> the function 'v4l2_compat_translate_cmd' doesn't have a 'translation'
>> for the cmd and so 'cmd' is returned as is. This cause
>> a failure '-ENOTTY' in __video_do_ioctl.
>
> I don't really see how this would happen. As far as I can tell (and admittedly,
> the code is hard to follow) on such a system there is no need for any 32-bit to
> 64-bit conversion in v4l2-compat-ioctl32.c since struct v4l2_event_time32 is the
> same for both architectures.
>
> And in v4l2-ioctl.c VIDIOC_DQEVENT_TIME32 is handled correctly as far as I can
> tell. Where exactly does the return -ENOTTY happen?
>
> It definitely does not make sense to modify v4l2-compat-ioctl32.c if there is
> nothing to convert between 32 and 64 bit. If there is a bug, then that should be
> in v4l2-ioctl.c.
>
> Note that it is perfectly possible that there is a bug for arm64 and this ioctl,
> since that's the one combination I've never tested since I don't have a test setup
> for that.
How did you test this anyway? With which libc/compiler? It would be nice if I could
test this myself on e.g. a Raspberry Pi.
Regards,
Hans
>
> Regards,
>
> Hans
>
>> This patch fixes it by defining VIDIOC_DQEVENT32_TIME32 to be
>> VIDIOC_DQEVENT_TIME32 for non x86-64 arch and translates it
>> to VIDIOC_DQEVENT.
>> In addition, add a call to put_v4l2_event_time32 to copy the data
>> to userspace for non x86-64 arch
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
>> ---
>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 21 ++++++++++++-------
>> include/media/v4l2-ioctl.h | 3 +++
>> 2 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> index 5623003a9705..20a80880d9b7 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> @@ -871,9 +871,13 @@ static int put_v4l2_edid32(struct v4l2_edid *p64,
>> #define VIDIOC_QUERYBUF32_TIME32 _IOWR('V', 9, struct v4l2_buffer32_time32)
>> #define VIDIOC_QBUF32_TIME32 _IOWR('V', 15, struct v4l2_buffer32_time32)
>> #define VIDIOC_DQBUF32_TIME32 _IOWR('V', 17, struct v4l2_buffer32_time32)
>> +
>> #ifdef CONFIG_X86_64
>> #define VIDIOC_DQEVENT32_TIME32 _IOR ('V', 89, struct v4l2_event32_time32)
>> +#else
>> +#define VIDIOC_DQEVENT32_TIME32 VIDIOC_DQEVENT_TIME32
>> #endif
>> +
>> #define VIDIOC_PREPARE_BUF32_TIME32 _IOWR('V', 93, struct v4l2_buffer32_time32)
>> #endif
>>
>> @@ -899,6 +903,8 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd)
>> return VIDIOC_DQBUF;
>> case VIDIOC_PREPARE_BUF32_TIME32:
>> return VIDIOC_PREPARE_BUF;
>> + case VIDIOC_DQEVENT32_TIME32:
>> + return VIDIOC_DQEVENT;
>> #endif
>> case VIDIOC_QUERYBUF32:
>> return VIDIOC_QUERYBUF;
>> @@ -927,10 +933,6 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd)
>> #ifdef CONFIG_X86_64
>> case VIDIOC_DQEVENT32:
>> return VIDIOC_DQEVENT;
>> -#ifdef CONFIG_COMPAT_32BIT_TIME
>> - case VIDIOC_DQEVENT32_TIME32:
>> - return VIDIOC_DQEVENT;
>> -#endif
>> #endif
>> }
>> return cmd;
>> @@ -996,6 +998,13 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd)
>> case VIDIOC_DQBUF32_TIME32:
>> case VIDIOC_PREPARE_BUF32_TIME32:
>> return put_v4l2_buffer32_time32(parg, arg);
>> + case VIDIOC_DQEVENT32_TIME32:
>> +#ifdef CONFIG_X86_64
>> + return put_v4l2_event32_time32(parg, arg);
>> +#else
>> + return put_v4l2_event_time32(parg, arg);
>> +#endif
>> +
>> #endif
>> case VIDIOC_QUERYBUF32:
>> case VIDIOC_QBUF32:
>> @@ -1023,10 +1032,6 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd)
>> #ifdef CONFIG_X86_64
>> case VIDIOC_DQEVENT32:
>> return put_v4l2_event32(parg, arg);
>> -#ifdef CONFIG_COMPAT_32BIT_TIME
>> - case VIDIOC_DQEVENT32_TIME32:
>> - return put_v4l2_event32_time32(parg, arg);
>> -#endif
>> #endif
>> }
>> return 0;
>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>> index 0c209bbbc76f..3840a3ea384c 100644
>> --- a/include/media/v4l2-ioctl.h
>> +++ b/include/media/v4l2-ioctl.h
>> @@ -786,7 +786,10 @@ struct v4l2_buffer_time32 {
>> };
>>
>> /*
>> + * For architectures other than x86_64, the struct v4l2_event32_time32
>> + * is the same as v4l2_event_time32.
>> * This function is used to copy the struct v4l2_event_time32 to userspace
>> + * if either the kernel is not 64-bit or if the architecture is other than x86_64.
>> */
>> int put_v4l2_event_time32(void *parg, void __user *arg);
>>
>>
>
