From: Sunyoung Kang <[email protected]>
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 <[email protected]>
---
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
Em Fri, 17 Apr 2020 11:45:23 +0900
[email protected] escreveu:
> From: Sunyoung Kang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Fri, Apr 17, 2020 at 11:45:23AM +0900, [email protected] wrote:
> From: Sunyoung Kang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> Sent: Friday, April 17, 2020 4:24 PM
> To: [email protected]
> Cc: Hans Verkuil <[email protected]>; Arnd Bergmann
<[email protected]>;
> Greg Kroah-Hartman <[email protected]>; Thomas Gleixner
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH] media: v4l2-compat-ioctl32.c: copy reserved2 field in
> get_v4l2_buffer32
>
> Em Fri, 17 Apr 2020 11:45:23 +0900
> [email protected] escreveu:
>
> > From: Sunyoung Kang <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
Exynos video codec driver uses reserved2 value. How will reserved2 be used
for future use?
Thanks
Sunyoung
> -----Original Message-----
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Friday, April 17, 2020 5:35 PM
> To: [email protected]
> Cc: [email protected]; Hans Verkuil <[email protected]>; Arnd
> Bergmann <[email protected]>; Thomas Gleixner <[email protected]>; linux-
> [email protected]; [email protected]
> 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, [email protected] wrote:
> > From: Sunyoung Kang <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
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
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' <[email protected]>
> Sent: Saturday, April 18, 2020 4:37 PM
> To: Sunyoung Kang <[email protected]>
> Cc: [email protected]; 'Hans Verkuil' <[email protected]>; 'Arnd
> Bergmann' <[email protected]>; 'Thomas Gleixner' <[email protected]>; linux-
> [email protected]; [email protected]
> 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
On Mon, Apr 20, 2020 at 2:40 AM Sunyoung Kang <[email protected]> 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
Thank you for your detailed guide.
And I'll look into how to handle the additional information.
Thanks
Sunyoung
> -----Original Message-----
> From: Arnd Bergmann <[email protected]>
> Sent: Monday, April 20, 2020 8:23 PM
> To: Sunyoung Kang <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Mauro Carvalho Chehab
> <[email protected]>; Hans Verkuil <[email protected]>; Thomas
> Gleixner <[email protected]>; Linux Media Mailing List <linux-
> [email protected]>; [email protected]
> 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 <[email protected]>
> 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
Hi Sunyoung,
Em Tue, 21 Apr 2020 12:33:42 +0900
"Sunyoung Kang" <[email protected]> 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 <[email protected]>
> > Sent: Monday, April 20, 2020 8:23 PM
> > To: Sunyoung Kang <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>; Mauro Carvalho Chehab
> > <[email protected]>; Hans Verkuil <[email protected]>; Thomas
> > Gleixner <[email protected]>; Linux Media Mailing List <linux-
> > [email protected]>; [email protected]
> > 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 <[email protected]>
> > 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