set_ldisc notification fixes

PATCH New Peter Hurley 1 participant 6 messages Nov 5, 2014

Subsystems (4)

TTY layer and serial drivers Arm/Microchip (AT91) SoC support Arm primecell UART PL010 and PL011 drivers Microchip AT91 serial driver

lore

Share & tools

lore

Peter Hurley

author

2014-11-05 18:11 UTC #0 lore raw

cover set_ldisc notification fixes

Hi Greg,

This patch series fixes several locking and functional problems with the .set_ldisc() notification sent by the tty core:
1) termios is used without holding termios_rwsem
2) uart port flags are changed without holding port mutex
3) several of the drivers call their enable_ms() routines directly without holding port lock
4) The set_ldisc notification will turn on modem status interrupts if the CD line is being used for N_PPS time signal, but doesn't turn modem status interrupts back off if N_PPS is being unset.

Note: this series depends on the 26-patch 'tty locking changes' series and the 10-patch 'serial core fixes' series.

Regards,

Peter Hurley (5):
tty: Allow safe access to termios for set_ldisc() handlers
serial: core: Claim port mutex for set_ldisc()
serial: core: Pass termios to set_ldisc() notifications
serial: Take uart port lock for direct *_enable_ms()
serial: Test/disable MSIs if switching from N_PPS

 drivers/tty/serial/8250/8250_core.c | 27 ++++++++++++++++++++++++---
 drivers/tty/serial/amba-pl010.c     | 24 +++++++++++++++++++++---
 drivers/tty/serial/atmel_serial.c   | 11 +++++++++--
 drivers/tty/serial/bfin_uart.c      |  5 +++--
 drivers/tty/serial/clps711x.c       |  5 +++--
 drivers/tty/serial/serial_core.c    |  7 +++++--
 drivers/tty/tty_ldisc.c             |  5 ++++-
 include/linux/serial_core.h         |  2 +-
 8 files changed, 70 insertions(+), 16 deletions(-)

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 18:11 UTC #1 lore raw

PATCH 1/5 tty: Allow safe access to termios for set_ldisc() handlers

Allow a tty driver to safely access termios settings while handling the set_ldisc() notification. UART drivers use the set_ldisc() notification to check if the N_PPS line discipline is being enabled; if so, modem status interrupts may also need to be enabled. Conversely, modem status interrupts may need to be disabled if switching away from the N_PPS line discipline.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/tty_ldisc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/tty/tty_ldisc.c +4 -1

index b66a81d..3737f55 100644
@@ -572,8 +572,11 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 		tty_ldisc_restore(tty, old_ldisc);
 	}
 
-	if (tty->ldisc->ops->num != old_ldisc->ops->num && tty->ops->set_ldisc)
+	if (tty->ldisc->ops->num != old_ldisc->ops->num && tty->ops->set_ldisc) {
+		down_read(&tty->termios_rwsem);
 		tty->ops->set_ldisc(tty);
+		up_read(&tty->termios_rwsem);
+	}
 
 	/* At this point we hold a reference to the new ldisc and a
 	   reference to the old ldisc, or we hold two references to

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 18:11 UTC #2 lore raw

PATCH 2/5 serial: core: Claim port mutex for set_ldisc()

In reply to Peter Hurley

Three UART drivers (8250, atmel & amba-pl010) enable modem status interrupts if the line discipline is changed to N_PPS. However, the uart port flags may only be safely modified while holding the port mutex.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/serial/serial_core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/tty/serial/serial_core.c +4 -1

index 1a2d90f..5209eaa 100644
@@ -1245,8 +1245,11 @@ static void uart_set_ldisc(struct tty_struct *tty)
 	struct uart_state *state = tty->driver_data;
 	struct uart_port *uport = state->uart_port;
 
-	if (uport->ops->set_ldisc)
+	if (uport->ops->set_ldisc) {
+		mutex_lock(&state->port.mutex);
 		uport->ops->set_ldisc(uport, tty->termios.c_line);
+		mutex_unlock(&state->port.mutex);
+	}
 }
 
 static void uart_set_termios(struct tty_struct *tty,

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 18:11 UTC #3 lore raw

PATCH 3/5 serial: core: Pass termios to set_ldisc() notifications

In reply to Peter Hurley

UART drivers which enable modem status interrupts when switching to N_PPS line discipline need to determine if modem status interrupts should be disabled when switching from N_PPS. Specifically, the set_ldisc() notification needs to evaluate UART_ENABLE_MS() which requires termios->c_cflag.

Convert in-tree UART drivers to new interface.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/serial/8250/8250_core.c | 4 ++--
 drivers/tty/serial/amba-pl010.c     | 4 ++--
 drivers/tty/serial/atmel_serial.c   | 4 ++--
 drivers/tty/serial/bfin_uart.c      | 5 +++--
 drivers/tty/serial/clps711x.c       | 5 +++--
 drivers/tty/serial/serial_core.c    | 2 +-
 include/linux/serial_core.h         | 2 +-
 7 files changed, 14 insertions(+), 12 deletions(-)

drivers/tty/serial/8250/8250_core.c +2 -2

index ca5cfdc..6cfdb7b 100644
@@ -2603,9 +2603,9 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
 }
 
 static void
-serial8250_set_ldisc(struct uart_port *port, int new)
+serial8250_set_ldisc(struct uart_port *port, struct ktermios *termios)
 {
-	if (new == N_PPS) {
+	if (termios->c_line == N_PPS) {
 		port->flags |= UPF_HARDPPS_CD;
 		serial8250_enable_ms(port);
 	} else

drivers/tty/serial/amba-pl010.c +2 -2

index 2064d31..76e40b3 100644
@@ -468,9 +468,9 @@ pl010_set_termios(struct uart_port *port, struct ktermios *termios,
 	spin_unlock_irqrestore(&uap->port.lock, flags);
 }
 
-static void pl010_set_ldisc(struct uart_port *port, int new)
+static void pl010_set_ldisc(struct uart_port *port, struct ktermios *termios)
 {
-	if (new == N_PPS) {
+	if (termios->c_line == N_PPS) {
 		port->flags |= UPF_HARDPPS_CD;
 		pl010_enable_ms(port);
 	} else

drivers/tty/serial/atmel_serial.c +2 -2

index 8a84034..a2374d9 100644
@@ -2042,9 +2042,9 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 	spin_unlock_irqrestore(&port->lock, flags);
 }
 
-static void atmel_set_ldisc(struct uart_port *port, int new)
+static void atmel_set_ldisc(struct uart_port *port, struct ktermios *termios)
 {
-	if (new == N_PPS) {
+	if (termios->c_line == N_PPS) {
 		port->flags |= UPF_HARDPPS_CD;
 		atmel_enable_ms(port);
 	} else {

drivers/tty/serial/bfin_uart.c +3 -2

index 7da9911..44b27ec 100644
@@ -944,12 +944,13 @@ bfin_serial_verify_port(struct uart_port *port, struct serial_struct *ser)
  * Enable the IrDA function if tty->ldisc.num is N_IRDA.
  * In other cases, disable IrDA function.
  */
-static void bfin_serial_set_ldisc(struct uart_port *port, int ld)
+static void bfin_serial_set_ldisc(struct uart_port *port,
+				  struct ktermios *termios)
 {
 	struct bfin_serial_port *uart = (struct bfin_serial_port *)port;
 	unsigned int val;
 
-	switch (ld) {
+	switch (termios->c_line) {
 	case N_IRDA:
 		val = UART_GET_GCTL(uart);
 		val |= (UMOD_IRDA | RPOLC);

drivers/tty/serial/clps711x.c +3 -2

index acfe317..f963c4c 100644
@@ -225,13 +225,14 @@ static void uart_clps711x_break_ctl(struct uart_port *port, int break_state)
 	writel(ubrlcr, port->membase + UBRLCR_OFFSET);
 }
 
-static void uart_clps711x_set_ldisc(struct uart_port *port, int ld)
+static void uart_clps711x_set_ldisc(struct uart_port *port,
+				    struct ktermios *termios)
 {
 	if (!port->line) {
 		struct clps711x_port *s = dev_get_drvdata(port->dev);
 
 		regmap_update_bits(s->syscon, SYSCON_OFFSET, SYSCON1_SIREN,
-				   (ld == N_IRDA) ? SYSCON1_SIREN : 0);
+				   (termios->c_line == N_IRDA) ? SYSCON1_SIREN : 0);
 	}
 }

drivers/tty/serial/serial_core.c +1 -1

index 5209eaa..c5fb08c 100644
@@ -1247,7 +1247,7 @@ static void uart_set_ldisc(struct tty_struct *tty)
 
 	if (uport->ops->set_ldisc) {
 		mutex_lock(&state->port.mutex);
-		uport->ops->set_ldisc(uport, tty->termios.c_line);
+		uport->ops->set_ldisc(uport, &tty->termios);
 		mutex_unlock(&state->port.mutex);
 	}
 }

include/linux/serial_core.h +1 -1

index ad93296..40b4cc4 100644
@@ -63,7 +63,7 @@ struct uart_ops {
 	void		(*flush_buffer)(struct uart_port *);
 	void		(*set_termios)(struct uart_port *, struct ktermios *new,
 				       struct ktermios *old);
-	void		(*set_ldisc)(struct uart_port *, int new);
+	void		(*set_ldisc)(struct uart_port *, struct ktermios *);
 	void		(*pm)(struct uart_port *, unsigned int state,
 			      unsigned int oldstate);

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 18:11 UTC #4 lore raw

PATCH 4/5 serial: Take uart port lock for direct *_enable_ms()

In reply to Peter Hurley

Three UART drivers (8250, atmel & amba-pl010) directly call their enable_ms() method; the uart port lock must be acquired before any h/w programming.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/serial/8250/8250_core.c | 2 ++
 drivers/tty/serial/amba-pl010.c     | 2 ++
 drivers/tty/serial/atmel_serial.c   | 2 ++
 3 files changed, 6 insertions(+)

drivers/tty/serial/8250/8250_core.c +2 -0

index 6cfdb7b..8eb0654 100644
@@ -2607,7 +2607,9 @@ serial8250_set_ldisc(struct uart_port *port, struct ktermios *termios)
 {
 	if (termios->c_line == N_PPS) {
 		port->flags |= UPF_HARDPPS_CD;
+		spin_lock_irq(&port->lock);
 		serial8250_enable_ms(port);
+		spin_unlock_irq(&port->lock);
 	} else
 		port->flags &= ~UPF_HARDPPS_CD;
 }

drivers/tty/serial/amba-pl010.c +2 -0

index 76e40b3..04b83bf 100644
@@ -472,7 +472,9 @@ static void pl010_set_ldisc(struct uart_port *port, struct ktermios *termios)
 {
 	if (termios->c_line == N_PPS) {
 		port->flags |= UPF_HARDPPS_CD;
+		spin_lock_irq(&port->lock);
 		pl010_enable_ms(port);
+		spin_unlock_irq(&port->lock);
 	} else
 		port->flags &= ~UPF_HARDPPS_CD;
 }

drivers/tty/serial/atmel_serial.c +2 -0

index a2374d9..b68dfa5 100644
@@ -2046,7 +2046,9 @@ static void atmel_set_ldisc(struct uart_port *port, struct ktermios *termios)
 {
 	if (termios->c_line == N_PPS) {
 		port->flags |= UPF_HARDPPS_CD;
+		spin_lock_irq(&port->lock);
 		atmel_enable_ms(port);
+		spin_unlock_irq(&port->lock);
 	} else {
 		port->flags &= ~UPF_HARDPPS_CD;
 	}

Signature · 1 line

2.1.3

Peter Hurley

author

2014-11-05 18:11 UTC #5 lore raw

PATCH 5/5 serial: Test/disable MSIs if switching from N_PPS

In reply to Peter Hurley

Switching to the N_PPS line discipline may require enabling modem status interrupts; conversely switching from N_PPS may require disabling modem status interrupts.

Affected drivers:
8250
amba-pl010
atmel

Cc Nicolas Ferre <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/serial/8250/8250_core.c | 21 ++++++++++++++++++++-
 drivers/tty/serial/amba-pl010.c     | 18 +++++++++++++++++-
 drivers/tty/serial/atmel_serial.c   |  5 +++++
 3 files changed, 42 insertions(+), 2 deletions(-)

drivers/tty/serial/8250/8250_core.c +20 -1

index 8eb0654..095319b 100644
@@ -1397,6 +1397,19 @@ static void serial8250_stop_rx(struct uart_port *port)
 	serial8250_rpm_put(up);
 }
 
+static void serial8250_disable_ms(struct uart_port *port)
+{
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
+
+	/* no MSR capabilities */
+	if (up->bugs & UART_BUG_NOMSR)
+		return;
+
+	up->ier &= ~UART_IER_MSI;
+	serial_port_out(port, UART_IER, up->ier);
+}
+
 static void serial8250_enable_ms(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);

@@ -2610,8 +2623,14 @@ serial8250_set_ldisc(struct uart_port *port, struct ktermios *termios)
 		spin_lock_irq(&port->lock);
 		serial8250_enable_ms(port);
 		spin_unlock_irq(&port->lock);
-	} else
+	} else {
 		port->flags &= ~UPF_HARDPPS_CD;
+		if (!UART_ENABLE_MS(port, termios->c_cflag)) {
+			spin_lock_irq(&port->lock);
+			serial8250_disable_ms(port);
+			spin_unlock_irq(&port->lock);
+		}
+	}
 }

drivers/tty/serial/amba-pl010.c +17 -1

index 04b83bf..6f8464e 100644
@@ -103,6 +103,16 @@ static void pl010_stop_rx(struct uart_port *port)
 	writel(cr, uap->port.membase + UART010_CR);
 }
 
+static void pl010_disable_ms(struct uart_port *port)
+{
+	struct uart_amba_port *uap = (struct uart_amba_port *)port;
+	unsigned int cr;
+
+	cr = readb(uap->port.membase + UART010_CR);
+	cr &= ~UART010_CR_MSIE;
+	writel(cr, uap->port.membase + UART010_CR);
+}
+
 static void pl010_enable_ms(struct uart_port *port)
 {
 	struct uart_amba_port *uap = (struct uart_amba_port *)port;

@@ -475,8 +485,14 @@ static void pl010_set_ldisc(struct uart_port *port, struct ktermios *termios)
 		spin_lock_irq(&port->lock);
 		pl010_enable_ms(port);
 		spin_unlock_irq(&port->lock);
-	} else
+	} else {
 		port->flags &= ~UPF_HARDPPS_CD;
+		if (!UART_ENABLE_MS(port, termios->c_cflag)) {
+			spin_lock_irq(&port->lock);
+			pl010_disable_ms(port);
+			spin_unlock_irq(&port->lock);
+		}
+	}
 }
 
 static const char *pl010_type(struct uart_port *port)

drivers/tty/serial/atmel_serial.c +5 -0

index b68dfa5..21380b6 100644
@@ -2051,6 +2051,11 @@ static void atmel_set_ldisc(struct uart_port *port, struct ktermios *termios)
 		spin_unlock_irq(&port->lock);
 	} else {
 		port->flags &= ~UPF_HARDPPS_CD;
+		if (!UART_ENABLE_MS(port, termios->c_cflag)) {
+			spin_lock_irq(&port->lock);
+			atmel_disable_ms(port);
+			spin_unlock_irq(&port->lock);
+		}
 	}
 }

Signature · 1 line

2.1.3