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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <478A95DB.80407@suspend2.net>
Date:	Mon, 14 Jan 2008 09:51:07 +1100
From:	nigel@...pend2.net
To:	Pavel Machek <pavel@....cz>
CC:	Russell King <rmk+lkml@....linux.org.uk>,
	Linux Kernel List <linux-kernel@...r.kernel.org>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't
 drain

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@....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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/