| Message ID | 20230318214626.28655-1-mibodhi@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | netconsole: various improvements | expand |
Hi Tony, On Sun, 19 Mar 2023 at 10:46, Tony Dinh <mibodhi@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 > - 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 Can you convert this to 'if IS_ENABLED(...)' at the same time? > - /* 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? > 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. Regards, SImon
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. 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 > - 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 >
Hi Simon, On Sun, Mar 19, 2023 at 12:30 PM Simon Glass <sjg@chromium.org> wrote: > > Hi Tony, > > On Sun, 19 Mar 2023 at 10:46, Tony Dinh <mibodhi@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@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
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. 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 > >
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 > > >
Hi Pali, On Mon, Mar 20, 2023 at 1:00 AM Pali Rohár <pali@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@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@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 > > > >
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 */
- When Netconsole is running, stdin/stdout/stderr are set to nc. Reset stdin/stdout/stderr to serial (a sane deffault) before booting kernel. - 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(-)