2004-10-07 17:37:16

by linux

[permalink] [raw]
Subject: video_usercopy() enforces change of VideoText IOCTLs since 2.6.8

Hello Gerd,

from kernel 2.6.7 to 2.6.8 function video_usercopy() in
videodev.c was modified in a way that IOCTLs which do not
use the _IO macros defined in linux/ioctl.h will get their
driver's ioctl function called with arg = 0.

This means that the videotext drivers (saa5246a and saa5249)
only produce segmentation faults in kernel version >= 2.6.8.

Shall we change the IOCTL definitions in include/linux/videotext.h?
This would mean that existing userspace programs will have to be
recompiled in order to work with the latest kernel version.

Nevertheless I suggest changing these IOCTLs to the new method.
I will work out a patch if you agree.

Below is the change I'm talking about:

int
video_usercopy(struct inode *inode, struct file *file,
unsigned int cmd, unsigned long arg,
int (*func)(struct inode *inode, struct file *file,
unsigned int cmd, void *arg))
{
char sbuf[128];
void *mbuf = NULL;
void *parg = NULL;
int err = -EINVAL;

cmd = video_fix_command(cmd);

/* Copy arguments into temp kernel buffer */
switch (_IOC_DIR(cmd)) {
case _IOC_NONE:
===> 2.6.7:
parg = (void *)arg;
===> 2.6.8:
parg = NULL;
...

/* call driver */
err = func(inode, file, cmd, parg);

Bye,
Michael


2004-10-08 11:00:20

by Gerd Knorr

[permalink] [raw]
Subject: Re: video_usercopy() enforces change of VideoText IOCTLs since 2.6.8

On Thu, Oct 07, 2004 at 06:54:10PM +0200, Michael Geng wrote:
> from kernel 2.6.7 to 2.6.8 function video_usercopy() in
> videodev.c was modified

> case _IOC_NONE:
> ===> 2.6.7:
> parg = (void *)arg;
> ===> 2.6.8:
> parg = NULL;
> ...

Yes, that change is wrong. It breaks all ioctls which use arg to pass
a integer value directly (i.e. not a pointer to a integer).

The patch below reverses it. Please apply,

Gerd

Index: linux-2.6.8/drivers/media/video/videodev.c
===================================================================
--- linux-2.6.8.orig/drivers/media/video/videodev.c 2004-10-08 11:55:08.000000000 +0200
+++ linux-2.6.8/drivers/media/video/videodev.c 2004-10-08 12:46:12.571630864 +0200
@@ -183,7 +183,7 @@ video_usercopy(struct inode *inode, stru
/* Copy arguments into temp kernel buffer */
switch (_IOC_DIR(cmd)) {
case _IOC_NONE:
- parg = NULL;
+ parg = (void*)arg;
break;
case _IOC_READ:
case _IOC_WRITE:

2004-10-08 20:57:09

by Andrew Morton

[permalink] [raw]
Subject: Re: video_usercopy() enforces change of VideoText IOCTLs since 2.6.8

Gerd Knorr <[email protected]> wrote:
>
> Yes, that change is wrong. It breaks all ioctls which use arg to pass
> a integer value directly (i.e. not a pointer to a integer).
>
> The patch below reverses it. Please apply,
>
> Gerd
>
> Index: linux-2.6.8/drivers/media/video/videodev.c
> ===================================================================
> --- linux-2.6.8.orig/drivers/media/video/videodev.c 2004-10-08 11:55:08.000000000 +0200
> +++ linux-2.6.8/drivers/media/video/videodev.c 2004-10-08 12:46:12.571630864 +0200
> @@ -183,7 +183,7 @@ video_usercopy(struct inode *inode, stru
> /* Copy arguments into temp kernel buffer */
> switch (_IOC_DIR(cmd)) {
> case _IOC_NONE:
> - parg = NULL;
> + parg = (void*)arg;
> break;

(the typecast is unneeded)

Seems that with this change we are now sometimes passing a user pointer
into (*func)(). And we're sometimes passing a kernel pointer, yes?

Are all the implementations of (*func)() handling that correctly?

It all looks fishy.

2004-10-09 09:40:33

by Gerd Knorr

[permalink] [raw]
Subject: Re: video_usercopy() enforces change of VideoText IOCTLs since 2.6.8

> > /* Copy arguments into temp kernel buffer */
> > switch (_IOC_DIR(cmd)) {
> > case _IOC_NONE:
> > - parg = NULL;
> > + parg = (void*)arg;
> > break;
>
> (the typecast is unneeded)
>
> Seems that with this change we are now sometimes passing a user pointer
> into (*func)(). And we're sometimes passing a kernel pointer, yes?

Assuming that ioctls passing _pointers_ are declared correctly with _IO*
that shouldn't be the case: _IOC_DIR(cmd) == _IOC_NONE means _IO()
means no pointer passed in.

> Are all the implementations of (*func)() handling that correctly?

Hmm, it broke for videotext, checking ...

Ok, you can drop it. The videotext ioctls (include/videotext.h) don't
use the _IO*() macros but pass around pointers anyway, thats bad.

Michael, you'll have to fix the saa5246a driver. video_usercopy() will
not work for you because the videotext ioctls doesn't use the _IO()
macros. You have to do the userspace copying in the driver yourself.

Gerd

--
return -ENOSIG;

2004-10-09 11:29:40

by linux

[permalink] [raw]
Subject: Re: video_usercopy() enforces change of VideoText IOCTLs since 2.6.8

On Sat, Oct 09, 2004 at 11:28:01AM +0200, Gerd Knorr wrote:
> > > /* Copy arguments into temp kernel buffer */
> > > switch (_IOC_DIR(cmd)) {
> > > case _IOC_NONE:
> > > - parg = NULL;
> > > + parg = (void*)arg;
> > > break;
> >
> > (the typecast is unneeded)
> >
> > Seems that with this change we are now sometimes passing a user pointer
> > into (*func)(). And we're sometimes passing a kernel pointer, yes?
>
> Assuming that ioctls passing _pointers_ are declared correctly with _IO*
> that shouldn't be the case: _IOC_DIR(cmd) == _IOC_NONE means _IO()
> means no pointer passed in.
>
> > Are all the implementations of (*func)() handling that correctly?
>
> Hmm, it broke for videotext, checking ...
>
> Ok, you can drop it. The videotext ioctls (include/videotext.h) don't
> use the _IO*() macros but pass around pointers anyway, thats bad.
>
> Michael, you'll have to fix the saa5246a driver. video_usercopy() will
> not work for you because the videotext ioctls doesn't use the _IO()
> macros. You have to do the userspace copying in the driver yourself.

I would prefer fixing the IOCTLs in include/linux/videotext.h. These definitions
are older than the _IO macros in linux/ioctl.h AFAIK. So it is clear that they
can't conform to that definition. I would prefer redefining them with respect
to the arguments passed to the IOCTLs.

This would be a little step towards unifying the kernel. Implementing a private
usercopy in saa5246a.c and saa5249.c would be the opposite.

The concept with the _IO() macros is good and helps preventing errors in the
use of IOCTLs. So also the videotext drivers should use it.

Nevertheless there is one big disadvantage: The userspace programs
have to be recompiled because they of course have to use the same IOCTL
definitions.

Nevertheless, affected are only saa5246a.c and saa5249.c. I think that there
are not many people left in the world using these drivers. So I think we can
afford to make a break here.

Do you agree? If so then I will work out a patch.

Michael

2004-10-09 12:40:24

by Gerd Knorr

[permalink] [raw]
Subject: Re: video_usercopy() enforces change of VideoText IOCTLs since 2.6.8

> I would prefer redefining them with respect
> to the arguments passed to the IOCTLs.

Thats possible as well.

> Nevertheless there is one big disadvantage: The userspace programs
> have to be recompiled because they of course have to use the same IOCTL
> definitions.

You can just keep the old ones and map them, so both old and new ones
will work (and internally only the new ones are used).
video_fix_command() does that for a number of v4l2 ioctls which
initially got a _IO* define with wrong read/write bits. You can either
just add the videotext ones there or translate them in the drivers right
before calling video_usercopy().

Gerd

--
return -ENOSIG;

2004-10-10 08:56:09

by linux

[permalink] [raw]
Subject: Re: video_usercopy() enforces change of VideoText IOCTLs since 2.6.8

On Sat, Oct 09, 2004 at 02:18:45PM +0200, Gerd Knorr wrote:
> > I would prefer redefining them with respect
> > to the arguments passed to the IOCTLs.
>
> Thats possible as well.
>
> > Nevertheless there is one big disadvantage: The userspace programs
> > have to be recompiled because they of course have to use the same IOCTL
> > definitions.
>
> You can just keep the old ones and map them, so both old and new ones
> will work (and internally only the new ones are used).
> video_fix_command() does that for a number of v4l2 ioctls which
> initially got a _IO* define with wrong read/write bits. You can either
> just add the videotext ones there or translate them in the drivers right
> before calling video_usercopy().

That's a good idea. The patch below maps old IOCTLs to new ones in the
functions vtx_fix_command() in saa5246a.c and saa5249.c. The drivers now
internally use the new IOCTLs but old userspace programs will still work
without recompiling them.

For the new IOCTLs I need an identifying letter. According to
Documentation/ioctl-number.txt the letter 'q', seq# 0-0x1F is presently
allocated to the videotext drivers. I want to change this because
linux/serio.h also uses the letter 'q', seq# 0.

I now allocated 0x81 (the major number of /dev/vtx<n>), seq# 0-0x1F
to linux/videotext.h. I also added 'q', seq# 0-0x1F to linux/serio.h.
In the present ioctl-number.txt only a conflict is indicated but the
allocation for linux/serio.h is missing. I hope that's ok. Who else
must I ask for this change in Documentation/ioctl-number.txt?

In include/linux/videotext.h I additionally cleaned up some definitions
for a tuner because these definitions are not used any more anywhere in
the kernel.

Michael

----------------------

diff -u -N -r linux-2.6.8.1/Documentation/ioctl-number.txt linux/Documentation/ioctl-number.txt
--- linux-2.6.8.1/Documentation/ioctl-number.txt Sat Aug 14 12:56:22 2004
+++ linux/Documentation/ioctl-number.txt Sun Oct 10 08:34:44 2004
@@ -144,7 +144,7 @@
'p' 40-7F linux/nvram.h
'p' 80-9F user-space parport
<mailto:[email protected]>
-'q' 00-1F linux/videotext.h conflict!
+'q' 00-1F linux/serio.h
'q' 80-FF Internet PhoneJACK, Internet LineJACK
<http://www.quicknet.net>
'r' 00-1F linux/msdos_fs.h
@@ -162,6 +162,7 @@
'z' 40-7F CAN bus card
<mailto:[email protected]>
0x80 00-1F linux/fb.h
+0x81 00-1F linux/videotext.h
0x89 00-06 asm-i386/sockios.h
0x89 0B-DF linux/sockios.h
0x89 E0-EF linux/sockios.h SIOCPROTOPRIVATE range
diff -u -N -r linux-2.6.8.1/drivers/media/video/saa5246a.c linux/drivers/media/video/saa5246a.c
--- linux-2.6.8.1/drivers/media/video/saa5246a.c Sat Aug 14 12:55:22 2004
+++ linux/drivers/media/video/saa5246a.c Sun Oct 10 09:40:29 2004
@@ -682,6 +682,54 @@
}

/*
+ * Translates old vtx IOCTLs to new ones
+ *
+ * This keeps new kernel versions compatible with old userspace programs.
+ */
+static inline unsigned int vtx_fix_command(unsigned int cmd)
+{
+ switch (cmd) {
+ case VTXIOCGETINFO_OLD:
+ cmd = VTXIOCGETINFO;
+ break;
+ case VTXIOCCLRPAGE_OLD:
+ cmd = VTXIOCCLRPAGE;
+ break;
+ case VTXIOCCLRFOUND_OLD:
+ cmd = VTXIOCCLRFOUND;
+ break;
+ case VTXIOCPAGEREQ_OLD:
+ cmd = VTXIOCPAGEREQ;
+ break;
+ case VTXIOCGETSTAT_OLD:
+ cmd = VTXIOCGETSTAT;
+ break;
+ case VTXIOCGETPAGE_OLD:
+ cmd = VTXIOCGETPAGE;
+ break;
+ case VTXIOCSTOPDAU_OLD:
+ cmd = VTXIOCSTOPDAU;
+ break;
+ case VTXIOCPUTPAGE_OLD:
+ cmd = VTXIOCPUTPAGE;
+ break;
+ case VTXIOCSETDISP_OLD:
+ cmd = VTXIOCSETDISP;
+ break;
+ case VTXIOCPUTSTAT_OLD:
+ cmd = VTXIOCPUTSTAT;
+ break;
+ case VTXIOCCLRCACHE_OLD:
+ cmd = VTXIOCCLRCACHE;
+ break;
+ case VTXIOCSETVIRT_OLD:
+ cmd = VTXIOCSETVIRT;
+ break;
+ }
+ return cmd;
+}
+
+/*
* Handle the locking
*/
static int saa5246a_ioctl(struct inode *inode, struct file *file,
@@ -691,6 +739,7 @@
struct saa5246a_device *t = vd->priv;
int err;

+ cmd = vtx_fix_command(cmd);
down(&t->lock);
err = video_usercopy(inode, file, cmd, arg, do_saa5246a_ioctl);
up(&t->lock);
diff -u -N -r linux-2.6.8.1/drivers/media/video/saa5246a.h linux/drivers/media/video/saa5246a.h
--- linux-2.6.8.1/drivers/media/video/saa5246a.h Sat Aug 14 12:55:48 2004
+++ linux/drivers/media/video/saa5246a.h Sun Oct 10 08:34:13 2004
@@ -23,7 +23,7 @@
#define __SAA5246A_H__

#define MAJOR_VERSION 1 /* driver major version number */
-#define MINOR_VERSION 7 /* driver minor version number */
+#define MINOR_VERSION 8 /* driver minor version number */

#define IF_NAME "SAA5246A"

diff -u -N -r linux-2.6.8.1/drivers/media/video/saa5249.c linux/drivers/media/video/saa5249.c
--- linux-2.6.8.1/drivers/media/video/saa5249.c Sat Aug 14 12:55:48 2004
+++ linux/drivers/media/video/saa5249.c Sun Oct 10 09:40:33 2004
@@ -1,4 +1,7 @@
/*
+ * Modified in order to keep it compatible both with new and old videotext IOCTLs by
+ * Michael Geng <[email protected]>
+ *
* Cleaned up to use existing videodev interface and allow the idea
* of multiple teletext decoders on the video4linux iface. Changed i2c
* to cover addressing clashes on device busses. It's also rebuilt so
@@ -58,7 +61,7 @@
#include <asm/uaccess.h>

#define VTX_VER_MAJ 1
-#define VTX_VER_MIN 7
+#define VTX_VER_MIN 8



@@ -579,6 +582,54 @@
}

/*
+ * Translates old vtx IOCTLs to new ones
+ *
+ * This keeps new kernel versions compatible with old userspace programs.
+ */
+static inline unsigned int vtx_fix_command(unsigned int cmd)
+{
+ switch (cmd) {
+ case VTXIOCGETINFO_OLD:
+ cmd = VTXIOCGETINFO;
+ break;
+ case VTXIOCCLRPAGE_OLD:
+ cmd = VTXIOCCLRPAGE;
+ break;
+ case VTXIOCCLRFOUND_OLD:
+ cmd = VTXIOCCLRFOUND;
+ break;
+ case VTXIOCPAGEREQ_OLD:
+ cmd = VTXIOCPAGEREQ;
+ break;
+ case VTXIOCGETSTAT_OLD:
+ cmd = VTXIOCGETSTAT;
+ break;
+ case VTXIOCGETPAGE_OLD:
+ cmd = VTXIOCGETPAGE;
+ break;
+ case VTXIOCSTOPDAU_OLD:
+ cmd = VTXIOCSTOPDAU;
+ break;
+ case VTXIOCPUTPAGE_OLD:
+ cmd = VTXIOCPUTPAGE;
+ break;
+ case VTXIOCSETDISP_OLD:
+ cmd = VTXIOCSETDISP;
+ break;
+ case VTXIOCPUTSTAT_OLD:
+ cmd = VTXIOCPUTSTAT;
+ break;
+ case VTXIOCCLRCACHE_OLD:
+ cmd = VTXIOCCLRCACHE;
+ break;
+ case VTXIOCSETVIRT_OLD:
+ cmd = VTXIOCSETVIRT;
+ break;
+ }
+ return cmd;
+}
+
+/*
* Handle the locking
*/

@@ -589,6 +640,7 @@
struct saa5249_device *t=vd->priv;
int err;

+ cmd = vtx_fix_command(cmd);
down(&t->lock);
err = video_usercopy(inode,file,cmd,arg,do_saa5249_ioctl);
up(&t->lock);
diff -u -N -r linux-2.6.8.1/include/linux/videotext.h linux/include/linux/videotext.h
--- linux-2.6.8.1/include/linux/videotext.h Sat Aug 14 12:56:23 2004
+++ linux/include/linux/videotext.h Sun Oct 10 08:34:33 2004
@@ -1,7 +1,13 @@
#ifndef _VTX_H
#define _VTX_H

-/* $Id: videotext.h,v 1.1 1998/03/30 22:26:39 alan Exp $
+/*
+ * Teletext (=Videotext) hardware decoders using interface /dev/vtx
+ * Do not confuse with drivers using /dev/vbi which decode videotext by software
+ *
+ * Videotext IOCTLs changed in order to use _IO() macros defined in <linux/ioctl.h>,
+ * unused tuner IOCTLs cleaned up by
+ * Michael Geng <[email protected]>
*
* Copyright (c) 1994-97 Martin Buck <[email protected]>
* Read COPYING for more information
@@ -12,19 +18,32 @@
/*
* Videotext ioctls
*/
-#define VTXIOCGETINFO 0x7101 /* get version of driver & capabilities of vtx-chipset */
-#define VTXIOCCLRPAGE 0x7102 /* clear page-buffer */
-#define VTXIOCCLRFOUND 0x7103 /* clear bits indicating that page was found */
-#define VTXIOCPAGEREQ 0x7104 /* search for page */
-#define VTXIOCGETSTAT 0x7105 /* get status of page-buffer */
-#define VTXIOCGETPAGE 0x7106 /* get contents of page-buffer */
-#define VTXIOCSTOPDAU 0x7107 /* stop data acquisition unit */
-#define VTXIOCPUTPAGE 0x7108 /* display page on TV-screen */
-#define VTXIOCSETDISP 0x7109 /* set TV-mode */
-#define VTXIOCPUTSTAT 0x710a /* set status of TV-output-buffer */
-#define VTXIOCCLRCACHE 0x710b /* clear cache on VTX-interface (if avail.) */
-#define VTXIOCSETVIRT 0x710c /* turn on virtual mode (this disables TV-display) */
-
+#define VTXIOCGETINFO _IOR (0x81, 1, vtx_info_t)
+#define VTXIOCCLRPAGE _IOW (0x81, 2, vtx_pagereq_t)
+#define VTXIOCCLRFOUND _IOW (0x81, 3, vtx_pagereq_t)
+#define VTXIOCPAGEREQ _IOW (0x81, 4, vtx_pagereq_t)
+#define VTXIOCGETSTAT _IOW (0x81, 5, vtx_pagereq_t)
+#define VTXIOCGETPAGE _IOW (0x81, 6, vtx_pagereq_t)
+#define VTXIOCSTOPDAU _IOW (0x81, 7, vtx_pagereq_t)
+#define VTXIOCPUTPAGE _IO (0x81, 8)
+#define VTXIOCSETDISP _IO (0x81, 9)
+#define VTXIOCPUTSTAT _IO (0x81, 10)
+#define VTXIOCCLRCACHE _IO (0x81, 11)
+#define VTXIOCSETVIRT _IOW (0x81, 12, long)
+
+/* for compatibility, will go away some day */
+#define VTXIOCGETINFO_OLD 0x7101 /* get version of driver & capabilities of vtx-chipset */
+#define VTXIOCCLRPAGE_OLD 0x7102 /* clear page-buffer */
+#define VTXIOCCLRFOUND_OLD 0x7103 /* clear bits indicating that page was found */
+#define VTXIOCPAGEREQ_OLD 0x7104 /* search for page */
+#define VTXIOCGETSTAT_OLD 0x7105 /* get status of page-buffer */
+#define VTXIOCGETPAGE_OLD 0x7106 /* get contents of page-buffer */
+#define VTXIOCSTOPDAU_OLD 0x7107 /* stop data acquisition unit */
+#define VTXIOCPUTPAGE_OLD 0x7108 /* display page on TV-screen */
+#define VTXIOCSETDISP_OLD 0x7109 /* set TV-mode */
+#define VTXIOCPUTSTAT_OLD 0x710a /* set status of TV-output-buffer */
+#define VTXIOCCLRCACHE_OLD 0x710b /* clear cache on VTX-interface (if avail.) */
+#define VTXIOCSETVIRT_OLD 0x710c /* turn on virtual mode (this disables TV-display) */

/*
* Definitions for VTXIOCGETINFO
@@ -102,43 +121,5 @@
unsigned hamming : 1; /* hamming-error occurred */
}
vtx_pageinfo_t;
-
-
-/*
- * Definitions for VTXIOCSETDISP
- */
-
-typedef enum {
- DISPOFF, DISPNORM, DISPTRANS, DISPINS, INTERLACE_OFFSET
-} vtxdisp_t;
-
-
-
-/*
- * Tuner ioctls
- */

-#define TUNIOCGETINFO 0x7201 /* get version of driver & capabilities of tuner */
-#define TUNIOCRESET 0x7202 /* reset tuner */
-#define TUNIOCSETFREQ 0x7203 /* set tuning frequency (unit: kHz) */
-#define TUNIOCGETFREQ 0x7204 /* get tuning frequency (unit: kHz) */
-#define TUNIOCSETCHAN 0x7205 /* set tuning channel */
-#define TUNIOCGETCHAN 0x7206 /* get tuning channel */
-
-
-typedef struct
-{
- int version_major, version_minor; /* version of driver; if version_major changes, driver */
- /* is not backward compatible!!! CHECK THIS!!! */
- unsigned freq : 1; /* tuner can be set to given frequency */
- unsigned chan : 1; /* tuner stores several channels */
- unsigned scan : 1; /* tuner supports scanning */
- unsigned autoscan : 1; /* tuner supports scanning with automatic stop */
- unsigned afc : 1; /* tuner supports AFC */
- unsigned dummy1, dummy2, dummy3, dummy4, dummy5, dummy6, dummy7, dummy8, dummy9, dummy10,
- dummy11 : 1;
- int dummy12, dummy13, dummy14, dummy15, dummy16, dummy17, dummy18, dummy19;
-} tuner_info_t;
-
-
#endif /* _VTX_H */

2004-10-11 15:41:56

by Gerd Knorr

[permalink] [raw]
Subject: Re: video_usercopy() enforces change of VideoText IOCTLs since 2.6.8

> +#define VTXIOCGETINFO _IOR (0x81, 1, vtx_info_t)
> +#define VTXIOCCLRPAGE _IOW (0x81, 2, vtx_pagereq_t)
> +#define VTXIOCCLRFOUND _IOW (0x81, 3, vtx_pagereq_t)
> +#define VTXIOCPAGEREQ _IOW (0x81, 4, vtx_pagereq_t)
> +#define VTXIOCGETSTAT _IOW (0x81, 5, vtx_pagereq_t)
> +#define VTXIOCGETPAGE _IOW (0x81, 6, vtx_pagereq_t)
> +#define VTXIOCSTOPDAU _IOW (0x81, 7, vtx_pagereq_t)

Hmm, _IOW for VTXIOCGET* looks bogous, is that really correct?

Note that you often need RW for read/get ioctls because even these
often pass data to the driver as well (for example the vtx page number
you want query the status for). Please double-check that. Otherwise
the patch looks ok to me.

Gerd

--
return -ENOSIG;

2004-10-11 16:25:13

by linux

[permalink] [raw]
Subject: Re: video_usercopy() enforces change of VideoText IOCTLs since 2.6.8

On Mon, Oct 11, 2004 at 05:14:55PM +0200, Gerd Knorr wrote:
> > +#define VTXIOCGETINFO _IOR (0x81, 1, vtx_info_t)
> > +#define VTXIOCCLRPAGE _IOW (0x81, 2, vtx_pagereq_t)
> > +#define VTXIOCCLRFOUND _IOW (0x81, 3, vtx_pagereq_t)
> > +#define VTXIOCPAGEREQ _IOW (0x81, 4, vtx_pagereq_t)
> > +#define VTXIOCGETSTAT _IOW (0x81, 5, vtx_pagereq_t)
> > +#define VTXIOCGETPAGE _IOW (0x81, 6, vtx_pagereq_t)
> > +#define VTXIOCSTOPDAU _IOW (0x81, 7, vtx_pagereq_t)
>
> Hmm, _IOW for VTXIOCGET* looks bogous, is that really correct?

This is the definition of the argument to the VTXIOCGET* IOCTLs:

typedef struct
{
int page; /* number of requested page (hexadecimal) */
int hour; /* requested hour (hexadecimal) */
int minute; /* requested minute (hexadecimal) */
int pagemask; /* mask defining which values of the above are set */
int pgbuf; /* buffer where page will be stored */
int start; /* start of requested part of page */
int end; /* end of requested part of page */
void __user *buffer; /* pointer to beginning of destination buffer */
}
vtx_pagereq_t;

The driver returns all data in the buffer field. Copying there is done by a seperate call to
copy_to_user(). All other fields are never changed by the driver. So the _IOW definition is ok.

> Note that you often need RW for read/get ioctls because even these
> often pass data to the driver as well (for example the vtx page number
> you want query the status for). Please double-check that. Otherwise
> the patch looks ok to me.

Thank you! Andrew, could you please forward the patch?
Suggestion for a comment line:
Videotext: IOCTLs changed to match _IO macros in linux/ioctl.h

Michael

2004-10-13 12:04:25

by Alan

[permalink] [raw]
Subject: Re: video_usercopy() enforces change of VideoText IOCTLs since 2.6.8

On Llu, 2004-10-11 at 17:21, Michael Geng wrote:
> Thank you! Andrew, could you please forward the patch?
> Suggestion for a comment line:
> Videotext: IOCTLs changed to match _IO macros in linux/ioctl.h

But have any of them changed ioctl number in the process and broken
compatibility ?

2004-10-13 14:43:25

by linux

[permalink] [raw]
Subject: Re: video_usercopy() enforces change of VideoText IOCTLs since 2.6.8

On Wed, Oct 13, 2004 at 12:00:48PM +0100, Alan Cox wrote:
> On Llu, 2004-10-11 at 17:21, Michael Geng wrote:
> > Thank you! Andrew, could you please forward the patch?
> > Suggestion for a comment line:
> > Videotext: IOCTLs changed to match _IO macros in linux/ioctl.h
>
> But have any of them changed ioctl number in the process and broken
> compatibility ?

No compatibility is broken, because old IOCTLs are remapped to new
ones in the driver. The driver properly handles both old and new
IOCTLs after applying the patch. No user space programs have to be
changed or recompiled.

Michael