Re: [PATCH] netconsole: various improvements

["Pali Rohár" <pali@kernel.org>]

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

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