[1/3] serial: pl011: safeguard against impossible baudrates

Commit Message

Linus Walleij Sept. 20, 2012, 9:45 a.m. UTC

From: Linus Walleij <[e-mail]>

The PL011 generates it's bitrate from the clock to the block,
and this in turn will be divided by a minimum of 16 (ARM PL011)
or 8 (ST Micro PL011 derivate). Safeguard against illegal
rates exceeding that of what the port clock can be divided
down to.

Since the clkdiv variable is only ever used for calculating the
maximum baud rate, let's skip that variable and add a new
max_baud variable instead, then use this to check limits and
as parameter for getting the baud rate.

Cc: Par-Gunnar Hjalmdahl <[e-mail]>
Cc: Guillaume Jaunet <[e-mail]>
Cc: Christophe Arnal <[e-mail]>
Cc: Matthias Locher <[e-mail]>
Cc: Rajanikanth H.V <[e-mail]>
Signed-off-by: Linus Walleij <[e-mail]>
---
 drivers/tty/serial/amba-pl011.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Russell King - ARM Linux Sept. 20, 2012, 6:57 p.m. UTC | #1

On Thu, Sep 20, 2012 at 11:45:46AM +0200, Linus Walleij wrote:
> +	if ((termios->c_ispeed > max_baud) ||
> +	    (termios->c_ospeed > max_baud)) {
> +		dev_err(port->dev,
> +			"requested a baud rate > clock/mindivisor\n");
> +		return;
> +	}

Why do we need this check?  In any case, this is incorrect behaviour
just to 'return' from this function without doing anything.  You just
produce an error message, and userspace believes that it's request
has been satisfied.

uart_get_baud_rate() is there precisely to do the right thing for invalid
baud rates, which is to fallback to the old termios setting (and update
the new termios with that) if the old termios setting satisifies the
limits, if not it falls back to something within the limited range.

Linus Walleij Sept. 21, 2012, 8:26 a.m. UTC | #2

On Thu, Sep 20, 2012 at 8:57 PM, Russell King - ARM Linux
<[e-mail]> wrote:

> uart_get_baud_rate() is there precisely to do the right thing for invalid
> baud rates, which is to fallback to the old termios setting (and update
> the new termios with that) if the old termios setting satisifies the
> limits, if not it falls back to something within the limited range.

Yeah, hm, passing 4800000 as max rate to this function and
requesting 4050000 returns 4000000... (Other high baud rates
such as 3250000 or 3000000 works fine) so there is some fishy
bug in there we're just duct-taping around, I'll go hunt the real bug
instead.

Thanks!
Linus Walleij

Patch

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index d626d84..9e16ea6 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1492,18 +1492,25 @@  pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 	struct uart_amba_port *uap = (struct uart_amba_port *)port;
 	unsigned int lcr_h, old_cr;
 	unsigned long flags;
-	unsigned int baud, quot, clkdiv;
+	unsigned int max_baud, baud, quot;
 
 	if (uap->vendor->oversampling)
-		clkdiv = 8;
+		max_baud = port->uartclk / 8;
 	else
-		clkdiv = 16;
+		max_baud = port->uartclk / 16;
+
+	if ((termios->c_ispeed > max_baud) ||
+	    (termios->c_ospeed > max_baud)) {
+		dev_err(port->dev,
+			"requested a baud rate > clock/mindivisor\n");
+		return;
+	}
 
 	/*
 	 * Ask the core to calculate the divisor for us.
 	 */
 	baud = uart_get_baud_rate(port, termios, old, 0,
-				  port->uartclk / clkdiv);
+				  max_baud);
 
 	if (baud > port->uartclk/16)
 		quot = DIV_ROUND_CLOSEST(port->uartclk * 8, baud);