Message-ID: <87y19djqr0.fsf@jogness.linutronix.de>
Date: Tue, 16 Apr 2024 17:07:39 +0206
From: John Ogness <john.ogness@...utronix.de>
To: Petr Mladek <pmladek@...e.com>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>, Steven Rostedt
 <rostedt@...dmis.org>, Thomas Gleixner <tglx@...utronix.de>,
 linux-kernel@...r.kernel.org, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v4 06/27] printk: nbcon: Add callbacks to
 synchronize with driver

On 2024-04-09, Petr Mladek <pmladek@...e.com> 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>

I do not like this proposal. IMHO it is focussing only on nbcon
registration details (a corner case) rather than presenting the big
picture.

Yes, we will use these callbacks during registration/unregistration to
avoid some races, but that is just one usage (and not the primary
one). The main reason these callbacks exist is to provide driver
synchronization when printing.

We want to avoid using nbcon console ownership contention whenever
possible. In fact, there should _never_ be nbcon console owership
contention except for in emergency or panic situations.

In the normal case, printk will use the driver-specific locking for
synchronization. Previously this was achieved by implementing the
lock/unlock within the write() callback. But with nbcon consoles that is
not possible because the nbcon ownership must be taken outside of the
write callback:

con->device_lock()
nbcon_acquire()
con->write_atomic() or con->write_thread()
nbcon_release()
con->device_unlock()

How about this for the commit message:

printk: nbcon: Add callbacks to synchronize with driver

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.

John

P.S. I think it makes more sense to use your proposal for the commit
message of the follow-up patch "printk: nbcon: Use driver
synchronization while registering".

Powered by blists - more mailing lists