[PATCH: 1/2] [SERIAL] avoid waking up closed serial ports on resume

[PATCH: 1/2] [SERIAL] avoid waking up closed serial ports on resume

[Russell King]

When we boot, serial ports remain in low power mode until they're
used either by userspace or for the kernel console.

However, if you suspend the system, and then resume, all serial ports
will be taken out of low power mode.  This is bad news for embedded
devices where this can mean higher power consumption.

Only bring a serial port out of low power mode if the port is being
used as the kernel console, or is in use by userspace.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 3bb5d24..80486c6 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2029,8 +2029,6 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *port)
 	}
 	port->suspended = 0;
 
-	uart_change_pm(state, 0);
-
 	/*
 	 * Re-enable the console device after suspending.
 	 */
@@ -2049,6 +2047,7 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *port)
 		if (state->info && state->info->tty && termios.c_cflag == 0)
 			termios = *state->info->tty->termios;
 
+		uart_change_pm(state, 0);
 		port->ops->set_termios(port, &termios, NULL);
 		console_start(port->cons);
 	}
@@ -2057,6 +2056,7 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *port)
 		const struct uart_ops *ops = port->ops;
 		int ret;
 
+		uart_change_pm(state, 0);
 		ops->set_mctrl(port, 0);
 		ret = ops->startup(port);
 		if (ret == 0) {

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Russell King]

Some ports seem to be unable to drain their transmitters on shut down.
Such a problem can occur if the port is programmed for hardware imposed
flow control, characters are in the FIFO but the CTS signal is inactive.

Normally, this isn't a problem because most places where we wait for the
transmitter to drain have a time-out.  However, there is no timeout in
the suspend path.

Give a port 30ms to drain; this is an arbitary value chosen to avoid
long delays if there are many such ports in the system, while giving a
reasonable chance for a single port to drain.  Should a port not drain
within this timeout, issue a warning.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 3bb5d24..4ce219c 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -1977,6 +1977,7 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *port)
 
 	if (state->info && state->info->flags & UIF_INITIALIZED) {
 		const struct uart_ops *ops = port->ops;
+		int tries;
 
 		state->info->flags = (state->info->flags & ~UIF_INITIALIZED)
 				     | UIF_SUSPENDED;
@@ -1990,9 +1991,14 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *port)
 		/*
 		 * Wait for the transmitter to empty.
 		 */
-		while (!ops->tx_empty(port)) {
+		for (tries = 3; !ops->tx_empty(port) && tries; tries--) {
 			msleep(10);
 		}
+		if (!tries)
+			printk(KERN_ERR "%s%s%s%d: Unable to drain transmitter\n",
+			       port->dev ? port->dev->bus_id : "",
+			       port->dev ? ": " : "",
+			       drv->dev_name, port->line);
 
 		ops->shutdown(port);
 	}


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Andrew Morton]

On Tue, 8 Jan 2008 11:57:03 +0000
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> Some ports seem to be unable to drain their transmitters on shut down.
> Such a problem can occur if the port is programmed for hardware imposed
> flow control, characters are in the FIFO but the CTS signal is inactive.
> 
> Normally, this isn't a problem because most places where we wait for the
> transmitter to drain have a time-out.  However, there is no timeout in
> the suspend path.
> 
> Give a port 30ms to drain; this is an arbitary value chosen to avoid
> long delays if there are many such ports in the system, while giving a
> reasonable chance for a single port to drain.  Should a port not drain
> within this timeout, issue a warning.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
> index 3bb5d24..4ce219c 100644
> --- a/drivers/serial/serial_core.c
> +++ b/drivers/serial/serial_core.c
> @@ -1977,6 +1977,7 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *port)
>  
>  	if (state->info && state->info->flags & UIF_INITIALIZED) {
>  		const struct uart_ops *ops = port->ops;
> +		int tries;
>  
>  		state->info->flags = (state->info->flags & ~UIF_INITIALIZED)
>  				     | UIF_SUSPENDED;
> @@ -1990,9 +1991,14 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *port)
>  		/*
>  		 * Wait for the transmitter to empty.
>  		 */
> -		while (!ops->tx_empty(port)) {
> +		for (tries = 3; !ops->tx_empty(port) && tries; tries--) {
>  			msleep(10);
>  		}
> +		if (!tries)
> +			printk(KERN_ERR "%s%s%s%d: Unable to drain transmitter\n",
> +			       port->dev ? port->dev->bus_id : "",
> +			       port->dev ? ": " : "",
> +			       drv->dev_name, port->line);
>  
>  		ops->shutdown(port);
>  	}
> 

One hopes that doing a printk from within uart_suspend_port() will dtrt if
that port is (was?) being used as a console.

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Alan Cox]

> > Give a port 30ms to drain; this is an arbitary value chosen to avoid
> > long delays if there are many such ports in the system, while giving a
> > reasonable chance for a single port to drain.  Should a port not drain
> > within this timeout, issue a warning.

30mS is a bit short at low baud rates - could we set the timeout based on
baud + fifo size ?

ACK anyway - its clearly better than hanging if hardware flow control has
halted the uart!

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Russell King]

On Tue, Jan 08, 2008 at 04:06:10PM -0800, Andrew Morton wrote:
> One hopes that doing a printk from within uart_suspend_port() will dtrt if
> that port is (was?) being used as a console.

Well, the preceding code shuts down the console side if the port is a
console - so this printk should never make it back to the UART in that
case.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Pavel Machek]

On Tue 2008-01-08 11:57:03, Russell King wrote:
> Some ports seem to be unable to drain their transmitters on shut down.
> Such a problem can occur if the port is programmed for hardware imposed
> flow control, characters are in the FIFO but the CTS signal is inactive.
> 
> Normally, this isn't a problem because most places where we wait for the
> transmitter to drain have a time-out.  However, there is no timeout in
> the suspend path.
> 
> Give a port 30ms to drain; this is an arbitary value chosen to avoid
> long delays if there are many such ports in the system, while giving a
> reasonable chance for a single port to drain.  Should a port not drain
> within this timeout, issue a warning.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
> index 3bb5d24..4ce219c 100644
> --- a/drivers/serial/serial_core.c
> +++ b/drivers/serial/serial_core.c
> @@ -1977,6 +1977,7 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *port)
>  
>  	if (state->info && state->info->flags & UIF_INITIALIZED) {
>  		const struct uart_ops *ops = port->ops;
> +		int tries;
>  
>  		state->info->flags = (state->info->flags & ~UIF_INITIALIZED)
>  				     | UIF_SUSPENDED;
> @@ -1990,9 +1991,14 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *port)
>  		/*
>  		 * Wait for the transmitter to empty.
>  		 */
> -		while (!ops->tx_empty(port)) {
> +		for (tries = 3; !ops->tx_empty(port) && tries; tries--) {
>  			msleep(10);
>  		}
> +		if (!tries)
> +			printk(KERN_ERR "%s%s%s%d: Unable to drain transmitter\n",
> +			       port->dev ? port->dev->bus_id : "",
> +			       port->dev ? ": " : "",
> +			       drv->dev_name, port->line);
>  
>  		ops->shutdown(port);
>  	}
> 

Is printk() enough for 'we've just lost your data' condition? Maybe we
should abort suspend if we can't drain fifo?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[nigel]

Hi.

Pavel Machek wrote:
> On Tue 2008-01-08 11:57:03, Russell King wrote:
>> Some ports seem to be unable to drain their transmitters on shut down.
>> Such a problem can occur if the port is programmed for hardware imposed
>> flow control, characters are in the FIFO but the CTS signal is inactive.
>>
>> Normally, this isn't a problem because most places where we wait for the
>> transmitter to drain have a time-out.  However, there is no timeout in
>> the suspend path.
>>
>> Give a port 30ms to drain; this is an arbitary value chosen to avoid
>> long delays if there are many such ports in the system, while giving a
>> reasonable chance for a single port to drain.  Should a port not drain
>> within this timeout, issue a warning.
>>
>> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
>>
>> diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
>> index 3bb5d24..4ce219c 100644
>> --- a/drivers/serial/serial_core.c
>> +++ b/drivers/serial/serial_core.c
>> @@ -1977,6 +1977,7 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *port)
>>  
>>  	if (state->info && state->info->flags & UIF_INITIALIZED) {
>>  		const struct uart_ops *ops = port->ops;
>> +		int tries;
>>  
>>  		state->info->flags = (state->info->flags & ~UIF_INITIALIZED)
>>  				     | UIF_SUSPENDED;
>> @@ -1990,9 +1991,14 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *port)
>>  		/*
>>  		 * Wait for the transmitter to empty.
>>  		 */
>> -		while (!ops->tx_empty(port)) {
>> +		for (tries = 3; !ops->tx_empty(port) && tries; tries--) {
>>  			msleep(10);
>>  		}
>> +		if (!tries)
>> +			printk(KERN_ERR "%s%s%s%d: Unable to drain transmitter\n",
>> +			       port->dev ? port->dev->bus_id : "",
>> +			       port->dev ? ": " : "",
>> +			       drv->dev_name, port->line);
>>  
>>  		ops->shutdown(port);
>>  	}
>>
> 
> Is printk() enough for 'we've just lost your data' condition? Maybe we
> should abort suspend if we can't drain fifo?

No way. Think about this from a users' perspective. No one wants suspend
to ram or hibernate functionality that works sometimes and not others.
They want it to work reliably so they don't have to worry about their
laptop overheating while they're getting on the bus or airplane.
Aborting isn't an option.

Regards,

Nigel

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Alan Cox]

> > Is printk() enough for 'we've just lost your data' condition? Maybe we
> > should abort suspend if we can't drain fifo?
> 
> No way. Think about this from a users' perspective. No one wants suspend
> to ram or hibernate functionality that works sometimes and not others.
> They want it to work reliably so they don't have to worry about their
> laptop overheating while they're getting on the bus or airplane.
> Aborting isn't an option.

Dumb question on the printk however - what if the port that is sticking
is the console - don't we recurse and die ?

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[nigel]

Hi.

Alan Cox wrote:
>>> Is printk() enough for 'we've just lost your data' condition? Maybe we
>>> should abort suspend if we can't drain fifo?
>> No way. Think about this from a users' perspective. No one wants suspend
>> to ram or hibernate functionality that works sometimes and not others.
>> They want it to work reliably so they don't have to worry about their
>> laptop overheating while they're getting on the bus or airplane.
>> Aborting isn't an option.
> 
> Dumb question on the printk however - what if the port that is sticking
> is the console - don't we recurse and die ?

I don't know, but I'd argue that we shouldn't die. Things should be as
robust as possible.

Regards,

Nigel

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Russell King]

On Mon, Jan 14, 2008 at 01:40:16PM +1100, nigel@suspend2.net wrote:
> Hi.
> 
> Alan Cox wrote:
> >>> Is printk() enough for 'we've just lost your data' condition? Maybe we
> >>> should abort suspend if we can't drain fifo?
> >> No way. Think about this from a users' perspective. No one wants suspend
> >> to ram or hibernate functionality that works sometimes and not others.
> >> They want it to work reliably so they don't have to worry about their
> >> laptop overheating while they're getting on the bus or airplane.
> >> Aborting isn't an option.
> > 
> > Dumb question on the printk however - what if the port that is sticking
> > is the console - don't we recurse and die ?
> 
> I don't know, but I'd argue that we shouldn't die. Things should be as
> robust as possible.

Of course we should never die.  That's precisely what I'm trying to work
towards here.

Currently without this patch, various platforms I have here do precisely
that - you ask them to suspend and they shut down the majority of devices
and then die.  The recovery is either system reboot or a power cycle -
especially when the port in question is connected to something other than
an external serial port (eg, a serial port for connection to a non-existent
bluetooth module.)

While we're talking about robustness, since the serial wakeup support has
gone in, another couple of issues have appeared:

1. We no longer suspend ports marked as wakeup sources.  That means we
   never place them into a low power state (which might be required by
   hardware) - we need a flag from the driver or something to indicate
   that.

2. As a direct consequence, we no longer re-initialise the port at resume
   time, resulting in a completely deconfigured but open port.  Such a
   port may be the system console, which will cause any printks may be
   damned slow and the output will be garbage.

(2) is quite a serious problem for ARM platforms - you lose the console
entirely and you also lose control of the system.  Again, recovery from
such events is by either a power cycle or system reboot.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Russell King]

On Mon, Jan 14, 2008 at 12:49:59AM +0000, Alan Cox wrote:
> > > Is printk() enough for 'we've just lost your data' condition? Maybe we
> > > should abort suspend if we can't drain fifo?
> > 
> > No way. Think about this from a users' perspective. No one wants suspend
> > to ram or hibernate functionality that works sometimes and not others.
> > They want it to work reliably so they don't have to worry about their
> > laptop overheating while they're getting on the bus or airplane.
> > Aborting isn't an option.
> 
> Dumb question on the printk however - what if the port that is sticking
> is the console - don't we recurse and die ?

How do we recurse?  printk never calls the suspend method.

What *might* happen is that printk might call the serial console write
function, which waits a maximum of 10ms per character to be sent without
hardware console flow control, or one second per character with hardware
console flow control.

This will only happen when there isn't something asserting CTS connected
to the console port and hardware console flow control has been configured
at boot time.

This will also happen in normal operation if you set the system up as
above and deassert CTS - there's nothing suspend specific about that.

Finally note that hardware console flow control is entirely separate from
the user-level flow control settings or the hardware's ability.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Benjamin Herrenschmidt]

On Fri, 2008-01-11 at 10:17 +0000, Pavel Machek wrote:
> Is printk() enough for 'we've just lost your data' condition? Maybe we
> should abort suspend if we can't drain fifo?

YUCK !

Ben.

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Russell King]

On Fri, Jan 11, 2008 at 10:17:21AM +0000, Pavel Machek wrote:
> On Tue 2008-01-08 11:57:03, Russell King wrote:
> > +		if (!tries)
> > +			printk(KERN_ERR "%s%s%s%d: Unable to drain transmitter\n",
> > +			       port->dev ? port->dev->bus_id : "",
> > +			       port->dev ? ": " : "",
> > +			       drv->dev_name, port->line);
> >  
> >  		ops->shutdown(port);
> >  	}
> > 
> 
> Is printk() enough for 'we've just lost your data' condition? Maybe we
> should abort suspend if we can't drain fifo?

That would mean that a port set for hardware flow control, with hardware
implemented flow control, has CTS deasserted, you'll never suspend.

If you're suspending because your battery is almost dead what would you
prefer - the system being prevented from suspending and losing complete
power unexpectedly, resulting in complete data loss, or losing the
characters in the serial port and suspending?

Which is the lesser of two evils?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Pavel Machek]

On Mon 2008-01-14 00:29:12, Russell King wrote:
> On Fri, Jan 11, 2008 at 10:17:21AM +0000, Pavel Machek wrote:
> > On Tue 2008-01-08 11:57:03, Russell King wrote:
> > > +		if (!tries)
> > > +			printk(KERN_ERR "%s%s%s%d: Unable to drain transmitter\n",
> > > +			       port->dev ? port->dev->bus_id : "",
> > > +			       port->dev ? ": " : "",
> > > +			       drv->dev_name, port->line);
> > >  
> > >  		ops->shutdown(port);
> > >  	}
> > > 
> > 
> > Is printk() enough for 'we've just lost your data' condition? Maybe we
> > should abort suspend if we can't drain fifo?
> 
> That would mean that a port set for hardware flow control, with hardware
> implemented flow control, has CTS deasserted, you'll never suspend.

Yep. 

> If you're suspending because your battery is almost dead what would you
> prefer - the system being prevented from suspending and losing complete
> power unexpectedly, resulting in complete data loss, or losing the
> characters in the serial port and suspending?
> 
> Which is the lesser of two evils?

Not sure, but there's third option -- correct but more work. What
about saving/restoring fifo state? That will not stall suspend, nor
will loose data.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Russell King]

On Mon, Jan 14, 2008 at 10:04:59AM +0100, Pavel Machek wrote:
> On Mon 2008-01-14 00:29:12, Russell King wrote:
> > If you're suspending because your battery is almost dead what would you
> > prefer - the system being prevented from suspending and losing complete
> > power unexpectedly, resulting in complete data loss, or losing the
> > characters in the serial port and suspending?
> > 
> > Which is the lesser of two evils?
> 
> Not sure, but there's third option -- correct but more work. What
> about saving/restoring fifo state? That will not stall suspend, nor
> will loose data.

The only way you could do that on 8250 is to clear the RX FIFO, enable
loopback, wait for the port to transmit all its characters into the RX
FIFO and read them out that way.

That brings up other questions though:

- what if its a port with 256 characters in the FIFO, and its programmed
  for 110 baud?
- what if loopback isn't supported?
- what if, while doing this operation, the remote end is sending characters
  because you can't deassert RTS?

Finally, on many non-8250 ports, you don't have a loopback mode to
enable, so you have no way to read back the contents of the FIFO.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain

[Alan Cox]

> - what if its a port with 256 characters in the FIFO, and its programmed
>   for 110 baud?
> - what if loopback isn't supported?
> - what if, while doing this operation, the remote end is sending characters
>   because you can't deassert RTS?

More importantly it is unlikely that serial state will or should survive
across a suspend/resume cycle this way. The chances are you modem did not
suspend and resume the phone call.

Alan