Re: [PATCH 3/3] media: v4l2-ctl: Add support to VIDIOC_DQEVENT_TIME32 on non x86_64 arch

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

(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@collabora.com>
> >> ---
> >>  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