Re: [PATCH printk v3 08/19] printk: nbcon: Add context to usable() and emit()

[Petr Mladek]

On Mon 2024-07-22 19:25:28, John Ogness wrote:
> The nbcon consoles will have two callbacks to be used for
> different contexts. In order to determine if an nbcon console
> is usable, console_is_usable() must know if it is a context
> that will need to use the optional write_atomic() callback.
> Also, nbcon_emit_next_record() must know which callback it
> needs to call.
> 
> Add an extra parameter @use_atomic to console_is_usable() and
> nbcon_emit_next_record() to specify this.
> 
> Since so far only the write_atomic() callback exists,
> @use_atomic is set to true for all call sites.
> 
> For legacy consoles, @use_atomic is not used.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -938,6 +939,18 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
>  	unsigned long con_dropped;
>  	unsigned long dropped;
>  
> +	/*
> +	 * This function should never be called for consoles that have not
> +	 * implemented the necessary callback for writing: i.e. legacy
> +	 * consoles and, when atomic, nbcon consoles with no write_atomic().
> +	 * Handle it as if ownership was lost and try to continue.
> +	 */
> +	if (WARN_ON_ONCE((use_atomic && !con->write_atomic) ||
> +			 !(console_srcu_read_flags(con) & CON_NBCON))) {
> +		nbcon_context_release(ctxt);
> +		return false;
> +	}
> +
>  	/*
>  	 * The printk buffers are filled within an unsafe section. This
>  	 * prevents NBCON_PRIO_NORMAL and NBCON_PRIO_EMERGENCY from
> @@ -972,7 +985,7 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
>  	/* Initialize the write context for driver callbacks. */
>  	nbcon_write_context_set_buf(wctxt, &pmsg.pbufs->outbuf[0], pmsg.outbuf_len);
>  
> -	if (con->write_atomic) {
> +	if (use_atomic) {
>  		con->write_atomic(con, wctxt);
>  	} else {
>  		/*

This "else" code path duplicates the WARN_ON_ONCE() and nbcon_context_release(ctxt).
It could/should be removed.

BTW: There is the opposite bug in the next patch which adds con->write_thread().
     It removes this duplicate "else" path but it does not extend the initial check.

     I am sorry. These are mistakes from the refactoring which I have asked you
     to do.


Otherwise, this patch good.

Best Regards,
Petr