| Message ID | 20250606-printk-cleanup-part2-v1-5-f427c743dda0@suse.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | printk cleanup - part 2 | expand |
On Fri 2025-06-06 23:53:47, Marcos Paulo de Souza wrote: > All consoles found on for_each_console are registered, meaning that all of > them are CON_ENABLED. The code tries to find an active console, so check if the > console is not suspended instead. > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > --- > arch/um/kernel/kmsg_dump.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c > index 4190211752726593dd2847f66efd9d3a61cea982..f3025b2a813453f479d720618c630bee135d4e08 100644 > --- a/arch/um/kernel/kmsg_dump.c > +++ b/arch/um/kernel/kmsg_dump.c > @@ -31,7 +31,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper, > * expected to output the crash information. > */ > if (strcmp(con->name, "ttynull") != 0 && > - (console_srcu_read_flags(con) & CON_ENABLED)) { > + (console_srcu_read_flags(con) & CON_SUSPENDED) == 0) { > break; I think that we should actually replace the check of the CON_ENABLE/CON_SUSPENDED flag with is_console_usable(con, console_srcu_read_flags(con), true) And it should be done at the beginning of the patchset before changing the semantic of the flags. Motivation: There is the following comment at the beginning of the function: /* * If no consoles are available to output crash information, dump * the kmsg buffer to stdout. */ The if-condition checks for: + "ttynull" because this special console does not show any messages by definition + disabled/suspended consoles; note that this patchset is replacing CON_ENABLED with CON_SUSPENDED flag because it the state is changed during suspend. But it should check also for: + whether the console is NBCON_console and does not have con->write_atomic because such a console would not be able to show the messages in panic(). And it should also check the global "consoles_suspended" flag. Because consoles won't show anything when it is set. And all these is already done by "is_console_usable()" except for the check of "ttynull" which is very special. How does the sound, please? Best Regards, Petr > } > } > > -- > 2.49.0
On 2025-06-16, Petr Mladek <pmladek@suse.com> wrote: > On Fri 2025-06-06 23:53:47, Marcos Paulo de Souza wrote: >> All consoles found on for_each_console are registered, meaning that all of >> them are CON_ENABLED. The code tries to find an active console, so check if the >> console is not suspended instead. >> >> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> >> --- >> arch/um/kernel/kmsg_dump.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c >> index 4190211752726593dd2847f66efd9d3a61cea982..f3025b2a813453f479d720618c630bee135d4e08 100644 >> --- a/arch/um/kernel/kmsg_dump.c >> +++ b/arch/um/kernel/kmsg_dump.c >> @@ -31,7 +31,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper, >> * expected to output the crash information. >> */ >> if (strcmp(con->name, "ttynull") != 0 && >> - (console_srcu_read_flags(con) & CON_ENABLED)) { >> + (console_srcu_read_flags(con) & CON_SUSPENDED) == 0) { >> break; > > I think that we should actually replace the check of the > CON_ENABLE/CON_SUSPENDED flag with > > is_console_usable(con, console_srcu_read_flags(con), true) > > And it should be done at the beginning of the patchset before > changing the semantic of the flags. > > Motivation: > > There is the following comment at the beginning of the function: > > /* > * If no consoles are available to output crash information, dump > * the kmsg buffer to stdout. > */ > > The if-condition checks for: > > + "ttynull" because this special console does not show any messages > by definition > > + disabled/suspended consoles; note that this patchset is replacing > CON_ENABLED with CON_SUSPENDED flag because it the state is > changed during suspend. > > But it should check also for: > > + whether the console is NBCON_console and does not have con->write_atomic > because such a console would not be able to show the messages > in panic(). > > And it should also check the global "consoles_suspended" flag. Because > consoles won't show anything when it is set. > > And all these is already done by "is_console_usable()" except for > the check of "ttynull" which is very special. > > How does the sound, please? FWIW, I agree with all these points. John
diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c index 4190211752726593dd2847f66efd9d3a61cea982..f3025b2a813453f479d720618c630bee135d4e08 100644 --- a/arch/um/kernel/kmsg_dump.c +++ b/arch/um/kernel/kmsg_dump.c @@ -31,7 +31,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper, * expected to output the crash information. */ if (strcmp(con->name, "ttynull") != 0 && - (console_srcu_read_flags(con) & CON_ENABLED)) { + (console_srcu_read_flags(con) & CON_SUSPENDED) == 0) { break; } }
All consoles found on for_each_console are registered, meaning that all of them are CON_ENABLED. The code tries to find an active console, so check if the console is not suspended instead. Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> --- arch/um/kernel/kmsg_dump.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)