[PATCH 1/3] Char: mxser_new, fix recursive locking

[PATCH 1/3] Char: mxser_new, fix recursive locking

[Jiri Slaby]

Andrew, I think it would be good to have these 3 in 2.6.21.

--
mxser_new, fix recursive locking

Acquire a port lock only if not in_interrupt in some places, because ISR
holds the lock yet (and ldisc calls some of driver's routines which tries to
acquire it again due to tty->low_latency).

Thanks to Jan "Yenya" Kasprzak.

Cc: Jan "Yenya" Kasprzak <kas@fi.muni.cz>
Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit a3c71212efb38a67d344f35137136808e64537c6
tree c1ec123ca7007190dfae8c06fafec4702d88e00c
parent 0156510dee9d326af2ec52cf8b1a388ce9a839e9
author Jiri Slaby <jirislaby@gmail.com> Mon, 16 Apr 2007 13:24:46 +0200
committer Jiri Slaby <jirislaby@gmail.com> Mon, 16 Apr 2007 13:24:46 +0200

 drivers/char/mxser_new.c |   46 ++++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/char/mxser_new.c b/drivers/char/mxser_new.c
index 6362e78..626f491 100644
--- a/drivers/char/mxser_new.c
+++ b/drivers/char/mxser_new.c
@@ -85,6 +85,16 @@
 #define MXSER_HIGHBAUD	1
 #define MXSER_HAS2	2
 
+/* if we are in interrupt, lock is held by ISR, don't acquire it again! */
+#define mx_lock(info, flags) do {				\
+	if (!in_interrupt())					\
+		spin_lock_irqsave(&(info)->slock, flags);	\
+} while (0)
+#define mx_unlock(info, flags) do {				\
+	if (!in_interrupt())					\
+		spin_unlock_irqrestore(&(info)->slock, flags);	\
+} while (0)
+
 /* This is only for PCI */
 static const struct {
 	int type;
@@ -869,7 +879,7 @@ static void mxser_shutdown(struct mxser_port *info)
 	if (!(info->flags & ASYNC_INITIALIZED))
 		return;
 
-	spin_lock_irqsave(&info->slock, flags);
+	mx_lock(info, flags);
 
 	/*
 	 * clear delta_msr_wait queue to avoid mem leaks: we may free the irq
@@ -912,7 +922,7 @@ static void mxser_shutdown(struct mxser_port *info)
 	if (info->board->chip_flag)
 		SET_MOXA_MUST_NO_SOFTWARE_FLOW_CONTROL(info->ioaddr);
 
-	spin_unlock_irqrestore(&info->slock, flags);
+	mx_unlock(info, flags);
 }
 
 /*
@@ -1076,11 +1086,11 @@ static int mxser_write(struct tty_struct *tty, const unsigned char *buf, int cou
 			break;
 
 		memcpy(info->xmit_buf + info->xmit_head, buf, c);
-		spin_lock_irqsave(&info->slock, flags);
+		mx_lock(info, flags);
 		info->xmit_head = (info->xmit_head + c) &
 				  (SERIAL_XMIT_SIZE - 1);
 		info->xmit_cnt += c;
-		spin_unlock_irqrestore(&info->slock, flags);
+		mx_unlock(info, flags);
 
 		buf += c;
 		count -= c;
@@ -1091,12 +1101,12 @@ static int mxser_write(struct tty_struct *tty, const unsigned char *buf, int cou
 		if (!tty->hw_stopped ||
 				(info->type == PORT_16550A) ||
 				(info->board->chip_flag)) {
-			spin_lock_irqsave(&info->slock, flags);
+			mx_lock(info, flags);
 			outb(info->IER & ~UART_IER_THRI, info->ioaddr +
 					UART_IER);
 			info->IER |= UART_IER_THRI;
 			outb(info->IER, info->ioaddr + UART_IER);
-			spin_unlock_irqrestore(&info->slock, flags);
+			mx_unlock(info, flags);
 		}
 	}
 	return total;
@@ -1113,20 +1123,20 @@ static void mxser_put_char(struct tty_struct *tty, unsigned char ch)
 	if (info->xmit_cnt >= SERIAL_XMIT_SIZE - 1)
 		return;
 
-	spin_lock_irqsave(&info->slock, flags);
+	mx_lock(info, flags);
 	info->xmit_buf[info->xmit_head++] = ch;
 	info->xmit_head &= SERIAL_XMIT_SIZE - 1;
 	info->xmit_cnt++;
-	spin_unlock_irqrestore(&info->slock, flags);
+	mx_unlock(info, flags);
 	if (!tty->stopped) {
 		if (!tty->hw_stopped ||
 				(info->type == PORT_16550A) ||
 				info->board->chip_flag) {
-			spin_lock_irqsave(&info->slock, flags);
+			mx_lock(info, flags);
 			outb(info->IER & ~UART_IER_THRI, info->ioaddr + UART_IER);
 			info->IER |= UART_IER_THRI;
 			outb(info->IER, info->ioaddr + UART_IER);
-			spin_unlock_irqrestore(&info->slock, flags);
+			mx_unlock(info, flags);
 		}
 	}
 }
@@ -1146,13 +1156,13 @@ static void mxser_flush_chars(struct tty_struct *tty)
 			))
 		return;
 
-	spin_lock_irqsave(&info->slock, flags);
+	mx_lock(info, flags);
 
 	outb(info->IER & ~UART_IER_THRI, info->ioaddr + UART_IER);
 	info->IER |= UART_IER_THRI;
 	outb(info->IER, info->ioaddr + UART_IER);
 
-	spin_unlock_irqrestore(&info->slock, flags);
+	mx_unlock(info, flags);
 }
 
 static int mxser_write_room(struct tty_struct *tty)
@@ -1179,7 +1189,7 @@ static void mxser_flush_buffer(struct tty_struct *tty)
 	unsigned long flags;
 
 
-	spin_lock_irqsave(&info->slock, flags);
+	mx_lock(info, flags);
 	info->xmit_cnt = info->xmit_head = info->xmit_tail = 0;
 
 	fcr = inb(info->ioaddr + UART_FCR);
@@ -1187,7 +1197,7 @@ static void mxser_flush_buffer(struct tty_struct *tty)
 		info->ioaddr + UART_FCR);
 	outb(fcr, info->ioaddr + UART_FCR);
 
-	spin_unlock_irqrestore(&info->slock, flags);
+	mx_unlock(info, flags);
 
 	tty_wakeup(tty);
 }
@@ -1983,12 +1993,12 @@ static void mxser_stop(struct tty_struct *tty)
 	struct mxser_port *info = tty->driver_data;
 	unsigned long flags;
 
-	spin_lock_irqsave(&info->slock, flags);
+	mx_lock(info, flags);
 	if (info->IER & UART_IER_THRI) {
 		info->IER &= ~UART_IER_THRI;
 		outb(info->IER, info->ioaddr + UART_IER);
 	}
-	spin_unlock_irqrestore(&info->slock, flags);
+	mx_unlock(info, flags);
 }
 
 static void mxser_start(struct tty_struct *tty)
@@ -1996,13 +2006,13 @@ static void mxser_start(struct tty_struct *tty)
 	struct mxser_port *info = tty->driver_data;
 	unsigned long flags;
 
-	spin_lock_irqsave(&info->slock, flags);
+	mx_lock(info, flags);
 	if (info->xmit_cnt && info->xmit_buf) {
 		outb(info->IER & ~UART_IER_THRI, info->ioaddr + UART_IER);
 		info->IER |= UART_IER_THRI;
 		outb(info->IER, info->ioaddr + UART_IER);
 	}
-	spin_unlock_irqrestore(&info->slock, flags);
+	mx_unlock(info, flags);
 }
 
 static void mxser_set_termios(struct tty_struct *tty, struct ktermios *old_termios)

[PATCH 2/3] Char: mxser_new, fix TIOCMIWAIT

[Jiri Slaby]

mxser_new, fix TIOCMIWAIT

There was schedule() missing in the TIOCMIWAIT ioctl. Solve it by moving
the code to the wait_event_interruptible.

Cc: Jan "Yenya" Kasprzak <kas@fi.muni.cz>
Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit bd99756ce7fb8f3e9105b076a71046b0d8ad1f8f
tree b04c7d06790414f91897a8b0ff7c959b2226fe2b
parent a3c71212efb38a67d344f35137136808e64537c6
author Jiri Slaby <jirislaby@gmail.com> Mon, 16 Apr 2007 13:30:31 +0200
committer Jiri Slaby <jirislaby@gmail.com> Mon, 16 Apr 2007 13:30:31 +0200

 drivers/char/mxser_new.c |   38 +++++++++-----------------------------
 1 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/drivers/char/mxser_new.c b/drivers/char/mxser_new.c
index 626f491..ec4a561 100644
--- a/drivers/char/mxser_new.c
+++ b/drivers/char/mxser_new.c
@@ -1767,43 +1767,23 @@ static int mxser_ioctl(struct tty_struct *tty, struct file *file,
 		 *   (use |'ed TIOCM_RNG/DSR/CD/CTS for masking)
 		 * Caller should use TIOCGICOUNT to see which one it was
 		 */
-	case TIOCMIWAIT: {
-		DECLARE_WAITQUEUE(wait, current);
-		int ret;
+	case TIOCMIWAIT:
 		spin_lock_irqsave(&info->slock, flags);
-		cprev = info->icount;	/* note the counters on entry */
+		cnow = info->icount;	/* note the counters on entry */
 		spin_unlock_irqrestore(&info->slock, flags);
 
-		add_wait_queue(&info->delta_msr_wait, &wait);
-		while (1) {
+		wait_event_interruptible(info->delta_msr_wait, ({
+			cprev = cnow;
 			spin_lock_irqsave(&info->slock, flags);
 			cnow = info->icount;	/* atomic copy */
 			spin_unlock_irqrestore(&info->slock, flags);
 
-			set_current_state(TASK_INTERRUPTIBLE);
-			if (((arg & TIOCM_RNG) &&
-					(cnow.rng != cprev.rng)) ||
-					((arg & TIOCM_DSR) &&
-					(cnow.dsr != cprev.dsr)) ||
-					((arg & TIOCM_CD) &&
-					(cnow.dcd != cprev.dcd)) ||
-					((arg & TIOCM_CTS) &&
-					(cnow.cts != cprev.cts))) {
-				ret = 0;
-				break;
-			}
-			/* see if a signal did it */
-			if (signal_pending(current)) {
-				ret = -ERESTARTSYS;
-				break;
-			}
-			cprev = cnow;
-		}
-		current->state = TASK_RUNNING;
-		remove_wait_queue(&info->delta_msr_wait, &wait);
+			((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
+			((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
+			((arg & TIOCM_CD)  && (cnow.dcd != cprev.dcd)) ||
+			((arg & TIOCM_CTS) && (cnow.cts != cprev.cts));
+		}));
 		break;
-	}
-	/* NOTREACHED */
 	/*
 	 * Get counter of input serial line interrupts (DCD,RI,DSR,CTS)
 	 * Return: write counters to the user passed counter struct

[PATCH 3/3] Char: mxser, fix TIOCMIWAIT

[Jiri Slaby]

mxser, fix TIOCMIWAIT

There was schedule() missing in the TIOCMIWAIT ioctl. Solve it by moving
the code to the wait_event_interruptible.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 88c99e7bd7356c4b5a960f4a2b631d980bddc337
tree 4d713ce58282145839d6223a849c32a791e25a93
parent bd99756ce7fb8f3e9105b076a71046b0d8ad1f8f
author Jiri Slaby <jirislaby@gmail.com> Fri, 20 Apr 2007 12:26:08 +0200
committer Jiri Slaby <jirislaby@gmail.com> Fri, 20 Apr 2007 12:26:08 +0200

 drivers/char/mxser.c |   48 ++++++++++++++----------------------------------
 1 files changed, 14 insertions(+), 34 deletions(-)

diff --git a/drivers/char/mxser.c b/drivers/char/mxser.c
index 573b271..5953a45 100644
--- a/drivers/char/mxser.c
+++ b/drivers/char/mxser.c
@@ -1337,43 +1337,23 @@ static int mxser_ioctl(struct tty_struct *tty, struct file *file, unsigned int c
 		 *   (use |'ed TIOCM_RNG/DSR/CD/CTS for masking)
 		 * Caller should use TIOCGICOUNT to see which one it was
 		 */
-	case TIOCMIWAIT: {
-			DECLARE_WAITQUEUE(wait, current);
-			int ret;
+	case TIOCMIWAIT:
+		spin_lock_irqsave(&info->slock, flags);
+		cnow = info->icount;	/* note the counters on entry */
+		spin_unlock_irqrestore(&info->slock, flags);
+
+		wait_event_interruptible(info->delta_msr_wait, ({
+			cprev = cnow;
 			spin_lock_irqsave(&info->slock, flags);
-			cprev = info->icount;	/* note the counters on entry */
+			cnow = info->icount;	/* atomic copy */
 			spin_unlock_irqrestore(&info->slock, flags);
 
-			add_wait_queue(&info->delta_msr_wait, &wait);
-			while (1) {
-				spin_lock_irqsave(&info->slock, flags);
-				cnow = info->icount;	/* atomic copy */
-				spin_unlock_irqrestore(&info->slock, flags);
-
-				set_current_state(TASK_INTERRUPTIBLE);
-				if (((arg & TIOCM_RNG) &&
-						(cnow.rng != cprev.rng)) ||
-						((arg & TIOCM_DSR) &&
-						(cnow.dsr != cprev.dsr)) ||
-						((arg & TIOCM_CD) &&
-						(cnow.dcd != cprev.dcd)) ||
-						((arg & TIOCM_CTS) &&
-						(cnow.cts != cprev.cts))) {
-					ret = 0;
-					break;
-				}
-				/* see if a signal did it */
-				if (signal_pending(current)) {
-					ret = -ERESTARTSYS;
-					break;
-				}
-				cprev = cnow;
-			}
-			current->state = TASK_RUNNING;
-			remove_wait_queue(&info->delta_msr_wait, &wait);
-			break;
-		}
-		/* NOTREACHED */
+			((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
+			((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
+			((arg & TIOCM_CD)  && (cnow.dcd != cprev.dcd)) ||
+			((arg & TIOCM_CTS) && (cnow.cts != cprev.cts));
+		}));
+		break;
 		/*
 		 * Get counter of input serial line interrupts (DCD,RI,DSR,CTS)
 		 * Return: write counters to the user passed counter struct

Re: [PATCH 1/3] Char: mxser_new, fix recursive locking

[Alan Cox]

> Acquire a port lock only if not in_interrupt in some places, because ISR
> holds the lock yet (and ldisc calls some of driver's routines which tries to
> acquire it again due to tty->low_latency).

NAK

This is the wrong way to do it. If you don't support recursive entry then
don't use ->low_latency. If you do then ensure you drop the lock before
you call tty_flip_buffer_push().

The other way this could be tackled which has some merit is to require
that line discipline responses coming from a received frame call a new
tty method so drivers can tell callbacks from arriving data apart from
other events.

Alan

Re: [PATCH 2/3] Char: mxser_new, fix TIOCMIWAIT

[Jan Yenya Kasprzak]

Jiri Slaby wrote:
: mxser_new, fix TIOCMIWAIT
: 
: There was schedule() missing in the TIOCMIWAIT ioctl. Solve it by moving
: the code to the wait_event_interruptible.

	OK, this fixed the problem with your DCD-change monitoring
program (sorry that it took me too long to test it).

-Yenya

-- 
| Jan "Yenya" Kasprzak  <kas at {fi.muni.cz - work | yenya.net - private}> |
| GPG: ID 1024/D3498839      Fingerprint 0D99A7FB206605D7 8B35FCDE05B18A5E |
| http://www.fi.muni.cz/~kas/    Journal: http://www.fi.muni.cz/~kas/blog/ |
> I will never go to meetings again because I think  face to face meetings <
> are the biggest waste of time you can ever have.        --Linus Torvalds <

Re: [PATCH 1/3] Char: mxser_new, fix recursive locking

[Jan Yenya Kasprzak]

Alan Cox wrote:
: > Acquire a port lock only if not in_interrupt in some places, because ISR
: > holds the lock yet (and ldisc calls some of driver's routines which tries to
: > acquire it again due to tty->low_latency).
: 
: NAK
: 
: This is the wrong way to do it. If you don't support recursive entry then
: don't use ->low_latency. If you do then ensure you drop the lock before
: you call tty_flip_buffer_push().

	I did as you suggested, and I am not able to reproduce the problem
now. The patch is attached. I think it is quite minimal, so it should be
safe to apply it. What do you think, Jiri?

Signed-off-by: Jan "Yenya" Kasprzak <kas@fi.muni.cz>
Cc: Jiri Slaby <jirislaby@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>

--- linux-2.6.21-rc2/drivers/char/mxser_new.c.orig	2007-04-20 15:41:46.000000000 +0200
+++ linux-2.6.21-rc2/drivers/char/mxser_new.c	2007-04-20 15:46:46.000000000 +0200
@@ -2230,7 +2230,14 @@
 	port->mon_data.rxcnt += cnt;
 	port->mon_data.up_rxcnt += cnt;
 
+	/*
+	 * We are called from an interrupt context with &port->slock
+	 * being held. Drop it temporarily in order to prevent
+	 * recursive locking.
+	 */
+	spin_unlock(&port->slock);
 	tty_flip_buffer_push(tty);
+	spin_lock(&port->slock);
 }
 
 static void mxser_transmit_chars(struct mxser_port *port)


-Yenya

-- 
| Jan "Yenya" Kasprzak  <kas at {fi.muni.cz - work | yenya.net - private}> |
| GPG: ID 1024/D3498839      Fingerprint 0D99A7FB206605D7 8B35FCDE05B18A5E |
| http://www.fi.muni.cz/~kas/    Journal: http://www.fi.muni.cz/~kas/blog/ |
> I will never go to meetings again because I think  face to face meetings <
> are the biggest waste of time you can ever have.        --Linus Torvalds <

Re: [PATCH 1/3] Char: mxser_new, fix recursive locking

[Jiri Slaby]

On 4/20/07, Jan Yenya Kasprzak <kas@fi.muni.cz> wrote:
>         I did as you suggested, and I am not able to reproduce the problem
> now. The patch is attached. I think it is quite minimal, so it should be
> safe to apply it. What do you think, Jiri?
>
> Signed-off-by: Jan "Yenya" Kasprzak <kas@fi.muni.cz>
> Cc: Jiri Slaby <jirislaby@gmail.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>

Acked-by: Jiri Slaby <jirislaby@gmail.com>

Re: [PATCH 1/3] Char: mxser_new, fix recursive locking

[Alan Cox]

> Signed-off-by: Jan "Yenya" Kasprzak <kas@fi.muni.cz>
> Cc: Jiri Slaby <jirislaby@gmail.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>

Acked-by: Alan Cox <alan@redhat.com>
> 
> --- linux-2.6.21-rc2/drivers/char/mxser_new.c.orig	2007-04-20 15:41:46.000000000 +0200
> +++ linux-2.6.21-rc2/drivers/char/mxser_new.c	2007-04-20 15:46:46.000000000 +0200
> @@ -2230,7 +2230,14 @@
>  	port->mon_data.rxcnt += cnt;
>  	port->mon_data.up_rxcnt += cnt;
>  
> +	/*
> +	 * We are called from an interrupt context with &port->slock
> +	 * being held. Drop it temporarily in order to prevent
> +	 * recursive locking.
> +	 */
> +	spin_unlock(&port->slock);
>  	tty_flip_buffer_push(tty);
> +	spin_lock(&port->slock);
>  }
>