Re: [PATCH 5/7] arch: um: kmsg_dump: Don't check for CON_ENABLED

[John Ogness]

On 2025-06-16, Petr Mladek <pmladek@xxxxxxxx> 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@xxxxxxxx>
>> ---
>>  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