Skip to contentSkip to conversation

tty_ioctl: Soft carrier handling

PATCH In review Alan Cox 3 participants 4 messages Mar 10, 2008 – Mar 12, 2008 Char and misc drivers

lore
Share & tools
lore

Alan Cox

author
#0 lore raw

PATCH 1/1 tty_ioctl: Soft carrier handling

First cut at moving the soft carrier handling knowledge entirely into the core code. One or two drivers still needed to snoop these functions to track CLOCAL internally. Instead make TIOCSSOFTCAR generate the same driver calls as other termios ioctls changing the clocal flag. This allows us to remove any driver knowledge and special casing. Also while we are at it we can fix the error handling.

Signed-off-by Alan Cox <[email protected]>
drivers/char/tty_ioctl.c +27 -6
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c
@@ -753,6 +756,32 @@
 }
 
 /**
+ *	tty_change_softcar	-	carrier change ioctl helper
+ *	@tty: tty to update
+ *	@arg: enable/disable CLOCAL
+ *
+ *	Perform a change to the CLOCAL state and call into the driver
+ *	layer to make it visible. All done with the termios mutex
+ */
+ 
+static int tty_change_softcar(struct tty_struct *tty, int arg)
+{
+	int ret = 0;
+	int bit = arg ? CLOCAL : 0;
+	struct ktermios old = *tty->termios;
+	
+	mutex_lock(&tty->termios_mutex);
+	tty->termios->c_cflag &= ~CLOCAL;
+	tty->termios->c_cflag |= bit;
+	if (tty->driver->set_termios)
+		tty->driver->set_termios(tty, &old);
+	if ((tty->termios->c_cflag & CLOCAL) != bit)
+		ret = -EINVAL;	
+	mutex_unlock(&tty->termios_mutex);
+	return ret;
+}
+
+/**
  *	tty_mode_ioctl		-	mode related ioctls
  *	@tty: tty for the ioctl
  *	@file: file pointer for the tty

@@ -862,12 +891,7 @@
 	case TIOCSSOFTCAR:
 		if (get_user(arg, (unsigned int __user *) arg))
 			return -EFAULT;
-		mutex_lock(&tty->termios_mutex);
-		tty->termios->c_cflag =
-			((tty->termios->c_cflag & ~CLOCAL) |
-			 (arg ? CLOCAL : 0));
-		mutex_unlock(&tty->termios_mutex);
-		return 0;
+		return tty_change_softcar(tty, arg);
 	default:
 		return -ENOIOCTLCMD;
 	}
#1 lore raw
Quoted 13 lines · Alan Cox

First cut at moving the soft carrier handling knowledge entirely into the core code. One or two drivers still needed to snoop these functions to track CLOCAL internally. Instead make TIOCSSOFTCAR generate the same driver calls as other termios ioctls changing the clocal flag. This allows us to remove any driver knowledge and special casing. Also while we are at it we can fix the error handling.

Signed-off-by Alan Cox <[email protected]>

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c

drivers/char/tty_ioctl.c +0 -0
@@ -753,6 +756,32 @@

[...]

drivers/char/tty_ioctl.c line 771

Quoted 5 lines
drivers/char/tty_ioctl.c +5 -0
+static int tty_change_softcar(struct tty_struct *tty, int arg)
+{
+	int ret = 0;
+	int bit = arg ? CLOCAL : 0;
+	struct ktermios old = *tty->termios;

Maybe a dumb question, but shouldn't be the snapshot done in the critical section too?

Quoted 11 lines
drivers/char/tty_ioctl.c +11 -0
+	
+	mutex_lock(&tty->termios_mutex);
+	tty->termios->c_cflag &= ~CLOCAL;
+	tty->termios->c_cflag |= bit;
+	if (tty->driver->set_termios)
+		tty->driver->set_termios(tty, &old);
+	if ((tty->termios->c_cflag & CLOCAL) != bit)
+		ret = -EINVAL;	
+	mutex_unlock(&tty->termios_mutex);
+	return ret;
+}
author
#2 lore raw
Quoted 6 lines

drivers/char/tty_ioctl.c line 771

Quoted 3 lines
drivers/char/tty_ioctl.c +2 -0
+	int bit = arg ? CLOCAL : 0;
+	struct ktermios old = *tty->termios;

Maybe a dumb question, but shouldn't be the snapshot done in the critical section too?

Yes it should. I'll fix that in the next bits and double check the cases Andrew asks about (although I think they are ok with caller locking from memory)

Alan

#3 lore raw
In reply to Alan Cox
Quoted 21 lines · On Mon, 10 Mar 2008 21:56:13 +0000 Alan Cox <[email protected]> wrote:

First cut at moving the soft carrier handling knowledge entirely into the core code. One or two drivers still needed to snoop these functions to track CLOCAL internally. Instead make TIOCSSOFTCAR generate the same driver calls as other termios ioctls changing the clocal flag. This allows us to remove any driver knowledge and special casing. Also while we are at it we can fix the error handling.

Signed-off-by Alan Cox <[email protected]>

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c

drivers/char/tty_ioctl.c +5 -0
@@ -753,6 +756,32 @@
 }
 
 /**
+ *	tty_change_softcar	-	carrier change ioctl helper
+ *	@tty: tty to update
+ *	@arg: enable/disable CLOCAL
+ *
+ *	Perform a change to the CLOCAL state and call into the driver

drivers/char/tty_ioctl.c line 773

Quoted 10 lines
drivers/char/tty_ioctl.c +10 -0
+ *	layer to make it visible. All done with the termios mutex
+ */
+ 
+static int tty_change_softcar(struct tty_struct *tty, int arg)
+{
+	int ret = 0;
+	int bit = arg ? CLOCAL : 0;
+	struct ktermios old = *tty->termios;
+	
+	mutex_lock(&tty->termios_mutex);

change_termios() modifies tty->termios under tty->termios_mutex, which is very sensible.

Hence (surely?) all code which takes a copy of tty->termios should do so under tty->termios_mutex so that it doesn't get a copy of an internally inconsistent, half-overwritten structure.

But this code doesn't do that, nor does change_termios(), set_termios() and perhaps other places.

Confused.

Quoted 9 lines
drivers/char/tty_ioctl.c +9 -0
+	tty->termios->c_cflag &= ~CLOCAL;
+	tty->termios->c_cflag |= bit;
+	if (tty->driver->set_termios)
+		tty->driver->set_termios(tty, &old);
+	if ((tty->termios->c_cflag & CLOCAL) != bit)
+		ret = -EINVAL;	
+	mutex_unlock(&tty->termios_mutex);
+	return ret;
+}