Skip to contentSkip to conversation

fix serial console suspend on PNP

PATCH In review Peter Hurley 3 participants 10 messages Nov 5, 2014 – Nov 6, 2014

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

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
#1 lore raw

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
index c5fb08c..158e667 100644
@@ -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
#2 lore raw

PATCH 2/4 serial: 8250_pnp: Override pnp disable for no_console_suspend

In reply to Peter Hurley

When 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
index 682a2fb..50b7c89 100644
@@ -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
#3 lore raw

PATCH 3/4 PNP: Allow device to override ACPI device sleep

In reply to 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_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
index f748cc8..4e57d33 100644
@@ -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
index d2b780a..115bb73 100644
@@ -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
index 195aafc..5a5cbc2 100644
@@ -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
#4 lore raw

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_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).

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
index f748cc8..4e57d33 100644
@@ -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
index d2b780a..115bb73 100644
@@ -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
index 195aafc..5a5cbc2 100644
@@ -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.
#5 lore raw

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_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).

Quoted 7 lines
Quoted 1 line

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_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);
}

I can do that.

Regards,
Peter Hurley

#6 lore raw

PATCH 4/4 serial: 8250_pnp: Override pnp suspend for no_console_suspend

In reply to Peter Hurley

Prevent 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
index 50b7c89..f603581 100644
@@ -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
#7 lore raw

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_suspend command line option is set.

Whoops! I set the debug version <blush>.

Quoted 53 lines
---
 drivers/tty/serial/8250/8250_pnp.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)
drivers/tty/serial/8250/8250_pnp.c +17 -1
index 50b7c89..f603581 100644
@@ -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
#8 lore raw

PATCH v2 4/4 serial: 8250_pnp: Override pnp suspend for no_console_suspend

In reply to Peter Hurley

Prevent 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
index 50b7c89..4aefaf2 100644
@@ -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
#9 lore raw
In reply to Peter Hurley
Quoted 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 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

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