[PATCH v3 0/6] serial: Disable DMA and PM on kernel console

[PATCH v3 0/6] serial: Disable DMA and PM on kernel console

[Andy Shevchenko]

This is third version to get rid of problematic DMA and PM calls in
the serial kernel console code.

Patches 1, 3 and 4 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 in the patch 2.

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.

More details on topic are in the commit messages of the patches 5 and 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, 3]. Petr, may you confirm that [3] is
immutable or even send Greg KH a PR?

Greg, see above note before applying, thanks!

[1]: https://lore.kernel.org/patchwork/cover/905632/
[2]: https://lore.kernel.org/lkml/20200203133130.11591-1-andriy.shevchenko@linux.intel.com/
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/log/?h=for-5.7-console-exit

Cc: Russell King <rmk+kernel@armlinux.org.uk>
Cc: Petr Mladek <pmladek@suse.com>

Changelog v3:
- dropped applied patches
- dropped "cleanup" DMA patches, that they were not tested and actually are regressions
- added DEVICE_ATTR_RO/_RW conversion patches (Greg)
- added pr_*() to dev_*() conversion patch (Greg)
- updated commit message to note OMAP behaviour change (Russell)
- replace run-time PM callbacks to be _sync() (Tony)

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 (6):
  serial: core: Switch to use DEVICE_ATTR_RO()
  serial: core: Allow detach and attach serial device for console
  serial: 8250_port: Switch to use DEVICE_ATTR_RW()
  serial: 8250_port: Use dev_*() instead of pr_*()
  serial: 8250_port: Don't use power management for kernel console
  serial: 8250_port: Disable DMA operations for kernel console

 Documentation/ABI/testing/sysfs-tty |   7 ++
 drivers/tty/serial/8250/8250_core.c |   9 +++
 drivers/tty/serial/8250/8250_port.c |  64 +++++++++------
 drivers/tty/serial/serial_core.c    | 117 ++++++++++++++++++++--------
 include/linux/serial_8250.h         |   1 +
 5 files changed, 141 insertions(+), 57 deletions(-)

--
2.25.0

[PATCH v3 1/6] serial: core: Switch to use DEVICE_ATTR_RO()

[Andy Shevchenko]

Move device attributes to DEVICE_ATTR_RO() as that would make things
a lot more "obvious" what is happening here.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/serial_core.c | 57 ++++++++++++++++----------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 7564bbd3061c..5444293fe2e8 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2614,7 +2614,7 @@ struct tty_driver *uart_console_device(struct console *co, int *index)
 }
 EXPORT_SYMBOL_GPL(uart_console_device);
 
-static ssize_t uart_get_attr_uartclk(struct device *dev,
+static ssize_t uartclk_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2624,7 +2624,7 @@ static ssize_t uart_get_attr_uartclk(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.baud_base * 16);
 }
 
-static ssize_t uart_get_attr_type(struct device *dev,
+static ssize_t type_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2633,7 +2633,8 @@ static ssize_t uart_get_attr_type(struct device *dev,
 	uart_get_info(port, &tmp);
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.type);
 }
-static ssize_t uart_get_attr_line(struct device *dev,
+
+static ssize_t line_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2643,7 +2644,7 @@ static ssize_t uart_get_attr_line(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.line);
 }
 
-static ssize_t uart_get_attr_port(struct device *dev,
+static ssize_t port_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2657,7 +2658,7 @@ static ssize_t uart_get_attr_port(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "0x%lX\n", ioaddr);
 }
 
-static ssize_t uart_get_attr_irq(struct device *dev,
+static ssize_t irq_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2667,7 +2668,7 @@ static ssize_t uart_get_attr_irq(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.irq);
 }
 
-static ssize_t uart_get_attr_flags(struct device *dev,
+static ssize_t flags_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2677,7 +2678,7 @@ static ssize_t uart_get_attr_flags(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "0x%X\n", tmp.flags);
 }
 
-static ssize_t uart_get_attr_xmit_fifo_size(struct device *dev,
+static ssize_t xmit_fifo_size_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2687,8 +2688,7 @@ static ssize_t uart_get_attr_xmit_fifo_size(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.xmit_fifo_size);
 }
 
-
-static ssize_t uart_get_attr_close_delay(struct device *dev,
+static ssize_t close_delay_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2698,8 +2698,7 @@ static ssize_t uart_get_attr_close_delay(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.close_delay);
 }
 
-
-static ssize_t uart_get_attr_closing_wait(struct device *dev,
+static ssize_t closing_wait_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2709,7 +2708,7 @@ static ssize_t uart_get_attr_closing_wait(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.closing_wait);
 }
 
-static ssize_t uart_get_attr_custom_divisor(struct device *dev,
+static ssize_t custom_divisor_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2719,7 +2718,7 @@ static ssize_t uart_get_attr_custom_divisor(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.custom_divisor);
 }
 
-static ssize_t uart_get_attr_io_type(struct device *dev,
+static ssize_t io_type_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2729,7 +2728,7 @@ static ssize_t uart_get_attr_io_type(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.io_type);
 }
 
-static ssize_t uart_get_attr_iomem_base(struct device *dev,
+static ssize_t iomem_base_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2739,7 +2738,7 @@ static ssize_t uart_get_attr_iomem_base(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "0x%lX\n", (unsigned long)tmp.iomem_base);
 }
 
-static ssize_t uart_get_attr_iomem_reg_shift(struct device *dev,
+static ssize_t iomem_reg_shift_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct serial_struct tmp;
@@ -2749,28 +2748,28 @@ 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, 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 DEVICE_ATTR_RO(uartclk);
+static DEVICE_ATTR_RO(type);
+static DEVICE_ATTR_RO(line);
+static DEVICE_ATTR_RO(port);
+static DEVICE_ATTR_RO(irq);
+static DEVICE_ATTR_RO(flags);
+static DEVICE_ATTR_RO(xmit_fifo_size);
+static DEVICE_ATTR_RO(close_delay);
+static DEVICE_ATTR_RO(closing_wait);
+static DEVICE_ATTR_RO(custom_divisor);
+static DEVICE_ATTR_RO(io_type);
+static DEVICE_ATTR_RO(iomem_base);
+static DEVICE_ATTR_RO(iomem_reg_shift);
 
 static struct attribute *tty_dev_attrs[] = {
+	&dev_attr_uartclk.attr,
 	&dev_attr_type.attr,
 	&dev_attr_line.attr,
 	&dev_attr_port.attr,
 	&dev_attr_irq.attr,
 	&dev_attr_flags.attr,
 	&dev_attr_xmit_fifo_size.attr,
-	&dev_attr_uartclk.attr,
 	&dev_attr_close_delay.attr,
 	&dev_attr_closing_wait.attr,
 	&dev_attr_custom_divisor.attr,
-- 
2.25.0

[PATCH v3 2/6] 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 5444293fe2e8..20ab89300a98 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);
@@ -2748,6 +2748,56 @@ static ssize_t iomem_reg_shift_show(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", tmp.iomem_reg_shift);
 }
 
+static ssize_t console_show(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 console_store(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_RO(uartclk);
 static DEVICE_ATTR_RO(type);
 static DEVICE_ATTR_RO(line);
@@ -2761,6 +2811,7 @@ static DEVICE_ATTR_RO(custom_divisor);
 static DEVICE_ATTR_RO(io_type);
 static DEVICE_ATTR_RO(iomem_base);
 static DEVICE_ATTR_RO(iomem_reg_shift);
+static DEVICE_ATTR_RW(console);
 
 static struct attribute *tty_dev_attrs[] = {
 	&dev_attr_uartclk.attr,
@@ -2776,12 +2827,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 v3 3/6] serial: 8250_port: Switch to use DEVICE_ATTR_RW()

[Andy Shevchenko]

Move device attributes to DEVICE_ATTR_RW() as that would make things
a lot more "obvious" what is happening here.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 0325f2e53b74..4f5d4cce4c8e 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2880,7 +2880,7 @@ static int do_serial8250_get_rxtrig(struct tty_port *port)
 	return rxtrig_bytes;
 }
 
-static ssize_t serial8250_get_attr_rx_trig_bytes(struct device *dev,
+static ssize_t rx_trig_bytes_show(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
 	struct tty_port *port = dev_get_drvdata(dev);
@@ -2926,7 +2926,7 @@ static int do_serial8250_set_rxtrig(struct tty_port *port, unsigned char bytes)
 	return ret;
 }
 
-static ssize_t serial8250_set_attr_rx_trig_bytes(struct device *dev,
+static ssize_t rx_trig_bytes_store(struct device *dev,
 	struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct tty_port *port = dev_get_drvdata(dev);
@@ -2947,18 +2947,16 @@ static ssize_t serial8250_set_attr_rx_trig_bytes(struct device *dev,
 	return count;
 }
 
-static DEVICE_ATTR(rx_trig_bytes, S_IRUSR | S_IWUSR | S_IRGRP,
-		   serial8250_get_attr_rx_trig_bytes,
-		   serial8250_set_attr_rx_trig_bytes);
+static DEVICE_ATTR_RW(rx_trig_bytes);
 
 static struct attribute *serial8250_dev_attrs[] = {
 	&dev_attr_rx_trig_bytes.attr,
-	NULL,
-	};
+	NULL
+};
 
 static struct attribute_group serial8250_dev_attr_group = {
 	.attrs = serial8250_dev_attrs,
-	};
+};
 
 static void register_dev_spec_attr_grp(struct uart_8250_port *up)
 {
-- 
2.25.0

[PATCH v3 4/6] serial: 8250_port: Use dev_*() instead of pr_*()

[Andy Shevchenko]

Convert pr_*() calls to dev_*() ones. We have a port, we should use it.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 4f5d4cce4c8e..f398f162a1fd 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1318,8 +1318,8 @@ static void autoconfig(struct uart_8250_port *up)
 		fintek_8250_probe(up);
 
 	if (up->capabilities != old_capabilities) {
-		pr_warn("%s: detected caps %08x should be %08x\n",
-			port->name, old_capabilities, up->capabilities);
+		dev_warn(port->dev, "detected caps %08x should be %08x\n",
+			 old_capabilities, up->capabilities);
 	}
 out:
 	DEBUG_AUTOCONF("iir=%d ", scratch);
@@ -1683,7 +1683,7 @@ void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
 		lsr &= port->read_status_mask;
 
 		if (lsr & UART_LSR_BI) {
-			pr_debug("%s: handling break\n", __func__);
+			dev_dbg(port->dev, "handling break\n");
 			flag = TTY_BREAK;
 		} else if (lsr & UART_LSR_PE)
 			flag = TTY_PARITY;
@@ -2134,7 +2134,7 @@ int serial8250_do_startup(struct uart_port *port)
 	 */
 	if (!(port->flags & UPF_BUGGY_UART) &&
 	    (serial_port_in(port, UART_LSR) == 0xff)) {
-		pr_info_ratelimited("%s: LSR safety check engaged!\n", port->name);
+		dev_info_ratelimited(port->dev, "LSR safety check engaged!\n");
 		retval = -ENODEV;
 		goto out;
 	}
@@ -2166,8 +2166,7 @@ int serial8250_do_startup(struct uart_port *port)
 	     (port->type == PORT_ALTR_16550_F128)) && (port->fifosize > 1)) {
 		/* Bounds checking of TX threshold (valid 0 to fifosize-2) */
 		if ((up->tx_loadsz < 2) || (up->tx_loadsz > port->fifosize)) {
-			pr_err("%s TX FIFO Threshold errors, skipping\n",
-			       port->name);
+			dev_err(port->dev, "TX FIFO Threshold errors, skipping\n");
 		} else {
 			serial_port_out(port, UART_ALTR_AFR,
 					UART_ALTR_EN_TXFIFO_LW);
@@ -2268,8 +2267,7 @@ int serial8250_do_startup(struct uart_port *port)
 	if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT) {
 		if (!(up->bugs & UART_BUG_TXEN)) {
 			up->bugs |= UART_BUG_TXEN;
-			pr_debug("%s - enabling bad tx status workarounds\n",
-				 port->name);
+			dev_dbg(port->dev, "enabling bad tx status workarounds\n");
 		}
 	} else {
 		up->bugs &= ~UART_BUG_TXEN;
@@ -2296,8 +2294,7 @@ int serial8250_do_startup(struct uart_port *port)
 	if (up->dma) {
 		retval = serial8250_request_dma(up);
 		if (retval) {
-			pr_warn_ratelimited("%s - failed to request DMA\n",
-					    port->name);
+			dev_warn_ratelimited(port->dev, "failed to request DMA\n");
 			up->dma = NULL;
 		}
 	}
-- 
2.25.0

[PATCH v3 5/6] 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.

Note, there is a behaviour change for OMAP boards. It will require to detach
kernel console to become idle.

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 f398f162a1fd..f8a85244a6f1 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3122,6 +3122,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)
@@ -3133,8 +3136,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
@@ -3177,7 +3178,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)
@@ -3201,6 +3201,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;
@@ -3210,7 +3211,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_sync(port->dev);
+
+	return 0;
+}
+
+int serial8250_console_exit(struct uart_port *port)
+{
+	if (port->dev)
+		pm_runtime_put_sync(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 v3 6/6] 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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f8a85244a6f1..c1a49d671cdd 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2292,9 +2292,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) {
-			dev_warn_ratelimited(port->dev, "failed to request DMA\n");
+		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) {
+			dev_warn_ratelimited(port->dev, "%s\n", msg);
 			up->dma = NULL;
 		}
 	}
-- 
2.25.0

Re: [PATCH v3 0/6] serial: Disable DMA and PM on kernel console

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [200217 11:41]:
> This is third version to get rid of problematic DMA and PM calls in
> the serial kernel console code.
> 
> Patches 1, 3 and 4 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 in the patch 2.
> 
> 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.
> 
> More details on topic are in the commit messages of the patches 5 and 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, 3]. Petr, may you confirm that [3] is
> immutable or even send Greg KH a PR?
> 
> Greg, see above note before applying, thanks!
> 
> [1]: https://lore.kernel.org/patchwork/cover/905632/
> [2]: https://lore.kernel.org/lkml/20200203133130.11591-1-andriy.shevchenko@linux.intel.com/
> [3]: https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/log/?h=for-5.7-console-exit
> 
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Petr Mladek <pmladek@suse.com>
> 
> Changelog v3:
> - dropped applied patches
> - dropped "cleanup" DMA patches, that they were not tested and actually are regressions
> - added DEVICE_ATTR_RO/_RW conversion patches (Greg)
> - added pr_*() to dev_*() conversion patch (Greg)
> - updated commit message to note OMAP behaviour change (Russell)
> - replace run-time PM callbacks to be _sync() (Tony)

Nice, works for my test cases now! Please feel free to add:

Reviewed-by: Tony Lindgren <tony@atomide.com>
Tested-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH v3 0/6] serial: Disable DMA and PM on kernel console

[Petr Mladek]

On Mon 2020-02-17 13:40:10, Andy Shevchenko wrote:
> This is third version to get rid of problematic DMA and PM calls in
> the serial kernel console code.
> 
> Patches 1, 3 and 4 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 in the patch 2.
> 
> Note, it depends to recently submitted and applied patches in
> the core console code [2, 3]. Petr, may you confirm that [3] is
> immutable or even send Greg KH a PR?

Yes, the branch for-5.7-console-exit in printk.git is basically
immutable. I do my best to prevent rebasing.

> Greg, see above note before applying, thanks!
> 
> [1]: https://lore.kernel.org/patchwork/cover/905632/
> [2]: https://lore.kernel.org/lkml/20200203133130.11591-1-andriy.shevchenko@linux.intel.com/
> [3]: https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/log/?h=for-5.7-console-exit

Best Regards,
Petr

Re: [PATCH v3 0/6] serial: Disable DMA and PM on kernel console

[Andy Shevchenko]

On Tue, Feb 18, 2020 at 09:58:44AM +0100, Petr Mladek wrote:
> On Mon 2020-02-17 13:40:10, Andy Shevchenko wrote:
> > This is third version to get rid of problematic DMA and PM calls in
> > the serial kernel console code.
> > 
> > Patches 1, 3 and 4 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 in the patch 2.
> > 
> > Note, it depends to recently submitted and applied patches in
> > the core console code [2, 3]. Petr, may you confirm that [3] is
> > immutable or even send Greg KH a PR?
> 
> Yes, the branch for-5.7-console-exit in printk.git is basically
> immutable. I do my best to prevent rebasing.

Thanks for confirming this!

Greg, thanks for applying patches 1, 3, and 4. How can we proceed from this point?

> > Greg, see above note before applying, thanks!
> > 
> > [1]: https://lore.kernel.org/patchwork/cover/905632/
> > [2]: https://lore.kernel.org/lkml/20200203133130.11591-1-andriy.shevchenko@linux.intel.com/
> > [3]: https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/log/?h=for-5.7-console-exit

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/6] serial: Disable DMA and PM on kernel console

[Petr Mladek]

On Mon 2020-02-24 11:09:49, Andy Shevchenko wrote:
> On Tue, Feb 18, 2020 at 09:58:44AM +0100, Petr Mladek wrote:
> > On Mon 2020-02-17 13:40:10, Andy Shevchenko wrote:
> > > This is third version to get rid of problematic DMA and PM calls in
> > > the serial kernel console code.
> > > 
> > > Patches 1, 3 and 4 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 in the patch 2.
> > > 
> > > Note, it depends to recently submitted and applied patches in
> > > the core console code [2, 3]. Petr, may you confirm that [3] is
> > > immutable or even send Greg KH a PR?
> > 
> > Yes, the branch for-5.7-console-exit in printk.git is basically
> > immutable. I do my best to prevent rebasing.
> 
> Thanks for confirming this!
> 
> Greg, thanks for applying patches 1, 3, and 4. How can we proceed from this point?
> 
> > > Greg, see above note before applying, thanks!

The easiest solution would be when all patches go via one tree.

Greg, would you want to take the 7 patches from
https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/log/?h=for-5.7-console-exit
via your tree?

I have never done such shuffles between two maintainer trees.
Are there any preferred steps how to do so smoothly, please?

Best Regards,
Petr

Re: [PATCH v3 0/6] serial: Disable DMA and PM on kernel console

[Andy Shevchenko]

On Mon, Feb 24, 2020 at 01:23:46PM +0100, Petr Mladek wrote:
> On Mon 2020-02-24 11:09:49, Andy Shevchenko wrote:
> > On Tue, Feb 18, 2020 at 09:58:44AM +0100, Petr Mladek wrote:
> > > On Mon 2020-02-17 13:40:10, Andy Shevchenko wrote:
> > > > This is third version to get rid of problematic DMA and PM calls in
> > > > the serial kernel console code.
> > > > 
> > > > Patches 1, 3 and 4 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 in the patch 2.
> > > > 
> > > > Note, it depends to recently submitted and applied patches in
> > > > the core console code [2, 3]. Petr, may you confirm that [3] is
> > > > immutable or even send Greg KH a PR?
> > > 
> > > Yes, the branch for-5.7-console-exit in printk.git is basically
> > > immutable. I do my best to prevent rebasing.
> > 
> > Thanks for confirming this!
> > 
> > Greg, thanks for applying patches 1, 3, and 4. How can we proceed from this point?
> > 
> > > > Greg, see above note before applying, thanks!
> 
> The easiest solution would be when all patches go via one tree.
> 
> Greg, would you want to take the 7 patches from
> https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/log/?h=for-5.7-console-exit
> via your tree?

I think it would be nice to send formal PR to Greg.

> I have never done such shuffles between two maintainer trees.
> Are there any preferred steps how to do so smoothly, please?

Usually it's done via immutable branch / tag that all stakeholders
(including the initiator) can pull into their -next branches.

In any case, if Greg is considering next cycle, don't forget to apply this to
your -next :-) (I think it's already there)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/6] serial: Disable DMA and PM on kernel console

[Greg Kroah-Hartman]

On Mon, Feb 24, 2020 at 01:23:46PM +0100, Petr Mladek wrote:
> On Mon 2020-02-24 11:09:49, Andy Shevchenko wrote:
> > On Tue, Feb 18, 2020 at 09:58:44AM +0100, Petr Mladek wrote:
> > > On Mon 2020-02-17 13:40:10, Andy Shevchenko wrote:
> > > > This is third version to get rid of problematic DMA and PM calls in
> > > > the serial kernel console code.
> > > > 
> > > > Patches 1, 3 and 4 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 in the patch 2.
> > > > 
> > > > Note, it depends to recently submitted and applied patches in
> > > > the core console code [2, 3]. Petr, may you confirm that [3] is
> > > > immutable or even send Greg KH a PR?
> > > 
> > > Yes, the branch for-5.7-console-exit in printk.git is basically
> > > immutable. I do my best to prevent rebasing.
> > 
> > Thanks for confirming this!
> > 
> > Greg, thanks for applying patches 1, 3, and 4. How can we proceed from this point?
> > 
> > > > Greg, see above note before applying, thanks!
> 
> The easiest solution would be when all patches go via one tree.
> 
> Greg, would you want to take the 7 patches from
> https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/log/?h=for-5.7-console-exit
> via your tree?

I will pull this from your tree so please do not rebase :)

thanks,

greg k-h

Re: [PATCH v3 0/6] serial: Disable DMA and PM on kernel console

[Greg Kroah-Hartman]

On Tue, Mar 10, 2020 at 03:44:45PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 24, 2020 at 01:23:46PM +0100, Petr Mladek wrote:
> > On Mon 2020-02-24 11:09:49, Andy Shevchenko wrote:
> > > On Tue, Feb 18, 2020 at 09:58:44AM +0100, Petr Mladek wrote:
> > > > On Mon 2020-02-17 13:40:10, Andy Shevchenko wrote:
> > > > > This is third version to get rid of problematic DMA and PM calls in
> > > > > the serial kernel console code.
> > > > > 
> > > > > Patches 1, 3 and 4 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 in the patch 2.
> > > > > 
> > > > > Note, it depends to recently submitted and applied patches in
> > > > > the core console code [2, 3]. Petr, may you confirm that [3] is
> > > > > immutable or even send Greg KH a PR?
> > > > 
> > > > Yes, the branch for-5.7-console-exit in printk.git is basically
> > > > immutable. I do my best to prevent rebasing.
> > > 
> > > Thanks for confirming this!
> > > 
> > > Greg, thanks for applying patches 1, 3, and 4. How can we proceed from this point?
> > > 
> > > > > Greg, see above note before applying, thanks!
> > 
> > The easiest solution would be when all patches go via one tree.
> > 
> > Greg, would you want to take the 7 patches from
> > https://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git/log/?h=for-5.7-console-exit
> > via your tree?
> 
> I think it would be nice to send formal PR to Greg.
> 
> > I have never done such shuffles between two maintainer trees.
> > Are there any preferred steps how to do so smoothly, please?
> 
> Usually it's done via immutable branch / tag that all stakeholders
> (including the initiator) can pull into their -next branches.
> 
> In any case, if Greg is considering next cycle, don't forget to apply this to
> your -next :-) (I think it's already there)

I've queued all of these up now.  I hope :)

If I have missed anything, please let me know.

thanks,

greg k-h

Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Guenter Roeck]

Hi,

On Mon, Feb 17, 2020 at 01:40:12PM +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 5444293fe2e8..20ab89300a98 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))

This results in lockdep splashes such as the one attached below. Is there
any special reason for this change ? It is not really explained in the
commit description.

Thanks,
Guenter

---
[   15.439094] INFO: trying to register non-static key.
[   15.439146] the code is fine but needs lockdep annotation.
[   15.439196] turning off the locking correctness validator.
[   15.439392] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0-rc6-00244-gcaffb99b6929 #1
[   15.439469] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
[   15.439887] [<c0112578>] (unwind_backtrace) from [<c010c4f4>] (show_stack+0x10/0x14)
[   15.439982] [<c010c4f4>] (show_stack) from [<c06dfcb0>] (dump_stack+0xe4/0x11c)
[   15.440053] [<c06dfcb0>] (dump_stack) from [<c01883e4>] (register_lock_class+0x8a0/0x924)
[   15.440127] [<c01883e4>] (register_lock_class) from [<c01884d4>] (__lock_acquire+0x6c/0x2e80)
[   15.440202] [<c01884d4>] (__lock_acquire) from [<c018756c>] (lock_acquire+0xf8/0x4f4)
[   15.440274] [<c018756c>] (lock_acquire) from [<c0ddf02c>] (_raw_spin_lock_irqsave+0x50/0x64)
[   15.440350] [<c0ddf02c>] (_raw_spin_lock_irqsave) from [<c07af5d8>] (uart_add_one_port+0x3a4/0x504)
[   15.440431] [<c07af5d8>] (uart_add_one_port) from [<c089c990>] (platform_drv_probe+0x48/0x98)
[   15.440506] [<c089c990>] (platform_drv_probe) from [<c089a708>] (really_probe+0x214/0x344)
[   15.440578] [<c089a708>] (really_probe) from [<c089a948>] (driver_probe_device+0x5c/0x16c)
[   15.440650] [<c089a948>] (driver_probe_device) from [<c089ac00>] (device_driver_attach+0x58/0x60)
[   15.440727] [<c089ac00>] (device_driver_attach) from [<c089ac8c>] (__driver_attach+0x84/0xc0)
[   15.440800] [<c089ac8c>] (__driver_attach) from [<c08987e8>] (bus_for_each_dev+0x70/0xb4)
[   15.440874] [<c08987e8>] (bus_for_each_dev) from [<c08999a4>] (bus_add_driver+0x154/0x1e0)
[   15.440946] [<c08999a4>] (bus_add_driver) from [<c089ba38>] (driver_register+0x74/0x108)
[   15.441020] [<c089ba38>] (driver_register) from [<c144edb8>] (imx_uart_init+0x20/0x40)
[   15.441090] [<c144edb8>] (imx_uart_init) from [<c010232c>] (do_one_initcall+0x80/0x3ac)
[   15.441162] [<c010232c>] (do_one_initcall) from [<c1400ff0>] (kernel_init_freeable+0x170/0x204)
[   15.441241] [<c1400ff0>] (kernel_init_freeable) from [<c0dd5c48>] (kernel_init+0x8/0x118)
[   15.441313] [<c0dd5c48>] (kernel_init) from [<c0100134>] (ret_from_fork+0x14/0x20)
[   15.441414] Exception stack(0xc609ffb0 to 0xc609fff8)
[   15.441571] ffa0:                                     00000000 00000000 00000000 00000000
[   15.441738] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   15.441872] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000

Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Andy Shevchenko]

On Sun, May 24, 2020 at 10:10:32AM -0700, Guenter Roeck wrote:
> On Mon, Feb 17, 2020 at 01:40:12PM +0200, Andy Shevchenko wrote:

> > -	if (uart_console_enabled(port))
> > +	if (uart_console(port))
> 
> This results in lockdep splashes such as the one attached below. Is there
> any special reason for this change ? It is not really explained in the
> commit description.

Thanks for the report.

Yes, because imx_uart_init() doesn't properly register a console.
I'll send a quick fix for that soon.

> [   15.439094] INFO: trying to register non-static key.
> [   15.439146] the code is fine but needs lockdep annotation.
> [   15.439196] turning off the locking correctness validator.
> [   15.439392] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0-rc6-00244-gcaffb99b6929 #1
> [   15.439469] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> [   15.439887] [<c0112578>] (unwind_backtrace) from [<c010c4f4>] (show_stack+0x10/0x14)
> [   15.439982] [<c010c4f4>] (show_stack) from [<c06dfcb0>] (dump_stack+0xe4/0x11c)
> [   15.440053] [<c06dfcb0>] (dump_stack) from [<c01883e4>] (register_lock_class+0x8a0/0x924)
> [   15.440127] [<c01883e4>] (register_lock_class) from [<c01884d4>] (__lock_acquire+0x6c/0x2e80)
> [   15.440202] [<c01884d4>] (__lock_acquire) from [<c018756c>] (lock_acquire+0xf8/0x4f4)
> [   15.440274] [<c018756c>] (lock_acquire) from [<c0ddf02c>] (_raw_spin_lock_irqsave+0x50/0x64)
> [   15.440350] [<c0ddf02c>] (_raw_spin_lock_irqsave) from [<c07af5d8>] (uart_add_one_port+0x3a4/0x504)
> [   15.440431] [<c07af5d8>] (uart_add_one_port) from [<c089c990>] (platform_drv_probe+0x48/0x98)
> [   15.440506] [<c089c990>] (platform_drv_probe) from [<c089a708>] (really_probe+0x214/0x344)
> [   15.440578] [<c089a708>] (really_probe) from [<c089a948>] (driver_probe_device+0x5c/0x16c)
> [   15.440650] [<c089a948>] (driver_probe_device) from [<c089ac00>] (device_driver_attach+0x58/0x60)
> [   15.440727] [<c089ac00>] (device_driver_attach) from [<c089ac8c>] (__driver_attach+0x84/0xc0)
> [   15.440800] [<c089ac8c>] (__driver_attach) from [<c08987e8>] (bus_for_each_dev+0x70/0xb4)
> [   15.440874] [<c08987e8>] (bus_for_each_dev) from [<c08999a4>] (bus_add_driver+0x154/0x1e0)
> [   15.440946] [<c08999a4>] (bus_add_driver) from [<c089ba38>] (driver_register+0x74/0x108)
> [   15.441020] [<c089ba38>] (driver_register) from [<c144edb8>] (imx_uart_init+0x20/0x40)
> [   15.441090] [<c144edb8>] (imx_uart_init) from [<c010232c>] (do_one_initcall+0x80/0x3ac)
> [   15.441162] [<c010232c>] (do_one_initcall) from [<c1400ff0>] (kernel_init_freeable+0x170/0x204)
> [   15.441241] [<c1400ff0>] (kernel_init_freeable) from [<c0dd5c48>] (kernel_init+0x8/0x118)
> [   15.441313] [<c0dd5c48>] (kernel_init) from [<c0100134>] (ret_from_fork+0x14/0x20)
> [   15.441414] Exception stack(0xc609ffb0 to 0xc609fff8)
> [   15.441571] ffa0:                                     00000000 00000000 00000000 00000000
> [   15.441738] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [   15.441872] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Guenter Roeck]

On 5/25/20 3:38 AM, Andy Shevchenko wrote:
> On Sun, May 24, 2020 at 10:10:32AM -0700, Guenter Roeck wrote:
>> On Mon, Feb 17, 2020 at 01:40:12PM +0200, Andy Shevchenko wrote:
> 
>>> -	if (uart_console_enabled(port))
>>> +	if (uart_console(port))
>>
>> This results in lockdep splashes such as the one attached below. Is there
>> any special reason for this change ? It is not really explained in the
>> commit description.
> 
> Thanks for the report.
> 
> Yes, because imx_uart_init() doesn't properly register a console.
> I'll send a quick fix for that soon.
> 

A quick scan suggests that this is not the only driver with 'struct console'
which doesn't call register_console(). atmel_serial.c and ar933x_uart.c are
two more examples. Are you saying that all such drivers now generate this
lockdep splat ? Does that really make sense ?

Guenter

>> [   15.439094] INFO: trying to register non-static key.
>> [   15.439146] the code is fine but needs lockdep annotation.
>> [   15.439196] turning off the locking correctness validator.
>> [   15.439392] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0-rc6-00244-gcaffb99b6929 #1
>> [   15.439469] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
>> [   15.439887] [<c0112578>] (unwind_backtrace) from [<c010c4f4>] (show_stack+0x10/0x14)
>> [   15.439982] [<c010c4f4>] (show_stack) from [<c06dfcb0>] (dump_stack+0xe4/0x11c)
>> [   15.440053] [<c06dfcb0>] (dump_stack) from [<c01883e4>] (register_lock_class+0x8a0/0x924)
>> [   15.440127] [<c01883e4>] (register_lock_class) from [<c01884d4>] (__lock_acquire+0x6c/0x2e80)
>> [   15.440202] [<c01884d4>] (__lock_acquire) from [<c018756c>] (lock_acquire+0xf8/0x4f4)
>> [   15.440274] [<c018756c>] (lock_acquire) from [<c0ddf02c>] (_raw_spin_lock_irqsave+0x50/0x64)
>> [   15.440350] [<c0ddf02c>] (_raw_spin_lock_irqsave) from [<c07af5d8>] (uart_add_one_port+0x3a4/0x504)
>> [   15.440431] [<c07af5d8>] (uart_add_one_port) from [<c089c990>] (platform_drv_probe+0x48/kk0x98)
>> [   15.440506] [<c089c990>] (platform_drv_probe) from [<c089a708>] (really_probe+0x214/0x344)
>> [   15.440578] [<c089a708>] (really_probe) from [<c089a948>] (driver_probe_device+0x5c/0x16c)
>> [   15.440650] [<c089a948>] (driver_probe_device) from [<c089ac00>] (device_driver_attach+0x58/0x60)
>> [   15.440727] [<c089ac00>] (device_driver_attach) from [<c089ac8c>] (__driver_attach+0x84/0xc0)
>> [   15.440800] [<c089ac8c>] (__driver_attach) from [<c08987e8>] (bus_for_each_dev+0x70/0xb4)
>> [   15.440874] [<c08987e8>] (bus_for_each_dev) from [<c08999a4>] (bus_add_driver+0x154/0x1e0)
>> [   15.440946] [<c08999a4>] (bus_add_driver) from [<c089ba38>] (driver_register+0x74/0x108)
>> [   15.441020] [<c089ba38>] (driver_register) from [<c144edb8>] (imx_uart_init+0x20/0x40)
>> [   15.441090] [<c144edb8>] (imx_uart_init) from [<c010232c>] (do_one_initcall+0x80/0x3ac)
>> [   15.441162] [<c010232c>] (do_one_initcall) from [<c1400ff0>] (kernel_init_freeable+0x170/0x204)
>> [   15.441241] [<c1400ff0>] (kernel_init_freeable) from [<c0dd5c48>] (kernel_init+0x8/0x118)
>> [   15.441313] [<c0dd5c48>] (kernel_init) from [<c0100134>] (ret_from_fork+0x14/0x20)
>> [   15.441414] Exception stack(0xc609ffb0 to 0xc609fff8)
>> [   15.441571] ffa0:                                     00000000 00000000 00000000 00000000
>> [   15.441738] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [   15.441872] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
> 

Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Geert Uytterhoeven]

Hi Andy,

On Sun, May 24, 2020 at 7:11 PM Guenter Roeck <linux@roeck-us.net> wrote:
> On Mon, Feb 17, 2020 at 01:40:12PM +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>

> > --- 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))
>
> This results in lockdep splashes such as the one attached below. Is there

Or "BUG: spinlock bad magic on CPU#3, swapper/0/1", cfr. [1].
So far I hadn't noticed that, as the issue only shows up when using the
legacy way of passing a "console=ttyS*" kernel command line parameter,
and not when relying on the modern "chosen/stdout-path" DT property.

> any special reason for this change ? It is not really explained in the
> commit description.

Indeed. Why this change?

I also don't agree with your typical fix for drivers, which is like:

    @@ -567,6 +567,9 @@ static int hv_probe(struct platform_device *op)
            sunserial_console_match(&sunhv_console, op->dev.of_node,
                                    &sunhv_reg, port->line, false);

    +       /* We need to initialize lock even for non-registered console */
    +       spin_lock_init(&port->lock);
    +
            err = uart_add_one_port(&sunhv_reg, port);
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                  calls uart_port_spin_lock_init()

            if (err)
                    goto out_unregister_driver;

as this initializes the spinlock twice for non-console= ports.

> [   15.439094] INFO: trying to register non-static key.
> [   15.439146] the code is fine but needs lockdep annotation.
> [   15.439196] turning off the locking correctness validator.
> [   15.439392] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0-rc6-00244-gcaffb99b6929 #1
> [   15.439469] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> [   15.439887] [<c0112578>] (unwind_backtrace) from [<c010c4f4>] (show_stack+0x10/0x14)
> [   15.439982] [<c010c4f4>] (show_stack) from [<c06dfcb0>] (dump_stack+0xe4/0x11c)
> [   15.440053] [<c06dfcb0>] (dump_stack) from [<c01883e4>] (register_lock_class+0x8a0/0x924)
> [   15.440127] [<c01883e4>] (register_lock_class) from [<c01884d4>] (__lock_acquire+0x6c/0x2e80)
> [   15.440202] [<c01884d4>] (__lock_acquire) from [<c018756c>] (lock_acquire+0xf8/0x4f4)
> [   15.440274] [<c018756c>] (lock_acquire) from [<c0ddf02c>] (_raw_spin_lock_irqsave+0x50/0x64)
> [   15.440350] [<c0ddf02c>] (_raw_spin_lock_irqsave) from [<c07af5d8>] (uart_add_one_port+0x3a4/0x504)
> [   15.440431] [<c07af5d8>] (uart_add_one_port) from [<c089c990>] (platform_drv_probe+0x48/0x98)
> [   15.440506] [<c089c990>] (platform_drv_probe) from [<c089a708>] (really_probe+0x214/0x344)
> [   15.440578] [<c089a708>] (really_probe) from [<c089a948>] (driver_probe_device+0x5c/0x16c)
> [   15.440650] [<c089a948>] (driver_probe_device) from [<c089ac00>] (device_driver_attach+0x58/0x60)
> [   15.440727] [<c089ac00>] (device_driver_attach) from [<c089ac8c>] (__driver_attach+0x84/0xc0)
> [   15.440800] [<c089ac8c>] (__driver_attach) from [<c08987e8>] (bus_for_each_dev+0x70/0xb4)
> [   15.440874] [<c08987e8>] (bus_for_each_dev) from [<c08999a4>] (bus_add_driver+0x154/0x1e0)
> [   15.440946] [<c08999a4>] (bus_add_driver) from [<c089ba38>] (driver_register+0x74/0x108)
> [   15.441020] [<c089ba38>] (driver_register) from [<c144edb8>] (imx_uart_init+0x20/0x40)
> [   15.441090] [<c144edb8>] (imx_uart_init) from [<c010232c>] (do_one_initcall+0x80/0x3ac)
> [   15.441162] [<c010232c>] (do_one_initcall) from [<c1400ff0>] (kernel_init_freeable+0x170/0x204)
> [   15.441241] [<c1400ff0>] (kernel_init_freeable) from [<c0dd5c48>] (kernel_init+0x8/0x118)
> [   15.441313] [<c0dd5c48>] (kernel_init) from [<c0100134>] (ret_from_fork+0x14/0x20)
> [   15.441414] Exception stack(0xc609ffb0 to 0xc609fff8)
> [   15.441571] ffa0:                                     00000000 00000000 00000000 00000000
> [   15.441738] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [   15.441872] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000

Thanks for your answer!

[1] https://lore.kernel.org/r/1593618100-2151-1-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Tony Lindgren]

Hi,

* Geert Uytterhoeven <geert@linux-m68k.org> [200702 14:50]:
> On Sun, May 24, 2020 at 7:11 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > any special reason for this change ? It is not really explained in the
> > commit description.
> 
> Indeed. Why this change?

For a kernel console, we want it to work for important oopses
etc without trying to enable DMA or power on regulators for example.

And we want to get rid of pm_runtime_irq_safe() usage as that takes
a permanent usage count on the parent device. This causes issue where
uart can keep an interconnect instance from idling with let's say
genpd :)

Also, we cannot easily make the serial driver PM runtime generic
without removing the pm_runtime_irq_safe() dependency as it would
currently wrongly impose the same nasty dependency to all the serial
drivers.

So when a kernel console is attached, we want to keep it from idling.

For PM related testing, just detaching kernel console and configuring
it's autosuspend timeout works nicely with dmesg -w. Or actually in
that case using pstore console is even better.

Andy has a series pending for generic serial PM runtime support,
I guess he's going to be posting it as soon as the pending regressions
are dealt with.

Regards,

Tony

Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Geert Uytterhoeven]

Hi Tony,

On Thu, Jul 2, 2020 at 9:35 PM Tony Lindgren <tony@atomide.com> wrote:
> * Geert Uytterhoeven <geert@linux-m68k.org> [200702 14:50]:
> > On Sun, May 24, 2020 at 7:11 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > any special reason for this change ? It is not really explained in the
> > > commit description.
> >
> > Indeed. Why this change?
>
> For a kernel console, we want it to work for important oopses
> etc without trying to enable DMA or power on regulators for example.

[...]

Thanks for the explanation about irqsafe consoles!
I think I cannot disagree with that ;-)

Sorry for being a bit unclear, but my question (and I guess Günter's
question, too) was about this particular change:

     static inline void uart_port_spin_lock_init(struct uart_port *port)
     {
    -     if (uart_console_enabled(port))
    +     if (uart_console(port))

This change seems to be completely unrelated, is not explained in the
commit description, and is the cause of the regression we're seeing.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Guenter Roeck]

On 7/2/20 1:03 PM, Geert Uytterhoeven wrote:
> Hi Tony,
> 
> On Thu, Jul 2, 2020 at 9:35 PM Tony Lindgren <tony@atomide.com> wrote:
>> * Geert Uytterhoeven <geert@linux-m68k.org> [200702 14:50]:
>>> On Sun, May 24, 2020 at 7:11 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>> any special reason for this change ? It is not really explained in the
>>>> commit description.
>>>
>>> Indeed. Why this change?
>>
>> For a kernel console, we want it to work for important oopses
>> etc without trying to enable DMA or power on regulators for example.
> 
> [...]
> 
> Thanks for the explanation about irqsafe consoles!
> I think I cannot disagree with that ;-)
> 
> Sorry for being a bit unclear, but my question (and I guess Günter's
> question, too) was about this particular change:
> 
>      static inline void uart_port_spin_lock_init(struct uart_port *port)
>      {
>     -     if (uart_console_enabled(port))
>     +     if (uart_console(port))
> 
> This change seems to be completely unrelated, is not explained in the
> commit description, and is the cause of the regression we're seeing.
> 

Yes, that was my question as well. I still fail to understand what exactly
the explanation has to do with the problem. The same happened before,
so I concluded that I must be completely clueless, and gave up asking.

Guenter

Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Tony Lindgren]

* Geert Uytterhoeven <geert@linux-m68k.org> [200702 20:04]:
> Hi Tony,
> 
> On Thu, Jul 2, 2020 at 9:35 PM Tony Lindgren <tony@atomide.com> wrote:
> > * Geert Uytterhoeven <geert@linux-m68k.org> [200702 14:50]:
> > > On Sun, May 24, 2020 at 7:11 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > > any special reason for this change ? It is not really explained in the
> > > > commit description.
> > >
> > > Indeed. Why this change?
> >
> > For a kernel console, we want it to work for important oopses
> > etc without trying to enable DMA or power on regulators for example.
> 
> [...]
> 
> Thanks for the explanation about irqsafe consoles!
> I think I cannot disagree with that ;-)

You're welcome..

> Sorry for being a bit unclear, but my question (and I guess Günter's
> question, too) was about this particular change:
> 
>      static inline void uart_port_spin_lock_init(struct uart_port *port)
>      {
>     -     if (uart_console_enabled(port))
>     +     if (uart_console(port))
> 
> This change seems to be completely unrelated, is not explained in the
> commit description, and is the cause of the regression we're seeing.

..sorry looks I missed the context a bit then :) Hmm yeah not sure about
this change above.

Regards,

Tony

Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Geert Uytterhoeven]

Hi Andy,

On Thu, Jul 2, 2020 at 4:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Sun, May 24, 2020 at 7:11 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > On Mon, Feb 17, 2020 at 01:40:12PM +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>
>
> > > --- 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))
> >
> > This results in lockdep splashes such as the one attached below. Is there
>
> Or "BUG: spinlock bad magic on CPU#3, swapper/0/1", cfr. [1].
> So far I hadn't noticed that, as the issue only shows up when using the
> legacy way of passing a "console=ttyS*" kernel command line parameter,
> and not when relying on the modern "chosen/stdout-path" DT property.
>
> > any special reason for this change ? It is not really explained in the
> > commit description.
>
> Indeed. Why this change?
>
> I also don't agree with your typical fix for drivers, which is like:
>
>     @@ -567,6 +567,9 @@ static int hv_probe(struct platform_device *op)
>             sunserial_console_match(&sunhv_console, op->dev.of_node,
>                                     &sunhv_reg, port->line, false);
>
>     +       /* We need to initialize lock even for non-registered console */
>     +       spin_lock_init(&port->lock);
>     +
>             err = uart_add_one_port(&sunhv_reg, port);
>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                   calls uart_port_spin_lock_init()
>
>             if (err)
>                     goto out_unregister_driver;
>
> as this initializes the spinlock twice for non-console= ports.

I had a deeper look...

    /*
     * 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(port))
                    return;

            spin_lock_init(&port->lock);
            lockdep_set_class(&port->lock, &port_lock_key);
    }

So according to the comment, the spinlock is assumed to be already
initialized, as the port is already in use as a console.  Makes sense.
Now, where should it be initialized?
  1. For modern DT systems, chosen/stdout-path is used, and the spinlock
     is initialized in register_earlycon(), just before calling
     register_console(). And everything's fine.

  2. With "console=" (even on DT systems with chosen/stdout-path),
     the serial console must gets registered differently.
     Naively, I assumed that's done in the serial driver, but apparently
     that is no longer the case: the single register_console() call in
     drivers/tty/serial/sh-sci.c is used on legacy SuperH only.
     So we're back to drivers/tty/serial/serial_core.c, which calls
     register_console(), but does so _after_ taking the spinlock:

         uart_add_one_port()
             uart_port_spin_lock_init() /* skips spin_lock_init()! */
             uart_configure_port()
                 spin_lock_irqsave(&port->lock, flags); /* BUG! */
                 register_console())

So who's to blame for _not_ initializing the spinlock?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Andy Shevchenko]

On Fri, Jul 03, 2020 at 01:31:24PM +0200, Geert Uytterhoeven wrote:
> On Thu, Jul 2, 2020 at 4:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Sun, May 24, 2020 at 7:11 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > On Mon, Feb 17, 2020 at 01:40:12PM +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>
> >
> > > > --- 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))
> > >
> > > This results in lockdep splashes such as the one attached below. Is there
> >
> > Or "BUG: spinlock bad magic on CPU#3, swapper/0/1", cfr. [1].
> > So far I hadn't noticed that, as the issue only shows up when using the
> > legacy way of passing a "console=ttyS*" kernel command line parameter,
> > and not when relying on the modern "chosen/stdout-path" DT property.
> >
> > > any special reason for this change ? It is not really explained in the
> > > commit description.
> >
> > Indeed. Why this change?
> >
> > I also don't agree with your typical fix for drivers, which is like:
> >
> >     @@ -567,6 +567,9 @@ static int hv_probe(struct platform_device *op)
> >             sunserial_console_match(&sunhv_console, op->dev.of_node,
> >                                     &sunhv_reg, port->line, false);
> >
> >     +       /* We need to initialize lock even for non-registered console */
> >     +       spin_lock_init(&port->lock);
> >     +
> >             err = uart_add_one_port(&sunhv_reg, port);
> >                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >                   calls uart_port_spin_lock_init()
> >
> >             if (err)
> >                     goto out_unregister_driver;
> >
> > as this initializes the spinlock twice for non-console= ports.
> 
> I had a deeper look...
> 
>     /*
>      * 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(port))
>                     return;
> 
>             spin_lock_init(&port->lock);
>             lockdep_set_class(&port->lock, &port_lock_key);
>     }
> 
> So according to the comment, the spinlock is assumed to be already
> initialized, as the port is already in use as a console.  Makes sense.

Thanks, Geert! Yes, the change makes code aligned with a comment. I did it due
to some issues with attaching / detaching consoles (I can try to reproduce
later, perhaps next week, I'm a bit limited now to fulfil kernel work /
testing).

> Now, where should it be initialized?
>   1. For modern DT systems, chosen/stdout-path is used, and the spinlock
>      is initialized in register_earlycon(), just before calling
>      register_console(). And everything's fine.
> 
>   2. With "console=" (even on DT systems with chosen/stdout-path),
>      the serial console must gets registered differently.
>      Naively, I assumed that's done in the serial driver, but apparently
>      that is no longer the case: the single register_console() call in
>      drivers/tty/serial/sh-sci.c is used on legacy SuperH only.
>      So we're back to drivers/tty/serial/serial_core.c, which calls
>      register_console(), but does so _after_ taking the spinlock:
> 
>          uart_add_one_port()
>              uart_port_spin_lock_init() /* skips spin_lock_init()! */
>              uart_configure_port()
>                  spin_lock_irqsave(&port->lock, flags); /* BUG! */
>                  register_console())
> 
> So who's to blame for _not_ initializing the spinlock?

This is a very good question. Code is so old and I don't know why we have such
interesting implementation among serial drivers. The 8250 does this
initialisation at console_initcall() when it *properly* calls
register_console() before adding port (yet).

    /*
     * If this driver supports console, and it hasn't been
     * successfully registered yet, try to re-register it.
     * It may be that the port was not available.
     */
    if (port->cons && !(port->cons->flags & CON_ENABLED))
            register_console(port->cons);

Seems like a chicken-egg problem. Any advice?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/6] serial: core: Allow detach and attach serial device for console

[Andy Shevchenko]

On Sat, Jul 04, 2020 at 06:43:26PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 03, 2020 at 01:31:24PM +0200, Geert Uytterhoeven wrote:
> > On Thu, Jul 2, 2020 at 4:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Sun, May 24, 2020 at 7:11 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > > On Mon, Feb 17, 2020 at 01:40:12PM +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>
> > >
> > > > > --- 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))
> > > >
> > > > This results in lockdep splashes such as the one attached below. Is there
> > >
> > > Or "BUG: spinlock bad magic on CPU#3, swapper/0/1", cfr. [1].
> > > So far I hadn't noticed that, as the issue only shows up when using the
> > > legacy way of passing a "console=ttyS*" kernel command line parameter,
> > > and not when relying on the modern "chosen/stdout-path" DT property.
> > >
> > > > any special reason for this change ? It is not really explained in the
> > > > commit description.
> > >
> > > Indeed. Why this change?
> > >
> > > I also don't agree with your typical fix for drivers, which is like:
> > >
> > >     @@ -567,6 +567,9 @@ static int hv_probe(struct platform_device *op)
> > >             sunserial_console_match(&sunhv_console, op->dev.of_node,
> > >                                     &sunhv_reg, port->line, false);
> > >
> > >     +       /* We need to initialize lock even for non-registered console */
> > >     +       spin_lock_init(&port->lock);
> > >     +
> > >             err = uart_add_one_port(&sunhv_reg, port);
> > >                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >                   calls uart_port_spin_lock_init()
> > >
> > >             if (err)
> > >                     goto out_unregister_driver;
> > >
> > > as this initializes the spinlock twice for non-console= ports.
> > 
> > I had a deeper look...
> > 
> >     /*
> >      * 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(port))
> >                     return;
> > 
> >             spin_lock_init(&port->lock);
> >             lockdep_set_class(&port->lock, &port_lock_key);
> >     }
> > 
> > So according to the comment, the spinlock is assumed to be already
> > initialized, as the port is already in use as a console.  Makes sense.
> 
> Thanks, Geert! Yes, the change makes code aligned with a comment. I did it due
> to some issues with attaching / detaching consoles (I can try to reproduce
> later, perhaps next week, I'm a bit limited now to fulfil kernel work /
> testing).
> 
> > Now, where should it be initialized?
> >   1. For modern DT systems, chosen/stdout-path is used, and the spinlock
> >      is initialized in register_earlycon(), just before calling
> >      register_console(). And everything's fine.
> > 
> >   2. With "console=" (even on DT systems with chosen/stdout-path),
> >      the serial console must gets registered differently.
> >      Naively, I assumed that's done in the serial driver, but apparently
> >      that is no longer the case: the single register_console() call in
> >      drivers/tty/serial/sh-sci.c is used on legacy SuperH only.
> >      So we're back to drivers/tty/serial/serial_core.c, which calls
> >      register_console(), but does so _after_ taking the spinlock:
> > 
> >          uart_add_one_port()
> >              uart_port_spin_lock_init() /* skips spin_lock_init()! */
> >              uart_configure_port()
> >                  spin_lock_irqsave(&port->lock, flags); /* BUG! */
> >                  register_console())
> > 
> > So who's to blame for _not_ initializing the spinlock?
> 
> This is a very good question. Code is so old and I don't know why we have such
> interesting implementation among serial drivers. The 8250 does this
> initialisation at console_initcall() when it *properly* calls
> register_console() before adding port (yet).
> 
>     /*
>      * If this driver supports console, and it hasn't been
>      * successfully registered yet, try to re-register it.
>      * It may be that the port was not available.
>      */
>     if (port->cons && !(port->cons->flags & CON_ENABLED))
>             register_console(port->cons);
> 
> Seems like a chicken-egg problem. Any advice?

This comment even more interesting...

    /*
     * Ensure that the modem control lines are de-activated.
     * keep the DTR setting that is set in uart_set_options()
     * We probably don't need a spinlock around this, but
     */

Investigation shows that this comes from (see history.git from history group)
commit 33c0d1b0c3ebb61243d9b19ce70d9063acff2aac
Author: Russell King <rmk@arm.linux.org.uk>
Date:   Sun Jul 21 02:39:46 2002 -0700

    [PATCH] Serial driver stuff

According to documentation ->set_mctrl() is called with lock taken. Hmm...

So, removing that spin lock and moving spin lock initialisation call after
uart_configure_port should fix this for all. But I'm not sure we don't break
anything.

From d361b1b35f4e8d318c584f224d67240b775fbe7f Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Sat, 4 Jul 2020 19:30:39 +0300
Subject: [PATCH 1/1] serial: core: Drop ambiguous spin lock in
 uart_configure_port()

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 3cc183acf7ba..b4ed1e20dd5c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2374,11 +2374,8 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 		/*
 		 * Ensure that the modem control lines are de-activated.
 		 * keep the DTR setting that is set in uart_set_options()
-		 * We probably don't need a spinlock around this, but
 		 */
-		spin_lock_irqsave(&port->lock, flags);
 		port->ops->set_mctrl(port, port->mctrl & TIOCM_DTR);
-		spin_unlock_irqrestore(&port->lock, flags);
 
 		/*
 		 * If this driver supports console, and it hasn't been
@@ -2886,8 +2883,6 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 		goto out;
 	}
 
-	uart_port_spin_lock_init(uport);
-
 	if (uport->cons && uport->dev)
 		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
 
@@ -2896,6 +2891,8 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 
 	port->console = uart_console(uport);
 
+	uart_port_spin_lock_init(uport);
+
 	num_groups = 2;
 	if (uport->attr_group)
 		num_groups++;

-- 
With Best Regards,
Andy Shevchenko