Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
> - 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Mon, Jan 14, 2008 at 01:40:16PM +1100, [EMAIL PROTECTED] 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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Mon, Jan 14, 2008 at 12:49:59AM +, 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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Mon 2008-01-14 00:29:12, Russell King wrote: > On Fri, Jan 11, 2008 at 10:17:21AM +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Mon 2008-01-14 00:29:12, Russell King wrote: On Fri, Jan 11, 2008 at 10:17:21AM +, 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Mon, Jan 14, 2008 at 12:49:59AM +, 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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Mon, Jan 14, 2008 at 01:40:16PM +1100, [EMAIL PROTECTED] 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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
- 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
> > 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 ? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Fri, Jan 11, 2008 at 10:17:21AM +, 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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Fri, 2008-01-11 at 10:17 +, 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 <[EMAIL PROTECTED]> >> >> 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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 <[EMAIL PROTECTED]> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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 [EMAIL PROTECTED] 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Fri, 2008-01-11 at 10:17 +, 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Fri, Jan 11, 2008 at 10:17:21AM +, 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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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 ? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
> > 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! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Tue, 8 Jan 2008 11:57:03 + Russell King <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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 <[EMAIL PROTECTED]> 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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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 [EMAIL PROTECTED] 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 kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
On Tue, 8 Jan 2008 11:57:03 + Russell King [EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH: 2/2] [SERIAL] avoid stalling suspend if serial port won't drain
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! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/