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

[Petr Mladek <pmladek@suse.com>]

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?

> --- 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().

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.

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);
}

> +		/*
> +		 * Ownership was lost and reacquired by the driver.
> +		 * Handle it as if ownership was lost.
> +		 */
> +		nbcon_context_release(ctxt);
> +		return false;
> +	}
> +
>  	/*
>  	 * Since any dropped message was successfully output, reset the
>  	 * dropped count for the console.

Best Regards,
Petr