On Tue, May 10, 2022 at 11:37:23AM +0200, Petr Mladek wrote: > On Mon 2022-05-09 09:36:40, Neil Armstrong wrote: > > On 08/05/2022 12:35, John Ogness wrote: > > > The uart_ops startup() callback is called without interrupts > > > disabled and without port->lock locked, relatively late during the > > > boot process (from the call path of console_on_rootfs()). If the > > > device is a console, it was already previously registered and could > > > be actively printing messages. > > > > > > Since the startup() callback is reading/writing registers used by > > > the console write() callback (AML_UART_CONTROL), its access must > > > be synchronized using the port->lock. Currently it is not. > > > > > > The startup() callback is the only function that explicitly enables > > > interrupts. Without the synchronization, it is possible that > > > interrupts become accidentally permanently disabled. > > > > > > CPU0 CPU1 > > > meson_serial_console_write meson_uart_startup > > > -------------------------- ------------------ > > > spin_lock(port->lock) > > > val = readl(AML_UART_CONTROL) > > > uart_console_write() > > > writel(INT_EN, AML_UART_CONTROL) > > > writel(val, AML_UART_CONTROL) > > > spin_unlock(port->lock) > > > > > > Add port->lock synchronization to meson_uart_startup() to avoid > > > racing with meson_serial_console_write(). > > > > > > Also add detailed comments to meson_uart_reset() explaining why it > > > is *not* using port->lock synchronization. > > > > > > Link: https://lore.kernel.org/lkml/2a82eae7-a256-f70c-fd82-4e510750906e@xxxxxxxxxxx > > > Reported-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> > > > Signed-off-by: John Ogness <john.ogness@xxxxxxxxxxxxx> > > > Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> > > > --- > > > drivers/tty/serial/meson_uart.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c > > > index 2bf1c57e0981..39021dac09cc 100644 > > > --- a/drivers/tty/serial/meson_uart.c > > > +++ b/drivers/tty/serial/meson_uart.c > > > @@ -253,6 +253,14 @@ static const char *meson_uart_type(struct uart_port *port) > > > return (port->type == PORT_MESON) ? "meson_uart" : NULL; > > > } > > > +/* > > > + * This function is called only from probe() using a temporary io mapping > > > + * in order to perform a reset before setting up the device. Since the > > > + * temporarily mapped region was successfully requested, there can be no > > > + * console on this port at this time. Hence it is not necessary for this > > > + * function to acquire the port->lock. (Since there is no console on this > > > + * port at this time, the port->lock is not initialized yet.) > > > + */ > > > static void meson_uart_reset(struct uart_port *port) > > > { > > > u32 val; > > > @@ -267,9 +275,12 @@ static void meson_uart_reset(struct uart_port *port) > > > static int meson_uart_startup(struct uart_port *port) > > > { > > > + unsigned long flags; > > > u32 val; > > > int ret = 0; > > > + spin_lock_irqsave(&port->lock, flags); > > > + > > > val = readl(port->membase + AML_UART_CONTROL); > > > val |= AML_UART_CLEAR_ERR; > > > writel(val, port->membase + AML_UART_CONTROL); > > > @@ -285,6 +296,8 @@ static int meson_uart_startup(struct uart_port *port) > > > val = (AML_UART_RECV_IRQ(1) | AML_UART_XMIT_IRQ(port->fifosize / 2)); > > > writel(val, port->membase + AML_UART_MISC); > > > + spin_unlock_irqrestore(&port->lock, flags); > > > + > > > ret = request_irq(port->irq, meson_uart_interrupt, 0, > > > port->name, port); > > > > > > base-commit: 672c0c5173427e6b3e2a9bbb7be51ceeec78093a > > > > Thanks for fixing this, it may also fix an uart apparent lockup I encountered > > several time while developing on the platform, but the target was still alive > > so it matches. > > > > So I'll add: > > Fixes: ff7693d079e5 ("ARM: meson: serial: add MesonX SoC on-chip uart driver") > > > > and > > > > Acked-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx> > > Neil, may I assume that you are going to queue this fix for 5.19, please? I can take it, thanks. greg k-h
