Re: [PATCH printk v4 05/27] printk: nbcon: Add detailed doc for write_atomic()

[Petr Mladek <pmladek@suse.com>]

On Wed 2024-04-03 00:17:07, John Ogness wrote:
> The write_atomic() callback has special requirements and is
> allowed to use special helper functions. Provide detailed
> documentation of the callback so that a developer has a
> chance of implementing it correctly.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Petr Mladek <pmladek@suse.com>

I have re-read the text and found a mistake.

---
>  include/linux/console.h | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 54b98e4f0544..e4028d4079e1 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -285,7 +285,7 @@ struct nbcon_write_context {
>  /**
>   * struct console - The console descriptor structure
>   * @name:		The name of the console driver
> - * @write:		Write callback to output messages (Optional)
> + * @write:		Legacy write callback to output messages (Optional)
>   * @read:		Read callback for console input (Optional)
>   * @device:		The underlying TTY device driver (Optional)
>   * @unblank:		Callback to unblank the console (Optional)
> @@ -302,7 +302,6 @@ struct nbcon_write_context {
>   * @data:		Driver private data
>   * @node:		hlist node for the console list
>   *
> - * @write_atomic:	Write callback for atomic context
>   * @nbcon_state:	State for nbcon consoles
>   * @nbcon_seq:		Sequence number of the next record for nbcon to print
>   * @pbufs:		Pointer to nbcon private buffer
> @@ -327,8 +326,32 @@ struct console {
>  	struct hlist_node	node;
>  
>  	/* nbcon console specific members */
> -	void			(*write_atomic)(struct console *con,
> -						struct nbcon_write_context *wctxt);
> +
> +	/**
> +	 * @write_atomic:
> +	 *
> +	 * NBCON callback to write out text in any context.
> +	 *
> +	 * This callback is called with the console already acquired. The
> +	 * callback can use nbcon_can_proceed() at any time to verify that
> +	 * it is still the owner of the console. In the case that it has
> +	 * lost ownership, it is no longer allowed to go forward. In this
> +	 * case it must back out immediately and carefully. The buffer
> +	 * content is also no longer trusted since it no longer belongs to
> +	 * the context.
> +	 *
> +	 * If the callback needs to perform actions where ownership is not
> +	 * allowed to be taken over, nbcon_enter_unsafe() and
> +	 * nbcon_exit_unsafe() can be used to mark such sections.

IMHO, the word 'can' is wrong. The callback has to enter unsafe state
when the ownership is not allowed to be taken over.

We should probably be more clear what this exactly means.

Thinking more about this. We should also be more clear about when
to use nbcon_can_proceed() and what it guarantees. Is it actually
useful to call it directly in practice?


> +	 * functions are also points of possible ownership transfer. If
> +	 * either function returns false, ownership has been lost.
> +	 *
> +	 * This callback can be called from any context (including NMI).
> +	 * Therefore it must avoid usage of any locking and instead rely
> +	 * on the console ownership for synchronization.
> +	 */

My proposal:

	/**
	 * @write_atomic:
	 *
	 * NBCON callback to write out text in any context.
	 *
	 * This callback is called with the nbcon context acquired. But
	 * a higher priority context is allowed to take over it by default.
	 *
	 * The callback has to call nbcon_enter_unsafe() and nbcon_exit_unsafe()
	 * around any code where the takeover is not safe, for example, when
	 * manipulating the serial port registers.
	 *
	 * nbcon_enter_unsafe() might fail when the context has lost
	 * the console ownership in the meantime. In this case, the callback
	 * is no longer allowed to go forward. It must back out immediately and
	 * carefully. The buffer content is also no longer trusted since it
	 * no longer belongs to the context.
	 *
	 * The callback should allow the takeover whenever it is safe.
	 * It increases the chance to see messages when the system is
	 * in troubles.
	 *
	 * The callback is called from any context (including NMI).
	 * Therefore it must avoid usage of any locking and instead rely
	 * on the console ownership for synchronization.
	 */

> +	void (*write_atomic)(struct console *con, struct nbcon_write_context *wctxt);
> +
>  	atomic_t		__private nbcon_state;
>  	atomic_long_t		__private nbcon_seq;
>  	struct printk_buffers	*pbufs;


Best Regards,
Petr