Skip to contentSkip to conversation

tty add compat_ioctl

PATCH In review Paul Fulghum 1 participant 4 messages May 3, 2007 – May 4, 2007 1 message not in the archive

Subsystems (2)
TTY layer and serial drivers Char and misc drivers
lore
Share & tools
lore

Paul Fulghum

author
#0 lore raw

PATCH 1/1 tty add compat_ioctl

Add compat_ioctl method for tty code to allow processing of 32 bit ioctl calls on 64 bit systems by tty core, tty drivers, and line disciplines.

Based on patch by Arnd Bergmann:
http://www.uwsg.iu.edu/hypermail/linux/kernel/0511.0/1732.html

Signed-off-by Paul Fulghum <[email protected]>
CC Arnd Bergmann <[email protected]>
drivers/char/n_tty.c +13 -16
@@ -1544,21 +1544,18 @@ static unsigned int normal_poll(struct t
 }
 
 struct tty_ldisc tty_ldisc_N_TTY = {
-	TTY_LDISC_MAGIC,	/* magic */
-	"n_tty",		/* name */
-	0,			/* num */
-	0,			/* flags */
-	n_tty_open,		/* open */
-	n_tty_close,		/* close */
-	n_tty_flush_buffer,	/* flush_buffer */
-	n_tty_chars_in_buffer,	/* chars_in_buffer */
-	read_chan,		/* read */
-	write_chan,		/* write */
-	n_tty_ioctl,		/* ioctl */
-	n_tty_set_termios,	/* set_termios */
-	normal_poll,		/* poll */
-	NULL,			/* hangup */
-	n_tty_receive_buf,	/* receive_buf */
-	n_tty_write_wakeup	/* write_wakeup */
+	.magic           = TTY_LDISC_MAGIC,
+	.name            = "n_tty",
+	.open            = n_tty_open,
+	.close           = n_tty_close,
+	.flush_buffer    = n_tty_flush_buffer,
+	.chars_in_buffer = n_tty_chars_in_buffer,
+	.read            = read_chan,
+	.write           = write_chan,
+	.ioctl           = n_tty_ioctl,
+	.set_termios     = n_tty_set_termios,
+	.poll            = normal_poll,
+	.receive_buf     = n_tty_receive_buf,
+	.write_wakeup    = n_tty_write_wakeup
 };
include/linux/tty_driver.h +9 -0
@@ -52,6 +52,11 @@
  * 	This routine allows the tty driver to implement
  *	device-specific ioctl's.  If the ioctl number passed in cmd
  * 	is not recognized by the driver, it should return ENOIOCTLCMD.
+ *
+ * long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+ * 	                unsigned int cmd, unsigned long arg);
+ *
+ * 	implement ioctl processing for 32 bit process on 64 bit system
  * 
  * void (*set_termios)(struct tty_struct *tty, struct ktermios * old);
  *

@@ -132,6 +137,8 @@ struct tty_operations {
 	int  (*chars_in_buffer)(struct tty_struct *tty);
 	int  (*ioctl)(struct tty_struct *tty, struct file * file,
 		    unsigned int cmd, unsigned long arg);
+	long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+			     unsigned int cmd, unsigned long arg);
 	void (*set_termios)(struct tty_struct *tty, struct ktermios * old);
 	void (*throttle)(struct tty_struct * tty);
 	void (*unthrottle)(struct tty_struct * tty);

@@ -193,6 +200,8 @@ struct tty_driver {
 	int  (*chars_in_buffer)(struct tty_struct *tty);
 	int  (*ioctl)(struct tty_struct *tty, struct file * file,
 		    unsigned int cmd, unsigned long arg);
+	long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+			     unsigned int cmd, unsigned long arg);
 	void (*set_termios)(struct tty_struct *tty, struct ktermios * old);
 	void (*throttle)(struct tty_struct * tty);
 	void (*unthrottle)(struct tty_struct * tty);
drivers/char/tty_io.c +38 -3

89 lines not on this page · raw message

include/linux/tty_ldisc.h +7 -0
@@ -59,6 +59,11 @@
  * 	low-level driver can "grab" an ioctl request before the line
  * 	discpline has a chance to see it.
  * 
+ * long	(*compat_ioctl)(struct tty_struct * tty, struct file * file,
+ * 		        unsigned int cmd, unsigned long arg);
+ *
+ *      Process ioctl calls from 32-bit process on 64-bit system
+ *
  * void	(*set_termios)(struct tty_struct *tty, struct ktermios * old);
  *
  * 	This function notifies the line discpline that a change has

@@ -118,6 +123,8 @@ struct tty_ldisc {
 			 const unsigned char * buf, size_t nr);	
 	int	(*ioctl)(struct tty_struct * tty, struct file * file,
 			 unsigned int cmd, unsigned long arg);
+	long	(*compat_ioctl)(struct tty_struct * tty, struct file * file,
+				unsigned int cmd, unsigned long arg);
 	void	(*set_termios)(struct tty_struct *tty, struct ktermios * old);
 	unsigned int (*poll)(struct tty_struct *, struct file *,
 			     struct poll_table_struct *);

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#2 lore raw
Quoted 6 lines · Arnd Bergmann

- The return value of the new compat_ioctl methods should probably
'int', not 'long'. We've had the discussion before and then
decided not to change the existing compat_ioctl and unlocked_ioctl functions -- even though int is more appropriate, but having the same prototype has the advantage that a driver
can use the same function for both ->ioctl and ->compat_ioctl
if all calls are compatible.

I noticed that but thought the change in return value type had some higher purpose I had not perceived. If it can be int that would be the way to go.

Quoted 8 lines

- In your driver you don't get the big kernel lock in the compat_ioctl function. I assume that this is correct for the particular driver, but it may be nice if you could consequently also add an unlocked_ioctl function that can be used without the BKL for native ioctls. It would be good to hear an opinon on this from someone who has an insight in tty locking issues though, so I'm Cc:ing some people who have touched that recently.

I don't count on higher level locking for
synchronization issues specific to the driver.

I thought the current compat_ioctl() was already meant to *not* have the BKL just like unlocked_ioctl. My thought was that any driver getting a recent update like compat_ioctl() would need to be reviewed for BKL safety and take the lock manually if necessary.

Drivers that are falling behind wont have a compat_ioctl defined at all.

--
Paul

author
#3 lore raw
Quoted 10 lines · Paul Fulghum
Quoted 8 lines · Arnd Bergmann

- In your driver you don't get the big kernel lock in the compat_ioctl function. I assume that this is correct for the particular driver, but it may be nice if you could consequently also add an unlocked_ioctl function that can be used without the BKL for native ioctls. It would be good to hear an opinon on this from someone who has an insight in tty locking issues though, so I'm Cc:ing some people who have touched that recently.

Quoted 8 lines

I don't count on higher level locking for synchronization issues specific to the driver.

I thought the current compat_ioctl() was already meant to *not* have the BKL just like unlocked_ioctl. My thought was that any driver getting a recent update like compat_ioctl() would need to be reviewed for BKL safety and take the lock manually if necessary.

Nevermind. I misread what you wrote (I'm tired).
Yes, adding an unlocked_ioctl() makes sense.

--
Paul

author
#4 lore raw
In reply to a message not in the archive
Quoted 6 lines · Arnd Bergmann

- The return value of the new compat_ioctl methods should probably
'int', not 'long'. We've had the discussion before and then
decided not to change the existing compat_ioctl and unlocked_ioctl functions -- even though int is more appropriate, but having the same prototype has the advantage that a driver
can use the same function for both ->ioctl and ->compat_ioctl
if all calls are compatible.

Any comments on this or should I assume that
compat_ioctl() will stay with ulong return type?

Signature · 2 lines
Paul Fulghum
Microgate Systems, Ltd.