Re: [PATCH printk v2 5/8] printk: nbcon: Add sequence handling

[John Ogness <john.ogness@linutronix.de>]

On 2023-08-10, Petr Mladek <pmladek@suse.com> wrote:
>>  void console_flush_on_panic(enum con_flush_mode mode)
>>  {
>> +	struct console *c;
>>  	bool handover;
>> -	u64 next_seq;
>> +	short flags;
>> +	int cookie;
>> +	u64 seq;
>> +
>> +	seq = prb_first_valid_seq(prb);
>> +
>> +	/*
>> +	 * Safely handle the atomic consoles before trying to flush any
>> +	 * legacy consoles.
>> +	 */
>
> This is a bit weird because the loop below just sets sequence
> number for NBCON consoles. But they are not really flushed.
>
> I think that you already agreed with it for v3. But let me mention
> it here for completeness.
>
> I would prefer to just add the API and use it later when some
> particular action get supported. And the flush could not do
> anything until nbcon_write() callback is added.

Actually I agreed with this for v2, but didn't take it far enough. I now
also agree that console_flush_on_panic() should not be modified at all
until the actual flushing functions are available.

>> @@ -3829,7 +3846,9 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
>>  			if (!console_is_usable(c))
>>  				continue;
>>  
>> -			if (locked)
>> +			if (flags & CON_NBCON)
>> +				printk_seq = nbcon_seq_read(c);
>> +			else if (locked)
>>  				printk_seq = c->seq;
>>  			else
>>  				continue;
>
> I think that I mentioned this already in a previous patch. The "else
> continue" path is bad. It allows quietly skip messages for classic
> consoles when "locked" is false. I know that it should not happen
> but...
>
> A solution would be to add WARN_ON_ONCE() before the continue.

I like the WARN_ON_ONCE() suggestion. For v3 I will do:

        if (flags & CON_NBCON) {
                printk_seq = nbcon_seq_read(c);
        } else {
                WARN_ON_ONCE(!locked);
                printk_seq = c->seq;
        }

>> +static bool nbcon_seq_try_update(struct nbcon_context *ctxt)
>> +{
>> +	struct console *con = ctxt->console;
>> +	u64 con_seq = nbcon_seq_read(con);
>> +
>> +	while (con_seq < ctxt->seq) {
>
> What if anyone called nbcon_seq_force() to reply the entire log
> in the meantime?
>
> IMHO, we should remember the original nbcon_seq before
> the context handle a line. And this function should update
> nbcon_seq only when it has not been changed by other context
> in the meantime.

Makes sense. I will do that for v3.

John