Skip to contentSkip to conversation

synclink_gt add compat_ioctl

PATCH In review Paul Fulghum 2 participants 5 messages May 2, 2007 Char and misc drivers

lore
Share & tools
lore

Paul Fulghum

author
#0 lore raw

PATCH 1/1 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]>
drivers/char/synclink_gt.c +112 -0
@@ -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,
include/linux/synclink.h +20 -0
@@ -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)
maintainer
#1 lore raw

drivers/char/synclink_gt.c static int ioctl(struct tty_struct *tty, · line 1197

Quoted 9 lines · Paul Fulghum
drivers/char/synclink_gt.c +9 -0
+	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.

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+	spin_lock_irqsave(&info->lock, flags);

no need for _irqsave, just use spin_{,un}lock_irq() when you know that interrupts are enabled.

Quoted 13 lines
include/linux/synclink.h +7 -0
@@ -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;

include/linux/synclink.h typedef struct _MGSL_PARAMS · line 186

Quoted 10 lines
include/linux/synclink.h +10 -0
+	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 <><
author
#2 lore raw
Quoted 5 lines · Arnd Bergmann

drivers/char/synclink_gt.c static int ioctl(struct tty_struct *tty, · line 1197

Quoted 3 lines
drivers/char/synclink_gt.c +3 -0
+	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.

Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+	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.

Quoted 13 lines
Quoted 13 lines
include/linux/synclink.h +13 -0
+/* 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;
Quoted 8 lines

include/linux/synclink.h typedef struct _MGSL_PARAMS · line 186

Quoted 4 lines
include/linux/synclink.h +4 -0
+	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

maintainer
#3 lore raw
Quoted 6 lines · Paul Fulghum
Quoted 5 lines
Quoted 2 lines

+
+    spin_lock_irqsave(&info->lock, flags);

no need for _irqsave, just use spin_{,un}lock_irq() when you know that interrupts are enabled.

Quoted 8 lines

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 <><
author
#4 lore raw
Quoted 3 lines · Arnd Bergmann

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