On Sat, Mar 2, 2024 at 12:48 PM Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > > On Mon, Feb 26, 2024 at 11:23:26AM -0800, Peter Collingbourne wrote: > > During the handoff from earlycon to the real console driver, we have > > two separate drivers operating on the same device concurrently. In the > > case of the 8250 driver these concurrent accesses cause problems due > > to the driver's use of banked registers, controlled by LCR.DLAB. It is > > possible for the setup(), config_port(), pm() and set_mctrl() callbacks > > to set DLAB, which can cause the earlycon code that intends to access > > TX to instead access DLL, leading to missed output and corruption on > > the serial line due to unintended modifications to the baud rate. > > > > In particular, for setup() we have: > > > > univ8250_console_setup() > > -> serial8250_console_setup() > > -> uart_set_options() > > -> serial8250_set_termios() > > -> serial8250_do_set_termios() > > -> serial8250_do_set_divisor() > > > > For config_port() we have: > > > > serial8250_config_port() > > -> autoconfig() > > > > For pm() we have: > > > > serial8250_pm() > > -> serial8250_do_pm() > > -> serial8250_set_sleep() > > > > For set_mctrl() we have (for some devices): > > > > serial8250_set_mctrl() > > -> omap8250_set_mctrl() > > -> __omap8250_set_mctrl() > > > > To avoid such problems, let's make it so that the console is locked > > during pre-registration calls to these callbacks, which will prevent > > the earlycon driver from running concurrently. > > > > Remove the partial solution to this problem in the 8250 driver > > that locked the console only during autoconfig_irq(), as this would > > result in a deadlock with the new approach. The console continues > > to be locked during autoconfig_irq() because it can only be called > > through uart_configure_port(). > > > > Although this patch introduces more locking than strictly necessary > > (and in particular it also locks during the call to rs485_config() > > which is not affected by this issue as far as I can tell), it follows > > the principle that it is the responsibility of the generic console > > code to manage the earlycon handoff by ensuring that earlycon and real > > console driver code cannot run concurrently, and not the individual > > drivers. > > > > Signed-off-by: Peter Collingbourne <pcc@xxxxxxxxxx> > > Reviewed-by: John Ogness <john.ogness@xxxxxxxxxxxxx> > > Link: https://linux-review.googlesource.com/id/I7cf8124dcebf8618e6b2ee543fa5b25532de55d8 > > Why is a link to a gerrit review with no context other than this same > commit needed here? I usually add that to my patches so that the progression of the patch can be tracked. See: https://lore.kernel.org/all/CAMn1gO53G6-sZE8RiLAD2uERbW1XtvyZbRonNGbHonzD058yAA@xxxxxxxxxxxxxx/ https://lore.kernel.org/all/CAHk-=whFbgy4RXG11c_=S7O-248oWmwB_aZOcWzWMVh3w7=RCw@xxxxxxxxxxxxxx/ > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Cc: stable@xxxxxxxxxxxxxxx > > --- > > drivers/tty/serial/8250/8250_port.c | 6 ------ > > drivers/tty/serial/serial_core.c | 12 ++++++++++++ > > kernel/printk/printk.c | 21 ++++++++++++++++++--- > > 3 files changed, 30 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > > index 8ca061d3bbb9..1d65055dde27 100644 > > --- a/drivers/tty/serial/8250/8250_port.c > > +++ b/drivers/tty/serial/8250/8250_port.c > > @@ -1329,9 +1329,6 @@ static void autoconfig_irq(struct uart_8250_port *up) > > inb_p(ICP); > > } > > > > - if (uart_console(port)) > > - console_lock(); > > - > > /* forget possible initially masked and pending IRQ */ > > probe_irq_off(probe_irq_on()); > > save_mcr = serial8250_in_MCR(up); > > @@ -1371,9 +1368,6 @@ static void autoconfig_irq(struct uart_8250_port *up) > > if (port->flags & UPF_FOURPORT) > > outb_p(save_ICP, ICP); > > > > - if (uart_console(port)) > > - console_unlock(); > > - > > port->irq = (irq > 0) ? irq : 0; > > } > > > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > > index d6a58a9e072a..ff85ebd3a007 100644 > > --- a/drivers/tty/serial/serial_core.c > > +++ b/drivers/tty/serial/serial_core.c > > @@ -2608,7 +2608,12 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state, > > port->type = PORT_UNKNOWN; > > flags |= UART_CONFIG_TYPE; > > } > > + /* Synchronize with possible boot console. */ > > + if (uart_console(port)) > > + console_lock(); > > port->ops->config_port(port, flags); > > + if (uart_console(port)) > > + console_unlock(); > > } > > > > if (port->type != PORT_UNKNOWN) { > > @@ -2616,6 +2621,10 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state, > > > > uart_report_port(drv, port); > > > > + /* Synchronize with possible boot console. */ > > + if (uart_console(port)) > > + console_lock(); > > + > > /* Power up port for set_mctrl() */ > > uart_change_pm(state, UART_PM_STATE_ON); > > > > @@ -2632,6 +2641,9 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state, > > > > uart_rs485_config(port); > > > > + if (uart_console(port)) > > + console_unlock(); > > + > > /* > > * If this driver supports console, and it hasn't been > > * successfully registered yet, try to re-register it. > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > > index f2444b581e16..f51e4e5a869d 100644 > > --- a/kernel/printk/printk.c > > +++ b/kernel/printk/printk.c > > @@ -3263,6 +3263,21 @@ static int __init keep_bootcon_setup(char *str) > > > > early_param("keep_bootcon", keep_bootcon_setup); > > > > +static int console_call_setup(struct console *newcon, char *options) > > +{ > > + int err; > > + > > + if (!newcon->setup) > > + return 0; > > + > > + /* Synchronize with possible boot console. */ > > + console_lock(); > > + err = newcon->setup(newcon, options); > > + console_unlock(); > > + > > + return err; > > +} > > + > > /* > > * This is called by register_console() to try to match > > * the newly registered console with any of the ones selected > > @@ -3298,8 +3313,8 @@ static int try_enable_preferred_console(struct console *newcon, > > if (_braille_register_console(newcon, c)) > > return 0; > > > > - if (newcon->setup && > > - (err = newcon->setup(newcon, c->options)) != 0) > > + err = console_call_setup(newcon, c->options); > > + if (err != 0) > > Didn't checkpatch complain about this? It should be: > if (err) > right? No, it didn't complain. (It complained about the pre-existing assignment within an if, which is why I moved it out, but not about that.) Looks like if (err) is more popular, so I changed it in v3. > > return err; > > } > > newcon->flags |= CON_ENABLED; > > @@ -3325,7 +3340,7 @@ static void try_enable_default_console(struct console *newcon) > > if (newcon->index < 0) > > newcon->index = 0; > > > > - if (newcon->setup && newcon->setup(newcon, NULL) != 0) > > + if (console_call_setup(newcon, NULL) != 0) > > return; > > No way to pass an error back here? The only caller, register_console(), ignores errors from this function. Arguably it shouldn't, but that's a pre-existing issue. Peter
