[PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in get_v4l2_buffer32

[PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in get_v4l2_buffer32

[sy0816.kang]

From: Sunyoung Kang <sy0816.kang@samsung.com>

get_v4l2_buffer32() didn't copy reserved2 field from userspace to driver.
So the reserved2 value is not received through compat-ioctl32 in driver.
This patch copy reserved2 field of v4l2_buffer in get_v4l2_buffer32().

Signed-off-by: Sunyoung Kang <sy0816.kang@samsung.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index a99e82ec9ab6..e9b2b9c0ec9a 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -665,6 +665,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *p64,
 	if (V4L2_TYPE_IS_OUTPUT(type))
 		if (assign_in_user(&p64->bytesused, &p32->bytesused) ||
 		    assign_in_user(&p64->field, &p32->field) ||
+		    assign_in_user(&p64->reserved2, &p32->reserved2) ||
 		    assign_in_user(&p64->timestamp.tv_sec,
 				   &p32->timestamp.tv_sec) ||
 		    assign_in_user(&p64->timestamp.tv_usec,
-- 
2.20.1

Re: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in get_v4l2_buffer32

[Mauro Carvalho Chehab]

Em Fri, 17 Apr 2020 11:45:23 +0900
sy0816.kang@samsung.com escreveu:

> From: Sunyoung Kang <sy0816.kang@samsung.com>
> 
> get_v4l2_buffer32() didn't copy reserved2 field from userspace to driver.
> So the reserved2 value is not received through compat-ioctl32 in driver.
> This patch copy reserved2 field of v4l2_buffer in get_v4l2_buffer32().

Why should it copy reserved values? Those should not be used anywhere.

> 
> Signed-off-by: Sunyoung Kang <sy0816.kang@samsung.com>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index a99e82ec9ab6..e9b2b9c0ec9a 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -665,6 +665,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *p64,
>  	if (V4L2_TYPE_IS_OUTPUT(type))
>  		if (assign_in_user(&p64->bytesused, &p32->bytesused) ||
>  		    assign_in_user(&p64->field, &p32->field) ||
> +		    assign_in_user(&p64->reserved2, &p32->reserved2) ||
>  		    assign_in_user(&p64->timestamp.tv_sec,
>  				   &p32->timestamp.tv_sec) ||
>  		    assign_in_user(&p64->timestamp.tv_usec,



Thanks,
Mauro

Re: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in get_v4l2_buffer32

[Greg Kroah-Hartman]

On Fri, Apr 17, 2020 at 11:45:23AM +0900, sy0816.kang@samsung.com wrote:
> From: Sunyoung Kang <sy0816.kang@samsung.com>
> 
> get_v4l2_buffer32() didn't copy reserved2 field from userspace to driver.
> So the reserved2 value is not received through compat-ioctl32 in driver.
> This patch copy reserved2 field of v4l2_buffer in get_v4l2_buffer32().
> 
> Signed-off-by: Sunyoung Kang <sy0816.kang@samsung.com>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 1 +
>  1 file changed, 1 insertion(+)

What driver is using the reserved fields?  They should be ignored as
they are "reserved" for future use.

thanks,

greg k-h

RE: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in get_v4l2_buffer32

[Sunyoung Kang]

It uses the reserved 2 field to receive additional information about the
buffer from the user. 
Additional information is for special functions.

Copy the Reserved2 value to put_v4l2_buffer32(), but it is missing in
get_v4l2_buffer32(). Can't I put it in get_v4l2_buffer32() also?

Thanks,
Sunyoung

> -----Original Message-----
> From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Sent: Friday, April 17, 2020 4:24 PM
> To: sy0816.kang@samsung.com
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>; Arnd Bergmann
<arnd@arndb.de>;
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Thomas Gleixner
> <tglx@linutronix.de>; linux-media@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in
> get_v4l2_buffer32
> 
> Em Fri, 17 Apr 2020 11:45:23 +0900
> sy0816.kang@samsung.com escreveu:
> 
> > From: Sunyoung Kang <sy0816.kang@samsung.com>
> >
> > get_v4l2_buffer32() didn't copy reserved2 field from userspace to
driver.
> > So the reserved2 value is not received through compat-ioctl32 in driver.
> > This patch copy reserved2 field of v4l2_buffer in get_v4l2_buffer32().
> 
> Why should it copy reserved values? Those should not be used anywhere.
> 
> >
> > Signed-off-by: Sunyoung Kang <sy0816.kang@samsung.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > index a99e82ec9ab6..e9b2b9c0ec9a 100644
> > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > @@ -665,6 +665,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer
> __user *p64,
> >  	if (V4L2_TYPE_IS_OUTPUT(type))
> >  		if (assign_in_user(&p64->bytesused, &p32->bytesused) ||
> >  		    assign_in_user(&p64->field, &p32->field) ||
> > +		    assign_in_user(&p64->reserved2, &p32->reserved2) ||
> >  		    assign_in_user(&p64->timestamp.tv_sec,
> >  				   &p32->timestamp.tv_sec) ||
> >  		    assign_in_user(&p64->timestamp.tv_usec,
> 
> 
> 
> Thanks,
> Mauro

RE: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in get_v4l2_buffer32

[Sunyoung Kang]

Exynos video codec driver uses reserved2 value. How will reserved2 be used
for future use?

Thanks
Sunyoung

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Friday, April 17, 2020 5:35 PM
> To: sy0816.kang@samsung.com
> Cc: mchehab@kernel.org; Hans Verkuil <hverkuil-cisco@xs4all.nl>; Arnd
> Bergmann <arnd@arndb.de>; Thomas Gleixner <tglx@linutronix.de>; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in
> get_v4l2_buffer32
> 
> On Fri, Apr 17, 2020 at 11:45:23AM +0900, sy0816.kang@samsung.com wrote:
> > From: Sunyoung Kang <sy0816.kang@samsung.com>
> >
> > get_v4l2_buffer32() didn't copy reserved2 field from userspace to
driver.
> > So the reserved2 value is not received through compat-ioctl32 in driver.
> > This patch copy reserved2 field of v4l2_buffer in get_v4l2_buffer32().
> >
> > Signed-off-by: Sunyoung Kang <sy0816.kang@samsung.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 1 +
> >  1 file changed, 1 insertion(+)
> 
> What driver is using the reserved fields?  They should be ignored as they
> are "reserved" for future use.
> 
> thanks,
> 
> greg k-h

Re: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in get_v4l2_buffer32

['Greg Kroah-Hartman']

On Sat, Apr 18, 2020 at 12:14:09PM +0900, Sunyoung Kang wrote:
> Exynos video codec driver uses reserved2 value. How will reserved2 be used
> for future use?

No driver should be using the "reserved" fields, as they are "reserved"
by the api for future expansion of it.  They are not driver-specific
fields to be used that way at all.

thanks,

greg k-h

RE: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in get_v4l2_buffer32

[Sunyoung Kang]

I understand what you mean.
However, the way to transmit information about the buffer is only flags in
v4l2_buffer
In flags in v4l2_buffer, there is no reserved bit field that can be used for
custom.
Additional information about the buffer is needed to provide various
functions required by the customers but flags is not enough. So reserved2 is
used as an alternative.
Can you suggest a better opinion?

And copy the reserved2 value in put_v4l2_buffer32(), but it is missing only
in get_v4l2_buffer32().
Can't I put it in get_v4l2_buffer32()?

Thanks,
Sunyoung

> -----Original Message-----
> From: 'Greg Kroah-Hartman' <gregkh@linuxfoundation.org>
> Sent: Saturday, April 18, 2020 4:37 PM
> To: Sunyoung Kang <sy0816.kang@samsung.com>
> Cc: mchehab@kernel.org; 'Hans Verkuil' <hverkuil-cisco@xs4all.nl>; 'Arnd
> Bergmann' <arnd@arndb.de>; 'Thomas Gleixner' <tglx@linutronix.de>; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in
> get_v4l2_buffer32
> 
> On Sat, Apr 18, 2020 at 12:14:09PM +0900, Sunyoung Kang wrote:
> > Exynos video codec driver uses reserved2 value. How will reserved2 be
> > used for future use?
> 
> No driver should be using the "reserved" fields, as they are "reserved"
> by the api for future expansion of it.  They are not driver-specific
> fields to be used that way at all.
> 
> thanks,
> 
> greg k-h

Re: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in get_v4l2_buffer32

[Arnd Bergmann]

On Mon, Apr 20, 2020 at 2:40 AM Sunyoung Kang <sy0816.kang@samsung.com> wrote:
>
> I understand what you mean.
> However, the way to transmit information about the buffer is only flags in
> v4l2_buffer
> In flags in v4l2_buffer, there is no reserved bit field that can be used for
> custom.
> Additional information about the buffer is needed to provide various
> functions required by the customers but flags is not enough. So reserved2 is
> used as an alternative.
> Can you suggest a better opinion?

If you have a driver that needs to pass additional information that is not
supported by the subsystem, this is generally either because there is something
wrong in the driver, or because there is something wrong in the subsystem.

Whichever is at fault should be fixed. If it's the subsystem, then you should
explain why it's wrong and make a suggestion for how to address it, e.g.
introducing a new ioctl command or redefining the reserved members to
be defined in the way you need.

In any case, the ioctl commands should be driver independent, so that
any hardware with the same feature as your driver can work with the
same user space.

     Arnd

RE: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in get_v4l2_buffer32

[Sunyoung Kang]

Thank you for your detailed guide.
And I'll look into how to handle the additional information.

Thanks
Sunyoung

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Monday, April 20, 2020 8:23 PM
> To: Sunyoung Kang <sy0816.kang@samsung.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; Hans Verkuil <hverkuil-cisco@xs4all.nl>; Thomas
> Gleixner <tglx@linutronix.de>; Linux Media Mailing List <linux-
> media@vger.kernel.org>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in
> get_v4l2_buffer32
> 
> On Mon, Apr 20, 2020 at 2:40 AM Sunyoung Kang <sy0816.kang@samsung.com>
> wrote:
> >
> > I understand what you mean.
> > However, the way to transmit information about the buffer is only
> > flags in v4l2_buffer In flags in v4l2_buffer, there is no reserved bit
> > field that can be used for custom.
> > Additional information about the buffer is needed to provide various
> > functions required by the customers but flags is not enough. So
> > reserved2 is used as an alternative.
> > Can you suggest a better opinion?
> 
> If you have a driver that needs to pass additional information that is not
> supported by the subsystem, this is generally either because there is
> something wrong in the driver, or because there is something wrong in the
> subsystem.
> 
> Whichever is at fault should be fixed. If it's the subsystem, then you
> should explain why it's wrong and make a suggestion for how to address it,
> e.g.
> introducing a new ioctl command or redefining the reserved members to be
> defined in the way you need.
> 
> In any case, the ioctl commands should be driver independent, so that any
> hardware with the same feature as your driver can work with the same user
> space.
> 
>      Arnd

Re: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in get_v4l2_buffer32

[Mauro Carvalho Chehab]

Hi Sunyoung,

Em Tue, 21 Apr 2020 12:33:42 +0900
"Sunyoung Kang" <sy0816.kang@samsung.com> escreveu:

> Thank you for your detailed guide.
> And I'll look into how to handle the additional information.

Please don't top post. See my comments below.

> 
> Thanks
> Sunyoung
> 
> > -----Original Message-----
> > From: Arnd Bergmann <arnd@arndb.de>
> > Sent: Monday, April 20, 2020 8:23 PM
> > To: Sunyoung Kang <sy0816.kang@samsung.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Mauro Carvalho Chehab
> > <mchehab@kernel.org>; Hans Verkuil <hverkuil-cisco@xs4all.nl>; Thomas
> > Gleixner <tglx@linutronix.de>; Linux Media Mailing List <linux-  
> > media@vger.kernel.org>; linux-kernel@vger.kernel.org  
> > Subject: Re: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in
> > get_v4l2_buffer32
> > 
> > On Mon, Apr 20, 2020 at 2:40 AM Sunyoung Kang <sy0816.kang@samsung.com>
> > wrote:  
> > >
> > > I understand what you mean.
> > > However, the way to transmit information about the buffer is only
> > > flags in v4l2_buffer In flags in v4l2_buffer, there is no reserved bit
> > > field that can be used for custom.
> > > Additional information about the buffer is needed to provide various
> > > functions required by the customers but flags is not enough. So
> > > reserved2 is used as an alternative.
> > > Can you suggest a better opinion?  
> > 
> > If you have a driver that needs to pass additional information that is not
> > supported by the subsystem, this is generally either because there is
> > something wrong in the driver, or because there is something wrong in the
> > subsystem.
> > 
> > Whichever is at fault should be fixed. If it's the subsystem, then you
> > should explain why it's wrong and make a suggestion for how to address it,
> > e.g.
> > introducing a new ioctl command or redefining the reserved members to be
> > defined in the way you need.
> > 
> > In any case, the ioctl commands should be driver independent, so that any
> > hardware with the same feature as your driver can work with the same user
> > space.

I guess the problem here is that the driver that Sunyoung is working
is not upstream.

The right approach here is to upstream the driver. Once we see the code,
we can help addressing the issues. This could either involve using some
reserved space at the ioctl for some usage or propose some other solution
that would address your needs.

This has to be discussed case by case, as it is really hard to say what
to do with "additional information that is not supported by the subsystem".
What does that exactly means? We need to see the code to better understand
it ;-)

Thanks,
Mauro