[PATCH] synclink_gt add compat_ioctl

[PATCH] synclink_gt add compat_ioctl

[Paul Fulghum]

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 <paulkf@microgate.com>

--- 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,

Re: [PATCH] synclink_gt add compat_ioctl

[Andrew Morton]

On Thu, 03 May 2007 13:01:17 -0500
Paul Fulghum <paulkf@microgate.com> 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 <akpm@linux-foundation.org>

- 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 <alan@lxorguk.ukuu.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Paul Fulghum <paulkf@microgate.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 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
_

Re: [PATCH] synclink_gt add compat_ioctl

[Paul Fulghum]

Andrew Morton wrote:
> On Thu, 03 May 2007 13:01:17 -0500
> Paul Fulghum <paulkf@microgate.com> 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

Re: [PATCH] synclink_gt add compat_ioctl

[Paul Fulghum]

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.

Re: [PATCH] synclink_gt add compat_ioctl

[Andrew Morton]

On Thu, 03 May 2007 13:01:17 -0500
Paul Fulghum <paulkf@microgate.com> 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.

Re: [PATCH] synclink_gt add compat_ioctl

[Arnd Bergmann]

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 <><

Re: [PATCH] synclink_gt add compat_ioctl

[Paul Fulghum]

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

Re: [PATCH] synclink_gt add compat_ioctl

[Arnd Bergmann]

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;
}

Re: [PATCH] synclink_gt add compat_ioctl

[Paul Fulghum]

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

Re: [PATCH] synclink_gt add compat_ioctl

[Arnd Bergmann]

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 <><

Re: [PATCH] synclink_gt add compat_ioctl

[Paul Fulghum]

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

[PATCH] synclink_gt add compat_ioctl

[Paul Fulghum]

Add support for 32 bit ioctl on 64 bit systems for synclink_gt

Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Paul Fulghum <paulkf@microgate.com>

--- a/include/linux/Kbuild	2007-04-25 22:08:32.000000000 -0500
+++ b/include/linux/Kbuild	2007-05-09 10:11:22.000000000 -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.000000000 -0500
+++ b/include/linux/synclink.h	2007-05-09 10:37:20.000000000 -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 <linux/compat.h>
+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.c	2007-04-25 22:08:32.000000000 -0500
+++ b/drivers/char/synclink_gt.c	2007-05-09 10:12:29.000000000 -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));
+		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;
+}
+#else
+#define slgt_compat_ioctl NULL
+#endif /* ifdef CONFIG_COMPAT */
+
+/*
  * proc fs support
  */
 static inline int line_info(char *buf, struct slgt_info *info)
@@ -3443,6 +3549,7 @@ static const struct tty_operations ops =
 	.chars_in_buffer = chars_in_buffer,
 	.flush_buffer = flush_buffer,
 	.ioctl = ioctl,
+	.compat_ioctl = slgt_compat_ioctl,
 	.throttle = throttle,
 	.unthrottle = unthrottle,
 	.send_xchar = send_xchar,

Re: [PATCH] synclink_gt add compat_ioctl

[Paul Fulghum]

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

Re: [PATCH] synclink_gt add compat_ioctl

[Paul Fulghum]

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 int	mode;
>> +	unsigned char	loopback;
>> +	unsigned short	flags;
>> +	unsigned char	encoding;
>> +	unsigned int	clock_speed;
>> +	unsigned char	addr_filter;
>> +	unsigned short	crc_type;
>> +	unsigned char	preamble_length;
>> +	unsigned char	preamble;
>> +	unsigned int	data_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

Re: [PATCH] synclink_gt add compat_ioctl

[Arnd Bergmann]

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 <><

Re: [PATCH] synclink_gt add compat_ioctl

[Arnd Bergmann]

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.h	2007-04-25 22:08:32.000000000 -0500
> +++ b/include/linux/synclink.h	2007-05-02 14:59:17.000000000 -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 int	mode;
> +	unsigned char	loopback;
> +	unsigned short	flags;
> +	unsigned char	encoding;
> +	unsigned int	clock_speed;
> +	unsigned char	addr_filter;
> +	unsigned short	crc_type;
> +	unsigned char	preamble_length;
> +	unsigned char	preamble;
> +	unsigned int	data_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 <><

[PATCH] synclink_gt add compat_ioctl

[Paul Fulghum]

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 <paulkf@microgate.com>


--- a/drivers/char/synclink_gt.c	2007-04-25 22:08:32.000000000 -0500
+++ b/drivers/char/synclink_gt.c	2007-05-02 14:52:48.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, (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;
+	info->params.data_bits       = tmp_params.data_bits;
+	info->params.stop_bits       = tmp_params.stop_bits;
+	info->params.parity          = tmp_params.parity;
+	spin_unlock_irqrestore(&info->lock, flags);
+
+ 	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 +3552,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,
--- a/include/linux/synclink.h	2007-04-25 22:08:32.000000000 -0500
+++ b/include/linux/synclink.h	2007-05-02 14:59:17.000000000 -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 int	mode;
+	unsigned char	loopback;
+	unsigned short	flags;
+	unsigned char	encoding;
+	unsigned int	clock_speed;
+	unsigned char	addr_filter;
+	unsigned short	crc_type;
+	unsigned char	preamble_length;
+	unsigned char	preamble;
+	unsigned int	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 +294,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)