On Wed, Apr 19, 2023 at 6:22 PM Tony Dinh <mibo...@gmail.com> wrote: > > HI Simon, > > On Tue, Apr 18, 2023 at 6:46 PM Simon Glass <s...@chromium.org> wrote: > > > > Hi Tony, > > > > On Mon, 3 Apr 2023 at 15:42, Tony Dinh <mibo...@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 <mibo...@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