v4l & compat_ioctl

v4l & compat_ioctl

[Jiri Slaby]

Hi,

wouldn't make sense to add v4l ioctls to ioctl_start (fs/compat_ioctl.c) table 
to allow i386 stuff such as skype to run video on x86_64? The ioctl structures 
seem to be pretty aliasable, so even no special handler wouldn't be neded...

Otherwise people are getting crap like this:
ioctl32(skype:29183): Unknown cmd fd(13) cmd(80685600){t:'V';sz:104} 
arg(f6571054) on /dev/video0

Re: v4l & compat_ioctl

[Andi Kleen]

Jiri Slaby <jirislaby@gmail.com> writes:

> wouldn't make sense to add v4l ioctls to ioctl_start
> (fs/compat_ioctl.c) table to allow i386 stuff such as skype to run
> video on x86_64?

Don't make it a question.

In general any ioctl in any driver runnable on any 64bit
architecture that has a 32bit compat layer should have compat 
support. Not having it is a bug.

However the new standard way to do this is to not 
add new stuff into compat_ioctl.c, but define ->compat_ioctl
entry points in the low level drivers (and ideally converted
them to ->unlocked_ioctl too while you're at it)

-Andi

Re: v4l & compat_ioctl

[Jiri Slaby]

On 03/28/2008 12:25 PM, Andi Kleen wrote:
> However the new standard way to do this is to not 
> add new stuff into compat_ioctl.c, but define ->compat_ioctl
> entry points in the low level drivers (and ideally converted
> them to ->unlocked_ioctl too while you're at it)

Andi, the problem here is, that v4l has only open in fops which in turn changes 
fops to the driver's one, so this would mean change all the drivers in v4l...

I second the converting to unlocked_ioctl and compat_ioctl in longer term (for 
new drivers as a merging rule), but wouldn't be feasible to add them to compat 
ioctl for now?

Re: v4l & compat_ioctl

[Andi Kleen]

On Fri, Mar 28, 2008 at 12:31:33PM +0100, Jiri Slaby wrote:
> On 03/28/2008 12:25 PM, Andi Kleen wrote:
> >However the new standard way to do this is to not 
> >add new stuff into compat_ioctl.c, but define ->compat_ioctl
> >entry points in the low level drivers (and ideally converted
> >them to ->unlocked_ioctl too while you're at it)
> 
> Andi, the problem here is, that v4l has only open in fops which in turn 
> changes fops to the driver's one, so this would mean change all the drivers 
> in v4l...

And?  Are there that many? It's a simple mechanical operation.

> I second the converting to unlocked_ioctl and compat_ioctl in longer term 
> (for new drivers as a merging rule), but wouldn't be feasible to add them 
> to compat ioctl for now?

The problem with compat_ioctl is that it doesn't handle overlapping ioctl
ranges and that the compat code cannot be made modular (although if they
are compatible that is no big problem) 

BTW i haven't audited them, but if there is u64 or similar in there anywhere
be careful about alignment.

-Andi

Re: v4l & compat_ioctl

[Jiri Slaby]

On 03/28/2008 12:38 PM, Andi Kleen wrote:
> BTW i haven't audited them, but if there is u64 or similar in there anywhere
> be careful about alignment.

Well, not good, some ioctls have different numbers on 32 and 64 bit:
struct v4l2_ext_controls {
         __u32 ctrl_class;
         __u32 count;
         __u32 error_idx;
         __u32 reserved[2];
         struct v4l2_ext_control *controls;
};
#define VIDIOC_G_EXT_CTRLS      _IOWR ('V', 71, struct v4l2_ext_controls)
as an example.

And alignment is a problem here (v4l2_std_id is u64):
struct v4l2_standard {
         __u32                index;
         v4l2_std_id          id;
         __u8                 name[24];
         struct v4l2_fract    frameperiod; /* Frames, not fields */
         __u32                framelines;
         __u32                reserved[4];
};
which results in different ioctl numbers too.

Re: v4l & compat_ioctl

[Andi Kleen]

On Fri, Mar 28, 2008 at 01:22:48PM +0100, Jiri Slaby wrote:
> On 03/28/2008 12:38 PM, Andi Kleen wrote:
> >BTW i haven't audited them, but if there is u64 or similar in there 
> >anywhere
> >be careful about alignment.
> 
> Well, not good, some ioctls have different numbers on 32 and 64 bit:

Then you need compat handlers to translate the numbers.

> struct v4l2_ext_controls {
>         __u32 ctrl_class;
>         __u32 count;
>         __u32 error_idx;
>         __u32 reserved[2];
>         struct v4l2_ext_control *controls;

Definitely conversion needed for the pointer too.

> };
> #define VIDIOC_G_EXT_CTRLS      _IOWR ('V', 71, struct v4l2_ext_controls)
> as an example.
> 
> And alignment is a problem here (v4l2_std_id is u64):

Yes needs conversion for the structure too.

> struct v4l2_standard {
>         __u32                index;
>         v4l2_std_id          id;
>         __u8                 name[24];
>         struct v4l2_fract    frameperiod; /* Frames, not fields */
>         __u32                framelines;
>         __u32                reserved[4];
> };
> which results in different ioctl numbers too.

-andi

> 

Re: v4l & compat_ioctl

[Mauro Carvalho Chehab]

On Fri, 28 Mar 2008 14:35:37 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> On Fri, Mar 28, 2008 at 01:22:48PM +0100, Jiri Slaby wrote:
> > On 03/28/2008 12:38 PM, Andi Kleen wrote:
> > >BTW i haven't audited them, but if there is u64 or similar in there 
> > >anywhere
> > >be careful about alignment.
> > 
> > Well, not good, some ioctls have different numbers on 32 and 64 bit:
> 
> Then you need compat handlers to translate the numbers.

V4L has its 32 bits compat module already, defined on
drivers/media/video/compat_ioctl32.c.

Also, the drivers should have this:

        .compat_ioctl = v4l_compat_ioctl32,

on their fops tables:

$ ls -C `grep -l compat_ioctl32 *.c|grep -v mod`

arv.c             meye.c              radio-rtrack2.c   stk-webcam.c
bttv-driver.c     miropcm20-radio.c   radio-sf16fmi.c   stradis.c
bw-qcam.c         ov511.c             radio-sf16fmr2.c  stv680.c
compat_ioctl32.c  pms.c               radio-si470x.c    usbvideo.c
cpia2_v4l.c       pwc-if.c            radio-terratec.c  usbvision-video.c
cpia.c            radio-aimslab.c     radio-trust.c     vicam.c
c-qcam.c          radio-aztech.c      radio-typhoon.c   w9966.c
cx23885-video.c   radio-cadet.c       radio-zoltrix.c   w9968cf.c
cx88-video.c      radio-gemtek.c      saa5249.c         zc0301_core.c
dsbr100.c         radio-gemtek-pci.c  saa7134-video.c   zoran_driver.c
em28xx-video.c    radio-maestro.c     se401.c
et61x251_core.c   radio-maxiradio.c   sn9c102_core.c

It seems that the problem you're suffering is specific to some driver.


Cheers,
Mauro

Re: v4l & compat_ioctl

[Jiri Slaby]

On 03/28/2008 06:24 PM, Mauro Carvalho Chehab wrote:
> It seems that the problem you're suffering is specific to some driver.

It's pseudo vivi.c

Re: v4l & compat_ioctl

[Mauro Carvalho Chehab]

On Fri, 28 Mar 2008 18:26:30 +0100
Jiri Slaby <jirislaby@gmail.com> wrote:

> On 03/28/2008 06:24 PM, Mauro Carvalho Chehab wrote:
> > It seems that the problem you're suffering is specific to some driver.
> 
> It's pseudo vivi.c

Ok, I've added the handler for compat32. I didn't test with skype yet.

Cheers,
Mauro

Re: v4l & compat_ioctl

[Jiri Slaby]

On 03/28/2008 06:47 PM, Mauro Carvalho Chehab wrote:
> On Fri, 28 Mar 2008 18:26:30 +0100
> Jiri Slaby <jirislaby@gmail.com> wrote:
> 
>> On 03/28/2008 06:24 PM, Mauro Carvalho Chehab wrote:
>>> It seems that the problem you're suffering is specific to some driver.
>> It's pseudo vivi.c
> 
> Ok, I've added the handler for compat32. I didn't test with skype yet.

It works, thanks a lot!