[tty,v1,01/74] serial: core: Provide port lock wrappers

  From: Thomas Gleixner <tglx@linutronix.de>

When a serial port is used for kernel console output, then all
modifications to the UART registers which are done from other contexts,
e.g. getty, termios, are interference points for the kernel console.

So far this has been ignored and the printk output is based on the
principle of hope. The rework of the console infrastructure which aims to
support threaded and atomic consoles, requires to mark sections which
modify the UART registers as unsafe. This allows the atomic write function
to make informed decisions and eventually to restore operational state. It
also allows to prevent the regular UART code from modifying UART registers
while printk output is in progress.

All modifications of UART registers are guarded by the UART port lock,
which provides an obvious synchronization point with the console
infrastructure.

Provide wrapper functions for spin_[un]lock*(port->lock) invocations so
that the console mechanics can be applied later on at a single place and
does not require to copy the same logic all over the drivers.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/serial_core.h | 79 +++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)
  

On Thu 2023-09-14 20:43:18, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> When a serial port is used for kernel console output, then all
> modifications to the UART registers which are done from other contexts,
> e.g. getty, termios, are interference points for the kernel console.
> 
> So far this has been ignored and the printk output is based on the
> principle of hope. The rework of the console infrastructure which aims to
> support threaded and atomic consoles, requires to mark sections which
> modify the UART registers as unsafe. This allows the atomic write function
> to make informed decisions and eventually to restore operational state. It
> also allows to prevent the regular UART code from modifying UART registers
> while printk output is in progress.
> 
> All modifications of UART registers are guarded by the UART port lock,
> which provides an obvious synchronization point with the console
> infrastructure.
> 
> Provide wrapper functions for spin_[un]lock*(port->lock) invocations so
> that the console mechanics can be applied later on at a single place and
> does not require to copy the same logic all over the drivers.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  include/linux/serial_core.h | 79 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index bb6f073bc159..f1d5c0d1568c 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> +/**
> + * uart_port_lock_irqsave - Lock the UART port, save and disable interrupts
> + * @up:		Pointer to UART port structure
> + * @flags:	Pointer to interrupt flags storage
> + */
> +static inline void uart_port_lock_irqsave(struct uart_port *up, unsigned long *flags)
> +{
> +	spin_lock_irqsave(&up->lock, *flags);
> +}

IMHO, it would have been better to pass the flags variable directly
via a macro as it is done in most *_lock_*_irqsafe() APIs. I mean
something like:

/**
 * uart_port_trylock_irqsave - Try to lock the UART port, save and disable interrupts
 * @up:		Pointer to UART port structure
 * @flags:	Interrupt flags storage
 *
 * Returns: True if lock was acquired, false otherwise
 */
#define uart_port_lock_irqsave(up, flags)		\
({							\
	local_irq_save(flags);				\
	uart_port_lock(lock)				\
})

> +
> +/**
> + * uart_port_trylock - Try to lock the UART port
> + * @up:		Pointer to UART port structure
> + *
> + * Returns: True if lock was acquired, false otherwise
> + */
> +static inline bool uart_port_trylock(struct uart_port *up)
> +{
> +	return spin_trylock(&up->lock);
> +}
> +
> +/**
> + * uart_port_trylock_irqsave - Try to lock the UART port, save and disable interrupts
> + * @up:		Pointer to UART port structure
> + * @flags:	Pointer to interrupt flags storage
> + *
> + * Returns: True if lock was acquired, false otherwise
> + */
> +static inline bool uart_port_trylock_irqsave(struct uart_port *up, unsigned long *flags)
> +{
> +	return spin_trylock_irqsave(&up->lock, *flags);
> +}

Similar here:

/**
 * uart_port_trylock_irqsave - Try to lock the UART port, save and disable interrupts
 * @up:		Pointer to UART port structure
 * @flags:	Interrupt flags storage
 *
 * Returns: True if lock was acquired, false otherwise
 */
#define uart_port_trylock_irqsave(up, flags)			\
({								\
	bool __ret;						\
								\
	local_irq_save(flags);					\
	__ret = uart_port_trylock(lock)				\
	if (!__ret)						\
		local_irq_restore(flags);			\
	__ret;							\
})

I do not resist on this rather cosmetic change. The current code seems
to be doing what is expected. Feel free to keep it and use:

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

Best Regards,
Petr

PS: I am sorry for the late review. I have made a quick look on Monday
    and it looked straightforward. I have got this idea today when
    having a closer look.
  

On Tue, Sep 19, 2023 at 04:24:48PM +0200, Petr Mladek wrote:
> On Thu 2023-09-14 20:43:18, John Ogness wrote:
> > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > When a serial port is used for kernel console output, then all
> > modifications to the UART registers which are done from other contexts,
> > e.g. getty, termios, are interference points for the kernel console.
> > 
> > So far this has been ignored and the printk output is based on the
> > principle of hope. The rework of the console infrastructure which aims to
> > support threaded and atomic consoles, requires to mark sections which
> > modify the UART registers as unsafe. This allows the atomic write function
> > to make informed decisions and eventually to restore operational state. It
> > also allows to prevent the regular UART code from modifying UART registers
> > while printk output is in progress.
> > 
> > All modifications of UART registers are guarded by the UART port lock,
> > which provides an obvious synchronization point with the console
> > infrastructure.
> > 
> > Provide wrapper functions for spin_[un]lock*(port->lock) invocations so
> > that the console mechanics can be applied later on at a single place and
> > does not require to copy the same logic all over the drivers.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  include/linux/serial_core.h | 79 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 79 insertions(+)
> > 
> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > index bb6f073bc159..f1d5c0d1568c 100644
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > +/**
> > + * uart_port_lock_irqsave - Lock the UART port, save and disable interrupts
> > + * @up:		Pointer to UART port structure
> > + * @flags:	Pointer to interrupt flags storage
> > + */
> > +static inline void uart_port_lock_irqsave(struct uart_port *up, unsigned long *flags)
> > +{
> > +	spin_lock_irqsave(&up->lock, *flags);
> > +}
> 
> IMHO, it would have been better to pass the flags variable directly
> via a macro as it is done in most *_lock_*_irqsafe() APIs. I mean
> something like:
> 
> /**
>  * uart_port_trylock_irqsave - Try to lock the UART port, save and disable interrupts
>  * @up:		Pointer to UART port structure
>  * @flags:	Interrupt flags storage
>  *
>  * Returns: True if lock was acquired, false otherwise
>  */
> #define uart_port_lock_irqsave(up, flags)		\
> ({							\
> 	local_irq_save(flags);				\
> 	uart_port_lock(lock)				\
> })
> 
> > +
> > +/**
> > + * uart_port_trylock - Try to lock the UART port
> > + * @up:		Pointer to UART port structure
> > + *
> > + * Returns: True if lock was acquired, false otherwise
> > + */
> > +static inline bool uart_port_trylock(struct uart_port *up)
> > +{
> > +	return spin_trylock(&up->lock);
> > +}
> > +
> > +/**
> > + * uart_port_trylock_irqsave - Try to lock the UART port, save and disable interrupts
> > + * @up:		Pointer to UART port structure
> > + * @flags:	Pointer to interrupt flags storage
> > + *
> > + * Returns: True if lock was acquired, false otherwise
> > + */
> > +static inline bool uart_port_trylock_irqsave(struct uart_port *up, unsigned long *flags)
> > +{
> > +	return spin_trylock_irqsave(&up->lock, *flags);
> > +}
> 
> Similar here:
> 
> /**
>  * uart_port_trylock_irqsave - Try to lock the UART port, save and disable interrupts
>  * @up:		Pointer to UART port structure
>  * @flags:	Interrupt flags storage
>  *
>  * Returns: True if lock was acquired, false otherwise
>  */
> #define uart_port_trylock_irqsave(up, flags)			\
> ({								\
> 	bool __ret;						\
> 								\
> 	local_irq_save(flags);					\
> 	__ret = uart_port_trylock(lock)				\
> 	if (!__ret)						\
> 		local_irq_restore(flags);			\
> 	__ret;							\
> })

What is the difference here of a macro vs. an inline function going to
do for the resulting binary?  The important thing is now we have wrapper
functions, people can tweak them all they want to see if we can get
better results :)

thanks for the review!

greg k-h
  

On Tue, Sep 19 2023 at 16:24, Petr Mladek wrote:
> On Thu 2023-09-14 20:43:18, John Ogness wrote:
> IMHO, it would have been better to pass the flags variable directly
> via a macro as it is done in most *_lock_*_irqsafe() APIs. I mean
> something like:
>
> /**
>  * uart_port_trylock_irqsave - Try to lock the UART port, save and disable interrupts
>  * @up:		Pointer to UART port structure
>  * @flags:	Interrupt flags storage
>  *
>  * Returns: True if lock was acquired, false otherwise
>  */
> #define uart_port_lock_irqsave(up, flags)		\
> ({							\
> 	local_irq_save(flags);				\
> 	uart_port_lock(lock)				\
> })

It's worse.

     1) Macros are not type safe by themself and rely on type safety
        of the inner workings.

     2) Macros are bad for grep as you can't search for a 'struct foo *'
        argument. Even semantic parsers have their problems with macro
        constructs. I just learned that again when doing this.

     3) Macros are just horrible to read

     4) If you want to out of line the wrapper later, then you still
        have to keep the macro around because the 'flags' argument is by
        value and not a pointer.

From a code generation point of view it's completely irrelevant whether
you have a macro or an inline. That was different 25 years ago, but who
cares about museum compilers today.

Thanks,

        tglx
  

Adding Peter into Cc who also has a big experience with locking APIs.
Well, I think that he would not care ;-)

On Tue 2023-09-19 21:51:12, Thomas Gleixner wrote:
> On Tue, Sep 19 2023 at 16:24, Petr Mladek wrote:
> > On Thu 2023-09-14 20:43:18, John Ogness wrote:
> > IMHO, it would have been better to pass the flags variable directly
> > via a macro as it is done in most *_lock_*_irqsafe() APIs. I mean
> > something like:
> >
> > /**
> >  * uart_port_trylock_irqsave - Try to lock the UART port, save and disable interrupts
> >  * @up:		Pointer to UART port structure
> >  * @flags:	Interrupt flags storage
> >  *
> >  * Returns: True if lock was acquired, false otherwise
> >  */
> > #define uart_port_lock_irqsave(up, flags)		\
> > ({							\
> > 	local_irq_save(flags);				\
> > 	uart_port_lock(lock)				\
> > })
> 
> It's worse.
> 
>      1) Macros are not type safe by themself and rely on type safety
>         of the inner workings.
> 
>      2) Macros are bad for grep as you can't search for a 'struct foo *'
>         argument. Even semantic parsers have their problems with macro
>         constructs. I just learned that again when doing this.
> 
>      3) Macros are just horrible to read
> 
>      4) If you want to out of line the wrapper later, then you still
>         have to keep the macro around because the 'flags' argument is by
>         value and not a pointer.
> 
> >From a code generation point of view it's completely irrelevant whether
> you have a macro or an inline. That was different 25 years ago, but who
> cares about museum compilers today.

I probably was not clear enough. The difference and the motivation
is to pass the "flags" variable instead of pointer "*flags".

Both most common APIs, local_irq_save(flags) and
spin_lock_irqsave(lock, flags) pass the variable. Also most
subsystem specific wrappers do so.

IMHO, some consistency makes the life easier for developers,
especially for frequently used API. And the patchset seems to be
adding >350 users of the uart_port_lock_*irqsave() API.

I do not know. Maybe using macros was bad decision for local_irq*()
and spin_lock*() API. Anyway, we are going to live with
the uart_port_lock*() API for a looong time. And I want to be sure
that we do not create (small) headaches for future generations.

OK, maybe this particular difference is not important enough.
As I said, I do not resist on the change.

Best Regards,
Petr