[PATCH] netconsole: various improvements

[Tony Dinh]

Hi Pali,

On Sun, Mar 19, 2023 at 12:36 PM Pali Rohár <pali at kernel.org> wrote:
>
> On Saturday 18 March 2023 14:46:25 Tony Dinh wrote:
> > - When Netconsole is running, stdin/stdout/stderr are set to nc. Reset
> > stdin/stdout/stderr to serial (a sane deffault) before booting kernel.
>
> This can be a problematic. serial output does not have to be available
> for all devices. For example on Nokia N900 phone is available only
> lcd/vga display device.

Here is a shortcoming of the current implementation of netconsole.
When it starts, the user sets the stdin/stdout/stderr envs to nc, and
we lose the previous state of the console. Especially with the
CONSOLE_MUX is not enabled. There is no way to set them back by
setting the envs in booting logic.

It sounds like we need to implement internal envs to save the previous
state, and then restore it before shutting down the network and
booting the kernel.
I recall in previous Linux kernel versions many years ago Linux still
booted OK, but recently it can no longer boot with the stdio set to nc
(I've tested this failure case). Perhaps nc should _not_ be set
explicitly by the user. We might need a u-boot command to start
netconsole, and that would include checking if the serverip is
running?

>
> Also this can break CONSOLE_MUX support when more devices are specified
> in stdin/stdout/stderr env variables. With CONSOLE_MUX, output is send
> to more than one device. User may set it to both serial and nc or also
> to other output (e.g. to lcd like on Nokia N900).
>
> So in my opinion, if "nc" is is going to be turned off then just "nc"
> string should be removed from env variable and let all other devices
> stay in env variables.
>
> Maybe you can use something like this? (taken from common/usb_kbd.c)
>
> #if CONFIG_IS_ENABLED(CONSOLE_MUX)
>         if (iomux_replace_device(stdin, "nc", "nulldev"))
>                 return 1;
> #endif

Thanks for pointing out that fact about CONSOLE_MUX. Yes I will
incorporate that as part of the logic.

All the best,
Tony

>
> > - Enable net_timeout when netconsole starts will give a better user
> > experience if netconsole server is not running.
> >
> > Signed-off-by: Tony Dinh <mibodhi at gmail.com>
> > ---
> >
> >  boot/bootm.c             | 16 +++++++++++++++-
> >  drivers/net/netconsole.c |  2 +-
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/boot/bootm.c b/boot/bootm.c
> > index 2eec60ec7b..c4a3aaf1bd 100644
> > --- a/boot/bootm.c
> > +++ b/boot/bootm.c
> > @@ -473,7 +473,21 @@ ulong bootm_disable_interrupts(void)
> >        */
> >       iflag = disable_interrupts();
> >  #ifdef CONFIG_NETCONSOLE
> > -     /* Stop the ethernet stack if NetConsole could have left it up */
> > +     /*
> > +      * Make sure that the starting kernel message printed out.
> > +      * Reset stdin/out/err back to serial and stop the ethernet
> > +      * stack if NetConsole could have left it up
> > +      */
> > +     char *s;
> > +     int ret;
> > +
> > +     s = env_get("stdout");
> > +     if (strcmp(s, "nc") == 0) {
> > +             printf("\n\nStarting kernel ...\n");
> > +             ret = env_set("stdin", "serial");
> > +             ret = env_set("stdout", "serial");
> > +             ret = env_set("stderr", "serial");
> > +     }
> >       eth_halt();
> >  #endif
> >
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 151bc55e07..2091014918 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -20,7 +20,7 @@ static int input_size; /* char count in input buffer */
> >  static int input_offset; /* offset to valid chars in input buffer */
> >  static int input_recursion;
> >  static int output_recursion;
> > -static int net_timeout;
> > +static int net_timeout = 1;
> >  static uchar nc_ether[6]; /* server enet address */
> >  static struct in_addr nc_ip; /* server ip */
> >  static short nc_out_port; /* target output port */
> > --
> > 2.30.2
> >