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

[nigel@suspend2.net]

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