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

From: John Ogness
Date: Mon Aug 26 2024 - 21:33:05 EST


On 2024-07-30, Petr Mladek <pmladek@xxxxxxxx> wrote:
> My idea was: "If we still own the context that we have owned it all
> the time and con-write_atomic() succeeded."
>
> The race is is not important. If we lose the ownership before updating
> nbcon_seq then the line will get written again anyway.
>
>> Once a reacquire has occurred, the driver is allowed to proceed. It just
>> is not allowed to print (because its buffer is gone).
>
> I see. My idea does not work because the driver is going to reacquire
> the ownership. It means that nbcon_can_proceed() would return true
> even when con->atomic_write() failed.
>
> But it is not documented anywhere. And what if the driver has a bug
> and does not call reacquire. Or what if the driver does not need
> to restore anything.
>
> IMHO, nbcon_emit_next_record() should check both:
>
> if (use_atomic)
> con->write_atomic(con, wctxt);
> else
> con->write_thread(con, wctxt);
>
> /* Still owns the console? */
> if (!nbcon_can_proceed(wctxt)
> return false;
>
> if (!wctxt->outbuf) {
> /*
> * Ownership was lost and reacquired by the driver.
> * Handle it as if ownership was lost.
> */
> nbcon_context_release(ctxt);
> return false;
> }

Except that the next thing nbcon_emit_next_record() does is
nbcon_context_enter_unsafe(), which checks ownership. So your suggested
nbcon_can_proceed() is redundant.

For v4 I can add comments explaining this. It would look like this (at
this point in the series):

/* Initialize the write context for driver callbacks. */
nbcon_write_context_set_buf(wctxt, &pmsg.pbufs->outbuf[0], pmsg.outbuf_len);

if (con->write_atomic) {
con->write_atomic(con, wctxt);
} else {
/*
* This function should never be called for legacy consoles.
* Handle it as if ownership was lost and try to continue.
*/
WARN_ON_ONCE(1);
nbcon_context_release(ctxt);
return false;
}

if (!wctxt->outbuf) {
/*
* Ownership was lost and reacquired by the driver. Handle it
* as if ownership was lost.
*/
nbcon_context_release(ctxt);
return false;
}

/*
* Ownership may have been lost but _not_ reacquired by the driver.
* This case is detected and handled when entering unsafe to update
* dropped/seq values.
*/

/*
* Since any dropped message was successfully output, reset the
* dropped count for the console.
*/
dropped = 0;
update_con:
/*
* The dropped count and the sequence number are updated within an
* unsafe section. This limits update races to the panic context and
* allows the panic context to win.
*/

if (!nbcon_context_enter_unsafe(ctxt))
return false;

John Ogness