diff mbox series

[2/2] serial: core: fix console port-lock regression

Message ID 20200909143101.15389-3-johan@kernel.org
State New
Headers show
Series [1/2] serial: core: fix port-lock initialisation | expand

Commit Message

Johan Hovold Sept. 9, 2020, 2:31 p.m. UTC
Fix the port-lock initialisation regression introduced by commit
a3cb39d258ef ("serial: core: Allow detach and attach serial device for
console") by making sure that the lock is again initialised during
console setup.

The console may be registered before the serial controller has been
probed in which case the port lock needs to be initialised during
console setup by a call to uart_set_options(). The console-detach
changes introduced a regression in several drivers by effectively
removing that initialisation by not initialising the lock when the port
is used as a console (which is always the case during console setup).

Add back the early lock initialisation and instead use a new
console-reinit flag to handle the case where a console is being
re-attached through sysfs.

The question whether the console-detach interface should have been added
in the first place is left for another discussion.

Note that the console-enabled check in uart_set_options() is not
redundant because of kgdboc, which can end up reinitialising an already
enabled console (see commit 42b6a1baa3ec ("serial_core: Don't
re-initialize a previously initialized spinlock.")).

Fixes: a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
Cc: stable <stable@vger.kernel.org>     # 5.7
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 32 +++++++++++++++-----------------
 include/linux/serial_core.h      |  1 +
 2 files changed, 16 insertions(+), 17 deletions(-)

Comments

Tony Lindgren Sept. 18, 2020, 7:16 a.m. UTC | #1
* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [200916 12:34]:
> On Mon, Sep 14, 2020 at 11:09:16AM +0300, Tony Lindgren wrote:

> > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [200910 09:27]:

> > > +Cc: Tony, let me add Tony to the discussion.

> > > 

> > > On Thu, Sep 10, 2020 at 09:35:27AM +0200, Johan Hovold wrote:

> > > > And what about power management

> > > > which was the reason for wanting this on OMAP in the first place; tty

> > > > core never calls shutdown() for a console port, not even when it's been

> > > > detached using the new interface.

> > > 

> > > That is interesting... Tony, do we have OMAP case working because of luck?

> > 

> > 8250_omap won't do anything unless autosuspend_timeout is configured for

> > the uart(s). If configured, then the 8250_omap will idle when console is

> > detached and the PM runtime usage count held by console is decremented, and

> > the configured autosuspend_timeout expires.

> > 

> > The console is still kept open by getty, so I don't see why shutdown() would

> > be called for the console port. But maybe I don't follow what you're

> > concerned about, let me know if you want me to check something :)

> 

> Is it possible to test configuration when you have kernel console enabled but

> no getty is run on it (perhaps something with ssh enabled access)?

> 

> Then kernel console should call ->shutdown on detaching, right?


I gave this a quick try and Johan is right, shutdown is not called on console
uart still even if detached and no getty running on that uart. I could not
figure out easily where exactly this gets blocked.. So far I did try setting
port->console = 0 on detach, then init it again on attach.

Regards,

Tony
diff mbox series

Patch

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 53b79e1fcbc8..124524ecfe26 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1916,24 +1916,12 @@  static inline bool uart_console_enabled(struct uart_port *port)
 	return uart_console(port) && (port->cons->flags & CON_ENABLED);
 }
 
-static void __uart_port_spin_lock_init(struct uart_port *port)
+static void uart_port_spin_lock_init(struct uart_port *port)
 {
 	spin_lock_init(&port->lock);
 	lockdep_set_class(&port->lock, &port_lock_key);
 }
 
-/*
- * 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;
-
-	__uart_port_spin_lock_init(port);
-}
-
 #if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)
 /**
  *	uart_console_write - write a console message to a serial port
@@ -2086,7 +2074,15 @@  uart_set_options(struct uart_port *port, struct console *co,
 	struct ktermios termios;
 	static struct ktermios dummy;
 
-	uart_port_spin_lock_init(port);
+	/*
+	 * Ensure that the serial-console lock is initialised early.
+	 *
+	 * Note that the console-enabled check is needed because of kgdboc,
+	 * which can end up calling uart_set_options() for an already enabled
+	 * console via tty_find_polling_driver() and uart_poll_init().
+	 */
+	if (!uart_console_enabled(port) && !port->console_reinit)
+		uart_port_spin_lock_init(port);
 
 	memset(&termios, 0, sizeof(struct ktermios));
 
@@ -2794,10 +2790,12 @@  static ssize_t console_store(struct device *dev,
 		if (oldconsole && !newconsole) {
 			ret = unregister_console(uport->cons);
 		} else if (!oldconsole && newconsole) {
-			if (uart_console(uport))
+			if (uart_console(uport)) {
+				uport->console_reinit = 1;
 				register_console(uport->cons);
-			else
+			} else {
 				ret = -ENOENT;
+			}
 		}
 	} else {
 		ret = -ENXIO;
@@ -2898,7 +2896,7 @@  int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	 * initialised.
 	 */
 	if (!uart_console_enabled(uport))
-		__uart_port_spin_lock_init(uport);
+		uart_port_spin_lock_init(uport);
 
 	if (uport->cons && uport->dev)
 		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 01fc4d9c9c54..8a99279a579b 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -248,6 +248,7 @@  struct uart_port {
 
 	unsigned char		hub6;			/* this should be in the 8250 driver */
 	unsigned char		suspended;
+	unsigned char		console_reinit;
 	const char		*name;			/* port name */
 	struct attribute_group	*attr_group;		/* port specific attributes */
 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */