Re: [PATCH printk v4 06/27] printk: nbcon: Add callbacks to synchronize with driver

[Petr Mladek]

On Wed 2024-04-03 00:17:08, John Ogness wrote:
> Console drivers typically must deal with access to the hardware
> via user input/output (such as an interactive login shell) and
> output of kernel messages via printk() calls.
> 
> Follow-up commits require that the printk subsystem is able to
> synchronize with the driver. Require nbcon consoles to implement
> two new callbacks (device_lock(), device_unlock()) that will
> use whatever synchronization mechanism the driver is using for
> itself (for example, the port lock for uart serial consoles).

We should explain here the bigger picture, see my comment
of the word "whenever" below.

<proposal>
Console drivers typically have to deal with access to the hardware
via user input/output (such as an interactive login shell) and
output of kernel messages via printk() calls.

They use some classic locking mechanism in most situations. But
console->write_atomic() callbacks, used by nbcon consoles,
must be synchronized only by acquiring the console context.

The synchronization via the console context ownership is possible
only when the console driver is registered. It is when a particular
device driver is connected with a particular console driver.

The two synchronization mechanisms must be synchronized between
each other. It is tricky because the console context ownership
is quite special. It might be taken over by a higher priority
context. Also CPU migration has to be disabled.

The most tricky part is to (dis)connect these two mechanism during
the console (un)registration. Let's make it easier by adding two new
callbacks: device_lock(), device_unlock(). They would allow
to take the device-specific lock while the device is being
(un)registered by the related console driver.

For example, these callbacks would lock/unlock the port lock
for serial port drivers.
</proposal>

> 
> Signed-off-by: John Ogness <john.ogness@xxxxxxxxxxxxx>
> ---
>  include/linux/console.h | 42 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index e4028d4079e1..ad85594e070e 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -352,6 +352,48 @@ struct console {
>  	 */
>  	void (*write_atomic)(struct console *con, struct nbcon_write_context *wctxt);
>  
> +	/**
> +	 * @device_lock:
> +	 *
> +	 * NBCON callback to begin synchronization with driver code.
> +	 *
> +	 * Console drivers typically must deal with access to the hardware
> +	 * via user input/output (such as an interactive login shell) and
> +	 * output of kernel messages via printk() calls. This callback is
> +	 * called by the printk-subsystem whenever it needs to synchronize
> +	 * with hardware access by the driver.

The word "whenever" can be translated as "always" (by dictionary
and by my head ;-) Then the description sounds like this would be
the primary synchronization mechanism between printk and the driver.

In fact, this API would have only one purpose to synchronize
the console registration/unregistration.

IMHO, we should explain here the relation between the driver-specific
locking and nbcon console context locking. It would describe the big
picture and hopefully reduce confusion and eventual misuse.


> + It should be implemented to
> +	 * use whatever synchronization mechanism the driver is using for
> +	 * itself (for example, the port lock for uart serial consoles).
> +	 *
> +	 * This callback is always called from task context. It may use any
> +	 * synchronization method required by the driver. BUT this callback
> +	 * MUST also disable migration. The console driver may be using a
> +	 * synchronization mechanism that already takes care of this (such as
> +	 * spinlocks). Otherwise this function must explicitly call
> +	 * migrate_disable().
> +	 *
> +	 * The flags argument is provided as a convenience to the driver. It
> +	 * will be passed again to device_unlock(). It can be ignored if the
> +	 * driver does not need it.
> +	 */

<proposal>
	/**
	 * @device_lock:
	 *
	 * NBCON callback to serialize registration of a device driver
	 * for a console driver.
	 *
	 * Console drivers typically have to deal with access to the hardware
	 * via user input/output (such as an interactive login shell) and
	 * Output of kernel messages via printk() calls.
	 *
	 * They use some classic locking mechanism in most situations. But
	 * console->write_atomic() callbacks, used by nbcon consoles,
	 * must be synchronized only by acquiring the console context.
	 *
	 * The synchronization via the console context ownership is possible
	 * only when the console driver is registered. It is when a particular
	 * device driver is connected with a particular console driver.
	 *
	 * The device_lock() callback must block operations on the device
	 * while it is being (un)registered as a console driver. It will
	 * make sure that the classic device locking is aware of the console
	 * context locking when it might be acquired by the related nbcon
	 * console driver.
	 *
	 * This callback is always called from task context. It may use any
	 * synchronization method required by the driver, for example
	 * port lock for serial ports.
	 *
	 * IMPORTANT: This callback MUST also disable migration. The console
	 *	driver may be using a synchronization mechanism that already
	 *	takes care of this (such as spinlocks). Otherwise this function
	 *	must explicitly call migrate_disable().
	 *
	 *	The flags argument is provided as a convenience to the driver.
	 *	It will be passed again to device_unlock(). It can be ignored
	 *	if the driver does not need it.
	 */
</proposal>


> +	void (*device_lock)(struct console *con, unsigned long *flags);
> +
> +	/**
> +	 * @device_unlock:
> +	 *
> +	 * NBCON callback to finish synchronization with driver code.
> +	 *
> +	 * It is the counterpart to device_lock().
> +	 *
> +	 * This callback is always called from task context. It must
> +	 * appropriately re-enable migration (depending on how device_lock()
> +	 * disabled migration).
> +	 *
> +	 * The flags argument is the value of the same variable that was
> +	 * passed to device_lock().
> +	 */
> +	void (*device_unlock)(struct console *con, unsigned long flags);
> +
>  	atomic_t		__private nbcon_state;
>  	atomic_long_t		__private nbcon_seq;
>  	struct printk_buffers	*pbufs;

With the updated commit message and comment:

Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>

Best Regards,
Petr