diff mbox series

[RESEND,v2] netconsole: various improvements

Message ID 20230403214152.20225-1-mibodhi@gmail.com
State Changes Requested
Delegated to: Ramon Fried
Headers show
Series [RESEND,v2] netconsole: various improvements | expand

Commit Message

Tony Dinh April 3, 2023, 9:41 p.m. UTC
Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
stdin/stdout/stder must be set to some primary console, in addtion to nc.
For example, stdin=serial,nc. Some recent Linux kernels will not boot with
only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
nc from the list of devices in stdin/stdout/stderr.

Signed-off-by: Tony Dinh <mibodhi@gmail.com>
---

Changes in v2:
- Select CONFIG_CONSOLE_MUX if CONFIG_NETCONSOLE is enabled
- Add new functions in netconsole driver to support CONSOLE_MUX
- Add new function to encapsulate the process of stopping netconsole
from bootm
- Remove unecessary net_timeout initial value = 1
- Resend to correct missing <stdio_dev.h> include in bootm.c

 boot/bootm.c             | 14 +++++++---
 drivers/net/Kconfig      | 10 +++++++
 drivers/net/netconsole.c | 60 ++++++++++++++++++++++++++++++++++++++++
 include/stdio_dev.h      |  1 +
 4 files changed, 81 insertions(+), 4 deletions(-)

Comments

Tony Dinh April 13, 2023, 10:30 p.m. UTC | #1
Hi Ramon,
Hi Joe,

Any comments on this patch?

Thanks,
Tony

On Mon, Apr 3, 2023 at 2:42 PM Tony Dinh <mibodhi@gmail.com> wrote:
>
> Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> stdin/stdout/stder must be set to some primary console, in addtion to nc.
> For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> nc from the list of devices in stdin/stdout/stderr.
>
> Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> ---
>
> Changes in v2:
> - Select CONFIG_CONSOLE_MUX if CONFIG_NETCONSOLE is enabled
> - Add new functions in netconsole driver to support CONSOLE_MUX
> - Add new function to encapsulate the process of stopping netconsole
> from bootm
> - Remove unecessary net_timeout initial value = 1
> - Resend to correct missing <stdio_dev.h> include in bootm.c
>
>  boot/bootm.c             | 14 +++++++---
>  drivers/net/Kconfig      | 10 +++++++
>  drivers/net/netconsole.c | 60 ++++++++++++++++++++++++++++++++++++++++
>  include/stdio_dev.h      |  1 +
>  4 files changed, 81 insertions(+), 4 deletions(-)
>
> diff --git a/boot/bootm.c b/boot/bootm.c
> index 2eec60ec7b..1b2542b570 100644
> --- a/boot/bootm.c
> +++ b/boot/bootm.c
> @@ -18,6 +18,7 @@
>  #include <malloc.h>
>  #include <mapmem.h>
>  #include <net.h>
> +#include <stdio_dev.h>
>  #include <asm/cache.h>
>  #include <asm/global_data.h>
>  #include <asm/io.h>
> @@ -472,11 +473,16 @@ ulong bootm_disable_interrupts(void)
>          * recover from any failures any more...
>          */
>         iflag = disable_interrupts();
> -#ifdef CONFIG_NETCONSOLE
> -       /* Stop the ethernet stack if NetConsole could have left it up */
> -       eth_halt();
> -#endif
>
> +       if (IS_ENABLED(CONFIG_NETCONSOLE)) {
> +               /*
> +                * Make sure that the starting kernel message printed out,
> +                * stop netconsole and the ethernet stack
> +                */
> +               printf("\n\nStarting kernel ...\n\n");
> +               drv_nc_stop();
> +               eth_halt();
> +       }
>  #if defined(CONFIG_CMD_USB)
>         /*
>          * turn off USB to prevent the host controller from writing to the
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index ceadee98a1..0661059dfa 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -945,4 +945,14 @@ config MDIO_MUX_MESON_G12A
>           This driver is used for the MDIO mux found on the Amlogic G12A & compatible
>           SoCs.
>
> +config NETCONSOLE
> +       bool "Enable netconsole"
> +       select CONSOLE_MUX
> +       help
> +         NetConsole requires CONSOLE_MUX, i.e. at least one default console such
> +         must be specified in addition to nc console. For example, for boards that
> +         the main console is serial, set each of the envs stdin/stdout/stderr to serial,nc.
> +         See CONFIG_CONSOLE_MUX and CONFIG_SYS_CONSOLE_IS_IN_ENV help for detailed
> +         description of usage.
> +
>  endif # NETDEVICES
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 151bc55e07..bb92d2e130 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -7,6 +7,7 @@
>  #include <common.h>
>  #include <command.h>
>  #include <env.h>
> +#include <iomux.h>
>  #include <log.h>
>  #include <stdio_dev.h>
>  #include <net.h>
> @@ -33,6 +34,12 @@ static int output_packet_len;
>   */
>  enum proto_t net_loop_last_protocol = BOOTP;
>
> +/*
> + * Net console helpers
> + */
> +static void usage(void);
> +static void remove_nc_from(const int console);
> +
>  static void nc_wait_arp_handler(uchar *pkt, unsigned dest,
>                                  struct in_addr sip, unsigned src,
>                                  unsigned len)
> @@ -111,6 +118,21 @@ static int refresh_settings_from_env(void)
>         return 0;
>  }
>
> +static void remove_nc_from(const int console)
> +{
> +       int ret;
> +
> +       ret = iomux_replace_device(console, "nc", "nulldev");
> +       if (ret) {
> +               printf("\n*** Warning: Cannot remove nc from %s Error=%d\n",
> +                       stdio_names[console], ret);
> +               printf("%s=", stdio_names[console]);
> +               iomux_printdevs(console);
> +               usage();
> +               flush();
> +       }
> +}
> +
>  /**
>   * Called from net_loop in net/net.c before each packet
>   */
> @@ -241,6 +263,29 @@ static int nc_stdio_start(struct stdio_dev *dev)
>         return 0;
>  }
>
> +void nc_stdio_stop(void)
> +{
> +       if (IS_ENABLED(CONFIG_CONSOLE_MUX)) {
> +               int ret;
> +               struct stdio_dev *sdev;
> +
> +               /* Remove nc from each stdio file  */
> +               remove_nc_from(stdin);
> +               remove_nc_from(stderr);
> +               remove_nc_from(stdout);
> +
> +               /* Deregister nc device */
> +               sdev = stdio_get_by_name("nc");
> +               ret = stdio_deregister_dev(sdev, true);
> +               if (ret)
> +                       printf("\nWarning: Cannot deregister nc console Error=%d\n", ret);
> +       } else {
> +               printf("\n*** Warning: CONFIG_CONSOLE_MUX must be enabled for netconsole\n"
> +                       "to work properly. The nc console will be removed when\n"
> +                       "netconsole driver stops\n");
> +       }
> +}
> +
>  static void nc_stdio_putc(struct stdio_dev *dev, char c)
>  {
>         if (output_recursion)
> @@ -316,6 +361,16 @@ static int nc_stdio_tstc(struct stdio_dev *dev)
>         return input_size != 0;
>  }
>
> +static void usage(void)
> +{
> +       printf("USAGE:\n"
> +               "NetConsole requires CONFIG_CONSOLE_MUX. At least one default console\n"
> +               "must be specified in addition to nc console. For example, for boards\n"
> +               "that the main console is serial, set the each of envs stdin/stdout/stderr\n"
> +               "to serial,nc. Some kernel might fail to boot when nc is the only device in\n"
> +               "stdout list\n");
> +}
> +
>  int drv_nc_init(void)
>  {
>         struct stdio_dev dev;
> @@ -335,3 +390,8 @@ int drv_nc_init(void)
>
>         return (rc == 0) ? 1 : rc;
>  }
> +
> +void drv_nc_stop(void)
> +{
> +       nc_stdio_stop();
> +}
> diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> index 3105928970..9d2375a67e 100644
> --- a/include/stdio_dev.h
> +++ b/include/stdio_dev.h
> @@ -112,6 +112,7 @@ int drv_keyboard_init(void);
>  int drv_usbtty_init(void);
>  int drv_usbacm_init(void);
>  int drv_nc_init(void);
> +void drv_nc_stop(void);
>  int drv_jtag_console_init(void);
>  int cbmemc_init(void);
>
> --
> 2.30.2
>
Simon Glass April 19, 2023, 1:46 a.m. UTC | #2
Hi Tony,

On Mon, 3 Apr 2023 at 15:42, Tony Dinh <mibodhi@gmail.com> wrote:
>
> Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> stdin/stdout/stder must be set to some primary console, in addtion to nc.
> For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> nc from the list of devices in stdin/stdout/stderr.
>
> Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> ---
>
> Changes in v2:
> - Select CONFIG_CONSOLE_MUX if CONFIG_NETCONSOLE is enabled
> - Add new functions in netconsole driver to support CONSOLE_MUX
> - Add new function to encapsulate the process of stopping netconsole
> from bootm
> - Remove unecessary net_timeout initial value = 1
> - Resend to correct missing <stdio_dev.h> include in bootm.c
>
>  boot/bootm.c             | 14 +++++++---
>  drivers/net/Kconfig      | 10 +++++++
>  drivers/net/netconsole.c | 60 ++++++++++++++++++++++++++++++++++++++++
>  include/stdio_dev.h      |  1 +
>  4 files changed, 81 insertions(+), 4 deletions(-)

This seems OK to me but for a few thoughts below.

But can we use this opportunity to move this to driver model? The
netconsole driver could be bound in eth_post_bind() and the code in
netconsole.c converted to be a driver.

I think that would be better than building on an out-of-date driver.

>
> diff --git a/boot/bootm.c b/boot/bootm.c
> index 2eec60ec7b..1b2542b570 100644
> --- a/boot/bootm.c
> +++ b/boot/bootm.c
> @@ -18,6 +18,7 @@
>  #include <malloc.h>
>  #include <mapmem.h>
>  #include <net.h>
> +#include <stdio_dev.h>
>  #include <asm/cache.h>
>  #include <asm/global_data.h>
>  #include <asm/io.h>
> @@ -472,11 +473,16 @@ ulong bootm_disable_interrupts(void)
>          * recover from any failures any more...
>          */
>         iflag = disable_interrupts();
> -#ifdef CONFIG_NETCONSOLE
> -       /* Stop the ethernet stack if NetConsole could have left it up */
> -       eth_halt();
> -#endif
>
> +       if (IS_ENABLED(CONFIG_NETCONSOLE)) {
> +               /*
> +                * Make sure that the starting kernel message printed out,
> +                * stop netconsole and the ethernet stack
> +                */
> +               printf("\n\nStarting kernel ...\n\n");
> +               drv_nc_stop();
> +               eth_halt();
> +       }
>  #if defined(CONFIG_CMD_USB)
>         /*
>          * turn off USB to prevent the host controller from writing to the
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index ceadee98a1..0661059dfa 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -945,4 +945,14 @@ config MDIO_MUX_MESON_G12A
>           This driver is used for the MDIO mux found on the Amlogic G12A & compatible
>           SoCs.
>
> +config NETCONSOLE
> +       bool "Enable netconsole"
> +       select CONSOLE_MUX
> +       help
> +         NetConsole requires CONSOLE_MUX, i.e. at least one default console such
> +         must be specified in addition to nc console. For example, for boards that
> +         the main console is serial, set each of the envs stdin/stdout/stderr to serial,nc.
> +         See CONFIG_CONSOLE_MUX and CONFIG_SYS_CONSOLE_IS_IN_ENV help for detailed
> +         description of usage.
> +
>  endif # NETDEVICES
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 151bc55e07..bb92d2e130 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -7,6 +7,7 @@
>  #include <common.h>
>  #include <command.h>
>  #include <env.h>
> +#include <iomux.h>
>  #include <log.h>
>  #include <stdio_dev.h>
>  #include <net.h>
> @@ -33,6 +34,12 @@ static int output_packet_len;
>   */
>  enum proto_t net_loop_last_protocol = BOOTP;
>
> +/*
> + * Net console helpers
> + */
> +static void usage(void);
> +static void remove_nc_from(const int console);
> +
>  static void nc_wait_arp_handler(uchar *pkt, unsigned dest,
>                                  struct in_addr sip, unsigned src,
>                                  unsigned len)
> @@ -111,6 +118,21 @@ static int refresh_settings_from_env(void)
>         return 0;
>  }
>
> +static void remove_nc_from(const int console)
> +{
> +       int ret;
> +
> +       ret = iomux_replace_device(console, "nc", "nulldev");
> +       if (ret) {
> +               printf("\n*** Warning: Cannot remove nc from %s Error=%d\n",
> +                       stdio_names[console], ret);
> +               printf("%s=", stdio_names[console]);
> +               iomux_printdevs(console);
> +               usage();
> +               flush();
> +       }
> +}
> +
>  /**
>   * Called from net_loop in net/net.c before each packet
>   */
> @@ -241,6 +263,29 @@ static int nc_stdio_start(struct stdio_dev *dev)
>         return 0;
>  }
>
> +void nc_stdio_stop(void)
> +{
> +       if (IS_ENABLED(CONFIG_CONSOLE_MUX)) {
> +               int ret;
> +               struct stdio_dev *sdev;
> +
> +               /* Remove nc from each stdio file  */
> +               remove_nc_from(stdin);
> +               remove_nc_from(stderr);
> +               remove_nc_from(stdout);
> +
> +               /* Deregister nc device */
> +               sdev = stdio_get_by_name("nc");
> +               ret = stdio_deregister_dev(sdev, true);
> +               if (ret)
> +                       printf("\nWarning: Cannot deregister nc console Error=%d\n", ret);
> +       } else {
> +               printf("\n*** Warning: CONFIG_CONSOLE_MUX must be enabled for netconsole\n"
> +                       "to work properly. The nc console will be removed when\n"
> +                       "netconsole driver stops\n");
> +       }
> +}
> +
>  static void nc_stdio_putc(struct stdio_dev *dev, char c)
>  {
>         if (output_recursion)
> @@ -316,6 +361,16 @@ static int nc_stdio_tstc(struct stdio_dev *dev)
>         return input_size != 0;
>  }
>
> +static void usage(void)
> +{
> +       printf("USAGE:\n"
> +               "NetConsole requires CONFIG_CONSOLE_MUX. At least one default console\n"
> +               "must be specified in addition to nc console. For example, for boards\n"
> +               "that the main console is serial, set the each of envs stdin/stdout/stderr\n"
> +               "to serial,nc. Some kernel might fail to boot when nc is the only device in\n"
> +               "stdout list\n");

How about creating a doc/ file with info about net console? This is a
bit too much text to put in a user message.

> +}
> +
>  int drv_nc_init(void)
>  {
>         struct stdio_dev dev;
> @@ -335,3 +390,8 @@ int drv_nc_init(void)
>
>         return (rc == 0) ? 1 : rc;
>  }
> +
> +void drv_nc_stop(void)
> +{
> +       nc_stdio_stop();
> +}
> diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> index 3105928970..9d2375a67e 100644
> --- a/include/stdio_dev.h
> +++ b/include/stdio_dev.h
> @@ -112,6 +112,7 @@ int drv_keyboard_init(void);
>  int drv_usbtty_init(void);
>  int drv_usbacm_init(void);
>  int drv_nc_init(void);
> +void drv_nc_stop(void);

Once this is a driver we won't need this.

>  int drv_jtag_console_init(void);
>  int cbmemc_init(void);

Regards,
Simon
Tony Dinh April 20, 2023, 1:22 a.m. UTC | #3
HI Simon,

On Tue, Apr 18, 2023 at 6:46 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tony,
>
> On Mon, 3 Apr 2023 at 15:42, Tony Dinh <mibodhi@gmail.com> wrote:
> >
> > Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> > stdin/stdout/stder must be set to some primary console, in addtion to nc.
> > For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> > only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> > nc from the list of devices in stdin/stdout/stderr.
> >
> > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> > ---
> >
> > Changes in v2:
> > - Select CONFIG_CONSOLE_MUX if CONFIG_NETCONSOLE is enabled
> > - Add new functions in netconsole driver to support CONSOLE_MUX
> > - Add new function to encapsulate the process of stopping netconsole
> > from bootm
> > - Remove unecessary net_timeout initial value = 1
> > - Resend to correct missing <stdio_dev.h> include in bootm.c
> >
> >  boot/bootm.c             | 14 +++++++---
> >  drivers/net/Kconfig      | 10 +++++++
> >  drivers/net/netconsole.c | 60 ++++++++++++++++++++++++++++++++++++++++
> >  include/stdio_dev.h      |  1 +
> >  4 files changed, 81 insertions(+), 4 deletions(-)
>
> This seems OK to me but for a few thoughts below.
>
> But can we use this opportunity to move this to driver model? The
> netconsole driver could be bound in eth_post_bind() and the code in
> netconsole.c converted to be a driver.
>
> I think that would be better than building on an out-of-date driver.

I'd agree that converting to a driver model is the logical next step.
My motivation is to fix the booting problem for the Linux kernel first
(I'm having problems booting some kernels without this patch). And
then in the next go round, I'll convert netconsole to a driver. Most
of the code in this small patch will be needed whether it is an old
driver or DM anyway.

>
> >
> > diff --git a/boot/bootm.c b/boot/bootm.c
> > index 2eec60ec7b..1b2542b570 100644
> > --- a/boot/bootm.c
> > +++ b/boot/bootm.c
> > @@ -18,6 +18,7 @@
> >  #include <malloc.h>
> >  #include <mapmem.h>
> >  #include <net.h>
> > +#include <stdio_dev.h>
> >  #include <asm/cache.h>
> >  #include <asm/global_data.h>
> >  #include <asm/io.h>
> > @@ -472,11 +473,16 @@ ulong bootm_disable_interrupts(void)
> >          * recover from any failures any more...
> >          */
> >         iflag = disable_interrupts();
> > -#ifdef CONFIG_NETCONSOLE
> > -       /* Stop the ethernet stack if NetConsole could have left it up */
> > -       eth_halt();
> > -#endif
> >
> > +       if (IS_ENABLED(CONFIG_NETCONSOLE)) {
> > +               /*
> > +                * Make sure that the starting kernel message printed out,
> > +                * stop netconsole and the ethernet stack
> > +                */
> > +               printf("\n\nStarting kernel ...\n\n");
> > +               drv_nc_stop();
> > +               eth_halt();
> > +       }
> >  #if defined(CONFIG_CMD_USB)
> >         /*
> >          * turn off USB to prevent the host controller from writing to the
> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > index ceadee98a1..0661059dfa 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -945,4 +945,14 @@ config MDIO_MUX_MESON_G12A
> >           This driver is used for the MDIO mux found on the Amlogic G12A & compatible
> >           SoCs.
> >
> > +config NETCONSOLE
> > +       bool "Enable netconsole"
> > +       select CONSOLE_MUX
> > +       help
> > +         NetConsole requires CONSOLE_MUX, i.e. at least one default console such
> > +         must be specified in addition to nc console. For example, for boards that
> > +         the main console is serial, set each of the envs stdin/stdout/stderr to serial,nc.
> > +         See CONFIG_CONSOLE_MUX and CONFIG_SYS_CONSOLE_IS_IN_ENV help for detailed
> > +         description of usage.
> > +
> >  endif # NETDEVICES
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 151bc55e07..bb92d2e130 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -7,6 +7,7 @@
> >  #include <common.h>
> >  #include <command.h>
> >  #include <env.h>
> > +#include <iomux.h>
> >  #include <log.h>
> >  #include <stdio_dev.h>
> >  #include <net.h>
> > @@ -33,6 +34,12 @@ static int output_packet_len;
> >   */
> >  enum proto_t net_loop_last_protocol = BOOTP;
> >
> > +/*
> > + * Net console helpers
> > + */
> > +static void usage(void);
> > +static void remove_nc_from(const int console);
> > +
> >  static void nc_wait_arp_handler(uchar *pkt, unsigned dest,
> >                                  struct in_addr sip, unsigned src,
> >                                  unsigned len)
> > @@ -111,6 +118,21 @@ static int refresh_settings_from_env(void)
> >         return 0;
> >  }
> >
> > +static void remove_nc_from(const int console)
> > +{
> > +       int ret;
> > +
> > +       ret = iomux_replace_device(console, "nc", "nulldev");
> > +       if (ret) {
> > +               printf("\n*** Warning: Cannot remove nc from %s Error=%d\n",
> > +                       stdio_names[console], ret);
> > +               printf("%s=", stdio_names[console]);
> > +               iomux_printdevs(console);
> > +               usage();
> > +               flush();
> > +       }
> > +}
> > +
> >  /**
> >   * Called from net_loop in net/net.c before each packet
> >   */
> > @@ -241,6 +263,29 @@ static int nc_stdio_start(struct stdio_dev *dev)
> >         return 0;
> >  }
> >
> > +void nc_stdio_stop(void)
> > +{
> > +       if (IS_ENABLED(CONFIG_CONSOLE_MUX)) {
> > +               int ret;
> > +               struct stdio_dev *sdev;
> > +
> > +               /* Remove nc from each stdio file  */
> > +               remove_nc_from(stdin);
> > +               remove_nc_from(stderr);
> > +               remove_nc_from(stdout);
> > +
> > +               /* Deregister nc device */
> > +               sdev = stdio_get_by_name("nc");
> > +               ret = stdio_deregister_dev(sdev, true);
> > +               if (ret)
> > +                       printf("\nWarning: Cannot deregister nc console Error=%d\n", ret);
> > +       } else {
> > +               printf("\n*** Warning: CONFIG_CONSOLE_MUX must be enabled for netconsole\n"
> > +                       "to work properly. The nc console will be removed when\n"
> > +                       "netconsole driver stops\n");
> > +       }
> > +}
> > +
> >  static void nc_stdio_putc(struct stdio_dev *dev, char c)
> >  {
> >         if (output_recursion)
> > @@ -316,6 +361,16 @@ static int nc_stdio_tstc(struct stdio_dev *dev)
> >         return input_size != 0;
> >  }
> >
> > +static void usage(void)
> > +{
> > +       printf("USAGE:\n"
> > +               "NetConsole requires CONFIG_CONSOLE_MUX. At least one default console\n"
> > +               "must be specified in addition to nc console. For example, for boards\n"
> > +               "that the main console is serial, set the each of envs stdin/stdout/stderr\n"
> > +               "to serial,nc. Some kernel might fail to boot when nc is the only device in\n"
> > +               "stdout list\n");
>
> How about creating a doc/ file with info about net console? This is a
> bit too much text to put in a user message.

Sure.

>
> > +}
> > +
> >  int drv_nc_init(void)
> >  {
> >         struct stdio_dev dev;
> > @@ -335,3 +390,8 @@ int drv_nc_init(void)
> >
> >         return (rc == 0) ? 1 : rc;
> >  }
> > +
> > +void drv_nc_stop(void)
> > +{
> > +       nc_stdio_stop();
> > +}
> > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > index 3105928970..9d2375a67e 100644
> > --- a/include/stdio_dev.h
> > +++ b/include/stdio_dev.h
> > @@ -112,6 +112,7 @@ int drv_keyboard_init(void);
> >  int drv_usbtty_init(void);
> >  int drv_usbacm_init(void);
> >  int drv_nc_init(void);
> > +void drv_nc_stop(void);
>
> Once this is a driver we won't need this.

Yes, but we still need the nc_stdio_stop() to do the cleanup, i.e.
removing nc from stdin/stdout/stderr and deregistering the nc device.
Except that it will be a uclass member like .pre_remove()?

It will be a bit of a learning curve for me to do the DM netconsole.
But I will give it a shot.

Thanks,
Tony

>
> >  int drv_jtag_console_init(void);
> >  int cbmemc_init(void);
>
> Regards,
> Simon
Tony Dinh April 20, 2023, 3:55 a.m. UTC | #4
On Wed, Apr 19, 2023 at 6:22 PM Tony Dinh <mibodhi@gmail.com> wrote:
>
> HI Simon,
>
> On Tue, Apr 18, 2023 at 6:46 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Tony,
> >
> > On Mon, 3 Apr 2023 at 15:42, Tony Dinh <mibodhi@gmail.com> wrote:
> > >
> > > Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> > > stdin/stdout/stder must be set to some primary console, in addtion to nc.
> > > For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> > > only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> > > nc from the list of devices in stdin/stdout/stderr.
> > >
> > > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> > > ---
> > >
> > > Changes in v2:
> > > - Select CONFIG_CONSOLE_MUX if CONFIG_NETCONSOLE is enabled
> > > - Add new functions in netconsole driver to support CONSOLE_MUX
> > > - Add new function to encapsulate the process of stopping netconsole
> > > from bootm
> > > - Remove unecessary net_timeout initial value = 1
> > > - Resend to correct missing <stdio_dev.h> include in bootm.c
> > >
> > >  boot/bootm.c             | 14 +++++++---
> > >  drivers/net/Kconfig      | 10 +++++++
> > >  drivers/net/netconsole.c | 60 ++++++++++++++++++++++++++++++++++++++++
> > >  include/stdio_dev.h      |  1 +
> > >  4 files changed, 81 insertions(+), 4 deletions(-)
> >
> > This seems OK to me but for a few thoughts below.
> >
> > But can we use this opportunity to move this to driver model? The
> > netconsole driver could be bound in eth_post_bind() and the code in
> > netconsole.c converted to be a driver.
> >
> > I think that would be better than building on an out-of-date driver.
>
> I'd agree that converting to a driver model is the logical next step.
> My motivation is to fix the booting problem for the Linux kernel first
> (I'm having problems booting some kernels without this patch). And
> then in the next go round, I'll convert netconsole to a driver. Most
> of the code in this small patch will be needed whether it is an old
> driver or DM anyway.
>
> >
> > >
> > > diff --git a/boot/bootm.c b/boot/bootm.c
> > > index 2eec60ec7b..1b2542b570 100644
> > > --- a/boot/bootm.c
> > > +++ b/boot/bootm.c
> > > @@ -18,6 +18,7 @@
> > >  #include <malloc.h>
> > >  #include <mapmem.h>
> > >  #include <net.h>
> > > +#include <stdio_dev.h>
> > >  #include <asm/cache.h>
> > >  #include <asm/global_data.h>
> > >  #include <asm/io.h>
> > > @@ -472,11 +473,16 @@ ulong bootm_disable_interrupts(void)
> > >          * recover from any failures any more...
> > >          */
> > >         iflag = disable_interrupts();
> > > -#ifdef CONFIG_NETCONSOLE
> > > -       /* Stop the ethernet stack if NetConsole could have left it up */
> > > -       eth_halt();
> > > -#endif
> > >
> > > +       if (IS_ENABLED(CONFIG_NETCONSOLE)) {
> > > +               /*
> > > +                * Make sure that the starting kernel message printed out,
> > > +                * stop netconsole and the ethernet stack
> > > +                */
> > > +               printf("\n\nStarting kernel ...\n\n");
> > > +               drv_nc_stop();
> > > +               eth_halt();
> > > +       }
> > >  #if defined(CONFIG_CMD_USB)
> > >         /*
> > >          * turn off USB to prevent the host controller from writing to the
> > > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > > index ceadee98a1..0661059dfa 100644
> > > --- a/drivers/net/Kconfig
> > > +++ b/drivers/net/Kconfig
> > > @@ -945,4 +945,14 @@ config MDIO_MUX_MESON_G12A
> > >           This driver is used for the MDIO mux found on the Amlogic G12A & compatible
> > >           SoCs.
> > >
> > > +config NETCONSOLE
> > > +       bool "Enable netconsole"
> > > +       select CONSOLE_MUX
> > > +       help
> > > +         NetConsole requires CONSOLE_MUX, i.e. at least one default console such
> > > +         must be specified in addition to nc console. For example, for boards that
> > > +         the main console is serial, set each of the envs stdin/stdout/stderr to serial,nc.
> > > +         See CONFIG_CONSOLE_MUX and CONFIG_SYS_CONSOLE_IS_IN_ENV help for detailed
> > > +         description of usage.
> > > +
> > >  endif # NETDEVICES
> > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > > index 151bc55e07..bb92d2e130 100644
> > > --- a/drivers/net/netconsole.c
> > > +++ b/drivers/net/netconsole.c
> > > @@ -7,6 +7,7 @@
> > >  #include <common.h>
> > >  #include <command.h>
> > >  #include <env.h>
> > > +#include <iomux.h>
> > >  #include <log.h>
> > >  #include <stdio_dev.h>
> > >  #include <net.h>
> > > @@ -33,6 +34,12 @@ static int output_packet_len;
> > >   */
> > >  enum proto_t net_loop_last_protocol = BOOTP;
> > >
> > > +/*
> > > + * Net console helpers
> > > + */
> > > +static void usage(void);
> > > +static void remove_nc_from(const int console);
> > > +
> > >  static void nc_wait_arp_handler(uchar *pkt, unsigned dest,
> > >                                  struct in_addr sip, unsigned src,
> > >                                  unsigned len)
> > > @@ -111,6 +118,21 @@ static int refresh_settings_from_env(void)
> > >         return 0;
> > >  }
> > >
> > > +static void remove_nc_from(const int console)
> > > +{
> > > +       int ret;
> > > +
> > > +       ret = iomux_replace_device(console, "nc", "nulldev");
> > > +       if (ret) {
> > > +               printf("\n*** Warning: Cannot remove nc from %s Error=%d\n",
> > > +                       stdio_names[console], ret);
> > > +               printf("%s=", stdio_names[console]);
> > > +               iomux_printdevs(console);
> > > +               usage();
> > > +               flush();
> > > +       }
> > > +}
> > > +
> > >  /**
> > >   * Called from net_loop in net/net.c before each packet
> > >   */
> > > @@ -241,6 +263,29 @@ static int nc_stdio_start(struct stdio_dev *dev)
> > >         return 0;
> > >  }
> > >
> > > +void nc_stdio_stop(void)
> > > +{
> > > +       if (IS_ENABLED(CONFIG_CONSOLE_MUX)) {
> > > +               int ret;
> > > +               struct stdio_dev *sdev;
> > > +
> > > +               /* Remove nc from each stdio file  */
> > > +               remove_nc_from(stdin);
> > > +               remove_nc_from(stderr);
> > > +               remove_nc_from(stdout);
> > > +
> > > +               /* Deregister nc device */
> > > +               sdev = stdio_get_by_name("nc");
> > > +               ret = stdio_deregister_dev(sdev, true);
> > > +               if (ret)
> > > +                       printf("\nWarning: Cannot deregister nc console Error=%d\n", ret);
> > > +       } else {
> > > +               printf("\n*** Warning: CONFIG_CONSOLE_MUX must be enabled for netconsole\n"
> > > +                       "to work properly. The nc console will be removed when\n"
> > > +                       "netconsole driver stops\n");
> > > +       }
> > > +}
> > > +
> > >  static void nc_stdio_putc(struct stdio_dev *dev, char c)
> > >  {
> > >         if (output_recursion)
> > > @@ -316,6 +361,16 @@ static int nc_stdio_tstc(struct stdio_dev *dev)
> > >         return input_size != 0;
> > >  }
> > >
> > > +static void usage(void)
> > > +{
> > > +       printf("USAGE:\n"
> > > +               "NetConsole requires CONFIG_CONSOLE_MUX. At least one default console\n"
> > > +               "must be specified in addition to nc console. For example, for boards\n"
> > > +               "that the main console is serial, set the each of envs stdin/stdout/stderr\n"
> > > +               "to serial,nc. Some kernel might fail to boot when nc is the only device in\n"
> > > +               "stdout list\n");
> >
> > How about creating a doc/ file with info about net console? This is a
> > bit too much text to put in a user message.
>
> Sure.
>
> >
> > > +}
> > > +
> > >  int drv_nc_init(void)
> > >  {
> > >         struct stdio_dev dev;
> > > @@ -335,3 +390,8 @@ int drv_nc_init(void)
> > >
> > >         return (rc == 0) ? 1 : rc;
> > >  }
> > > +
> > > +void drv_nc_stop(void)
> > > +{
> > > +       nc_stdio_stop();
> > > +}
> > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > index 3105928970..9d2375a67e 100644
> > > --- a/include/stdio_dev.h
> > > +++ b/include/stdio_dev.h
> > > @@ -112,6 +112,7 @@ int drv_keyboard_init(void);
> > >  int drv_usbtty_init(void);
> > >  int drv_usbacm_init(void);
> > >  int drv_nc_init(void);
> > > +void drv_nc_stop(void);
> >
> > Once this is a driver we won't need this.
>
> Yes, but we still need the nc_stdio_stop() to do the cleanup, i.e.
> removing nc from stdin/stdout/stderr and deregistering the nc device.
> Except that it will be a uclass member like .pre_remove()?
>
> It will be a bit of a learning curve for me to do the DM netconsole.
> But I will give it a shot.

In the meantime, can this patch get merged on its own merit as a bug fix?

All the best,
Tony

>
> Thanks,
> Tony
>
> >
> > >  int drv_jtag_console_init(void);
> > >  int cbmemc_init(void);
> >
> > Regards,
> > Simon
Simon Glass April 24, 2023, 7:42 p.m. UTC | #5
Hi Tony,

On Wed, 19 Apr 2023 at 21:55, Tony Dinh <mibodhi@gmail.com> wrote:
>
> On Wed, Apr 19, 2023 at 6:22 PM Tony Dinh <mibodhi@gmail.com> wrote:
> >
> > HI Simon,
> >
> > On Tue, Apr 18, 2023 at 6:46 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Tony,
> > >
> > > On Mon, 3 Apr 2023 at 15:42, Tony Dinh <mibodhi@gmail.com> wrote:
> > > >
> > > > Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> > > > stdin/stdout/stder must be set to some primary console, in addtion to nc.
> > > > For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> > > > only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> > > > nc from the list of devices in stdin/stdout/stderr.
> > > >
> > > > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Select CONFIG_CONSOLE_MUX if CONFIG_NETCONSOLE is enabled
> > > > - Add new functions in netconsole driver to support CONSOLE_MUX
> > > > - Add new function to encapsulate the process of stopping netconsole
> > > > from bootm
> > > > - Remove unecessary net_timeout initial value = 1
> > > > - Resend to correct missing <stdio_dev.h> include in bootm.c
> > > >
> > > >  boot/bootm.c             | 14 +++++++---
> > > >  drivers/net/Kconfig      | 10 +++++++
> > > >  drivers/net/netconsole.c | 60 ++++++++++++++++++++++++++++++++++++++++
> > > >  include/stdio_dev.h      |  1 +
> > > >  4 files changed, 81 insertions(+), 4 deletions(-)
> > >
> > > This seems OK to me but for a few thoughts below.
> > >
> > > But can we use this opportunity to move this to driver model? The
> > > netconsole driver could be bound in eth_post_bind() and the code in
> > > netconsole.c converted to be a driver.
> > >
> > > I think that would be better than building on an out-of-date driver.
> >
> > I'd agree that converting to a driver model is the logical next step.
> > My motivation is to fix the booting problem for the Linux kernel first
> > (I'm having problems booting some kernels without this patch). And
> > then in the next go round, I'll convert netconsole to a driver. Most
> > of the code in this small patch will be needed whether it is an old
> > driver or DM anyway.

This crosses the line to a feature, IMO...

[..]

> > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > index 3105928970..9d2375a67e 100644
> > > > --- a/include/stdio_dev.h
> > > > +++ b/include/stdio_dev.h
> > > > @@ -112,6 +112,7 @@ int drv_keyboard_init(void);
> > > >  int drv_usbtty_init(void);
> > > >  int drv_usbacm_init(void);
> > > >  int drv_nc_init(void);
> > > > +void drv_nc_stop(void);
> > >
> > > Once this is a driver we won't need this.
> >
> > Yes, but we still need the nc_stdio_stop() to do the cleanup, i.e.
> > removing nc from stdin/stdout/stderr and deregistering the nc device.
> > Except that it will be a uclass member like .pre_remove()?

Yes, or perhaps the driver's remove() method.

> >
> > It will be a bit of a learning curve for me to do the DM netconsole.
> > But I will give it a shot.
>
> In the meantime, can this patch get merged on its own merit as a bug fix?

I suppose so, so long as you can figure out the conversion. Let me
know if you have any questions.

Regards,
Simon
Tony Dinh April 25, 2023, 2:06 a.m. UTC | #6
Hi Simon,

On Mon, Apr 24, 2023 at 12:43 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tony,
>
> On Wed, 19 Apr 2023 at 21:55, Tony Dinh <mibodhi@gmail.com> wrote:
> >
> > On Wed, Apr 19, 2023 at 6:22 PM Tony Dinh <mibodhi@gmail.com> wrote:
> > >
> > > HI Simon,
> > >
> > > On Tue, Apr 18, 2023 at 6:46 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Tony,
> > > >
> > > > On Mon, 3 Apr 2023 at 15:42, Tony Dinh <mibodhi@gmail.com> wrote:
> > > > >
> > > > > Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> > > > > stdin/stdout/stder must be set to some primary console, in addtion to nc.
> > > > > For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> > > > > only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> > > > > nc from the list of devices in stdin/stdout/stderr.
> > > > >
> > > > > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> > > > > ---
> > > > >
> > > > > Changes in v2:
> > > > > - Select CONFIG_CONSOLE_MUX if CONFIG_NETCONSOLE is enabled
> > > > > - Add new functions in netconsole driver to support CONSOLE_MUX
> > > > > - Add new function to encapsulate the process of stopping netconsole
> > > > > from bootm
> > > > > - Remove unecessary net_timeout initial value = 1
> > > > > - Resend to correct missing <stdio_dev.h> include in bootm.c
> > > > >
> > > > >  boot/bootm.c             | 14 +++++++---
> > > > >  drivers/net/Kconfig      | 10 +++++++
> > > > >  drivers/net/netconsole.c | 60 ++++++++++++++++++++++++++++++++++++++++
> > > > >  include/stdio_dev.h      |  1 +
> > > > >  4 files changed, 81 insertions(+), 4 deletions(-)
> > > >
> > > > This seems OK to me but for a few thoughts below.
> > > >
> > > > But can we use this opportunity to move this to driver model? The
> > > > netconsole driver could be bound in eth_post_bind() and the code in
> > > > netconsole.c converted to be a driver.
> > > >
> > > > I think that would be better than building on an out-of-date driver.
> > >
> > > I'd agree that converting to a driver model is the logical next step.
> > > My motivation is to fix the booting problem for the Linux kernel first
> > > (I'm having problems booting some kernels without this patch). And
> > > then in the next go round, I'll convert netconsole to a driver. Most
> > > of the code in this small patch will be needed whether it is an old
> > > driver or DM anyway.
>
> This crosses the line to a feature, IMO...

My initial patch would be a pure bug fix. But it also breaks other
boards like Nokia, as Pali has pointed out. So in the v2 patch,
Console Mux is introduced to cover all boards. I don't think that we
are adding any features here.

It's just that I'm using netconsole often so I noticed the booting
problem with recent Linux kernels. It's kind of a catch-22, without a
serial console I cannot debug Linux early booting problems, and with
serial console active there is no booting problem.

All the best,
Tony

>
> [..]
>
> > > > > diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> > > > > index 3105928970..9d2375a67e 100644
> > > > > --- a/include/stdio_dev.h
> > > > > +++ b/include/stdio_dev.h
> > > > > @@ -112,6 +112,7 @@ int drv_keyboard_init(void);
> > > > >  int drv_usbtty_init(void);
> > > > >  int drv_usbacm_init(void);
> > > > >  int drv_nc_init(void);
> > > > > +void drv_nc_stop(void);
> > > >
> > > > Once this is a driver we won't need this.
> > >
> > > Yes, but we still need the nc_stdio_stop() to do the cleanup, i.e.
> > > removing nc from stdin/stdout/stderr and deregistering the nc device.
> > > Except that it will be a uclass member like .pre_remove()?
>
> Yes, or perhaps the driver's remove() method.
>
> > >
> > > It will be a bit of a learning curve for me to do the DM netconsole.
> > > But I will give it a shot.
> >
> > In the meantime, can this patch get merged on its own merit as a bug fix?
>
> I suppose so, so long as you can figure out the conversion. Let me
> know if you have any questions.
>
> Regards,
> Simon
Tom Rini April 25, 2023, 2:18 a.m. UTC | #7
On Mon, Apr 24, 2023 at 07:06:08PM -0700, Tony Dinh wrote:
> Hi Simon,
> 
> On Mon, Apr 24, 2023 at 12:43 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Tony,
> >
> > On Wed, 19 Apr 2023 at 21:55, Tony Dinh <mibodhi@gmail.com> wrote:
> > >
> > > On Wed, Apr 19, 2023 at 6:22 PM Tony Dinh <mibodhi@gmail.com> wrote:
> > > >
> > > > HI Simon,
> > > >
> > > > On Tue, Apr 18, 2023 at 6:46 PM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Tony,
> > > > >
> > > > > On Mon, 3 Apr 2023 at 15:42, Tony Dinh <mibodhi@gmail.com> wrote:
> > > > > >
> > > > > > Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> > > > > > stdin/stdout/stder must be set to some primary console, in addtion to nc.
> > > > > > For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> > > > > > only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> > > > > > nc from the list of devices in stdin/stdout/stderr.
> > > > > >
> > > > > > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> > > > > > ---
> > > > > >
> > > > > > Changes in v2:
> > > > > > - Select CONFIG_CONSOLE_MUX if CONFIG_NETCONSOLE is enabled
> > > > > > - Add new functions in netconsole driver to support CONSOLE_MUX
> > > > > > - Add new function to encapsulate the process of stopping netconsole
> > > > > > from bootm
> > > > > > - Remove unecessary net_timeout initial value = 1
> > > > > > - Resend to correct missing <stdio_dev.h> include in bootm.c
> > > > > >
> > > > > >  boot/bootm.c             | 14 +++++++---
> > > > > >  drivers/net/Kconfig      | 10 +++++++
> > > > > >  drivers/net/netconsole.c | 60 ++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/stdio_dev.h      |  1 +
> > > > > >  4 files changed, 81 insertions(+), 4 deletions(-)
> > > > >
> > > > > This seems OK to me but for a few thoughts below.
> > > > >
> > > > > But can we use this opportunity to move this to driver model? The
> > > > > netconsole driver could be bound in eth_post_bind() and the code in
> > > > > netconsole.c converted to be a driver.
> > > > >
> > > > > I think that would be better than building on an out-of-date driver.
> > > >
> > > > I'd agree that converting to a driver model is the logical next step.
> > > > My motivation is to fix the booting problem for the Linux kernel first
> > > > (I'm having problems booting some kernels without this patch). And
> > > > then in the next go round, I'll convert netconsole to a driver. Most
> > > > of the code in this small patch will be needed whether it is an old
> > > > driver or DM anyway.
> >
> > This crosses the line to a feature, IMO...
> 
> My initial patch would be a pure bug fix. But it also breaks other
> boards like Nokia, as Pali has pointed out. So in the v2 patch,
> Console Mux is introduced to cover all boards. I don't think that we
> are adding any features here.
> 
> It's just that I'm using netconsole often so I noticed the booting
> problem with recent Linux kernels. It's kind of a catch-22, without a
> serial console I cannot debug Linux early booting problems, and with
> serial console active there is no booting problem.

Yes, we should fix functionality then work on clean-ups.
Tom Rini May 5, 2023, 6:34 p.m. UTC | #8
On Mon, Apr 03, 2023 at 02:41:52PM -0700, Tony Dinh wrote:

> Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> stdin/stdout/stder must be set to some primary console, in addtion to nc.
> For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> nc from the list of devices in stdin/stdout/stderr.
> 
> Signed-off-by: Tony Dinh <mibodhi@gmail.com>

This breaks the ut_bootstd_bootflow_cmd_boot test on sandbox. I'm not
sure if it's a test issue or something else.
Tony Dinh May 8, 2023, 4:25 a.m. UTC | #9
Hi Tom,

On Fri, May 5, 2023 at 11:34 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Apr 03, 2023 at 02:41:52PM -0700, Tony Dinh wrote:
>
> > Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> > stdin/stdout/stder must be set to some primary console, in addtion to nc.
> > For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> > only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> > nc from the list of devices in stdin/stdout/stderr.
> >
> > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
>
> This breaks the ut_bootstd_bootflow_cmd_boot test on sandbox. I'm not
> sure if it's a test issue or something else.

Thanks for trying, I'm having some difficulty building a sandbox
u-boot, so I can't get to the point where I can see what's the problem
yet.

All the best,
Tony

>
> --
> Tom
Simon Glass May 8, 2023, 9:23 p.m. UTC | #10
Hi Tony,

On Sun, 7 May 2023 at 22:25, Tony Dinh <mibodhi@gmail.com> wrote:
>
> Hi Tom,
>
> On Fri, May 5, 2023 at 11:34 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Apr 03, 2023 at 02:41:52PM -0700, Tony Dinh wrote:
> >
> > > Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> > > stdin/stdout/stder must be set to some primary console, in addtion to nc.
> > > For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> > > only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> > > nc from the list of devices in stdin/stdout/stderr.
> > >
> > > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> >
> > This breaks the ut_bootstd_bootflow_cmd_boot test on sandbox. I'm not
> > sure if it's a test issue or something else.
>
> Thanks for trying, I'm having some difficulty building a sandbox
> u-boot, so I can't get to the point where I can see what's the problem
> yet.

What is going wrong? Does this help?

https://u-boot.readthedocs.io/en/latest/build/gcc.html

Regards,
Simon
Tony Dinh May 8, 2023, 10:53 p.m. UTC | #11
Hi Simon,

On Mon, May 8, 2023 at 2:23 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tony,
>
> On Sun, 7 May 2023 at 22:25, Tony Dinh <mibodhi@gmail.com> wrote:
> >
> > Hi Tom,
> >
> > On Fri, May 5, 2023 at 11:34 AM Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Mon, Apr 03, 2023 at 02:41:52PM -0700, Tony Dinh wrote:
> > >
> > > > Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> > > > stdin/stdout/stder must be set to some primary console, in addtion to nc.
> > > > For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> > > > only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> > > > nc from the list of devices in stdin/stdout/stderr.
> > > >
> > > > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> > >
> > > This breaks the ut_bootstd_bootflow_cmd_boot test on sandbox. I'm not
> > > sure if it's a test issue or something else.
> >
> > Thanks for trying, I'm having some difficulty building a sandbox
> > u-boot, so I can't get to the point where I can see what's the problem
> > yet.
>
> What is going wrong? Does this help?
>
> https://u-boot.readthedocs.io/en/latest/build/gcc.html

Yes, I've done that when I realized my ARM development box setup
missed several Debian packages building sandbox (I only build Linux
kernels and U-Boot boards natively on ARM). However, after installing
all those packages it is still not possible to build a sandbox on ARM.
So currently I am switching to cross compiling on my Intel laptop.
Later, I'll come back to the ARM box to see whether we need some
documentation update for doc/build/gcc.rst.

Thanks,
Tony

>
> Regards,
> Simon
Tony Dinh May 9, 2023, 1:16 a.m. UTC | #12
On Mon, May 8, 2023 at 3:53 PM Tony Dinh <mibodhi@gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, May 8, 2023 at 2:23 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Tony,
> >
> > On Sun, 7 May 2023 at 22:25, Tony Dinh <mibodhi@gmail.com> wrote:
> > >
> > > Hi Tom,
> > >
> > > On Fri, May 5, 2023 at 11:34 AM Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Mon, Apr 03, 2023 at 02:41:52PM -0700, Tony Dinh wrote:
> > > >
> > > > > Use CONFIG_CONSOLE_MUX for netconsole. When netconsole is running,
> > > > > stdin/stdout/stder must be set to some primary console, in addtion to nc.
> > > > > For example, stdin=serial,nc. Some recent Linux kernels will not boot with
> > > > > only nc on the stdout list, ie. stdout=nc. When netconsole exits, remove
> > > > > nc from the list of devices in stdin/stdout/stderr.
> > > > >
> > > > > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> > > >
> > > > This breaks the ut_bootstd_bootflow_cmd_boot test on sandbox. I'm not
> > > > sure if it's a test issue or something else.
> > >
> > > Thanks for trying, I'm having some difficulty building a sandbox
> > > u-boot, so I can't get to the point where I can see what's the problem
> > > yet.
> >
> > What is going wrong? Does this help?
> >
> > https://u-boot.readthedocs.io/en/latest/build/gcc.html
>
> Yes, I've done that when I realized my ARM development box setup
> missed several Debian packages building sandbox (I only build Linux
> kernels and U-Boot boards natively on ARM). However, after installing
> all those packages it is still not possible to build a sandbox on ARM.
> So currently I am switching to cross compiling on my Intel laptop.
> Later, I'll come back to the ARM box to see whether we need some
> documentation update for doc/build/gcc.rst.

FYI, what's missing in the doc/build/gcc.rst is the SDL option. I
could not build without NO_SDL=1.

make sandbox_defconfig all NO_SDL=1

All the best,
Tony
diff mbox series

Patch

diff --git a/boot/bootm.c b/boot/bootm.c
index 2eec60ec7b..1b2542b570 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -18,6 +18,7 @@ 
 #include <malloc.h>
 #include <mapmem.h>
 #include <net.h>
+#include <stdio_dev.h>
 #include <asm/cache.h>
 #include <asm/global_data.h>
 #include <asm/io.h>
@@ -472,11 +473,16 @@  ulong bootm_disable_interrupts(void)
 	 * recover from any failures any more...
 	 */
 	iflag = disable_interrupts();
-#ifdef CONFIG_NETCONSOLE
-	/* Stop the ethernet stack if NetConsole could have left it up */
-	eth_halt();
-#endif
 
+	if (IS_ENABLED(CONFIG_NETCONSOLE)) {
+		/*
+		 * Make sure that the starting kernel message printed out,
+		 * stop netconsole and the ethernet stack
+		 */
+		printf("\n\nStarting kernel ...\n\n");
+		drv_nc_stop();
+		eth_halt();
+	}
 #if defined(CONFIG_CMD_USB)
 	/*
 	 * turn off USB to prevent the host controller from writing to the
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index ceadee98a1..0661059dfa 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -945,4 +945,14 @@  config MDIO_MUX_MESON_G12A
 	  This driver is used for the MDIO mux found on the Amlogic G12A & compatible
 	  SoCs.
 
+config NETCONSOLE
+	bool "Enable netconsole"
+	select CONSOLE_MUX
+	help
+	  NetConsole requires CONSOLE_MUX, i.e. at least one default console such
+	  must be specified in addition to nc console. For example, for boards that
+	  the main console is serial, set each of the envs stdin/stdout/stderr to serial,nc.
+	  See CONFIG_CONSOLE_MUX and CONFIG_SYS_CONSOLE_IS_IN_ENV help for detailed
+	  description of usage.
+
 endif # NETDEVICES
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 151bc55e07..bb92d2e130 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -7,6 +7,7 @@ 
 #include <common.h>
 #include <command.h>
 #include <env.h>
+#include <iomux.h>
 #include <log.h>
 #include <stdio_dev.h>
 #include <net.h>
@@ -33,6 +34,12 @@  static int output_packet_len;
  */
 enum proto_t net_loop_last_protocol = BOOTP;
 
+/*
+ * Net console helpers
+ */
+static void usage(void);
+static void remove_nc_from(const int console);
+
 static void nc_wait_arp_handler(uchar *pkt, unsigned dest,
 				 struct in_addr sip, unsigned src,
 				 unsigned len)
@@ -111,6 +118,21 @@  static int refresh_settings_from_env(void)
 	return 0;
 }
 
+static void remove_nc_from(const int console)
+{
+	int ret;
+
+	ret = iomux_replace_device(console, "nc", "nulldev");
+	if (ret) {
+		printf("\n*** Warning: Cannot remove nc from %s Error=%d\n",
+			stdio_names[console], ret);
+		printf("%s=", stdio_names[console]);
+		iomux_printdevs(console);
+		usage();
+		flush();
+	}
+}
+
 /**
  * Called from net_loop in net/net.c before each packet
  */
@@ -241,6 +263,29 @@  static int nc_stdio_start(struct stdio_dev *dev)
 	return 0;
 }
 
+void nc_stdio_stop(void)
+{
+	if (IS_ENABLED(CONFIG_CONSOLE_MUX)) {
+		int ret;
+		struct stdio_dev *sdev;
+
+		/* Remove nc from each stdio file  */
+		remove_nc_from(stdin);
+		remove_nc_from(stderr);
+		remove_nc_from(stdout);
+
+		/* Deregister nc device */
+		sdev = stdio_get_by_name("nc");
+		ret = stdio_deregister_dev(sdev, true);
+		if (ret)
+			printf("\nWarning: Cannot deregister nc console Error=%d\n", ret);
+	} else {
+		printf("\n*** Warning: CONFIG_CONSOLE_MUX must be enabled for netconsole\n"
+			"to work properly. The nc console will be removed when\n"
+			"netconsole driver stops\n");
+	}
+}
+
 static void nc_stdio_putc(struct stdio_dev *dev, char c)
 {
 	if (output_recursion)
@@ -316,6 +361,16 @@  static int nc_stdio_tstc(struct stdio_dev *dev)
 	return input_size != 0;
 }
 
+static void usage(void)
+{
+	printf("USAGE:\n"
+		"NetConsole requires CONFIG_CONSOLE_MUX. At least one default console\n"
+		"must be specified in addition to nc console. For example, for boards\n"
+		"that the main console is serial, set the each of envs stdin/stdout/stderr\n"
+		"to serial,nc. Some kernel might fail to boot when nc is the only device in\n"
+		"stdout list\n");
+}
+
 int drv_nc_init(void)
 {
 	struct stdio_dev dev;
@@ -335,3 +390,8 @@  int drv_nc_init(void)
 
 	return (rc == 0) ? 1 : rc;
 }
+
+void drv_nc_stop(void)
+{
+	nc_stdio_stop();
+}
diff --git a/include/stdio_dev.h b/include/stdio_dev.h
index 3105928970..9d2375a67e 100644
--- a/include/stdio_dev.h
+++ b/include/stdio_dev.h
@@ -112,6 +112,7 @@  int drv_keyboard_init(void);
 int drv_usbtty_init(void);
 int drv_usbacm_init(void);
 int drv_nc_init(void);
+void drv_nc_stop(void);
 int drv_jtag_console_init(void);
 int cbmemc_init(void);