Re: [PATCH] serial: amba-pl011: Implement nbcon console

['Greg Kroah-Hartman' <gregkh@linuxfoundation.org>]

On Thu, Jan 09, 2025 at 01:04:36AM +0000, Toshiyuki Sato (Fujitsu) wrote:
> Hi, Greg. Thanks for the comment.
> 
> > On Wed, Jan 08, 2025 at 12:47:30AM +0000, Toshiyuki Sato wrote:
> > > Implement the callbacks required for an NBCON console [0] on the
> > > amba-pl011 console driver.
> > > The codes for legacy console are retained, and the module
> > > parameter (use_nbcon) allows switching between legacy and NBCON.
> > > The default is off (use legacy console) for now.
> > >
> > > Referred to the NBCON implementation work for 8250 [1] and imx [2].
> > >
> > > The normal-priority write_thread checks for console ownership
> > > each time a character is printed.
> > > write_atomic holds the console ownership until the entire string
> > > is printed.
> > >
> > > UART register operations are protected from other contexts by
> > > uart_port_lock, except for a final flush(nbcon_atomic_flush_unsafe)
> > > on panic.
> > >
> > > The patch has been verified to correctly handle the output and
> > > competition of messages with different priorities and flushing
> > > panic message to console after nmi panic using ARM64 QEMU and
> > > a physical machine(A64FX).
> > >
> > > [0] https://lore.kernel.org/all/ZuRRTbapH0DCj334@pathway.suse.cz/
> > > [1]
> > https://lore.kernel.org/all/20240913140538.221708-1-john.ogness@linutronix.d
> > e/T/
> > > [2]
> > https://lore.kernel.org/linux-arm-kernel/20240913-serial-imx-nbcon-v3-1-4c62
> > 7302335b@geanix.com/T/
> > >
> > > Signed-off-by: Toshiyuki Sato <fj6611ie@aa.jp.fujitsu.com>
> > > ---
> > >  drivers/tty/serial/amba-pl011.c | 113
> > ++++++++++++++++++++++++++++++++
> > >  1 file changed, 113 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > > index 69b7a3e1e..52fab3170 100644
> > > --- a/drivers/tty/serial/amba-pl011.c
> > > +++ b/drivers/tty/serial/amba-pl011.c
> > > @@ -41,6 +41,7 @@
> > >  #include <linux/sizes.h>
> > >  #include <linux/io.h>
> > >  #include <linux/acpi.h>
> > > +#include <linux/moduleparam.h>
> > >
> > >  #define UART_NR			14
> > >
> > > @@ -263,6 +264,7 @@ struct uart_amba_port {
> > >  	char			type[12];
> > >  	bool			rs485_tx_started;
> > >  	unsigned int		rs485_tx_drain_interval; /* usecs */
> > > +	bool			console_line_ended;
> > >  #ifdef CONFIG_DMA_ENGINE
> > >  	/* DMA stuff */
> > >  	unsigned int		dmacr;		/* dma control reg */
> > > @@ -274,6 +276,10 @@ struct uart_amba_port {
> > >  #endif
> > >  };
> > >
> > > +/* if non-zero, the console is nbcon. */
> > > +static int use_nbcon;
> > > +module_param(use_nbcon, int, 0444);
> > 
> > Why is a module parameter needed here?  That feels wrong and not
> > scalable at all.  What happens if you have multiple devices, which one
> > is nbcon and which isn't?
> 
> This module parameter is for pl011 driver.

Yes, and module parameters are from the 1990's, let's not add new ones
please.

> With this patch implemented, only one type will be usable, depending
> on the value of use_nbcon.
> I thought it would be more user-friendly if legacy operation could be
> selected via boot parameter.
> Would it be better to make the patch purely nbcon-compatible, without
> retaining the legacy functionality?

Why would you want the "legacy" functionality?  Aren't we converting all
consoles over to use the new stuff?

thanks,

greg k-h