On 30 November 2010 14:46, Stanislav Brabec <sbrabec@xxxxxxx> wrote:
> You cannot do it. There may exist a hardware, where resume depends on
> suspend previously called.
I don't buy that. Each step in the resume process is a primitive
operation such as "enable TX/RX" or setting of very specific
parameters.
If that's the case, I think its a driver bug. Or could be called one.
And looking at this in more detail, I think ca2e71aa8 is invalid.
Everything called inside the code block which is now disabled for
no_console_suspend is either safe to be called when there's "nothing
to do", or is so primitive that you can't imagine it causing any
failures. In other words, the serial driver for the platform on which
that platform was written probably has a bug, or could be trivially
modified to solve this issue without breaking the rest of the world.
But, even when that's done, even though it looks like we'd all be
happy for now, the story is not clear (and would probably lead to more
confused patch turmoil down the line).
We're acknowledging that platforms like XO-1 and zaurus have their
serial power cut in suspend. Therefore we should expect to do a
full-on reinitialization during resume, regardless of
no_console_suspend. But, that full-on reinit is trapped by this:
if (port->flags & ASYNC_SUSPENDED) {
and this flag is never set when no_console_suspend is set. I suspect
that a proper fix should make that full-on reinit sequence be executed
even when no_console_suspend is in place, to allow for platforms like
XO-1 and Zaurus.
So we have these 2 competing and confusing resume blocks of code which
don't flow well.
I tried to take a step back and make a more thorough approach at a
solution. My work in progress is attached. But I feel quite confused
and don't have much confidence that this is correct (since its my
first time touching this layer, and I know very little about
serial/tty/console stuff anyway).
I don't have time to continue with this, here's hoping that someone
else will pick it up.
Daniel
From fe77ab4d8e7c71a97223b7d03be28232148f6de5 Mon Sep 17 00:00:00 2001
From: Daniel Drake <dsd@xxxxxxxxxx>
Date: Fri, 3 Dec 2010 14:16:47 +0000
Subject: [PATCH] WIP: Fix serial console suspend/resume mess
The problems/challenges seem to be:
- On resume, restoring the right termios for the userspace getty
(if applicable)
- On resume, restoring the right termios for kernel console
(when there was no getty) (if applicable)
- Platforms behave differently. Some of them seem to cut power to the
serial chip when going into suspend, and some don't.
My approach is:
1. When configuring uart as a kernel, save the parameters somewhere where
serial_core can access them easily later (e.g. during resume).
Results in API changes that will have to be applied to all serial drivers.
2. uart_change_speed, already the "set the right termios for my configuration" function, is modified to work without a tty. In the case of there being no tty, it will apply the termios relevant to the kernel console configuration (if there was one).
3. uart_port_resume unconditionally does the resume/initialization (regardless if it was suspended or not on the way down)
TODO:
- understand more about serial and the linux implementation so that I can
be confident that this is a good approach
- figure out how to handle CONSOLE_POLL support
- test 16 configurations: 4 base cases: kernel console, kernel console + getty,
getty only, none. Each one tested with and without no_console_suspend, on
both XO-1 and XO-1.5 (XO-1 cuts serial chip power in suspend, XO-1.5 doesnt)
---
drivers/serial/8250.c | 9 +--
drivers/serial/altera_uart.c | 11 +--
drivers/serial/serial_core.c | 269 ++++++++++++++++++++++++------------------
include/linux/serial_core.h | 9 ++
4 files changed, 168 insertions(+), 130 deletions(-)
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 4d8e14b..ececf66 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2839,10 +2839,6 @@ serial8250_console_write(struct console *co, const char *s, unsigned int count)
static int __init serial8250_console_setup(struct console *co, char *options)
{
struct uart_port *port;
- int baud = 9600;
- int bits = 8;
- int parity = 'n';
- int flow = 'n';
/*
* Check whether an invalid uart number has been specified, and
@@ -2855,10 +2851,7 @@ static int __init serial8250_console_setup(struct console *co, char *options)
if (!port->iobase && !port->membase)
return -ENODEV;
- if (options)
- uart_parse_options(options, &baud, &parity, &bits, &flow);
-
- return uart_set_options(port, co, baud, parity, bits, flow);
+ return uart_setup_console(port, co, options, 9600, 'n', 8, 'n');
}
static int serial8250_console_early_setup(void)
diff --git a/drivers/serial/altera_uart.c b/drivers/serial/altera_uart.c
index 7212162..cecebeb 100644
--- a/drivers/serial/altera_uart.c
+++ b/drivers/serial/altera_uart.c
@@ -453,10 +453,6 @@ static void altera_uart_console_write(struct console *co, const char *s,
static int __init altera_uart_console_setup(struct console *co, char *options)
{
struct uart_port *port;
- int baud = CONFIG_SERIAL_ALTERA_UART_BAUDRATE;
- int bits = 8;
- int parity = 'n';
- int flow = 'n';
if (co->index < 0 || co->index >= CONFIG_SERIAL_ALTERA_UART_MAXPORTS)
return -EINVAL;
@@ -464,10 +460,9 @@ static int __init altera_uart_console_setup(struct console *co, char *options)
if (!port->membase)
return -ENODEV;
- if (options)
- uart_parse_options(options, &baud, &parity, &bits, &flow);
-
- return uart_set_options(port, co, baud, parity, bits, flow);
+ return uart_setup_console(port, co, options,
+ CONFIG_SERIAL_ALTERA_UART_BAUDRATE,
+ 'n', 8, 'n');
}
static struct uart_driver altera_uart_driver;
diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 9ffa5be..fd3e9cb 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -57,8 +57,7 @@ static struct lock_class_key port_lock_key;
#define uart_console(port) (0)
#endif
-static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
- struct ktermios *old_termios);
+static void uart_change_speed(struct uart_state *state);
static void __uart_wait_until_sent(struct uart_port *port, int timeout);
static void uart_change_pm(struct uart_state *state, int pm_state);
@@ -176,7 +175,7 @@ static int uart_startup(struct tty_struct *tty, struct uart_state *state, int in
/*
* Initialise the hardware port settings.
*/
- uart_change_speed(tty, state, NULL);
+ uart_change_speed(state);
/*
* Setup the RTS and DTR signals once the
@@ -427,22 +426,100 @@ uart_get_divisor(struct uart_port *port, unsigned int baud)
EXPORT_SYMBOL(uart_get_divisor);
+struct baud_rates {
+ unsigned int rate;
+ unsigned int cflag;
+};
+
+static const struct baud_rates baud_rates[] = {
+ { 921600, B921600 },
+ { 460800, B460800 },
+ { 230400, B230400 },
+ { 115200, B115200 },
+ { 57600, B57600 },
+ { 38400, B38400 },
+ { 19200, B19200 },
+ { 9600, B9600 },
+ { 4800, B4800 },
+ { 2400, B2400 },
+ { 1200, B1200 },
+ { 0, B38400 }
+};
+
+
+
+/* FIXME doc */
+static void
+uart_options_to_termios(struct ktermios *termios, int baud, int parity,
+ int bits, int flow)
+{
+ int i;
+
+ memset(termios, 0, sizeof(struct ktermios));
+
+ termios->c_cflag = CREAD | HUPCL | CLOCAL;
+
+ /*
+ * Construct a cflag setting.
+ */
+ for (i = 0; baud_rates[i].rate; i++)
+ if (baud_rates[i].rate <= baud)
+ break;
+
+ termios->c_cflag |= baud_rates[i].cflag;
+
+ if (bits == 7)
+ termios->c_cflag |= CS7;
+ else
+ termios->c_cflag |= CS8;
+
+ switch (parity) {
+ case 'o': case 'O':
+ termios->c_cflag |= PARODD;
+ /*fall through*/
+ case 'e': case 'E':
+ termios->c_cflag |= PARENB;
+ break;
+ }
+
+ if (flow == 'r')
+ termios->c_cflag |= CRTSCTS;
+}
+
+/* FIXME doc */
+static int
+uart_get_console_termios(struct uart_port *port, struct ktermios *termios)
+{
+ if (!port->is_console)
+ return 1;
+ uart_options_to_termios(termios, port->console_baud,
+ port->console_parity, port->console_bits,
+ port->console_flow);
+ return 0;
+}
+
/* FIXME: Consistent locking policy */
-static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
- struct ktermios *old_termios)
+static void uart_change_speed(struct uart_state *state)
{
struct tty_port *port = &state->port;
+ struct tty_struct *tty = port->tty;
struct uart_port *uport = state->uart_port;
struct ktermios *termios;
+ struct ktermios console_termios;
- /*
- * If we have no tty, termios, or the port does not exist,
- * then we can't set the parameters for this port.
- */
- if (!tty || !tty->termios || uport->type == PORT_UNKNOWN)
+ if (uport->type == PORT_UNKNOWN)
return;
- termios = tty->termios;
+ if (tty && tty->termios) {
+ /* if we have a tty, use its termios */
+ termios = tty->termios;
+ } else {
+ /* if we have a kernel console configuration, use its termios */
+ int ret = uart_get_console_termios(uport, &console_termios);
+ if (ret)
+ return; /* no console termios either */
+ termios = &console_termios;
+ }
/*
* Set flags based on termios cflag
@@ -457,7 +534,7 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
else
set_bit(ASYNCB_CHECK_CD, &port->flags);
- uport->ops->set_termios(uport, termios, old_termios);
+ uport->ops->set_termios(uport, termios, NULL);
}
static inline int __uart_put_char(struct uart_port *port,
@@ -866,7 +943,7 @@ static int uart_set_info(struct tty_struct *tty, struct uart_state *state,
"is deprecated.\n", current->comm,
tty_name(port->tty, buf));
}
- uart_change_speed(tty, state, NULL);
+ uart_change_speed(state);
}
} else
retval = uart_startup(tty, state, 1);
@@ -1214,7 +1291,7 @@ static void uart_set_termios(struct tty_struct *tty,
return;
}
- uart_change_speed(tty, state, old_termios);
+ uart_change_speed(state);
/* Handle transition to B0 status */
if ((old_termios->c_cflag & CBAUD) && !(cflag & CBAUD))
@@ -1499,7 +1576,7 @@ static void uart_update_termios(struct tty_struct *tty,
/*
* Make termios settings take effect.
*/
- uart_change_speed(tty, state, NULL);
+ uart_change_speed(state);
/*
* And finally enable the RTS and DTR signals.
@@ -1851,26 +1928,6 @@ uart_parse_options(char *options, int *baud, int *parity, int *bits, int *flow)
}
EXPORT_SYMBOL_GPL(uart_parse_options);
-struct baud_rates {
- unsigned int rate;
- unsigned int cflag;
-};
-
-static const struct baud_rates baud_rates[] = {
- { 921600, B921600 },
- { 460800, B460800 },
- { 230400, B230400 },
- { 115200, B115200 },
- { 57600, B57600 },
- { 38400, B38400 },
- { 19200, B19200 },
- { 9600, B9600 },
- { 4800, B4800 },
- { 2400, B2400 },
- { 1200, B1200 },
- { 0, B38400 }
-};
-
/**
* uart_set_options - setup the serial console parameters
* @port: pointer to the serial ports uart_port structure
@@ -1886,7 +1943,6 @@ uart_set_options(struct uart_port *port, struct console *co,
{
struct ktermios termios;
static struct ktermios dummy;
- int i;
/*
* Ensure that the serial console lock is initialised
@@ -1895,35 +1951,7 @@ uart_set_options(struct uart_port *port, struct console *co,
spin_lock_init(&port->lock);
lockdep_set_class(&port->lock, &port_lock_key);
- memset(&termios, 0, sizeof(struct ktermios));
-
- termios.c_cflag = CREAD | HUPCL | CLOCAL;
-
- /*
- * Construct a cflag setting.
- */
- for (i = 0; baud_rates[i].rate; i++)
- if (baud_rates[i].rate <= baud)
- break;
-
- termios.c_cflag |= baud_rates[i].cflag;
-
- if (bits == 7)
- termios.c_cflag |= CS7;
- else
- termios.c_cflag |= CS8;
-
- switch (parity) {
- case 'o': case 'O':
- termios.c_cflag |= PARODD;
- /*fall through*/
- case 'e': case 'E':
- termios.c_cflag |= PARENB;
- break;
- }
-
- if (flow == 'r')
- termios.c_cflag |= CRTSCTS;
+ uart_options_to_termios(&termios, baud, parity, bits, flow);
/*
* some uarts on other side don't support no flow control.
@@ -1942,6 +1970,32 @@ uart_set_options(struct uart_port *port, struct console *co,
return 0;
}
EXPORT_SYMBOL_GPL(uart_set_options);
+
+// FIXME: drop co argument, should be able to figure it out from port ?
+int
+uart_setup_console(struct uart_port *port, struct console *co, char *options,
+ int baud, int parity, int bits, int flow)
+{
+ // FIXME: kernel probably already knows tihs somehow? no need to add new
+ // flag
+ port->is_console = true;
+ if (options)
+ uart_parse_options(options, &baud, &parity, &bits, &flow);
+ port->console_baud = baud;
+ port->console_parity = parity;
+ port->console_bits = bits;
+ port->console_flow = flow;
+
+ /*
+ * some uarts on other side don't support no flow control.
+ * So we set * DTR in host uart to make them happy
+ */
+ port->mctrl |= TIOCM_DTR;
+
+ return 0;
+ //return do_uart_setup_console(port, co);
+}
+EXPORT_SYMBOL_GPL(uart_setup_console);
#endif /* CONFIG_SERIAL_CORE_CONSOLE */
static void uart_change_pm(struct uart_state *state, int pm_state)
@@ -1998,7 +2052,6 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
int tries;
if (console_suspend_enabled || !uart_console(uport)) {
- set_bit(ASYNCB_SUSPENDED, &port->flags);
clear_bit(ASYNCB_INITIALIZED, &port->flags);
spin_lock_irq(&uport->lock);
@@ -2045,8 +2098,11 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
struct tty_port *port = &state->port;
struct device *tty_dev;
struct uart_match match = {uport, drv};
- struct ktermios termios;
+ struct tty_struct *tty = port->tty;
+ const struct uart_ops *ops = uport->ops;
+ int ret;
+ /* Protected by port mutex for now */
mutex_lock(&port->mutex);
tty_dev = device_find_child(uport->dev, &match, serial_match_port);
@@ -2055,62 +2111,44 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
mutex_unlock(&port->mutex);
return 0;
}
- uport->suspended = 0;
/*
- * Re-enable the console device after suspending.
+ * Even though we might not have suspended/shutdown the port in
+ * uart_suspend_port() (because of no_console_suspend), various
+ * platforms cut power to this chip during suspend, so we do a full
+ * unconditional resume.
*/
- if (console_suspend_enabled && uart_console(uport)) {
- /*
- * First try to use the console cflag setting.
- */
- memset(&termios, 0, sizeof(struct ktermios));
- termios.c_cflag = uport->cons->cflag;
+ uport->suspended = 0;
+ uart_change_pm(state, 0);
+
+ spin_lock_irq(&uport->lock);
+ ops->set_mctrl(uport, 0);
+ spin_unlock_irq(&uport->lock);
+ ret = ops->startup(uport);
+ if (!ret) {
/*
- * If that's unset, use the tty termios setting.
+ * Failed to resume - maybe hardware went away?
+ * Clear the "initialized" flag so we won't try
+ * to call the low level drivers shutdown method.
*/
- if (port->tty && port->tty->termios && termios.c_cflag == 0)
- termios = *(port->tty->termios);
-
- uart_change_pm(state, 0);
- uport->ops->set_termios(uport, &termios, NULL);
- console_start(uport->cons);
+ uart_shutdown(tty, state);
+ goto out;
}
- if (port->flags & ASYNC_SUSPENDED) {
- const struct uart_ops *ops = uport->ops;
- int ret;
-
- uart_change_pm(state, 0);
- spin_lock_irq(&uport->lock);
- ops->set_mctrl(uport, 0);
- spin_unlock_irq(&uport->lock);
- if (console_suspend_enabled || !uart_console(uport)) {
- /* Protected by port mutex for now */
- struct tty_struct *tty = port->tty;
- ret = ops->startup(uport);
- if (ret == 0) {
- if (tty)
- uart_change_speed(tty, state, NULL);
- spin_lock_irq(&uport->lock);
- ops->set_mctrl(uport, uport->mctrl);
- ops->start_tx(uport);
- spin_unlock_irq(&uport->lock);
- set_bit(ASYNCB_INITIALIZED, &port->flags);
- } else {
- /*
- * Failed to resume - maybe hardware went away?
- * Clear the "initialized" flag so we won't try
- * to call the low level drivers shutdown method.
- */
- uart_shutdown(tty, state);
- }
- }
+ uart_change_speed(state); /* sets termios appropriately */
+ spin_lock_irq(&uport->lock);
+ ops->set_mctrl(uport, uport->mctrl);
+ ops->start_tx(uport);
+ spin_unlock_irq(&uport->lock);
+ set_bit(ASYNCB_INITIALIZED, &port->flags);
- clear_bit(ASYNCB_SUSPENDED, &port->flags);
- }
+ if (uart_console(uport))
+ console_start(uport->cons);
+ else
+ uart_change_pm(state, 3);
+out:
mutex_unlock(&port->mutex);
return 0;
@@ -2199,8 +2237,9 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
* successfully registered yet, try to re-register it.
* It may be that the port was not available.
*/
- if (port->cons && !(port->cons->flags & CON_ENABLED))
+ if (port->cons && !(port->cons->flags & CON_ENABLED)) {
register_console(port->cons);
+ }
/*
* Power down all ports by default, except the
@@ -2208,6 +2247,8 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
*/
if (!uart_console(port))
uart_change_pm(state, 3);
+ else
+ uart_change_speed(state);
}
}
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 212eb4c..67c1dc2 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -362,6 +362,13 @@ struct uart_port {
unsigned char hub6; /* this should be in the 8250 driver */
unsigned char suspended;
unsigned char unused[2];
+
+ bool is_console;
+ int console_baud;
+ int console_bits;
+ int console_parity;
+ int console_flow;
+
void *private_data; /* generic platform data pointer */
};
@@ -433,6 +440,8 @@ void uart_parse_options(char *options, int *baud, int *parity, int *bits,
int *flow);
int uart_set_options(struct uart_port *port, struct console *co, int baud,
int parity, int bits, int flow);
+int uart_setup_console(struct uart_port *port, struct console *co,
+ char *options, int baud, int parity, int bits, int flow);
struct tty_driver *uart_console_device(struct console *co, int *index);
void uart_console_write(struct uart_port *port, const char *s,
unsigned int count,
--
1.7.3.2
