Re: [PATCH printk v4 8/8] printk: nbcon: Allow drivers to mark unsafe regions and check state

[Petr Mladek <pmladek@suse.com>]

On Fri 2023-09-08 20:56:08, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> For the write_atomic callback, the console driver may have unsafe
> regions that need to be appropriately marked. Provide functions
> that accept the nbcon_write_context struct to allow for the driver
> to enter and exit unsafe regions.
> 
> Also provide a function for drivers to check if they are still the
> owner of the console.
> 
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index e2c274f4142e..04fac73c6e96 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -660,6 +660,32 @@ static bool nbcon_context_can_proceed(struct nbcon_context *ctxt, struct nbcon_s
>  	return false;
>  }
>  
> +/**
> + * nbcon_can_proceed - Check whether ownership can proceed
> + * @wctxt:	The write context that was handed to the write function
> + *
> + * Return:	True if this context still owns the console. False if
> + *		ownership was handed over or taken.
> + *
> + * Must be invoked at appropriate safe places in the driver.

This is a bit vague. I guess that enter_unsafe()/exit_unsafe() will be
used most of the time. The guestion is if this need to be called
in another locations explicitely.

I would write something similar as I suggested for nbcon_context_can_proceed():

  * It is used in nbcon_enter_unsafe() to make sure that it still owns the lock.
  * Also it is used in nbcon_exit_unsafe() to eventually free the lock
  * for a higher priority context which asked for the friendly handover.
  *
  * It can be called inside an unsafe section when the console is just
  * temporary in safe state instead of exiting and entering the unsafe
  * state.
  *
  * Also it can be called in the safe context before doing an expensive
  * safe operation. It does not make sense to do the operation when
  * a higher priority context took the lock.

> + *
> + * When this function returns false then the calling context no longer owns
> + * the console and 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 calling context.
> + */
> +bool nbcon_can_proceed(struct nbcon_write_context *wctxt)
> +{
> +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> +	struct console *con = ctxt->console;
> +	struct nbcon_state cur;
> +
> +	nbcon_state_read(con, &cur);
> +
> +	return nbcon_context_can_proceed(ctxt, &cur);
> +}
> +EXPORT_SYMBOL_GPL(nbcon_can_proceed);
> +

With the updated comment:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr