[PATCH] netconsole: various improvements

[Tony Dinh]

Hi Simon,

On Sun, Mar 19, 2023 at 12:30 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Tony,
>
> On Sun, 19 Mar 2023 at 10:46, Tony Dinh <mibodhi at gmail.com> wrote:
> >
> > - When Netconsole is running, stdin/stdout/stderr are set to nc. Reset
> > stdin/stdout/stderr to serial (a sane deffault) before booting kernel.
>
> spelling

OK.
>
> > - 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
>
> Can you convert this to 'if IS_ENABLED(...)' at the same time?

Sure I will.
>
> > -       /* 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;
>
> What is this change?

I've been carrying this one-line patch out-of-tree for a few years.
Back when I saw that the netconsole started transitioning faster from
the serial console (I tested with 2 consoles running) . But I can no
longer prove that anymore! code change... I think it does not hurt to
set it to timeout initially anyway, since netconsole is polling for
chars. But I will remove this part of the patch if others think it is
not necessary.

>
> >  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
> >
>
> Could you take a look at converting netconsole to driver model? It
> should be a child of the eth device probably. I think this patch is OK
> for, but it would be better if the driver could deregister itself from
> stdio in its remove() method, perhaps installing serial at that point.

Agreed. It would be nice to have netconsole as a driver model. It will
get more attention that it deserves. I'm a bit short for free time
atm, though. I will make it a task to work on it.

Thanks,
Tony

> Regards,
> SImon