Re: [PATCH printk v3 03/19] printk: nbcon: Add function for printers to reacquire ownership

[John Ogness]

On 2024-07-26, Petr Mladek <pmladek@xxxxxxxx> wrote:
> On Mon 2024-07-22 19:25:23, John Ogness wrote:
>> Since ownership can be lost at any time due to handover or
>> takeover, a printing context _must_ be prepared to back out
>> immediately and carefully. However, there are scenarios where
>> the printing context must reacquire ownership in order to
>> finalize or revert hardware changes.
>> 
>> One such example is when interrupts are disabled during
>> printing. No other context will automagically re-enable the
>> interrupts. For this case, the disabling context _must_
>> reacquire nbcon ownership so that it can re-enable the
>> interrupts.
>
> I am still not sure how this is going to be used. It is suspicious.
> If the context lost the ownership than another started flushing
> higher priority messages.
>
> Is it really safe to manipulate the HW at this point?
> Won't it break the higher priority context?

Why would it break anything? It spins until it normally and safely
acquires ownership again. The commit message provides a simple example
of why it is necessary. With a threaded printer, this situation happens
almost every time a warning occurs.

>> --- a/kernel/printk/nbcon.c
>> +++ b/kernel/printk/nbcon.c
>> @@ -911,6 +948,15 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
>>  		return false;
>>  	}
>>  
>> +	if (!wctxt->outbuf) {
>
> This check works only when con->write_atomic() called
> nbcon_reacquire_nobuf().

Exactly. That is what it is for.

> At least, we should clear the buffer also in nbcon_enter_unsafe() and
> nbcon_exit_unsafe() when they realize that they do own the context.

OK.

> Even better would be to add a check whether we still own the context.
> Something like:
>
> bool nbcon_can_proceed(struct nbcon_write_context *wctxt)
> {
> 	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> 	struct nbcon_state cur;
>
> 	nbcon_state_read(con, &cur);
>
> 	return nbcon_context_can_proceed(ctxt, &cur);
> }

nbcon_can_proceed() is meant to check ownership. And it only makes sense
to use it within an unsafe section. Otherwise it is racy.

Once a reacquire has occurred, the driver is allowed to proceed. It just
is not allowed to print (because its buffer is gone).

>> +		/*
>> +		 * Ownership was lost and reacquired by the driver.
>> +		 * Handle it as if ownership was lost.
>> +		 */
>> +		nbcon_context_release(ctxt);
>> +		return false;

John