fix serial console suspend on PNP
Subsystems (3)
cover fix serial console suspend on PNP
Hi Greg,
The kernel command line parameter, no_console_suspend, enables the serial console to continue to output kernel message during system suspend/resume.
However, the PNP bus will disable and power-down pnp devices on system suspend, which defeats the purpose of no_console_suspend; namely to continue to output messages in case those are the last messages before crashing.
The first patch is a cleanup of the baroque logic of uart_suspend_port(). The second patch overrides the PNP bus disabling the console device if no_console_suspend is set. This patch uses existing PNP bus logic but perhaps in a way not originally intended.
The third patch requires Rafael's ack, as it adds a capability to allow the device to override the power-down function.
The fourth patch actually performs the power-down override enabled by patch 3.
With these patches, the PNP0501 device (16550A uart) will continue to output messages while system suspend finishes, as intended by the no_console_suspend parameter. [System resume is another problem, which I'm working on].
Regards,
Peter Hurley (4):
serial: core: Simplify console suspend logic in uart_suspend_port()
serial: 8250_pnp: Override pnp disable for no_console_suspend
PNP: Allow device to override ACPI device sleep
serial: 8250_pnp: Override pnp suspend for no_console_suspend
drivers/pnp/driver.c | 2 +- drivers/pnp/pnpacpi/core.c | 2 +- drivers/tty/serial/8250/8250_pnp.c | 61 +++++++++++++++++++++++++++++++------- drivers/tty/serial/serial_core.c | 35 +++++++++++----------- include/linux/pnp.h | 3 ++ 5 files changed, 72 insertions(+), 31 deletions(-)
Signature · 1 line
2.1.3
PATCH 1/4 serial: core: Simplify console suspend logic in uart_suspend_port()
When the uart port being suspended is a console and consoles are not suspending (kernel command line contains no_console_suspend), then no action is performed for that port, and the function can return early.
If the function has not returned early, then one of the conditions is not true, so the expression
(console_suspend_enabled || !uart_console(uport))
must be true and can be eliminated.
Similarly, the expression
(console_suspend_enabled && uart_console(uport))
simplifies to just uart_console(uport).
--- drivers/tty/serial/serial_core.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-)
drivers/tty/serial/serial_core.c +17 -18
@@ -1959,23 +1959,24 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport) }put_device (tty_dev); -if (console_suspend_enabled || !uart_console (uport)) - uport->suspended =1 ; +/* Nothing to do if the console is not suspending */ +if (!console_suspend_enabled &&uart_console (uport)) + goto unlock; + + uport->suspended =1 ;if (port->flags &ASYNC_INITIALIZED ) {const struct uart_ops *ops = uport->ops;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); - ops->stop_tx (uport); - ops->set_mctrl (uport,0 ); - ops->stop_rx (uport); -spin_unlock_irq (&uport->lock); - } +set_bit (ASYNCB_SUSPENDED , &port->flags); +clear_bit (ASYNCB_INITIALIZED , &port->flags); + +spin_lock_irq (&uport->lock); + ops->stop_tx (uport); + ops->set_mctrl (uport,0 ); + ops->stop_rx (uport); +spin_unlock_irq (&uport->lock);/* * Wait for the transmitter to empty.
@@ -1987,19 +1988,17 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport) drv->dev_name, drv->tty_driver->name_base + uport->line); -if (console_suspend_enabled || !uart_console (uport)) - ops->shutdown (uport); + ops->shutdown (uport); }/* * Disable the console device before suspending. */ -if (console_suspend_enabled &&uart_console (uport)) +if (uart_console (uport))console_stop (uport->cons); -if (console_suspend_enabled || !uart_console (uport)) -uart_change_pm (state,UART_PM_STATE_OFF ); - +uart_change_pm (state,UART_PM_STATE_OFF ); +unlock:mutex_unlock (&port->mutex);return 0 ;
Signature · 1 line
2.1.3
PATCH 2/4 serial: 8250_pnp: Override pnp disable for no_console_suspend
In reply to Peter HurleyWhen the kernel command line parameter, no_console_suspend, is used, the console should continue to output console messages. For a serial console, the serial core ensures that the device is not shutdown when no_console_suspend is specified. However, the pnp bus subsequently disables the device and no further output occurs.
When registering the 8250 port, override PNP_DISABLE if the serial device is a console and consoles are not suspending. Restore the PNP_DISABLE capability (if overriden) on device removal.
--- drivers/tty/serial/8250/8250_pnp.c | 45 ++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 11 deletions(-)
drivers/tty/serial/8250/8250_pnp.c +34 -11
@@ -18,11 +18,17 @@#include <linux/kernel.h> #include <linux/serial_core.h> #include <linux/bitops.h> +#include <linux/console.h> #include <asm/byteorder.h> #include "8250.h" +struct serial_pnp_info { +int line; +bool override_disable; +}; +#define UNKNOWN_DEV 0x3000#define CIR_PORT 0x0800
@@ -426,7 +432,8 @@ static int serial_pnp_guess_board(struct pnp_dev *dev)static int serial_pnp_probe (struct pnp_dev *dev,const struct pnp_device_id *dev_id) { -struct uart_8250_port uart; +struct uart_8250_port uart, *port8250; +struct serial_pnp_info *info;int ret, line, flags = dev_id->driver_data;if (flags &UNKNOWN_DEV ) {
@@ -435,6 +442,10 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)return ret; } + info =devm_kzalloc (&dev->dev,sizeof (*info),GFP_KERNEL ); +if (!info) +return -ENOMEM ; +memset (&uart,0 ,sizeof (uart));if (pnp_irq_valid (dev,0 )) uart.port.irq =pnp_irq (dev,0 );
@@ -471,35 +482,47 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)if (line <0 || (flags &CIR_PORT ))return -ENODEV ; -pnp_set_drvdata (dev, (void *)((long )line +1 )); + info->line = line; + port8250 =serial8250_get_port (line); + +if (uart_console (&port8250->port) && !console_suspend_enabled) { + info->override_disable = !!(dev->capabilities &PNP_DISABLE ); + dev->capabilities &= ~PNP_DISABLE ; + } + +pnp_set_drvdata (dev, info);return 0 ; }static void serial_pnp_remove (struct pnp_dev *dev) { -long line = (long )pnp_get_drvdata (dev); -if (line) -serial8250_unregister_port (line -1 ); +struct serial_pnp_info *info =pnp_get_drvdata (dev); + +if (info) { +serial8250_unregister_port (info->line); +if (info->override_disable) + dev->capabilities |=PNP_DISABLE ; + } }#ifdef CONFIG_PM static int serial_pnp_suspend (struct pnp_dev *dev,pm_message_t state) { -long line = (long )pnp_get_drvdata (dev); +struct serial_pnp_info *info =pnp_get_drvdata (dev); -if (!line) +if (!info)return -ENODEV ; -serial8250_suspend_port (line -1 ); +serial8250_suspend_port (info->line);return 0 ; }static int serial_pnp_resume (struct pnp_dev *dev) { -long line = (long )pnp_get_drvdata (dev); +struct serial_pnp_info *info =pnp_get_drvdata (dev); -if (!line) +if (!info)return -ENODEV ; -serial8250_resume_port (line -1 ); +serial8250_resume_port (info->line);return 0 ; }#else
Signature · 1 line
2.1.3
PATCH 3/4 PNP: Allow device to override ACPI device sleep
In reply to Peter HurleyIf the serial console is an ACPI PNP device, the PNP bus attempts to power-down the device, even though the no_console_suspend command line parameter is specified (eg., debugging suspend/resume).
Add PNP_SUSPEND capability which is on by default, but when cleared, prevents pnpacpi_suspend() (which is the ACPI PNP protocol ->suspend() method).
--- drivers/pnp/driver.c | 2 +- drivers/pnp/pnpacpi/core.c | 2 +- include/linux/pnp.h | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-)
drivers/pnp/driver.c +1 -1
@@ -182,7 +182,7 @@ static int __pnp_bus_suspend(struct device *dev, pm_message_t state)return error; } -if (pnp_dev->protocol->suspend) +if (pnp_can_suspend (pnp_dev)) pnp_dev->protocol->suspend (pnp_dev, state);return 0 ; }
drivers/pnp/pnpacpi/core.c +1 -1
@@ -253,7 +253,7 @@ static int __init pnpacpi_add_device(struct acpi_device *device) dev->active = device->status.enabled;if (acpi_has_method (device->handle,"_SRS" )) dev->capabilities |=PNP_CONFIGURABLE ; - dev->capabilities |=PNP_READ ; + dev->capabilities |=PNP_READ |PNP_SUSPEND ;if (device->flags.dynamic_status && (dev->capabilities &PNP_CONFIGURABLE )) dev->capabilities |=PNP_WRITE ;if (device->flags.removable)
include/linux/pnp.h +3 -0
@@ -309,6 +309,7 @@ struct pnp_fixup {#define PNP_DISABLE 0x0004#define PNP_CONFIGURABLE 0x0008#define PNP_REMOVABLE 0x0010 +#define PNP_SUSPEND 0x0020#define pnp_can_read (dev) (((dev)->protocol->get) && \ ((dev)->capabilities & PNP_READ))
@@ -318,6 +319,8 @@ struct pnp_fixup { ((dev)->capabilities & PNP_DISABLE))#define pnp_can_configure (dev) ((!(dev)->active) && \ ((dev)->capabilities & PNP_CONFIGURABLE)) +#define pnp_can_suspend (dev) (((dev)->protocol->suspend) && \ + ((dev)->capabilities & PNP_SUSPEND))#ifdef CONFIG_ISAPNP extern struct pnp_protocol isapnp_protocol;
Signature · 1 line
2.1.3
Re: [PATCH 3/4] PNP: Allow device to override ACPI device sleep
Quoted 9 lines · Peter Hurley
If the serial console is an ACPI PNP device, the PNP bus attempts to power-down the device, even though the
no_console_suspendcommand line parameter is specified (eg., debugging suspend/resume).Add
PNP_SUSPENDcapability which is on by default, but when cleared, preventspnpacpi_suspend()(which is the ACPI PNP protocol ->suspend() method).Signed-off-by Peter Hurley <[email protected]>
While I'm not disagreeing with this entirely, I have a concern.
There are two types of suspend in Linux these days, runtime suspend and system suspend. What this is about is "device suspend during system suspend" only, so the flag name is somewhat confusing.
Also the flag is only going to be used for consoles, so why don't you introduce a PNP_CONSOLE flag and then define pnp_can_suspend() as
static inline pnp_can_suspend(struct pnp_dev *pnp_dev)
{
return pnp_dev->protocol->suspend &&
(!(pnp_dev->capabilities & PNP_CONSOLE) || console_suspend_enabled);
}Quoted 54 lines
--- drivers/pnp/driver.c | 2 +- drivers/pnp/pnpacpi/core.c | 2 +- include/linux/pnp.h | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-)drivers/pnp/driver.c +1
-1@@ -182,7 +182,7 @@ static int __pnp_bus_suspend(struct device *dev, pm_message_t state)return error; } -if (pnp_dev->protocol->suspend) +if (pnp_can_suspend (pnp_dev)) pnp_dev->protocol->suspend (pnp_dev, state);return 0 ; }drivers/pnp/pnpacpi/core.c +1
-1@@ -253,7 +253,7 @@ static int __init pnpacpi_add_device(struct acpi_device *device) dev->active = device->status.enabled;if (acpi_has_method (device->handle,"_SRS" )) dev->capabilities |=PNP_CONFIGURABLE ; - dev->capabilities |=PNP_READ ; + dev->capabilities |=PNP_READ |PNP_SUSPEND ;if (device->flags.dynamic_status && (dev->capabilities &PNP_CONFIGURABLE )) dev->capabilities |=PNP_WRITE ;if (device->flags.removable)include/linux/pnp.h +3
-0@@ -309,6 +309,7 @@ struct pnp_fixup {#define PNP_DISABLE 0x0004#define PNP_CONFIGURABLE 0x0008#define PNP_REMOVABLE 0x0010 +#define PNP_SUSPEND 0x0020#define pnp_can_read (dev) (((dev)->protocol->get) && \ ((dev)->capabilities & PNP_READ))@@ -318,6 +319,8 @@ struct pnp_fixup { ((dev)->capabilities & PNP_DISABLE))#define pnp_can_configure (dev) ((!(dev)->active) && \ ((dev)->capabilities & PNP_CONFIGURABLE)) +#define pnp_can_suspend (dev) (((dev)->protocol->suspend) && \ + ((dev)->capabilities & PNP_SUSPEND))#ifdef CONFIG_ISAPNP extern struct pnp_protocol isapnp_protocol;
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
Re: [PATCH 3/4] PNP: Allow device to override ACPI device sleep
Quoted 9 lines · Rafael J. Wysocki
Quoted 8 lines · Peter Hurley
If the serial console is an ACPI PNP device, the PNP bus attempts to power-down the device, even though the
no_console_suspendcommand line parameter is specified (eg., debugging suspend/resume).Add
PNP_SUSPENDcapability which is on by default, but when cleared, preventspnpacpi_suspend()(which is the ACPI PNP protocol ->suspend() method).
Quoted 7 lines
Quoted 1 line
Signed-off-by Peter Hurley <[email protected]>While I'm not disagreeing with this entirely, I have a concern.
There are two types of suspend in Linux these days, runtime suspend and system suspend. What this is about is "device suspend during system suspend" only, so the flag name is somewhat confusing.
Yeah, I see your point. I wouldn't want people thinking this had anything to do with runtime pm.
Quoted 8 lines
Also the flag is only going to be used for consoles, so why don't you introduce a
PNP_CONSOLEflag and then definepnp_can_suspend()asstatic inline
pnp_can_suspend(struct pnp_dev *pnp_dev){ return pnp_dev->protocol->suspend && (!(pnp_dev->capabilities & PNP_CONSOLE) || console_suspend_enabled); }
I can do that.
Regards,
Peter Hurley
PATCH 4/4 serial: 8250_pnp: Override pnp suspend for no_console_suspend
In reply to Peter HurleyPrevent PNP bus from powering down the serial console device when suspending if the no_console_suspend command line option is set.
--- drivers/tty/serial/8250/8250_pnp.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
drivers/tty/serial/8250/8250_pnp.c +17 -1
@@ -487,7 +487,7 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)if (uart_console (&port8250->port) && !console_suspend_enabled) { info->override_disable = !!(dev->capabilities &PNP_DISABLE ); - dev->capabilities &= ~PNP_DISABLE ; + dev->capabilities &= ~(PNP_DISABLE |PNP_SUSPEND ); }pnp_set_drvdata (dev, info);
@@ -502,6 +502,7 @@ static void serial_pnp_remove(struct pnp_dev *dev)serial8250_unregister_port (info->line);if (info->override_disable) dev->capabilities |=PNP_DISABLE ; + dev->capabilities |=PNP_SUSPEND ; } }
@@ -519,10 +520,25 @@ static int serial_pnp_suspend(struct pnp_dev *dev, pm_message_t state)static int serial_pnp_resume (struct pnp_dev *dev) {struct serial_pnp_info *info =pnp_get_drvdata (dev); +unsigned int ier, mcr, lcr, dll, dlm; +struct uart_8250_port *port8250;if (!info)return -ENODEV ; + + port8250 =serial8250_get_port (info->line); + ier =serial_port_in (&port8250->port,UART_IER ); + mcr =serial_port_in (&port8250->port,UART_MCR ); + lcr =serial_port_in (&port8250->port,UART_LCR ); +serial_port_out (&port8250->port,UART_LCR , lcr |UART_LCR_DLAB ); + dll =serial_port_in (&port8250->port,UART_DLL ); + dlm =serial_port_in (&port8250->port,UART_DLM ); +serial_port_out (&port8250->port,UART_LCR , lcr & ~UART_LCR_DLAB ); +serial8250_resume_port (info->line); + +dev_info (&dev->dev,"IER:%02x MCR:%02x LCR:%02x DL:%02x%02x\n" , + ier, mcr, lcr, dlm, dll);return 0 ; }#else
Signature · 1 line
2.1.3
Re: [PATCH 4/4] serial: 8250_pnp: Override pnp suspend for no_console_suspend
Quoted 2 lines · Peter Hurley
Prevent PNP bus from powering down the serial console device when suspending if the
no_console_suspendcommand line option is set.
Whoops! I set the debug version <blush>.
Quoted 53 lines
Signed-off-by Peter Hurley <[email protected]>--- drivers/tty/serial/8250/8250_pnp.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)drivers/tty/serial/8250/8250_pnp.c +17
-1@@ -487,7 +487,7 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)if (uart_console (&port8250->port) && !console_suspend_enabled) { info->override_disable = !!(dev->capabilities &PNP_DISABLE ); - dev->capabilities &= ~PNP_DISABLE ; + dev->capabilities &= ~(PNP_DISABLE |PNP_SUSPEND ); }pnp_set_drvdata (dev, info);@@ -502,6 +502,7 @@ static void serial_pnp_remove(struct pnp_dev *dev)serial8250_unregister_port (info->line);if (info->override_disable) dev->capabilities |=PNP_DISABLE ; + dev->capabilities |=PNP_SUSPEND ; } }@@ -519,10 +520,25 @@ static int serial_pnp_suspend(struct pnp_dev *dev, pm_message_t state)static int serial_pnp_resume (struct pnp_dev *dev) {struct serial_pnp_info *info =pnp_get_drvdata (dev); +unsigned int ier, mcr, lcr, dll, dlm; +struct uart_8250_port *port8250;if (!info)return -ENODEV ; + + port8250 =serial8250_get_port (info->line); + ier =serial_port_in (&port8250->port,UART_IER ); + mcr =serial_port_in (&port8250->port,UART_MCR ); + lcr =serial_port_in (&port8250->port,UART_LCR ); +serial_port_out (&port8250->port,UART_LCR , lcr |UART_LCR_DLAB ); + dll =serial_port_in (&port8250->port,UART_DLL ); + dlm =serial_port_in (&port8250->port,UART_DLM ); +serial_port_out (&port8250->port,UART_LCR , lcr & ~UART_LCR_DLAB ); +serial8250_resume_port (info->line); + +dev_info (&dev->dev,"IER:%02x MCR:%02x LCR:%02x DL:%02x%02x\n" , + ier, mcr, lcr, dlm, dll);return 0 ; }#else
PATCH v2 4/4 serial: 8250_pnp: Override pnp suspend for no_console_suspend
In reply to Peter HurleyPrevent PNP bus from powering down the serial console device when suspending if the no_console_suspend command line option is set.
--- drivers/tty/serial/8250/8250_pnp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/tty/serial/8250/8250_pnp.c +2 -1
@@ -487,7 +487,7 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)if (uart_console (&port8250->port) && !console_suspend_enabled) { info->override_disable = !!(dev->capabilities &PNP_DISABLE ); - dev->capabilities &= ~PNP_DISABLE ; + dev->capabilities &= ~(PNP_DISABLE |PNP_SUSPEND ); }pnp_set_drvdata (dev, info);
@@ -502,6 +502,7 @@ static void serial_pnp_remove(struct pnp_dev *dev)serial8250_unregister_port (info->line);if (info->override_disable) dev->capabilities |=PNP_DISABLE ; + dev->capabilities |=PNP_SUSPEND ; } }
Signature · 1 line
2.1.3
Greg Kroah-Hartman
maintainerQuoted 31 lines · Peter Hurley
Hi Greg,
The kernel command line parameter,
no_console_suspend, enables the serial console to continue to output kernel message during system suspend/resume.However, the PNP bus will disable and power-down pnp devices on system suspend, which defeats the purpose of
no_console_suspend; namely to continue to output messages in case those are the last messages before crashing.The first patch is a cleanup of the baroque logic of
uart_suspend_port(). The second patch overrides the PNP bus disabling the console device ifno_console_suspendis set. This patch uses existing PNP bus logic but perhaps in a way not originally intended.
The third patch requires Rafael's ack, as it adds a capability to allow the device to override the power-down function.
The fourth patch actually performs the power-down override enabled by patch 3.With these patches, the PNP0501 device (16550A uart) will continue to output messages while system suspend finishes, as intended by the
no_console_suspendparameter. [System resume is another problem, which I'm working on].Regards,
Peter Hurley (4):
serial: core: Simplify console suspend logic inuart_suspend_port()
serial: 8250_pnp: Override pnp disable forno_console_suspend
PNP: Allow device to override ACPI device sleep
serial: 8250_pnp: Override pnp suspend forno_console_suspend
Quoted 6 lines
drivers/pnp/driver.c | 2 +- drivers/pnp/pnpacpi/core.c | 2 +- drivers/tty/serial/8250/8250_pnp.c | 61 +++++++++++++++++++++++++++++++------- drivers/tty/serial/serial_core.c | 35 +++++++++++----------- include/linux/pnp.h | 3 ++ 5 files changed, 72 insertions(+), 31 deletions(-)
I'll drop these for now and wait for a second round of these patches.
thanks,
greg k-h