[PATCH v2 0/8] serial: Disable DMA and PM on kernel console

[PATCH v2 0/8] serial: Disable DMA and PM on kernel console

[Andy Shevchenko]

This is second attempt [1] to get rid of problematic DMA and PM calls
in the serial kernel console code.

Kernel console is sensitive to any kind of complex work needed to print
out anything on it. One such case is emergency print during Oops.

Patches 1-3 are preparatory ones.

After previous discussion Tony suggested to add a possibility to detach
and attach back kernel console from user space. It's done on patch 4 here.

More details on topic are in the commit messages of patches 5 and 6.

Patches 7 and 8 are deduplicate follow ups to patch 6.

The series has been tested on few Intel platforms.

Note, it depends to recently submitted and applied patches in the core
console code [2].

[1]: https://lore.kernel.org/patchwork/cover/905632/
[2]: https://lore.kernel.org/lkml/20200203133130.11591-1-andriy.shevchenko@linux.intel.com/

Changelog v2:
- added possibility to detach and attach back console from userspace (Tony)
- reworded commit messages in patches 5 and 6 (Sebastian)
- dropped console patch (it had been pushed separately [2])

Andy Shevchenko (8):
  serial: core: Introduce uart_console_enabled() helper
  serial: core: Consolidate spin lock initialization code
  serial: core: use octal permissions on module param
  serial: core: Allow detach and attach serial device for console
  serial: 8250_port: Don't use power management for kernel console
  serial: 8250_port: Disable DMA operations for kernel console
  serial: 8250_mtk: Remove duplicating code to disable DMA
  serial: 8250_omap: Remove duplicating code to disable DMA

 Documentation/ABI/testing/sysfs-tty |   7 ++
 drivers/tty/serial/8250/8250_core.c |   9 ++
 drivers/tty/serial/8250/8250_mtk.c  |   4 -
 drivers/tty/serial/8250/8250_omap.c |   4 -
 drivers/tty/serial/8250/8250_port.c |  36 ++++++--
 drivers/tty/serial/serial_core.c    | 123 ++++++++++++++++++++--------
 include/linux/serial_8250.h         |   1 +
 7 files changed, 134 insertions(+), 50 deletions(-)

-- 
2.25.0

[PATCH v2 1/8] serial: core: Introduce uart_console_enabled() helper

[Andy Shevchenko]

Introduce uart_console_enabled() helper which checks port to be console
and console is registered in the list.

Note, this helper will be used in the future as well.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/serial_core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 76e506ee335c..d956da0b6d9c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1908,6 +1908,11 @@ static int uart_proc_show(struct seq_file *m, void *v)
 }
 #endif
 
+static inline bool uart_console_enabled(struct uart_port *port)
+{
+	return uart_console(port) && (port->cons->flags & CON_ENABLED);
+}
+
 #if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)
 /**
  *	uart_console_write - write a console message to a serial port
@@ -2066,7 +2071,7 @@ uart_set_options(struct uart_port *port, struct console *co,
 	 * If this port is a console, then the spinlock is already
 	 * initialised.
 	 */
-	if (!(uart_console(port) && (port->cons->flags & CON_ENABLED))) {
+	if (!uart_console_enabled(port)) {
 		spin_lock_init(&port->lock);
 		lockdep_set_class(&port->lock, &port_lock_key);
 	}
@@ -2828,7 +2833,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	 * If this port is a console, then the spinlock is already
 	 * initialised.
 	 */
-	if (!(uart_console(uport) && (uport->cons->flags & CON_ENABLED))) {
+	if (!uart_console_enabled(uport)) {
 		spin_lock_init(&uport->lock);
 		lockdep_set_class(&uport->lock, &port_lock_key);
 	}
-- 
2.25.0

[PATCH v2 2/8] serial: core: Consolidate spin lock initialization code

[Andy Shevchenko]

We have two times duplicated excerpt where we initialize spin lock
for UART port. Consolidate it under uart_port_spin_lock_init() helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/serial_core.c | 34 +++++++++++++++-----------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index d956da0b6d9c..bb2287048108 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1913,6 +1913,19 @@ static inline bool uart_console_enabled(struct uart_port *port)
 	return uart_console(port) && (port->cons->flags & CON_ENABLED);
 }
 
+/*
+ * Ensure that the serial console lock is initialised early.
+ * If this port is a console, then the spinlock is already initialised.
+ */
+static inline void uart_port_spin_lock_init(struct uart_port *port)
+{
+	if (uart_console_enabled(port))
+		return;
+
+	spin_lock_init(&port->lock);
+	lockdep_set_class(&port->lock, &port_lock_key);
+}
+
 #if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)
 /**
  *	uart_console_write - write a console message to a serial port
@@ -2065,16 +2078,7 @@ uart_set_options(struct uart_port *port, struct console *co,
 	struct ktermios termios;
 	static struct ktermios dummy;
 
-	/*
-	 * Ensure that the serial console lock is initialised
-	 * early.
-	 * If this port is a console, then the spinlock is already
-	 * initialised.
-	 */
-	if (!uart_console_enabled(port)) {
-		spin_lock_init(&port->lock);
-		lockdep_set_class(&port->lock, &port_lock_key);
-	}
+	uart_port_spin_lock_init(port);
 
 	memset(&termios, 0, sizeof(struct ktermios));
 
@@ -2829,14 +2833,8 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 		goto out;
 	}
 
-	/*
-	 * If this port is a console, then the spinlock is already
-	 * initialised.
-	 */
-	if (!uart_console_enabled(uport)) {
-		spin_lock_init(&uport->lock);
-		lockdep_set_class(&uport->lock, &port_lock_key);
-	}
+	uart_port_spin_lock_init(uport);
+
 	if (uport->cons && uport->dev)
 		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
 
-- 
2.25.0

[PATCH v2 3/8] serial: core: use octal permissions on module param

[Andy Shevchenko]

Symbolic permissions 'S_IRUSR | S_IRGRP' are not preferred.
Use octal permissions '0440'. This also makes code shorter.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/serial_core.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index bb2287048108..7564bbd3061c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2749,19 +2749,19 @@ static ssize_t uart_get_attr_iomem_reg_shift(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.iomem_reg_shift);
 }
 
-static DEVICE_ATTR(type, S_IRUSR | S_IRGRP, uart_get_attr_type, NULL);
-static DEVICE_ATTR(line, S_IRUSR | S_IRGRP, uart_get_attr_line, NULL);
-static DEVICE_ATTR(port, S_IRUSR | S_IRGRP, uart_get_attr_port, NULL);
-static DEVICE_ATTR(irq, S_IRUSR | S_IRGRP, uart_get_attr_irq, NULL);
-static DEVICE_ATTR(flags, S_IRUSR | S_IRGRP, uart_get_attr_flags, NULL);
-static DEVICE_ATTR(xmit_fifo_size, S_IRUSR | S_IRGRP, uart_get_attr_xmit_fifo_size, NULL);
-static DEVICE_ATTR(uartclk, S_IRUSR | S_IRGRP, uart_get_attr_uartclk, NULL);
-static DEVICE_ATTR(close_delay, S_IRUSR | S_IRGRP, uart_get_attr_close_delay, NULL);
-static DEVICE_ATTR(closing_wait, S_IRUSR | S_IRGRP, uart_get_attr_closing_wait, NULL);
-static DEVICE_ATTR(custom_divisor, S_IRUSR | S_IRGRP, uart_get_attr_custom_divisor, NULL);
-static DEVICE_ATTR(io_type, S_IRUSR | S_IRGRP, uart_get_attr_io_type, NULL);
-static DEVICE_ATTR(iomem_base, S_IRUSR | S_IRGRP, uart_get_attr_iomem_base, NULL);
-static DEVICE_ATTR(iomem_reg_shift, S_IRUSR | S_IRGRP, uart_get_attr_iomem_reg_shift, NULL);
+static DEVICE_ATTR(type, 0440, uart_get_attr_type, NULL);
+static DEVICE_ATTR(line, 0440, uart_get_attr_line, NULL);
+static DEVICE_ATTR(port, 0440, uart_get_attr_port, NULL);
+static DEVICE_ATTR(irq, 0440, uart_get_attr_irq, NULL);
+static DEVICE_ATTR(flags, 0440, uart_get_attr_flags, NULL);
+static DEVICE_ATTR(xmit_fifo_size, 0440, uart_get_attr_xmit_fifo_size, NULL);
+static DEVICE_ATTR(uartclk, 0440, uart_get_attr_uartclk, NULL);
+static DEVICE_ATTR(close_delay, 0440, uart_get_attr_close_delay, NULL);
+static DEVICE_ATTR(closing_wait, 0440, uart_get_attr_closing_wait, NULL);
+static DEVICE_ATTR(custom_divisor, 0440, uart_get_attr_custom_divisor, NULL);
+static DEVICE_ATTR(io_type, 0440, uart_get_attr_io_type, NULL);
+static DEVICE_ATTR(iomem_base, 0440, uart_get_attr_iomem_base, NULL);
+static DEVICE_ATTR(iomem_reg_shift, 0440, uart_get_attr_iomem_reg_shift, NULL);
 
 static struct attribute *tty_dev_attrs[] = {
 	&dev_attr_type.attr,
-- 
2.25.0

[PATCH v2 4/8] serial: core: Allow detach and attach serial device for console

[Andy Shevchenko]

In the future we would like to disable power management on the serial devices
used as kernel consoles to avoid weird behaviour in some cases. However,
disabling PM may prevent system to go to deep sleep states, which in its turn
leads to the higher power consumption.

Tony Lindgren proposed a work around, i.e. allow user to detach such consoles
to make PM working again. In case user wants to see what's going on, it also
provides a mechanism to attach console back.

Link: https://lists.openwall.net/linux-kernel/2018/09/29/65
Suggested-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-tty |  7 ++++
 drivers/tty/serial/serial_core.c    | 60 +++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty
index 9eb3c2b6b040..e157130a6792 100644
--- a/Documentation/ABI/testing/sysfs-tty
+++ b/Documentation/ABI/testing/sysfs-tty
@@ -154,3 +154,10 @@ Description:
 		 device specification. For example, when user sets 7bytes on
 		 16550A, which has 1/4/8/14 bytes trigger, the RX trigger is
 		 automatically changed to 4 bytes.
+
+What:		/sys/class/tty/ttyS0/console
+Date:		February 2020
+Contact:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+Description:
+		 Allows user to detach or attach back the given device as
+		 kernel console. It shows and accepts a boolean variable.
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 7564bbd3061c..0415bb76efa0 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1919,7 +1919,7 @@ static inline bool uart_console_enabled(struct uart_port *port)
  */
 static inline void uart_port_spin_lock_init(struct uart_port *port)
 {
-	if (uart_console_enabled(port))
+	if (uart_console(port))
 		return;
 
 	spin_lock_init(&port->lock);
@@ -2749,6 +2749,56 @@ static ssize_t uart_get_attr_iomem_reg_shift(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.iomem_reg_shift);
 }
 
+static ssize_t uart_get_attr_console(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct tty_port *port = dev_get_drvdata(dev);
+	struct uart_state *state = container_of(port, struct uart_state, port);
+	struct uart_port *uport;
+	bool console = false;
+
+	mutex_lock(&port->mutex);
+	uport = uart_port_check(state);
+	if (uport)
+		console = uart_console_enabled(uport);
+	mutex_unlock(&port->mutex);
+
+	return sprintf(buf, "%c\n", console ? 'Y' : 'N');
+}
+
+static ssize_t uart_set_attr_console(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct tty_port *port = dev_get_drvdata(dev);
+	struct uart_state *state = container_of(port, struct uart_state, port);
+	struct uart_port *uport;
+	bool oldconsole, newconsole;
+	int ret;
+
+	ret = kstrtobool(buf, &newconsole);
+	if (ret)
+		return ret;
+
+	mutex_lock(&port->mutex);
+	uport = uart_port_check(state);
+	if (uport) {
+		oldconsole = uart_console_enabled(uport);
+		if (oldconsole && !newconsole) {
+			ret = unregister_console(uport->cons);
+		} else if (!oldconsole && newconsole) {
+			if (uart_console(uport))
+				register_console(uport->cons);
+			else
+				ret = -ENOENT;
+		}
+	} else {
+		ret = -ENXIO;
+	}
+	mutex_unlock(&port->mutex);
+
+	return ret < 0 ? ret : count;
+}
+
 static DEVICE_ATTR(type, 0440, uart_get_attr_type, NULL);
 static DEVICE_ATTR(line, 0440, uart_get_attr_line, NULL);
 static DEVICE_ATTR(port, 0440, uart_get_attr_port, NULL);
@@ -2762,6 +2812,7 @@ static DEVICE_ATTR(custom_divisor, 0440, uart_get_attr_custom_divisor, NULL);
 static DEVICE_ATTR(io_type, 0440, uart_get_attr_io_type, NULL);
 static DEVICE_ATTR(iomem_base, 0440, uart_get_attr_iomem_base, NULL);
 static DEVICE_ATTR(iomem_reg_shift, 0440, uart_get_attr_iomem_reg_shift, NULL);
+static DEVICE_ATTR(console, 0640, uart_get_attr_console, uart_set_attr_console);
 
 static struct attribute *tty_dev_attrs[] = {
 	&dev_attr_type.attr,
@@ -2777,12 +2828,13 @@ static struct attribute *tty_dev_attrs[] = {
 	&dev_attr_io_type.attr,
 	&dev_attr_iomem_base.attr,
 	&dev_attr_iomem_reg_shift.attr,
-	NULL,
-	};
+	&dev_attr_console.attr,
+	NULL
+};
 
 static const struct attribute_group tty_dev_attr_group = {
 	.attrs = tty_dev_attrs,
-	};
+};
 
 /**
  *	uart_add_one_port - attach a driver-defined port structure
-- 
2.25.0

[PATCH v2 5/8] serial: 8250_port: Don't use power management for kernel console

[Andy Shevchenko]

Doing any kind of power management for kernel console is really bad idea.

First of all, it runs in poll and atomic mode. This fact attaches a limitation
on the functions that might be called. For example, pm_runtime_get_sync() might
sleep and thus can't be used. This call needs, for example, to bring the device
to powered on state on the system, where the power on sequence may require
on-atomic operations, such as Intel Cherrytrail with ACPI enumerated UARTs.
That said, on ACPI enabled platforms it might even call firmware for a job.

On the other hand pm_runtime_get() doesn't guarantee that device will become
powered on fast enough.

Besides that, imagine the case when console is about to print a kernel Oops and
it's powered off. In such an emergency case calling the complex functions is
not the best what we can do, taking into consideration that user wants to see
at least something of the last kernel word before it passes away.

Here we modify the 8250 console code to prevent runtime power management.

Link: https://lists.openwall.net/linux-kernel/2018/09/29/65
Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_core.c |  9 +++++++++
 drivers/tty/serial/8250/8250_port.c | 24 ++++++++++++++++++++----
 include/linux/serial_8250.h         |  1 +
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index f2a33c9082a6..006d40853509 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -608,6 +608,14 @@ static int univ8250_console_setup(struct console *co, char *options)
 	return retval;
 }
 
+static int univ8250_console_exit(struct console *co)
+{
+	struct uart_port *port;
+
+	port = &serial8250_ports[co->index].port;
+	return serial8250_console_exit(port);
+}
+
 /**
  *	univ8250_console_match - non-standard console matching
  *	@co:	  registering console
@@ -666,6 +674,7 @@ static struct console univ8250_console = {
 	.write		= univ8250_console_write,
 	.device		= uart_console_device,
 	.setup		= univ8250_console_setup,
+	.exit		= univ8250_console_exit,
 	.match		= univ8250_console_match,
 	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
 	.index		= -1,
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 0325f2e53b74..6307a04c0cd9 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3127,6 +3127,9 @@ static void serial8250_console_restore(struct uart_8250_port *up)
  *	any possible real use of the port...
  *
  *	The console_lock must be held when we get here.
+ *
+ *	Doing runtime PM is really a bad idea for the kernel console.
+ *	Thus, we assume the function is called when device is powered up.
  */
 void serial8250_console_write(struct uart_8250_port *up, const char *s,
 			      unsigned int count)
@@ -3138,8 +3141,6 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 
 	touch_nmi_watchdog();
 
-	serial8250_rpm_get(up);
-
 	if (oops_in_progress)
 		locked = spin_trylock_irqsave(&port->lock, flags);
 	else
@@ -3182,7 +3183,6 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 
 	if (locked)
 		spin_unlock_irqrestore(&port->lock, flags);
-	serial8250_rpm_put(up);
 }
 
 static unsigned int probe_baud(struct uart_port *port)
@@ -3206,6 +3206,7 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 	int bits = 8;
 	int parity = 'n';
 	int flow = 'n';
+	int ret;
 
 	if (!port->iobase && !port->membase)
 		return -ENODEV;
@@ -3215,7 +3216,22 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 	else if (probe)
 		baud = probe_baud(port);
 
-	return uart_set_options(port, port->cons, baud, parity, bits, flow);
+	ret = uart_set_options(port, port->cons, baud, parity, bits, flow);
+	if (ret)
+		return ret;
+
+	if (port->dev)
+		pm_runtime_get_noresume(port->dev);
+
+	return 0;
+}
+
+int serial8250_console_exit(struct uart_port *port)
+{
+	if (port->dev)
+		pm_runtime_put_noidle(port->dev);
+
+	return 0;
 }
 
 #endif /* CONFIG_SERIAL_8250_CONSOLE */
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 6a8e8c48c882..8c7b16df7b09 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -176,6 +176,7 @@ void serial8250_set_defaults(struct uart_8250_port *up);
 void serial8250_console_write(struct uart_8250_port *up, const char *s,
 			      unsigned int count);
 int serial8250_console_setup(struct uart_port *port, char *options, bool probe);
+int serial8250_console_exit(struct uart_port *port);
 
 extern void serial8250_set_isa_configurator(void (*v)
 					(int port, struct uart_port *up,
-- 
2.25.0

[PATCH v2 6/8] serial: 8250_port: Disable DMA operations for kernel console

[Andy Shevchenko]

It would be too tricky and error prone to allow DMA operations on
kernel console.

One of the concern is when DMA is a separate device, for example on
Intel CherryTrail platforms, and might need special work around to be
functional, see the commit

  eebb3e8d8aaf ("ACPI / LPSS: override power state for LPSS DMA device")

for more information.

Another one is that kernel console is used in atomic context, e.g.
when printing crucial information to the user (Oops or crash),
and DMA may not serve due to power management complications
including non-atomic ACPI calls but not limited to it (see above).

Besides that, other concerns are described in the commit

  84b40e3b57ee ("serial: 8250: omap: Disable DMA for console UART")

done for OMAP UART and may be repeated here.

Disable any kind of DMA operations on kernel console due to above concerns.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 6307a04c0cd9..8ed22aa31add 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2294,10 +2294,14 @@ int serial8250_do_startup(struct uart_port *port)
 	 * Request DMA channels for both RX and TX.
 	 */
 	if (up->dma) {
-		retval = serial8250_request_dma(up);
-		if (retval) {
-			pr_warn_ratelimited("%s - failed to request DMA\n",
-					    port->name);
+		const char *msg = NULL;
+
+		if (uart_console(port))
+			msg = "forbid DMA for kernel console";
+		else if (serial8250_request_dma(up))
+			msg = "failed to request DMA";
+		if (msg) {
+			pr_warn_ratelimited("%s - %s\n", port->name, msg);
 			up->dma = NULL;
 		}
 	}
-- 
2.25.0

[PATCH v2 7/8] serial: 8250_mtk: Remove duplicating code to disable DMA

[Andy Shevchenko]

The core already handles the kernel console case and disables DMA
for any serial device used for above mentioned purposes.

Thus, remove duplicating code to disable DMA for kernel console.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_mtk.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index 4d067f515f74..3edd5791ada8 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -183,10 +183,6 @@ static int mtk8250_startup(struct uart_port *port)
 	struct uart_8250_port *up = up_to_u8250p(port);
 	struct mtk8250_data *data = port->private_data;
 
-	/* disable DMA for console */
-	if (uart_console(port))
-		up->dma = NULL;
-
 	if (up->dma) {
 		data->rx_status = DMA_RX_START;
 		uart_circ_clear(&port->state->xmit);
-- 
2.25.0

[PATCH v2 8/8] serial: 8250_omap: Remove duplicating code to disable DMA

[Andy Shevchenko]

The core already handles the kernel console case and disables DMA
for any serial device used for above mentioned purposes.

Thus, remove duplicating code to disable DMA for kernel console.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_omap.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 6f343ca08440..4f07fe8093a3 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -620,10 +620,6 @@ static int omap_8250_startup(struct uart_port *port)
 	up->lsr_saved_flags = 0;
 	up->msr_saved_flags = 0;
 
-	/* Disable DMA for console UART */
-	if (uart_console(port))
-		up->dma = NULL;
-
 	if (up->dma) {
 		ret = serial8250_request_dma(up);
 		if (ret) {
-- 
2.25.0

Re: [PATCH v2 5/8] serial: 8250_port: Don't use power management for kernel console

[Russell King - ARM Linux admin]

On Fri, Feb 14, 2020 at 01:43:36PM +0200, Andy Shevchenko wrote:
> Doing any kind of power management for kernel console is really bad idea.
> 
> First of all, it runs in poll and atomic mode. This fact attaches a limitation
> on the functions that might be called. For example, pm_runtime_get_sync() might
> sleep and thus can't be used. This call needs, for example, to bring the device
> to powered on state on the system, where the power on sequence may require
> on-atomic operations, such as Intel Cherrytrail with ACPI enumerated UARTs.
> That said, on ACPI enabled platforms it might even call firmware for a job.
> 
> On the other hand pm_runtime_get() doesn't guarantee that device will become
> powered on fast enough.
> 
> Besides that, imagine the case when console is about to print a kernel Oops and
> it's powered off. In such an emergency case calling the complex functions is
> not the best what we can do, taking into consideration that user wants to see
> at least something of the last kernel word before it passes away.
> 
> Here we modify the 8250 console code to prevent runtime power management.

It's probably also worth noting (and documenting) that this will likely
cause a PM regression for OMAP platforms since the serial port will no
longer be idled, and therefore the power domains will not hit retention
state.  Please wait for Tony to confirm.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v2 3/8] serial: core: use octal permissions on module param

[Greg Kroah-Hartman]

On Fri, Feb 14, 2020 at 01:43:34PM +0200, Andy Shevchenko wrote:
> Symbolic permissions 'S_IRUSR | S_IRGRP' are not preferred.
> Use octal permissions '0440'. This also makes code shorter.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/serial_core.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index bb2287048108..7564bbd3061c 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2749,19 +2749,19 @@ static ssize_t uart_get_attr_iomem_reg_shift(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.iomem_reg_shift);
>  }
>  
> -static DEVICE_ATTR(type, S_IRUSR | S_IRGRP, uart_get_attr_type, NULL);
> -static DEVICE_ATTR(line, S_IRUSR | S_IRGRP, uart_get_attr_line, NULL);
> -static DEVICE_ATTR(port, S_IRUSR | S_IRGRP, uart_get_attr_port, NULL);
> -static DEVICE_ATTR(irq, S_IRUSR | S_IRGRP, uart_get_attr_irq, NULL);
> -static DEVICE_ATTR(flags, S_IRUSR | S_IRGRP, uart_get_attr_flags, NULL);
> -static DEVICE_ATTR(xmit_fifo_size, S_IRUSR | S_IRGRP, uart_get_attr_xmit_fifo_size, NULL);
> -static DEVICE_ATTR(uartclk, S_IRUSR | S_IRGRP, uart_get_attr_uartclk, NULL);
> -static DEVICE_ATTR(close_delay, S_IRUSR | S_IRGRP, uart_get_attr_close_delay, NULL);
> -static DEVICE_ATTR(closing_wait, S_IRUSR | S_IRGRP, uart_get_attr_closing_wait, NULL);
> -static DEVICE_ATTR(custom_divisor, S_IRUSR | S_IRGRP, uart_get_attr_custom_divisor, NULL);
> -static DEVICE_ATTR(io_type, S_IRUSR | S_IRGRP, uart_get_attr_io_type, NULL);
> -static DEVICE_ATTR(iomem_base, S_IRUSR | S_IRGRP, uart_get_attr_iomem_base, NULL);
> -static DEVICE_ATTR(iomem_reg_shift, S_IRUSR | S_IRGRP, uart_get_attr_iomem_reg_shift, NULL);
> +static DEVICE_ATTR(type, 0440, uart_get_attr_type, NULL);
> +static DEVICE_ATTR(line, 0440, uart_get_attr_line, NULL);
> +static DEVICE_ATTR(port, 0440, uart_get_attr_port, NULL);
> +static DEVICE_ATTR(irq, 0440, uart_get_attr_irq, NULL);
> +static DEVICE_ATTR(flags, 0440, uart_get_attr_flags, NULL);
> +static DEVICE_ATTR(xmit_fifo_size, 0440, uart_get_attr_xmit_fifo_size, NULL);
> +static DEVICE_ATTR(uartclk, 0440, uart_get_attr_uartclk, NULL);
> +static DEVICE_ATTR(close_delay, 0440, uart_get_attr_close_delay, NULL);
> +static DEVICE_ATTR(closing_wait, 0440, uart_get_attr_closing_wait, NULL);
> +static DEVICE_ATTR(custom_divisor, 0440, uart_get_attr_custom_divisor, NULL);
> +static DEVICE_ATTR(io_type, 0440, uart_get_attr_io_type, NULL);
> +static DEVICE_ATTR(iomem_base, 0440, uart_get_attr_iomem_base, NULL);
> +static DEVICE_ATTR(iomem_reg_shift, 0440, uart_get_attr_iomem_reg_shift, NULL);

I'll take these, but we should move them all to DEVICE_ATTR_RO() as that
would make things a lot more "obvious" what is happening here.

thanks,

greg k-h

Re: [PATCH v2 4/8] serial: core: Allow detach and attach serial device for console

[Greg Kroah-Hartman]

On Fri, Feb 14, 2020 at 01:43:35PM +0200, Andy Shevchenko wrote:
> In the future we would like to disable power management on the serial devices
> used as kernel consoles to avoid weird behaviour in some cases. However,
> disabling PM may prevent system to go to deep sleep states, which in its turn
> leads to the higher power consumption.
> 
> Tony Lindgren proposed a work around, i.e. allow user to detach such consoles
> to make PM working again. In case user wants to see what's going on, it also
> provides a mechanism to attach console back.
> 
> Link: https://lists.openwall.net/linux-kernel/2018/09/29/65
> Suggested-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Documentation/ABI/testing/sysfs-tty |  7 ++++
>  drivers/tty/serial/serial_core.c    | 60 +++++++++++++++++++++++++++--
>  2 files changed, 63 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty
> index 9eb3c2b6b040..e157130a6792 100644
> --- a/Documentation/ABI/testing/sysfs-tty
> +++ b/Documentation/ABI/testing/sysfs-tty
> @@ -154,3 +154,10 @@ Description:
>  		 device specification. For example, when user sets 7bytes on
>  		 16550A, which has 1/4/8/14 bytes trigger, the RX trigger is
>  		 automatically changed to 4 bytes.
> +
> +What:		/sys/class/tty/ttyS0/console
> +Date:		February 2020
> +Contact:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> +Description:
> +		 Allows user to detach or attach back the given device as
> +		 kernel console. It shows and accepts a boolean variable.
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 7564bbd3061c..0415bb76efa0 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1919,7 +1919,7 @@ static inline bool uart_console_enabled(struct uart_port *port)
>   */
>  static inline void uart_port_spin_lock_init(struct uart_port *port)
>  {
> -	if (uart_console_enabled(port))
> +	if (uart_console(port))
>  		return;
>  
>  	spin_lock_init(&port->lock);
> @@ -2749,6 +2749,56 @@ static ssize_t uart_get_attr_iomem_reg_shift(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.iomem_reg_shift);
>  }
>  
> +static ssize_t uart_get_attr_console(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct tty_port *port = dev_get_drvdata(dev);
> +	struct uart_state *state = container_of(port, struct uart_state, port);
> +	struct uart_port *uport;
> +	bool console = false;
> +
> +	mutex_lock(&port->mutex);
> +	uport = uart_port_check(state);
> +	if (uport)
> +		console = uart_console_enabled(uport);
> +	mutex_unlock(&port->mutex);
> +
> +	return sprintf(buf, "%c\n", console ? 'Y' : 'N');
> +}
> +
> +static ssize_t uart_set_attr_console(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct tty_port *port = dev_get_drvdata(dev);
> +	struct uart_state *state = container_of(port, struct uart_state, port);
> +	struct uart_port *uport;
> +	bool oldconsole, newconsole;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &newconsole);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&port->mutex);
> +	uport = uart_port_check(state);
> +	if (uport) {
> +		oldconsole = uart_console_enabled(uport);
> +		if (oldconsole && !newconsole) {
> +			ret = unregister_console(uport->cons);
> +		} else if (!oldconsole && newconsole) {
> +			if (uart_console(uport))
> +				register_console(uport->cons);
> +			else
> +				ret = -ENOENT;
> +		}
> +	} else {
> +		ret = -ENXIO;
> +	}
> +	mutex_unlock(&port->mutex);
> +
> +	return ret < 0 ? ret : count;
> +}
> +
>  static DEVICE_ATTR(type, 0440, uart_get_attr_type, NULL);
>  static DEVICE_ATTR(line, 0440, uart_get_attr_line, NULL);
>  static DEVICE_ATTR(port, 0440, uart_get_attr_port, NULL);
> @@ -2762,6 +2812,7 @@ static DEVICE_ATTR(custom_divisor, 0440, uart_get_attr_custom_divisor, NULL);
>  static DEVICE_ATTR(io_type, 0440, uart_get_attr_io_type, NULL);
>  static DEVICE_ATTR(iomem_base, 0440, uart_get_attr_iomem_base, NULL);
>  static DEVICE_ATTR(iomem_reg_shift, 0440, uart_get_attr_iomem_reg_shift, NULL);
> +static DEVICE_ATTR(console, 0640, uart_get_attr_console, uart_set_attr_console);

Again, DEVICE_ATTR_RW() in the future?

thanks,

greg k-h

Re: [PATCH v2 6/8] serial: 8250_port: Disable DMA operations for kernel console

[Greg Kroah-Hartman]

On Fri, Feb 14, 2020 at 01:43:37PM +0200, Andy Shevchenko wrote:
> It would be too tricky and error prone to allow DMA operations on
> kernel console.
> 
> One of the concern is when DMA is a separate device, for example on
> Intel CherryTrail platforms, and might need special work around to be
> functional, see the commit
> 
>   eebb3e8d8aaf ("ACPI / LPSS: override power state for LPSS DMA device")
> 
> for more information.
> 
> Another one is that kernel console is used in atomic context, e.g.
> when printing crucial information to the user (Oops or crash),
> and DMA may not serve due to power management complications
> including non-atomic ACPI calls but not limited to it (see above).
> 
> Besides that, other concerns are described in the commit
> 
>   84b40e3b57ee ("serial: 8250: omap: Disable DMA for console UART")
> 
> done for OMAP UART and may be repeated here.
> 
> Disable any kind of DMA operations on kernel console due to above concerns.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 6307a04c0cd9..8ed22aa31add 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2294,10 +2294,14 @@ int serial8250_do_startup(struct uart_port *port)
>  	 * Request DMA channels for both RX and TX.
>  	 */
>  	if (up->dma) {
> -		retval = serial8250_request_dma(up);
> -		if (retval) {
> -			pr_warn_ratelimited("%s - failed to request DMA\n",
> -					    port->name);
> +		const char *msg = NULL;
> +
> +		if (uart_console(port))
> +			msg = "forbid DMA for kernel console";
> +		else if (serial8250_request_dma(up))
> +			msg = "failed to request DMA";
> +		if (msg) {
> +			pr_warn_ratelimited("%s - %s\n", port->name, msg);

dev_warn_ratelimited()?  You have a port, you should use it.

thanks,

greg k-h

Re: [PATCH v2 0/8] serial: Disable DMA and PM on kernel console

[Greg Kroah-Hartman]

On Fri, Feb 14, 2020 at 01:43:31PM +0200, Andy Shevchenko wrote:
> This is second attempt [1] to get rid of problematic DMA and PM calls
> in the serial kernel console code.
> 
> Kernel console is sensitive to any kind of complex work needed to print
> out anything on it. One such case is emergency print during Oops.
> 
> Patches 1-3 are preparatory ones.

I've applied these first 3, as they are "obvious" :)

I'll let others weigh in on the other patches here, as I'd like to see
if Tony and others feel it solves their issues or not.

thanks,

greg k-h

Re: [PATCH v2 5/8] serial: 8250_port: Don't use power management for kernel console

[Tony Lindgren]

* Russell King - ARM Linux admin <linux@armlinux.org.uk> [200214 13:40]:
> On Fri, Feb 14, 2020 at 01:43:36PM +0200, Andy Shevchenko wrote:
> > Doing any kind of power management for kernel console is really bad idea.
> > 
> > First of all, it runs in poll and atomic mode. This fact attaches a limitation
> > on the functions that might be called. For example, pm_runtime_get_sync() might
> > sleep and thus can't be used. This call needs, for example, to bring the device
> > to powered on state on the system, where the power on sequence may require
> > on-atomic operations, such as Intel Cherrytrail with ACPI enumerated UARTs.
> > That said, on ACPI enabled platforms it might even call firmware for a job.
> > 
> > On the other hand pm_runtime_get() doesn't guarantee that device will become
> > powered on fast enough.
> > 
> > Besides that, imagine the case when console is about to print a kernel Oops and
> > it's powered off. In such an emergency case calling the complex functions is
> > not the best what we can do, taking into consideration that user wants to see
> > at least something of the last kernel word before it passes away.
> > 
> > Here we modify the 8250 console code to prevent runtime power management.
> 
> It's probably also worth noting (and documenting) that this will likely
> cause a PM regression for OMAP platforms since the serial port will no
> longer be idled, and therefore the power domains will not hit retention
> state.  Please wait for Tony to confirm.

Well with patch 4 in this series we can now attach and detach the serial
console as we've discussed earlier.

This should remove the need for PM during serial console use hopefully :)

I'll try to test this series ASAP.

Regards,

Tony

Re: [PATCH v2 5/8] serial: 8250_port: Don't use power management for kernel console

[Andy Shevchenko]

On Fri, Feb 14, 2020 at 09:13:48AM -0800, Tony Lindgren wrote:
> * Russell King - ARM Linux admin <linux@armlinux.org.uk> [200214 13:40]:
> > On Fri, Feb 14, 2020 at 01:43:36PM +0200, Andy Shevchenko wrote:
> > > Doing any kind of power management for kernel console is really bad idea.
> > > 
> > > First of all, it runs in poll and atomic mode. This fact attaches a limitation
> > > on the functions that might be called. For example, pm_runtime_get_sync() might
> > > sleep and thus can't be used. This call needs, for example, to bring the device
> > > to powered on state on the system, where the power on sequence may require
> > > on-atomic operations, such as Intel Cherrytrail with ACPI enumerated UARTs.
> > > That said, on ACPI enabled platforms it might even call firmware for a job.
> > > 
> > > On the other hand pm_runtime_get() doesn't guarantee that device will become
> > > powered on fast enough.
> > > 
> > > Besides that, imagine the case when console is about to print a kernel Oops and
> > > it's powered off. In such an emergency case calling the complex functions is
> > > not the best what we can do, taking into consideration that user wants to see
> > > at least something of the last kernel word before it passes away.
> > > 
> > > Here we modify the 8250 console code to prevent runtime power management.
> > 
> > It's probably also worth noting (and documenting) that this will likely
> > cause a PM regression for OMAP platforms since the serial port will no
> > longer be idled, and therefore the power domains will not hit retention
> > state.  Please wait for Tony to confirm.
> 
> Well with patch 4 in this series we can now attach and detach the serial
> console as we've discussed earlier.
> 
> This should remove the need for PM during serial console use hopefully :)
> 
> I'll try to test this series ASAP.

Tony, I have realized that last patch (two patches) makes a regression. So,
I'll drop them in next version, please do not include them in your testing
bucket.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/8] serial: 8250_port: Don't use power management for kernel console

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [200214 11:44]:
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -608,6 +608,14 @@ static int univ8250_console_setup(struct console *co, char *options)
>  	return retval;
>  }
>  
> +static int univ8250_console_exit(struct console *co)
> +{
> +	struct uart_port *port;
> +
> +	port = &serial8250_ports[co->index].port;
> +	return serial8250_console_exit(port);
> +}
> +
>  /**
>   *	univ8250_console_match - non-standard console matching
>   *	@co:	  registering console
> @@ -666,6 +674,7 @@ static struct console univ8250_console = {
>  	.write		= univ8250_console_write,
>  	.device		= uart_console_device,
>  	.setup		= univ8250_console_setup,
> +	.exit		= univ8250_console_exit,
>  	.match		= univ8250_console_match,
>  	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
>  	.index		= -1,

You're missing adding exit to struct console or these patches
are based on some tree I don't have. I had to add the following
change to compile for you to fold into this patch.

Regards,

Tony

8< -------------------
diff --git a/include/linux/console.h b/include/linux/console.h
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -148,6 +148,7 @@ struct console {
 	struct tty_driver *(*device)(struct console *, int *);
 	void	(*unblank)(void);
 	int	(*setup)(struct console *, char *);
+	int	(*exit)(struct console *);
 	int	(*match)(struct console *, char *name, int idx, char *options);
 	short	flags;
 	short	index;

Re: [PATCH v2 5/8] serial: 8250_port: Don't use power management for kernel console

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [200214 18:10]:
> On Fri, Feb 14, 2020 at 09:13:48AM -0800, Tony Lindgren wrote:
> > * Russell King - ARM Linux admin <linux@armlinux.org.uk> [200214 13:40]:
> > > On Fri, Feb 14, 2020 at 01:43:36PM +0200, Andy Shevchenko wrote:
> > > > Doing any kind of power management for kernel console is really bad idea.
> > > > 
> > > > First of all, it runs in poll and atomic mode. This fact attaches a limitation
> > > > on the functions that might be called. For example, pm_runtime_get_sync() might
> > > > sleep and thus can't be used. This call needs, for example, to bring the device
> > > > to powered on state on the system, where the power on sequence may require
> > > > on-atomic operations, such as Intel Cherrytrail with ACPI enumerated UARTs.
> > > > That said, on ACPI enabled platforms it might even call firmware for a job.
> > > > 
> > > > On the other hand pm_runtime_get() doesn't guarantee that device will become
> > > > powered on fast enough.
> > > > 
> > > > Besides that, imagine the case when console is about to print a kernel Oops and
> > > > it's powered off. In such an emergency case calling the complex functions is
> > > > not the best what we can do, taking into consideration that user wants to see
> > > > at least something of the last kernel word before it passes away.
> > > > 
> > > > Here we modify the 8250 console code to prevent runtime power management.
> > > 
> > > It's probably also worth noting (and documenting) that this will likely
> > > cause a PM regression for OMAP platforms since the serial port will no
> > > longer be idled, and therefore the power domains will not hit retention
> > > state.  Please wait for Tony to confirm.
> > 
> > Well with patch 4 in this series we can now attach and detach the serial
> > console as we've discussed earlier.
> > 
> > This should remove the need for PM during serial console use hopefully :)
> > 
> > I'll try to test this series ASAP.
> 
> Tony, I have realized that last patch (two patches) makes a regression. So,
> I'll drop them in next version, please do not include them in your testing
> bucket.

OK. So far no luck getting console UART idled after detaching the
console with:

# echo N > /sys/class/tty/ttyS2/console

Any ideas what might be missing still?

Also, maybe we can get rid of the console_suspend_enabled stuff now
too in drivers and handle it in a generic way?

Regards,

Tony

Re: [PATCH v2 5/8] serial: 8250_port: Don't use power management for kernel console

[Andy Shevchenko]

On Fri, Feb 14, 2020 at 10:14:07AM -0800, Tony Lindgren wrote:
> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [200214 11:44]:
> > --- a/drivers/tty/serial/8250/8250_core.c
> > +++ b/drivers/tty/serial/8250/8250_core.c
> > @@ -608,6 +608,14 @@ static int univ8250_console_setup(struct console *co, char *options)
> >  	return retval;
> >  }
> >  
> > +static int univ8250_console_exit(struct console *co)
> > +{
> > +	struct uart_port *port;
> > +
> > +	port = &serial8250_ports[co->index].port;
> > +	return serial8250_console_exit(port);
> > +}
> > +
> >  /**
> >   *	univ8250_console_match - non-standard console matching
> >   *	@co:	  registering console
> > @@ -666,6 +674,7 @@ static struct console univ8250_console = {
> >  	.write		= univ8250_console_write,
> >  	.device		= uart_console_device,
> >  	.setup		= univ8250_console_setup,
> > +	.exit		= univ8250_console_exit,
> >  	.match		= univ8250_console_match,
> >  	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
> >  	.index		= -1,
> 
> You're missing adding exit to struct console or these patches
> are based on some tree I don't have. I had to add the following
> change to compile for you to fold into this patch.

In cover letter the [1] link answers to this.
Can you try again with that series being applied first?

> 
> Regards,
> 
> Tony
> 
> 8< -------------------
> diff --git a/include/linux/console.h b/include/linux/console.h
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -148,6 +148,7 @@ struct console {
>  	struct tty_driver *(*device)(struct console *, int *);
>  	void	(*unblank)(void);
>  	int	(*setup)(struct console *, char *);
> +	int	(*exit)(struct console *);
>  	int	(*match)(struct console *, char *name, int idx, char *options);
>  	short	flags;
>  	short	index;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/8] serial: 8250_port: Don't use power management for kernel console

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [200214 18:57]:
> On Fri, Feb 14, 2020 at 10:14:07AM -0800, Tony Lindgren wrote:
> > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [200214 11:44]:
> > > --- a/drivers/tty/serial/8250/8250_core.c
> > > +++ b/drivers/tty/serial/8250/8250_core.c
> > > @@ -608,6 +608,14 @@ static int univ8250_console_setup(struct console *co, char *options)
> > >  	return retval;
> > >  }
> > >  
> > > +static int univ8250_console_exit(struct console *co)
> > > +{
> > > +	struct uart_port *port;
> > > +
> > > +	port = &serial8250_ports[co->index].port;
> > > +	return serial8250_console_exit(port);
> > > +}
> > > +
> > >  /**
> > >   *	univ8250_console_match - non-standard console matching
> > >   *	@co:	  registering console
> > > @@ -666,6 +674,7 @@ static struct console univ8250_console = {
> > >  	.write		= univ8250_console_write,
> > >  	.device		= uart_console_device,
> > >  	.setup		= univ8250_console_setup,
> > > +	.exit		= univ8250_console_exit,
> > >  	.match		= univ8250_console_match,
> > >  	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
> > >  	.index		= -1,
> > 
> > You're missing adding exit to struct console or these patches
> > are based on some tree I don't have. I had to add the following
> > change to compile for you to fold into this patch.
> 
> In cover letter the [1] link answers to this.
> Can you try again with that series being applied first?

Ah OK thanks will try with that applied.

Regards,

Tony

Re: [PATCH v2 5/8] serial: 8250_port: Don't use power management for kernel console

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [200214 10:42]:
> OK. So far no luck getting console UART idled after detaching the
> console with:
> 
> # echo N > /sys/class/tty/ttyS2/console
> 
> Any ideas what might be missing still?

Looks like echo Y keeps increasing the usage count while
echo N will never decrease it currently. Is port->dev
maybe already NULL in serial8250_console_exit() or something?

This is still without the series you listed as a dependency
though. I need to test with that later on today when I get
a chance.

Regards,

Tony

Re: [PATCH v2 0/8] serial: Disable DMA and PM on kernel console

[Tony Lindgren]

* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [200214 20:03]:
> On Fri, Feb 14, 2020 at 01:43:31PM +0200, Andy Shevchenko wrote:
> > This is second attempt [1] to get rid of problematic DMA and PM calls
> > in the serial kernel console code.
> > 
> > Kernel console is sensitive to any kind of complex work needed to print
> > out anything on it. One such case is emergency print during Oops.
> > 
> > Patches 1-3 are preparatory ones.
> 
> I've applied these first 3, as they are "obvious" :)
> 
> I'll let others weigh in on the other patches here, as I'd like to see
> if Tony and others feel it solves their issues or not.

Yes with the pm_runtime_get_sync/pm_runtime_put_sync change for
patch 5/8 things work nicely for me. But as it looks like there will
be another revision of the patches so I'll wait for that before
acking.

Regards,

Tony

Re: [PATCH v2 3/8] serial: core: use octal permissions on module param

[Andy Shevchenko]

On Fri, Feb 14, 2020 at 08:27:19AM -0800, Greg Kroah-Hartman wrote:

> I'll take these,

Thanks!

> but we should move them all to DEVICE_ATTR_RO() as that
> would make things a lot more "obvious" what is happening here.

I'll do it in the next version as preparatory patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/8] serial: 8250_port: Disable DMA operations for kernel console

[Andy Shevchenko]

On Fri, Feb 14, 2020 at 08:30:58AM -0800, Greg Kroah-Hartman wrote:
> On Fri, Feb 14, 2020 at 01:43:37PM +0200, Andy Shevchenko wrote:

> > +		if (msg) {
> > +			pr_warn_ratelimited("%s - %s\n", port->name, msg);
> 
> dev_warn_ratelimited()?  You have a port, you should use it.

I will change this in next version, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/8] serial: Disable DMA and PM on kernel console

[Andy Shevchenko]

On Fri, Feb 14, 2020 at 01:41:28PM -0800, Tony Lindgren wrote:
> * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [200214 20:03]:
> > On Fri, Feb 14, 2020 at 01:43:31PM +0200, Andy Shevchenko wrote:
> > > This is second attempt [1] to get rid of problematic DMA and PM calls
> > > in the serial kernel console code.
> > > 
> > > Kernel console is sensitive to any kind of complex work needed to print
> > > out anything on it. One such case is emergency print during Oops.
> > > 
> > > Patches 1-3 are preparatory ones.
> > 
> > I've applied these first 3, as they are "obvious" :)
> > 
> > I'll let others weigh in on the other patches here, as I'd like to see
> > if Tony and others feel it solves their issues or not.
> 
> Yes with the pm_runtime_get_sync/pm_runtime_put_sync change for
> patch 5/8 things work nicely for me. But as it looks like there will
> be another revision of the patches so I'll wait for that before
> acking.

I will send a new version soon. Thanks for testing!

I hope to see Tested-by tags as well ;)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/8] serial: core: Allow detach and attach serial device for console

[Andy Shevchenko]

On Fri, Feb 14, 2020 at 08:28:47AM -0800, Greg Kroah-Hartman wrote:
> On Fri, Feb 14, 2020 at 01:43:35PM +0200, Andy Shevchenko wrote:

...

> >  static DEVICE_ATTR(iomem_base, 0440, uart_get_attr_iomem_base, NULL);
> >  static DEVICE_ATTR(iomem_reg_shift, 0440, uart_get_attr_iomem_reg_shift, NULL);
> > +static DEVICE_ATTR(console, 0640, uart_get_attr_console, uart_set_attr_console);
> 
> Again, DEVICE_ATTR_RW() in the future?

Fixed for next version, thanks!

-- 
With Best Regards,
Andy Shevchenko