[PATCH] synclink_gt add compat_ioctl
Add support for 32 bit ioctl on 64 bit systems for synclink_gt Cc: Arnd Bergmann <[EMAIL PROTECTED]> Signed-off-by: Paul Fulghum <[EMAIL PROTECTED]> --- a/include/linux/Kbuild 2007-04-25 22:08:32.0 -0500 +++ b/include/linux/Kbuild 2007-05-09 10:11:22.0 -0500 @@ -140,7 +140,6 @@ header-y += snmp.h header-y += sockios.h header-y += som.h header-y += sound.h -header-y += synclink.h header-y += telephony.h header-y += termios.h header-y += ticable.h @@ -315,6 +314,7 @@ unifdef-y += sonypi.h unifdef-y += soundcard.h unifdef-y += stat.h unifdef-y += stddef.h +unifdef-y += synclink.h unifdef-y += sysctl.h unifdef-y += tcp.h unifdef-y += time.h --- a/include/linux/synclink.h 2007-04-25 22:08:32.0 -0500 +++ b/include/linux/synclink.h 2007-05-09 10:37:20.0 -0500 @@ -291,4 +291,29 @@ struct gpio_desc { #define MGSL_IOCGGPIO _IOR(MGSL_MAGIC_IOC,17,struct gpio_desc) #define MGSL_IOCWAITGPIO _IOWR(MGSL_MAGIC_IOC,18,struct gpio_desc) +#ifdef __KERNEL__ +/* provide 32 bit ioctl compatibility on 64 bit systems */ +#ifdef CONFIG_COMPAT +#include +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 MGSL_IOCSPARAMS32 _IOW(MGSL_MAGIC_IOC,0,struct MGSL_PARAMS32) +#define MGSL_IOCGPARAMS32 _IOR(MGSL_MAGIC_IOC,1,struct MGSL_PARAMS32) +#endif +#endif + #endif /* _SYNCLINK_H_ */ --- a/drivers/char/synclink_gt.c2007-04-25 22:08:32.0 -0500 +++ b/drivers/char/synclink_gt.c2007-05-09 10:12:29.0 -0500 @@ -1171,6 +1171,112 @@ static int ioctl(struct tty_struct *tty, } /* + * support for 32 bit ioctl calls on 64 bit systems + */ +#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; +} + +static long slgt_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)); +
Re: [PATCH] synclink_gt add compat_ioctl
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
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 <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
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 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
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; } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
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 <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
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 _ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] synclink_gt add compat_ioctl
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.0 -0500 +++ b/include/linux/synclink.h 2007-05-03 12:44:09.0 -0500 @@ -9,6 +9,8 @@ * the terms of the GNU Public License (GPL) */ +#include + #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.c2007-04-25 22:08:32.0 -0500 +++ b/drivers/char/synclink_gt.c2007-05-03 12:40:36.0 -0500 @@ -73,6 +73,7 @@ #include #include #include +#include #include #include @@ -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; +
Re: [PATCH] synclink_gt add compat_ioctl
Arnd Bergmann wrote: The same function contains a copy_from_user(), which cannot be called with interrupts disabled, so yes, I am very certain it will not change. Good point. -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Thursday 03 May 2007, Paul Fulghum wrote: > >> + > >> +spin_lock_irqsave(&info->lock, flags); > > > > no need for _irqsave, just use spin_{,un}lock_irq() when you know that > > interrupts are enabled. > > That makes me a little uneasy. The locking > mechanisms (and just about everything else) above the driver > seem to change frequently. This involves not just the VFS but > the tty core as well. > > If you are confident this will not change, I will > switch to spin_lock(). I used spin_lock_irqsave() to be > more robust against changes to behavior outside my driver. The same function contains a copy_from_user(), which cannot be called with interrupts disabled, so yes, I am very certain it will not change. Arnd <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
Arnd Bergmann wrote: + case MGSL_IOCGPARAMS32: + rc = get_params32(info, (struct MGSL_PARAMS32 __user *)compat_ptr(arg)); + break; No need for the cast here. OK, for some reason I remember getting a warning error without it. I must have been mistaken. + + spin_lock_irqsave(&info->lock, flags); no need for _irqsave, just use spin_{,un}lock_irq() when you know that interrupts are enabled. That makes me a little uneasy. The locking mechanisms (and just about everything else) above the driver seem to change frequently. This involves not just the VFS but the tty core as well. If you are confident this will not change, I will switch to spin_lock(). I used spin_lock_irqsave() to be more robust against changes to behavior outside my driver. +/* provide 32 bit ioctl compatibility on 64 bit systems */ +struct MGSL_PARAMS32 +{ + unsigned intmode; + unsigned char loopback; + unsigned short flags; + unsigned char encoding; + unsigned intclock_speed; + unsigned char addr_filter; + unsigned short crc_type; + unsigned char preamble_length; + unsigned char preamble; + unsigned intdata_rate; + unsigned char data_bits; + unsigned char stop_bits; + unsigned char parity; +}; The definition is correct, but by convention it would be better to use compat_ulong_t instead of unsigned int for those fields that are an unsigned long in native mode. OK -- Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] synclink_gt add compat_ioctl
On Wednesday 02 May 2007, Paul Fulghum wrote: > + switch (cmd) { > + > + case MGSL_IOCSPARAMS32: > + rc = set_params32(info, (struct MGSL_PARAMS32 __user > *)compat_ptr(arg)); > + break; > + > + case MGSL_IOCGPARAMS32: > + rc = get_params32(info, (struct MGSL_PARAMS32 __user > *)compat_ptr(arg)); > + break; No need for the cast here. > + > + spin_lock_irqsave(&info->lock, flags); no need for _irqsave, just use spin_{,un}lock_irq() when you know that interrupts are enabled. > --- a/include/linux/synclink.h2007-04-25 22:08:32.0 -0500 > +++ b/include/linux/synclink.h2007-05-02 14:59:17.0 -0500 > @@ -167,6 +167,24 @@ typedef struct _MGSL_PARAMS > > } MGSL_PARAMS, *PMGSL_PARAMS; > > +/* provide 32 bit ioctl compatibility on 64 bit systems */ > +struct MGSL_PARAMS32 > +{ > + unsigned intmode; > + unsigned char loopback; > + unsigned short flags; > + unsigned char encoding; > + unsigned intclock_speed; > + unsigned char addr_filter; > + unsigned short crc_type; > + unsigned char preamble_length; > + unsigned char preamble; > + unsigned intdata_rate; > + unsigned char data_bits; > + unsigned char stop_bits; > + unsigned char parity; > +}; The definition is correct, but by convention it would be better to use compat_ulong_t instead of unsigned int for those fields that are an unsigned long in native mode. Arnd <>< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] synclink_gt add compat_ioctl
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/drivers/char/synclink_gt.c2007-04-25 22:08:32.0 -0500 +++ b/drivers/char/synclink_gt.c2007-05-02 14:52:48.0 -0500 @@ -73,6 +73,7 @@ #include #include #include +#include #include #include @@ -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, (struct MGSL_PARAMS32 __user *)compat_ptr(arg)); + break; + + case MGSL_IOCGPARAMS32: + rc = get_params32(info, (struct MGSL_PARAMS32 __user *)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,61 @@ 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= (unsigned int)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 = (unsigned int)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 = (unsigned int)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) +{ + unsigned long flags; + 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_irqsave(&info->lock, flags); + 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; +