Re: [PATCH printk v2 18/18] printk: nbcon: Add function for printers to reacquire ownership

[Petr Mladek]

On Tue 2024-06-04 01:30:53, 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.
> 
> Provide nbcon_reacquire() for exactly this purpose. It allows a
> printing context to reacquire ownership using the same priority
> as its previous ownership.
> 
> Note that after a successful reacquire the printing context
> will have no output buffer because that has been lost. This
> function cannot be used to resume printing.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -838,6 +838,38 @@ bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt)
>  }
>  EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
>  
> +/**
> + * nbcon_reacquire - Reacquire a console after losing ownership while printing
> + * @wctxt:	The write context that was handed to the write callback
> + *
> + * 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.
> + *
> + * This function allows a printing context to reacquire ownership using the
> + * same priority as its previous ownership.
> + *
> + * Note that after a successful reacquire the printing context will have no
> + * output buffer because that has been lost. This function cannot be used to
> + * resume printing.
> + */
> +void nbcon_reacquire(struct nbcon_write_context *wctxt)

I think about calling it "nbcon_reacquire_nobuf" to make it clear
that it is not a complete recovery.

> +{
> +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> +	struct console *con = ctxt->console;
> +	struct nbcon_state cur;
> +
> +	while (!nbcon_context_try_acquire(ctxt))
> +		cpu_relax();
> +
> +	wctxt->outbuf = NULL;
> +	wctxt->len = 0;
> +	nbcon_state_read(con, &cur);
> +	wctxt->unsafe_takeover = cur.unsafe_takeover;

The nbcon_state_read() makes it a bit tricky. I would hide it into
a helper script:

void nbcon_write_context_set_buf(struct nbcon_write_context *wctxt,
				 char *buf, unsigned int len)
{
	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
	struct console *con = ctxt->console;
	struct nbcon_state cur;

	wctxt->outbuf = buf;
	wctxt->len = len;
	nbcon_state_read(con, &cur);
	wctxt->unsafe_takeover = cur.unsafe_takeover;
}

It would help to keep it in sync with nbcon_emit_next_record().

> +}
> +EXPORT_SYMBOL_GPL(nbcon_reacquire);

Otherwise, it looks good. I do not resist on the proposed changes.

Best Regards,
Petr