Add compat_ioctl handler to synclink_gt driver.
The one case requiring a separate 32 bit handler could be
removed by redefining the associated structure in
a way compatible with both 32 and 64 bit systems. But that
approach would break existing native 64 bit user applications.
Signed-off-by: Paul Fulghum <[email protected]>
--- a/include/linux/synclink.h 2007-04-25 22:08:32.000000000 -0500
+++ b/include/linux/synclink.h 2007-05-03 12:44:09.000000000 -0500
@@ -9,6 +9,8 @@
* the terms of the GNU Public License (GPL)
*/
+#include <linux/compat.h>
+
#ifndef _SYNCLINK_H_
#define _SYNCLINK_H_
#define SYNCLINK_H_VERSION 3.6
@@ -167,6 +169,24 @@ typedef struct _MGSL_PARAMS
} MGSL_PARAMS, *PMGSL_PARAMS;
+/* provide 32 bit ioctl compatibility on 64 bit systems */
+struct MGSL_PARAMS32
+{
+ compat_ulong_t mode;
+ unsigned char loopback;
+ unsigned short flags;
+ unsigned char encoding;
+ compat_ulong_t clock_speed;
+ unsigned char addr_filter;
+ unsigned short crc_type;
+ unsigned char preamble_length;
+ unsigned char preamble;
+ compat_ulong_t data_rate;
+ unsigned char data_bits;
+ unsigned char stop_bits;
+ unsigned char parity;
+};
+
#define MICROGATE_VENDOR_ID 0x13c0
#define SYNCLINK_DEVICE_ID 0x0010
#define MGSCC_DEVICE_ID 0x0020
@@ -276,6 +296,8 @@ struct gpio_desc {
#define MGSL_MAGIC_IOC 'm'
#define MGSL_IOCSPARAMS _IOW(MGSL_MAGIC_IOC,0,struct _MGSL_PARAMS)
#define MGSL_IOCGPARAMS _IOR(MGSL_MAGIC_IOC,1,struct _MGSL_PARAMS)
+#define MGSL_IOCSPARAMS32 _IOW(MGSL_MAGIC_IOC,0,struct MGSL_PARAMS32)
+#define MGSL_IOCGPARAMS32 _IOR(MGSL_MAGIC_IOC,1,struct MGSL_PARAMS32)
#define MGSL_IOCSTXIDLE _IO(MGSL_MAGIC_IOC,2)
#define MGSL_IOCGTXIDLE _IO(MGSL_MAGIC_IOC,3)
#define MGSL_IOCTXENABLE _IO(MGSL_MAGIC_IOC,4)
--- a/drivers/char/synclink_gt.c 2007-04-25 22:08:32.000000000 -0500
+++ b/drivers/char/synclink_gt.c 2007-05-03 12:40:36.000000000 -0500
@@ -73,6 +73,7 @@
#include <linux/bitops.h>
#include <linux/workqueue.h>
#include <linux/hdlc.h>
+#include <linux/compat.h>
#include <asm/system.h>
#include <asm/io.h>
@@ -530,6 +531,10 @@ static int set_interface(struct slgt_in
static int set_gpio(struct slgt_info *info, struct gpio_desc __user *gpio);
static int get_gpio(struct slgt_info *info, struct gpio_desc __user *gpio);
static int wait_gpio(struct slgt_info *info, struct gpio_desc __user *gpio);
+#ifdef CONFIG_COMPAT
+static long set_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user *new_params);
+static long get_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user *new_params);
+#endif
/*
* driver functions
@@ -1170,6 +1175,55 @@ static int ioctl(struct tty_struct *tty,
return 0;
}
+#ifdef CONFIG_COMPAT
+static long compat_ioctl(struct tty_struct *tty, struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+ struct slgt_info *info = tty->driver_data;
+ int rc = -ENOIOCTLCMD;
+
+ if (sanity_check(info, tty->name, "compat_ioctl"))
+ return -ENODEV;
+ DBGINFO(("%s compat_ioctl() cmd=%08X\n", info->device_name, cmd));
+
+ switch (cmd) {
+
+ case MGSL_IOCSPARAMS32:
+ rc = set_params32(info, compat_ptr(arg));
+ break;
+
+ case MGSL_IOCGPARAMS32:
+ rc = get_params32(info, compat_ptr(arg));
+ break;
+
+ case MGSL_IOCGPARAMS:
+ case MGSL_IOCSPARAMS:
+ case MGSL_IOCGTXIDLE:
+ case MGSL_IOCGSTATS:
+ case MGSL_IOCWAITEVENT:
+ case MGSL_IOCGIF:
+ case MGSL_IOCSGPIO:
+ case MGSL_IOCGGPIO:
+ case MGSL_IOCWAITGPIO:
+ case TIOCGICOUNT:
+ rc = ioctl(tty, file, cmd, (unsigned long)(compat_ptr(arg)));
+ break;
+
+ case MGSL_IOCSTXIDLE:
+ case MGSL_IOCTXENABLE:
+ case MGSL_IOCRXENABLE:
+ case MGSL_IOCTXABORT:
+ case TIOCMIWAIT:
+ case MGSL_IOCSIF:
+ rc = ioctl(tty, file, cmd, arg);
+ break;
+ }
+
+ DBGINFO(("%s compat_ioctl() cmd=%08X rc=%d\n", info->device_name, cmd, rc));
+ return rc;
+}
+#endif
+
/*
* proc fs support
*/
@@ -2507,6 +2561,60 @@ static int set_params(struct slgt_info *
return 0;
}
+#ifdef CONFIG_COMPAT
+static long get_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user *user_params)
+{
+ struct MGSL_PARAMS32 tmp_params;
+
+ DBGINFO(("%s get_params32\n", info->device_name));
+ tmp_params.mode = (compat_ulong_t)info->params.mode;
+ tmp_params.loopback = info->params.loopback;
+ tmp_params.flags = info->params.flags;
+ tmp_params.encoding = info->params.encoding;
+ tmp_params.clock_speed = (compat_ulong_t)info->params.clock_speed;
+ tmp_params.addr_filter = info->params.addr_filter;
+ tmp_params.crc_type = info->params.crc_type;
+ tmp_params.preamble_length = info->params.preamble_length;
+ tmp_params.preamble = info->params.preamble;
+ tmp_params.data_rate = (compat_ulong_t)info->params.data_rate;
+ tmp_params.data_bits = info->params.data_bits;
+ tmp_params.stop_bits = info->params.stop_bits;
+ tmp_params.parity = info->params.parity;
+ if (copy_to_user(user_params, &tmp_params, sizeof(struct MGSL_PARAMS32)))
+ return -EFAULT;
+ return 0;
+}
+
+static long set_params32(struct slgt_info *info, struct MGSL_PARAMS32 __user *new_params)
+{
+ struct MGSL_PARAMS32 tmp_params;
+
+ DBGINFO(("%s set_params32\n", info->device_name));
+ if (copy_from_user(&tmp_params, new_params, sizeof(struct MGSL_PARAMS32)))
+ return -EFAULT;
+
+ spin_lock(&info->lock);
+ info->params.mode = tmp_params.mode;
+ info->params.loopback = tmp_params.loopback;
+ info->params.flags = tmp_params.flags;
+ info->params.encoding = tmp_params.encoding;
+ info->params.clock_speed = tmp_params.clock_speed;
+ info->params.addr_filter = tmp_params.addr_filter;
+ info->params.crc_type = tmp_params.crc_type;
+ info->params.preamble_length = tmp_params.preamble_length;
+ info->params.preamble = tmp_params.preamble;
+ info->params.data_rate = tmp_params.data_rate;
+ info->params.data_bits = tmp_params.data_bits;
+ info->params.stop_bits = tmp_params.stop_bits;
+ info->params.parity = tmp_params.parity;
+ spin_unlock(&info->lock);
+
+ change_params(info);
+
+ return 0;
+}
+#endif
+
static int get_txidle(struct slgt_info *info, int __user *idle_mode)
{
DBGINFO(("%s get_txidle=%d\n", info->device_name, info->idle_mode));
@@ -3443,6 +3551,9 @@ static const struct tty_operations ops =
.chars_in_buffer = chars_in_buffer,
.flush_buffer = flush_buffer,
.ioctl = ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = compat_ioctl,
+#endif
.throttle = throttle,
.unthrottle = unthrottle,
.send_xchar = send_xchar,
On Thu, 03 May 2007 13:01:17 -0500
Paul Fulghum <[email protected]> wrote:
> Add compat_ioctl handler to synclink_gt driver.
>
> The one case requiring a separate 32 bit handler could be
> removed by redefining the associated structure in
> a way compatible with both 32 and 64 bit systems. But that
> approach would break existing native 64 bit user applications.
A made a few changes here...
From: Andrew Morton <[email protected]>
- Fix i386 build:
In file included from drivers/char/synclink_gt.c:85:
include/linux/synclink.h:175: error: expected specifier-qualifier-list before 'compat_ulong_t'
- We might as well do the same ifdef-avoidery trick around compat_ioctl()
too. That required that it be renamed.
- It is fishy that apart from one outlier in kexec.h, synclink.h is the
only header file which uses compat_ulong_t. Are we doing this right?
Cc: Alan Cox <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Paul Fulghum <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
drivers/char/synclink_gt.c | 16 +++++++++-------
include/linux/synclink.h | 5 +++--
2 files changed, 12 insertions(+), 9 deletions(-)
diff -puN drivers/char/synclink_gt.c~synclink_gt-add-compat_ioctl-fix drivers/char/synclink_gt.c
--- a/drivers/char/synclink_gt.c~synclink_gt-add-compat_ioctl-fix
+++ a/drivers/char/synclink_gt.c
@@ -1176,15 +1176,16 @@ static int ioctl(struct tty_struct *tty,
}
#ifdef CONFIG_COMPAT
-static long compat_ioctl(struct tty_struct *tty, struct file *file,
+static long synclink_compat_ioctl(struct tty_struct *tty, struct file *file,
unsigned int cmd, unsigned long arg)
{
struct slgt_info *info = tty->driver_data;
int rc = -ENOIOCTLCMD;
- if (sanity_check(info, tty->name, "compat_ioctl"))
+ if (sanity_check(info, tty->name, "synclink_compat_ioctl"))
return -ENODEV;
- DBGINFO(("%s compat_ioctl() cmd=%08X\n", info->device_name, cmd));
+ DBGINFO(("%s synclink_compat_ioctl() cmd=%08X\n",
+ info->device_name, cmd));
switch (cmd) {
@@ -1219,9 +1220,12 @@ static long compat_ioctl(struct tty_stru
break;
}
- DBGINFO(("%s compat_ioctl() cmd=%08X rc=%d\n", info->device_name, cmd, rc));
+ DBGINFO(("%s synclink_compat_ioctl() cmd=%08X rc=%d\n",
+ info->device_name, cmd, rc));
return rc;
}
+#else
+#define synclink_compat_ioctl NULL
#endif
/*
@@ -3554,9 +3558,7 @@ static const struct tty_operations ops =
.chars_in_buffer = chars_in_buffer,
.flush_buffer = flush_buffer,
.ioctl = ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = compat_ioctl,
-#endif
+ .compat_ioctl = synclink_compat_ioctl,
.throttle = throttle,
.unthrottle = unthrottle,
.send_xchar = send_xchar,
diff -puN include/linux/synclink.h~synclink_gt-add-compat_ioctl-fix include/linux/synclink.h
--- a/include/linux/synclink.h~synclink_gt-add-compat_ioctl-fix
+++ a/include/linux/synclink.h
@@ -169,9 +169,9 @@ typedef struct _MGSL_PARAMS
} MGSL_PARAMS, *PMGSL_PARAMS;
+#ifdef CONFIG_COMPAT
/* provide 32 bit ioctl compatibility on 64 bit systems */
-struct MGSL_PARAMS32
-{
+struct MGSL_PARAMS32 {
compat_ulong_t mode;
unsigned char loopback;
unsigned short flags;
@@ -186,6 +186,7 @@ struct MGSL_PARAMS32
unsigned char stop_bits;
unsigned char parity;
};
+#endif
#define MICROGATE_VENDOR_ID 0x13c0
#define SYNCLINK_DEVICE_ID 0x0010
_
Andrew Morton wrote:
> In file included from drivers/char/synclink_gt.c:85:
> include/linux/synclink.h:175: error: expected specifier-qualifier-list before 'compat_ulong_t'
>
> - We might as well do the same ifdef-avoidery trick around compat_ioctl()
> too. That required that it be renamed.
>
> - It is fishy that apart from one outlier in kexec.h, synclink.h is the
> only header file which uses compat_ulong_t. Are we doing this right?
Arnd, do you have any comment on this?
It seems like the compatible types should be available
in something that is already commonly used like linux/types.h
I'm fine with it either way. I'm not in
a position to be making those kinds of decisions
for widely used infrastructure, so I'll leave
that for someone further up the food chain.
--
Paul Fulghum
Microgate Systems, Ltd.
On Thu, 03 May 2007 13:01:17 -0500
Paul Fulghum <[email protected]> wrote:
> Add compat_ioctl handler to synclink_gt driver.
i386 allmodconfig:
make[3]: *** No rule to make target `/usr/src/devel/usr/include/linux/.check.synclink.h', needed by `__headerscheck'. Stop.
I got tired of this patch - I think I'll drop it.
Andrew Morton wrote:
> On Thu, 03 May 2007 13:01:17 -0500
> Paul Fulghum <[email protected]> wrote:
>
>> Add compat_ioctl handler to synclink_gt driver.
>
> i386 allmodconfig:
>
> make[3]: *** No rule to make target `/usr/src/devel/usr/include/linux/.check.synclink.h', needed by `__headerscheck'. Stop.
>
> I got tired of this patch - I think I'll drop it.
This all seems to be related to the use of compat_ulong_t
Since my original patch worked fine using unsigned int,
how about I go back to that?
--
Paul
Arnd Bergmann wrote:
> On Friday 04 May 2007, Paul Fulghum wrote:
>>> - It is fishy that apart from one outlier in kexec.h, synclink.h is the
>>> only header file which uses compat_ulong_t. Are we doing this right?
>> Arnd, do you have any comment on this?
>
> I think most others just define the compat data structures in the
> same file that implements the headers, inside the same #ifdef that
> hides the functions using them.
> This makes sense, because the data structures here don't define
> an interface, but rather describe what the interface looks like
> in the 32 bit case.
OK, moving the compatible structure declarations from the header
to the individual source files will fix all the header mess and
the odd compilation errors on i386 when using the compat.h header
inside of another header.
That declaration will need to be duplicated in each driver that
uses it (4 drivers in my case). In that sense (a structure declaration
used by multiple code modules) it does seem like an interface definition.
If that is what is needed, I will do it.
--
Paul
On Friday 04 May 2007, Paul Fulghum wrote:
>
> > - It is fishy that apart from one outlier in kexec.h, synclink.h is the
> > ? only header file which uses compat_ulong_t. ?Are we doing this right?
>
> Arnd, do you have any comment on this?
I think most others just define the compat data structures in the
same file that implements the headers, inside the same #ifdef that
hides the functions using them.
This makes sense, because the data structures here don't define
an interface, but rather describe what the interface looks like
in the 32 bit case.
> It seems like the compatible types should be available
> in something that is already commonly used like linux/types.h
>
> I'm fine with it either way. I'm not in
> a position to be making those kinds of decisions
> for widely used infrastructure, so I'll leave
> that for someone further up the food chain.
All common compat_* data structures are defined in
include/{linux,asm}/compat.h, which are empty if CONFIG_COMPAT=n.
It's against our normal conventions to hide declarations
inside an #ifdef, but I can see that it does keep the code
size down to make it impossible to compile code that is used
for compat on architectures that don't need it.
Arnd <><
On Saturday 05 May 2007, Paul Fulghum wrote:
> That declaration will need to be duplicated in each driver that
> uses it (4 drivers in my case). In that sense (a structure declaration
> used by multiple code modules) it does seem like an interface definition.
>
> If that is what is needed, I will do it.
Now that you mention the duplication, this sounds wrong as well. The easiest
solution is probably to just put the definition of your data structure
inside of #ifdef CONFIG_COMPAT in the header file.
Or you could go really fancy and write a new file that does the synclink
compat_ioctl handling in a generic way end in the end just calls the
fops->{unlocked_,}ioctl() function.
Which reminds me that I have been meaning to do a patch that creates
a new generic_compat_ioctl() [1] function for some time, and convert
drivers to this if their handlers are all compatible.
Arnd <><
[1]
/*
* Can be used as the ->compat_ioctl method in the file_operations
* for any driver that does not need any conversion in its ioctl
* handler
*/
long generic_file_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
int ret;
arg = (unsigned long)compat_ptr(arg);
if (file->f_ops->unlocked_ioctl)
ret = file->f_ops->unlocked_ioctl(file, cmd, arg);
else {
lock_kernel();
ret = file->f_ops->ioctl(file, cmd, arg);
unlock_kernel();
} else
ret = -ENOIOCTLCMD;
return ret;
}
On Sun, 2007-05-06 at 02:27 +0200, Arnd Bergmann wrote:
> Now that you mention the duplication, this sounds wrong as well. The easiest
> solution is probably to just put the definition of your data structure
> inside of #ifdef CONFIG_COMPAT in the header file.
The structure definition was already inside of #ifdef CONFIG_COMPAT
The problem is including linux/compat.h inside of synclink.h
causes an error on i386. The headerscheck facility is spitting
out an error even if the #include <linux/compat.h> is inside of
#ifdef CONFIG_COMPAT
make[3]: *** No rule to make target
`/usr/src/devel/usr/include/linux/.check.synclink.h', needed by
`__headerscheck'. Stop.
linux/kexec.h includes linux/compat.h without a similar error,
though that is inside of a #ifdef CONFIG_KEXEC
Moving linux/compat.h from synclink.h to synclink_gt.c
removes the error.
This is the last error standing in my way and I'm trying
to figure out the rules for when and where you are allowed
to use compat.h, I'm not familiar with the headerscheck
facility so I'm not sure what it is looking for and the
error is not very helpful. There is nothing in Documentation
covering it.
--
Paul Fulghum
Microgate Systems, Ltd
On Tuesday 08 May 2007, Paul Fulghum wrote:
> make[3]: *** No rule to make target
> `/usr/src/devel/usr/include/linux/.check.synclink.h', needed by
> `__headerscheck'. ?Stop.
>
> linux/kexec.h includes linux/compat.h without a similar error,
> though that is inside of a #ifdef CONFIG_KEXEC
>
> Moving linux/compat.h from synclink.h to synclink_gt.c
> removes the error.
>
> This is the last error standing in my way and I'm trying
> to figure out the rules for when and where you are allowed
> to use compat.h, I'm not familiar with the headerscheck
> facility so I'm not sure what it is looking for and the
> error is not very helpful. There is nothing in Documentation
> covering it.
The warning is about the situation that linux/synclink.h gets
installed by make headers_install, but linux/compat.h does not
get installed, so any user program including linux/synclink.h
will fail to build.
To solve this, you can to change include/linux/Kbuild to list
synclink.h as unifdef-y instead of header-y, and put the parts
that you don't want to be in user space inside of #ifdef __KERNEL__.
Alternatively, you can put these kernel-internal definitions into
a private header file in drivers/char that does not get installed
in the first place. That would be particularly useful if you can
also move other parts of linux/synclink.h into the private header,
when they are not part of the external ABI.
Arnd <><
Arnd Bergmann wrote:
> To solve this, you can to change include/linux/Kbuild to list
> synclink.h as unifdef-y instead of header-y, and put the parts
> that you don't want to be in user space inside of #ifdef __KERNEL__.
>
> Alternatively, you can put these kernel-internal definitions into
> a private header file in drivers/char that does not get installed
> in the first place. That would be particularly useful if you can
> also move other parts of linux/synclink.h into the private header,
> when they are not part of the external ABI.
Understood. That is the last piece to the puzzle.
I think the first approach would be better as
synclink.h is all interface definitions. The kernel
specific parts are in the individual synclink drivers
(such as register definitions, etc). The only exception
to this is now the ioctl32 structures and I would hate
to break out a whole new header just for that.
Thanks again for your targeted and informative help.
(Thanks to Andrew for your patience in dealing
with a patch that never should have been submitted.)
--
Paul