LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] tty_ioctl: Soft carrier handling
@ 2008-03-10 21:56 Alan Cox
  2008-03-10 22:24 ` Jiri Slaby
  2008-03-11  4:26 ` Andrew Morton
  0 siblings, 2 replies; 4+ messages in thread
From: Alan Cox @ 2008-03-10 21:56 UTC (permalink / raw)
  To: akpm, linux-kernel

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 <alan@redhat.com>

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
--- linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c	2008-03-10 12:57:53.000000000 +0000
+++ linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c	2008-03-10 13:27:09.000000000 +0000
@@ -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;
 	}

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] tty_ioctl: Soft carrier handling
  2008-03-10 21:56 [PATCH] tty_ioctl: Soft carrier handling Alan Cox
@ 2008-03-10 22:24 ` Jiri Slaby
  2008-03-12 13:22   ` Alan Cox
  2008-03-11  4:26 ` Andrew Morton
  1 sibling, 1 reply; 4+ messages in thread
From: Jiri Slaby @ 2008-03-10 22:24 UTC (permalink / raw)
  To: Alan Cox; +Cc: akpm, linux-kernel

On 03/10/2008 10:56 PM, Alan Cox 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 <alan@redhat.com>
> 
> 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
> --- linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c	2008-03-10 12:57:53.000000000 +0000
> +++ linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c	2008-03-10 13:27:09.000000000 +0000
> @@ -753,6 +756,32 @@
[...]
> +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?

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] tty_ioctl: Soft carrier handling
  2008-03-10 21:56 [PATCH] tty_ioctl: Soft carrier handling Alan Cox
  2008-03-10 22:24 ` Jiri Slaby
@ 2008-03-11  4:26 ` Andrew Morton
  1 sibling, 0 replies; 4+ messages in thread
From: Andrew Morton @ 2008-03-11  4:26 UTC (permalink / raw)
  To: Alan Cox; +Cc: linux-kernel

On Mon, 10 Mar 2008 21:56:13 +0000 Alan Cox <alan@lxorguk.ukuu.org.uk> 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 <alan@redhat.com>
> 
> 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
> --- linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c	2008-03-10 12:57:53.000000000 +0000
> +++ linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c	2008-03-10 13:27:09.000000000 +0000
> @@ -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);

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.

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] tty_ioctl: Soft carrier handling
  2008-03-10 22:24 ` Jiri Slaby
@ 2008-03-12 13:22   ` Alan Cox
  0 siblings, 0 replies; 4+ messages in thread
From: Alan Cox @ 2008-03-12 13:22 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: akpm, linux-kernel


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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2008-03-12 13:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-10 21:56 [PATCH] tty_ioctl: Soft carrier handling Alan Cox
2008-03-10 22:24 ` Jiri Slaby
2008-03-12 13:22   ` Alan Cox
2008-03-11  4:26 ` Andrew Morton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).