[PATCH] netconsole: various improvements

[Tony Dinh]

Hi Pali,

On Mon, Mar 20, 2023 at 1:00 AM Pali Rohár <pali at kernel.org> wrote:
>
> On Sunday 19 March 2023 16:25:25 Tony Dinh wrote:
> > 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.
>
> Cannot user use console mux and set stdout to both serial and nc at the
> same time?

Agree, we can make that available. And a lot of existing boards out
there are setting
"setenv ncip $serverip; setenv stdin nc; setenv stdout nc; setenv stderr nc;"
We can say that with the new u-boot version, they have to upgrade
their netconsole related envs.

I've seen too many users having trouble with netconsole, so I think
perhaps a netconsole command performing the muxing and setting envs
internally might be helpful.

Thanks,
Tony

>
> > 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
> > > >