(CC'ing Arnd)
On Thu, Sep 02, 2021 at 10:39:46AM +0200, Hans Verkuil wrote:
> On 02/09/2021 10:24, Hans Verkuil wrote:
> > 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);
> >>
--
Regards,
Laurent Pinchart
