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. To provide the
necessary synchronization, usually some driver-specific locking
mechanism is used (for example, the port spinlock for uart
serial consoles).

Until now, usage of this driver-specific locking has been hidden
from the printk-subsystem and implemented within the various
console callbacks. However, for nbcon consoles, it is necessary
that the printk-subsystem uses the driver-specific locking so
that nbcon console ownership can be acquired _after_ the
driver-specific locking has succeeded. This allows for lock
contention to exist on the more context-friendly driver-specific
locking rather than nbcon console ownership (for non-emergency
and non-panic cases).

Require nbcon consoles to implement two new callbacks
(device_lock(), device_unlock()) that will use whatever
synchronization mechanism the driver is using for itself.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/console.h | 43 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/include/linux/console.h b/include/linux/console.h
index 3291cc340f1a..33a029d976c3 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -354,6 +354,49 @@ 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. It should be implemented to
+	 * use whatever synchronization mechanism the driver is using for
+	 * itself (for example, the port lock for uart serial consoles).
+	 *
+	 * The callback is always called from task context. It may use any
+	 * synchronization method required by the driver.
+	 *
+	 * IMPORTANT: The callback MUST 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.
+	 */
+	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;
-- 
2.39.2

Hi,

On Thu 2024-05-02 23:44:15, 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. To provide the
> necessary synchronization, usually some driver-specific locking
> mechanism is used (for example, the port spinlock for uart
> serial consoles).
>
> Until now, usage of this driver-specific locking has been hidden
> from the printk-subsystem and implemented within the various
> console callbacks. However, for nbcon consoles, it is necessary
> that the printk-subsystem uses the driver-specific locking so
> that nbcon console ownership can be acquired _after_ the
> driver-specific locking has succeeded. This allows for lock
> contention to exist on the more context-friendly driver-specific
> locking rather than nbcon console ownership (for non-emergency
> and non-panic cases).

Honestly, the above paragraph is kind of a puzzle. My first
understanding was that the driver-specific lock will always
need to be taken before acquiring the nbcon console ownership.

I believe that it is actually correct. But the right meaning
is hidden in the wording. I had to read it many times to get
it correctly and I knew what I was looking for.

I have to admit that providing a good explanation is probably
very hard. A proof is the long discussion in v4, see
https://lore.kernel.org/r/20240402221129.2613843-7-john.ogness@linutronix.de

BTW: I wonder if you use AI for generating the commit message.
     My experience is that AI produces longer fancy sentences
     which might be good for a novel but they sometimes hide
     the important details.



> Require nbcon consoles to implement two new callbacks
> (device_lock(), device_unlock()) that will use whatever
> synchronization mechanism the driver is using for itself.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

My attempt of a more strightforwward explanation:

<proposal>
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. To provide the
necessary synchronization, usually some driver-specific locking
mechanism is used (for example, the port spinlock for uart
serial consoles).

Until now, usage of this driver-specific locking has been hidden
from the printk-subsystem and implemented within the various
console callbacks. However, nbcon consoles would need to use it
even in the generic code.

Add device_lock() and device_unlock() callback which will need
to get implemented by nbcon consoles.

The callbacks will use whatever synchronization mechanism the driver
is using for itself. The minimum requirement is to prevent CPU
migration. It would allow a context friendly acquiring of nbcon
console ownership in non-emergency and non-panic context.
</proposal>

That said, I could live even with the original commit message.
Eitherway:

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

Best Regards,
Petr

On 2024-05-17, Petr Mladek <pmladek@suse.com> wrote:
> BTW: I wonder if you use AI for generating the commit message.
>      My experience is that AI produces longer fancy sentences
>      which might be good for a novel but they sometimes hide
>      the important details.

I do not know if that is a compliment or an insult.

For the record, I do not use AI. The "long fancy sentences hiding
important details" are coming from a sober brain... mine.

> My attempt of a more strightforwward explanation:

[...]

Your version does not mention why the generic code now needs to use the
driver-specific locking, but I suppose that does not matter (and only
adds confusion instead of explanation).

I will use your version.

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

Thanks.

John

On Fri 2024-05-17 16:06:30, John Ogness wrote:
> On 2024-05-17, Petr Mladek <pmladek@suse.com> wrote:
> > BTW: I wonder if you use AI for generating the commit message.
> >      My experience is that AI produces longer fancy sentences
> >      which might be good for a novel but they sometimes hide
> >      the important details.
> 
> I do not know if that is a compliment or an insult.
> 
> For the record, I do not use AI. The "long fancy sentences hiding
> important details" are coming from a sober brain... mine.

Ah, take it as a complaint then ;-) Complicated things are just hard
to explain.

> > My attempt of a more strightforwward explanation:
> 
> [...]
> 
> Your version does not mention why the generic code now needs to use the
> driver-specific locking, but I suppose that does not matter (and only
> adds confusion instead of explanation).

I removed these details on purpose. I think that they will be
easier to understand with the code.

Best Regards,
Petr