uart_ioctl OOPS with irtty-sir

uart_ioctl OOPS with irtty-sir

[Jean Tourrilhes]

	Hi Russel,

	Sorry to bring more bad news...

	In 2.5.66, somebody (maybe you) added a check to
tty_hung_up_p(filp) in uart_ioctl().
	The code now looks like this (drivers/serial/core.c) :
------------------------------------
static int
uart_ioctl(struct tty_struct *tty, struct file *filp, unsigned int cmd,
	   unsigned long arg)
{
[...]
	if (tty_hung_up_p(filp)) {
		ret = -EIO;
		goto out_up;
	}
[...]
	switch (cmd) {
	case TIOCMSET:
		ret = uart_set_modem_info(state->port, cmd,
					  (unsigned int *)arg);
		break;
------------------------------------

	Unfortunately, the irtty-sir driver, which is a TTY line
discipline and a network driver, need to be able to change the RTS and
DTR line from a kernel thread.
	The code looks like (drivers/net/irda/irtty-sir.c) :
----------------------------
static int irtty_set_dtr_rts(struct sir_dev *dev, int dtr, int rts)
{
[...]
	if (priv->tty->driver.ioctl(priv->tty, NULL, TIOCMSET, (unsigned long) &arg)) { 
		IRDA_DEBUG(2, "%s(), error doing ioctl!\n", __FUNCTION__);
	}
----------------------------

	You can guess the result : instant OPPS.


	I don't really see how I would be able to get hold of a "struct
file" in kernel space, so please advise.

	Have fun, and thanks in advance...

	Jean

Re: uart_ioctl OOPS with irtty-sir

[Russell King]

On Thu, Apr 03, 2003 at 05:34:05PM -0800, Jean Tourrilhes wrote:
> 	Unfortunately, the irtty-sir driver, which is a TTY line
> discipline and a network driver, need to be able to change the RTS and
> DTR line from a kernel thread.

I'd prefer if we added an tty API to allow line disciplines to read/set
the modem control lines to the tty later, rather than having line
disciplines play games with IOCTLs.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: uart_ioctl OOPS with irtty-sir

[Jean Tourrilhes]

On Fri, Apr 04, 2003 at 10:25:35AM +0100, Russell King wrote:
> On Thu, Apr 03, 2003 at 05:34:05PM -0800, Jean Tourrilhes wrote:
> > 	Unfortunately, the irtty-sir driver, which is a TTY line
> > discipline and a network driver, need to be able to change the RTS and
> > DTR line from a kernel thread.
> 
> I'd prefer if we added an tty API to allow line disciplines to read/set
> the modem control lines to the tty later, rather than having line
> disciplines play games with IOCTLs.

	Yes, that would look much cleaner, and it would avoid this
kind of confusion to come back again in the long term. And it would
also be slightly more efficient.

> Russell King

	Thanks for the quick answer !

	Jean

Re: uart_ioctl OOPS with irtty-sir

[Jean Tourrilhes]

On Fri, Apr 04, 2003 at 10:25:35AM +0100, Russell King wrote:
> On Thu, Apr 03, 2003 at 05:34:05PM -0800, Jean Tourrilhes wrote:
> > 	Unfortunately, the irtty-sir driver, which is a TTY line
> > discipline and a network driver, need to be able to change the RTS and
> > DTR line from a kernel thread.
> 
> I'd prefer if we added an tty API to allow line disciplines to read/set
> the modem control lines to the tty later, rather than having line
> disciplines play games with IOCTLs.

	So, the patch has to look like what is below.
	I've decided to use the internal set/clear API, because it
allow to ignore the default state of control line (all the troubles
with TIOCM_OUT2/TIOCM_OUT1 on various platforms), so it's definitely a
win.
	I'm not too sure about locking. I guess the spinlock in
uart_update_mctrl() is probably good enough.
	I was tempted to create the same API for setting the speed
(baud rate), but that may need to wait for another time.

	As you can't use IrDA SIR with 2.5.X, would you mind giving a
look at the issue ? Feel free to forward as needed to get comments
from all the interested parties. Thanks in advance...

	Have fun...

	Jean

---------------------------------------------------------------

diff -u -p linux/include/linux/tty_driver.d0.h linux/include/linux/tty_driver.h
--- linux/include/linux/tty_driver.d0.h	Tue Apr  8 09:56:21 2003
+++ linux/include/linux/tty_driver.h	Tue Apr  8 10:09:09 2003
@@ -157,6 +157,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);
+	int  (*modem_ctrl)(struct tty_struct *tty,
+			   unsigned int set, unsigned int clear);
 	void (*set_termios)(struct tty_struct *tty, struct termios * old);
 	void (*throttle)(struct tty_struct * tty);
 	void (*unthrottle)(struct tty_struct * tty);
diff -u -p linux/drivers/serial/core.d0.c linux/drivers/serial/core.c
--- linux/drivers/serial/core.d0.c	Tue Apr  8 09:51:00 2003
+++ linux/drivers/serial/core.c	Tue Apr  8 10:15:29 2003
@@ -1158,6 +1158,26 @@ uart_ioctl(struct tty_struct *tty, struc
 	return ret;
 }
 
+/*
+ * Called by line disciplines to change the various modem control bits...
+ * Line disciplines are implemented within the kernel, and therefore
+ * we don't want them to use the ioctl function above.
+ * Jean II
+ */
+static int
+uart_modem_ctrl(struct tty_struct *tty, unsigned int set, unsigned int clear)
+{
+	struct uart_state *state = tty->driver_data;
+
+	/* Set new values, if any */
+	/* Locking will be done in there */
+	uart_update_mctrl(state->port, set, clear);
+
+	/* Return new value */
+	return state->port->ops->get_mctrl(state->port);
+}
+
+
 static void uart_set_termios(struct tty_struct *tty, struct termios *old_termios)
 {
 	struct uart_state *state = tty->driver_data;
@@ -2150,6 +2170,7 @@ int uart_register_driver(struct uart_dri
 	normal->chars_in_buffer	= uart_chars_in_buffer;
 	normal->flush_buffer	= uart_flush_buffer;
 	normal->ioctl		= uart_ioctl;
+	normal->modem_ctrl	= uart_modem_ctrl;
 	normal->throttle	= uart_throttle;
 	normal->unthrottle	= uart_unthrottle;
 	normal->send_xchar	= uart_send_xchar;
diff -u -p linux/drivers/net/irda/irtty-sir.d0.c linux/drivers/net/irda/irtty-sir.c
--- linux/drivers/net/irda/irtty-sir.d0.c	Tue Apr  8 09:46:54 2003
+++ linux/drivers/net/irda/irtty-sir.c	Tue Apr  8 10:34:53 2003
@@ -180,32 +180,29 @@ static int irtty_change_speed(struct sir
 static int irtty_set_dtr_rts(struct sir_dev *dev, int dtr, int rts)
 {
 	struct sirtty_cb *priv = dev->priv;
-	int arg = 0;
+	int set = 0;
+	int clear = 0;
 
 	ASSERT(priv != NULL, return -1;);
 	ASSERT(priv->magic == IRTTY_MAGIC, return -1;);
 
-#ifdef TIOCM_OUT2 /* Not defined for ARM */
-	arg = TIOCM_OUT2;
-#endif
 	if (rts)
-		arg |= TIOCM_RTS;
+		set |= TIOCM_RTS;
+	else
+		clear |= TIOCM_RTS;
 	if (dtr)
-		arg |= TIOCM_DTR;
+		set |= TIOCM_DTR;
+	else
+		clear |= TIOCM_DTR;
 
 	/*
-	 * The ioctl() function, or actually set_modem_info() in serial.c
-	 * expects a pointer to the argument in user space. This is working
-	 * here because we are always called from the kIrDAd thread which
-	 * has set_fs(KERNEL_DS) permanently set. Therefore copy_from_user()
-	 * is happy with our arg-parameter being local here in kernel space.
+	 * We can't use ioctl() because it expects a non-null file structure,
+	 * and we don't have that here.
+	 * This function is not yet defined for all tty driver, so
+	 * let's be careful... Jean II
 	 */
-
-	lock_kernel();
-	if (priv->tty->driver.ioctl(priv->tty, NULL, TIOCMSET, (unsigned long) &arg)) { 
-		IRDA_DEBUG(2, "%s(), error doing ioctl!\n", __FUNCTION__);
-	}
-	unlock_kernel();
+	ASSERT(priv->tty->driver.modem_ctrl != NULL, return -1;);
+	priv->tty->driver.modem_ctrl(priv->tty, set, clear);
 
 	return 0;
 }
diff -u -p linux/drivers/net/irda/sir_kthread.d0.c linux/drivers/net/irda/sir_kthread.c
--- linux/drivers/net/irda/sir_kthread.d0.c	Mon Apr  7 18:56:28 2003
+++ linux/drivers/net/irda/sir_kthread.c	Mon Apr  7 18:59:31 2003
@@ -151,6 +151,13 @@ static int irda_thread(void *startup)
 
 	while (irda_rq_queue.thread != NULL) {
 
+		/* We use TASK_INTERRUPTIBLE, rather than
+		 * TASK_UNINTERRUPTIBLE.  Andrew Morton made this
+		 * change ; he told me that it is safe, because "signal
+		 * blocking is now handled in daemonize()", he added
+		 * that the problem is that "uninterruptible sleep
+		 * contributes to load average", making user worry.
+		 * Jean II */
 		set_task_state(current, TASK_INTERRUPTIBLE);
 		add_wait_queue(&irda_rq_queue.kick, &wait);
 		if (list_empty(&irda_rq_queue.request_list))

Re: uart_ioctl OOPS with irtty-sir

[Jean Tourrilhes]

On Fri, Apr 04, 2003 at 10:25:35AM +0100, Russell King wrote:
> On Thu, Apr 03, 2003 at 05:34:05PM -0800, Jean Tourrilhes wrote:
> > 	Unfortunately, the irtty-sir driver, which is a TTY line
> > discipline and a network driver, need to be able to change the RTS and
> > DTR line from a kernel thread.
> 
> I'd prefer if we added an tty API to allow line disciplines to read/set
> the modem control lines to the tty later, rather than having line
> disciplines play games with IOCTLs.

	Hi,

	Who is responsible to managing the TTY API ? If I want to add
a call to it, who am I supposed to send the patch to ?
	Thanks in advance...

	Jean

Re: uart_ioctl OOPS with irtty-sir

[Russell King]

Jean,

How about this patch - this also moves the ioctl parsing to the tty
layer.  I'm intending all drivers should provide tiocmget and tiocmset
methods if they implement modem control lines, and not parse the
corresponding ioctl command numbers in their ioctl method.

Note - this patch has only been compile-tested except for the last file,
and may apply with an offset(s).  The irtty-sir patch is actually yours;
I just edited your patch for that file since it was a straight forward
change.

--- ref/include/linux/tty_driver.h	Fri Jan 17 10:24:15 2003
+++ linux/include/linux/tty_driver.h	Sun Apr 13 14:03:11 2003
@@ -172,6 +172,9 @@
 			  int count, int *eof, void *data);
 	int (*write_proc)(struct file *file, const char *buffer,
 			  unsigned long count, void *data);
+	int (*tiocmget)(struct tty_struct *tty, struct file *file);
+	int (*tiocmset)(struct tty_struct *tty, struct file *file,
+			unsigned int set, unsigned int clear);
 	struct list_head tty_drivers;
 };
 
--- ref/drivers/char/tty_io.c	Sat Apr 12 15:02:35 2003
+++ linux/drivers/char/tty_io.c	Sun Apr 13 14:51:34 2003
@@ -1704,6 +1704,55 @@
 	return 0;
 }
 
+static int
+tty_tiocmget(struct tty_struct *tty, struct file *file, unsigned long arg)
+{
+	int retval = -ENOIOCTLCMD;
+
+	if (tty->driver.tiocmget) {
+		retval = tty->driver.tiocmget(tty, file);
+
+		if (retval >= 0)
+			retval = put_user(retval, (int *)arg);
+	}
+	return retval;
+}
+
+static int
+tty_tiocmset(struct tty_struct *tty, struct file *file, unsigned int cmd,
+	     unsigned long arg)
+{
+	int retval = -ENOIOCTLCMD;
+
+	if (tty->driver.tiocmset) {
+		unsigned int set, clear, val;
+
+		retval = get_user(val, (unsigned int *)arg);
+		if (retval)
+			return retval;
+
+		set = clear = 0;
+		switch (cmd) {
+		case TIOCMBIS:
+			set = val;
+			break;
+		case TIOCMBIC:
+			clear = val;
+			break;
+		case TIOCMSET:
+			set = val;
+			clear = ~val;
+			break;
+		}
+
+		set &= TIOCM_DTR|TIOCM_RTS|TIOCM_OUT1|TIOCM_OUT2;
+		clear &= TIOCM_DTR|TIOCM_RTS|TIOCM_OUT1|TIOCM_OUT2;
+
+		retval = tty->driver.tiocmset(tty, file, set, clear);
+	}
+	return retval;
+}
+
 /*
  * Split this up, as gcc can choke on it otherwise..
  */
@@ -1829,6 +1878,20 @@
 			return 0;
 		case TCSBRKP:	/* support for POSIX tcsendbreak() */	
 			return send_break(tty, arg ? arg*(HZ/10) : HZ/4);
+
+		case TIOCMGET:
+			retval = tty_tiocmget(tty, file, arg);
+			if (retval != -ENOIOCTLCMD)
+				return retval;
+			break;
+
+		case TIOCMSET:
+		case TIOCMBIC:
+		case TIOCMBIS:
+			retval = tty_tiocmset(tty, file, cmd, arg);
+			if (retval != -ENOIOCTLCMD)
+				return retval;
+			break;
 	}
 	if (tty->driver.ioctl) {
 		int retval = (tty->driver.ioctl)(tty, file, cmd, arg);
--- ref/drivers/serial/core.c	Tue Mar 18 00:27:13 2003
+++ linux/drivers/serial/core.c	Sun Apr 13 14:41:49 2003
@@ -874,45 +874,38 @@
 	return put_user(result, value);
 }
 
-static int uart_get_modem_info(struct uart_port *port, unsigned int *value)
+static int uart_tiocmget(struct tty_struct *tty, struct file *file)
 {
-	unsigned int result = port->mctrl;
+	struct uart_state *state = tty->driver_data;
+	struct uart_port *port = state->port;
+	int result = -EIO;
 
-	result |= port->ops->get_mctrl(port);
+	down(&state->sem);
+	if ((!file || !tty_hung_up_p(file)) &&
+	    !(tty->flags & (1 << TTY_IO_ERROR))) {
+		result = port->mctrl;
+		result |= port->ops->get_mctrl(port);
+	}
+	up(&state->sem);
 
-	return put_user(result, value);
+	return result;
 }
 
 static int
-uart_set_modem_info(struct uart_port *port, unsigned int cmd,
-		    unsigned int *value)
+uart_tiocmset(struct tty_struct *tty, struct file *file,
+	      unsigned int set, unsigned int clear)
 {
-	unsigned int arg, set, clear;
-	int ret = 0;
-
-	if (get_user(arg, value))
-		return -EFAULT;
-
-	arg &= TIOCM_DTR|TIOCM_RTS|TIOCM_OUT1|TIOCM_OUT2;
+	struct uart_state *state = tty->driver_data;
+	struct uart_port *port = state->port;
+	int ret = -EIO;
 
-	set = clear = 0;
-	switch (cmd) {
-	case TIOCMBIS:
-		set = arg;
-		break;
-	case TIOCMBIC:
-		clear = arg;
-		break;
-	case TIOCMSET:
-		set = arg;
-		clear = ~arg;
-		break;
-	default:
-		ret = -EINVAL;
-		break;
-	}
-	if (ret == 0)
+	down(&state->sem);
+	if ((!file || !tty_hung_up_p(file)) &&
+	    !(tty->flags & (1 << TTY_IO_ERROR))) {
 		uart_update_mctrl(port, set, clear);
+		ret = 0;
+	}
+	up(&state->sem);
 	return ret;
 }
 
@@ -923,8 +916,12 @@
 
 	BUG_ON(!kernel_locked());
 
+	down(&state->sem);
+
 	if (port->type != PORT_UNKNOWN)
 		port->ops->break_ctl(port, break_state);
+
+	up(&state->sem);
 }
 
 static int uart_do_autoconfig(struct uart_state *state)
@@ -1131,17 +1128,6 @@
 	 * protected against the tty being hung up.
 	 */
 	switch (cmd) {
-	case TIOCMGET:
-		ret = uart_get_modem_info(state->port, (unsigned int *)arg);
-		break;
-
-	case TIOCMBIS:
-	case TIOCMBIC:
-	case TIOCMSET:
-		ret = uart_set_modem_info(state->port, cmd,
-					  (unsigned int *)arg);
-		break;
-
 	case TIOCSERGETLSR: /* Get line status register */
 		ret = uart_get_lsr_info(state, (unsigned int *)arg);
 		break;
@@ -2163,6 +2149,8 @@
 #ifdef CONFIG_PROC_FS
 	normal->read_proc	= uart_read_proc;
 #endif
+	normal->tiocmget	= uart_tiocmget;
+	normal->tiocmset	= uart_tiocmset;
 
 	/*
 	 * Initialise the UART state(s).
--- ref/drivers/net/irda/irtty-sir.c	Tue Apr  8 09:46:54 2003
+++ linux/drivers/net/irda/irtty-sir.c	Tue Apr  8 10:34:53 2003
@@ -180,32 +180,29 @@ static int irtty_change_speed(struct sir
 static int irtty_set_dtr_rts(struct sir_dev *dev, int dtr, int rts)
 {
 	struct sirtty_cb *priv = dev->priv;
-	int arg = 0;
+	int set = 0;
+	int clear = 0;
 
 	ASSERT(priv != NULL, return -1;);
 	ASSERT(priv->magic == IRTTY_MAGIC, return -1;);
 
-#ifdef TIOCM_OUT2 /* Not defined for ARM */
-	arg = TIOCM_OUT2;
-#endif
 	if (rts)
-		arg |= TIOCM_RTS;
+		set |= TIOCM_RTS;
+	else
+		clear |= TIOCM_RTS;
 	if (dtr)
-		arg |= TIOCM_DTR;
+		set |= TIOCM_DTR;
+	else
+		clear |= TIOCM_DTR;
 
 	/*
-	 * The ioctl() function, or actually set_modem_info() in serial.c
-	 * expects a pointer to the argument in user space. This is working
-	 * here because we are always called from the kIrDAd thread which
-	 * has set_fs(KERNEL_DS) permanently set. Therefore copy_from_user()
-	 * is happy with our arg-parameter being local here in kernel space.
+	 * We can't use ioctl() because it expects a non-null file structure,
+	 * and we don't have that here.
+	 * This function is not yet defined for all tty driver, so
+	 * let's be careful... Jean II
 	 */
-
-	lock_kernel();
-	if (priv->tty->driver.ioctl(priv->tty, NULL, TIOCMSET, (unsigned long) &arg)) { 
-		IRDA_DEBUG(2, "%s(), error doing ioctl!\n", __FUNCTION__);
-	}
-	unlock_kernel();
+	ASSERT(priv->tty->driver.tiocmset != NULL, return -1;);
+	priv->tty->driver.tiocmset(priv->tty, NULL, set, clear);
 
 	return 0;
 }


-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: uart_ioctl OOPS with irtty-sir

[Russell King]

On Tue, Apr 08, 2003 at 10:44:43AM -0700, Jean Tourrilhes wrote:
> 	I was tempted to create the same API for setting the speed
> (baud rate), but that may need to wait for another time.

I'm not intending changing the settermios API - it would be rather
inefficient to have a set of "set baud rate", "set stop bits",
"set bits per character", "set parity error response" etc calls,
especially when many of these involve writing the same set of
registers in the UART.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: uart_ioctl OOPS with irtty-sir

[Jean Tourrilhes]

On Sun, Apr 13, 2003 at 05:04:39PM +0100, Russell King wrote:
> Jean,
> 
> How about this patch - this also moves the ioctl parsing to the tty
> layer.  I'm intending all drivers should provide tiocmget and tiocmset
> methods if they implement modem control lines, and not parse the
> corresponding ioctl command numbers in their ioctl method.
> 
> Note - this patch has only been compile-tested except for the last file,
> and may apply with an offset(s).  The irtty-sir patch is actually yours;
> I just edited your patch for that file since it was a straight forward
> change.

	Looks good. I'm going to test that and report.
	Thanks a lot !

	Jean

Re: uart_ioctl OOPS with irtty-sir

[Jean Tourrilhes]

On Sun, Apr 13, 2003 at 05:51:04PM +0100, Russell King wrote:
> On Tue, Apr 08, 2003 at 10:44:43AM -0700, Jean Tourrilhes wrote:
> > 	I was tempted to create the same API for setting the speed
> > (baud rate), but that may need to wait for another time.
> 
> I'm not intending changing the settermios API - it would be rather
> inefficient to have a set of "set baud rate", "set stop bits",
> "set bits per character", "set parity error response" etc calls,
> especially when many of these involve writing the same set of
> registers in the UART.

	Don't worry, the current situation is workable.

	Jean