Re: 8250: set_ier(), clear_ier() and the concept of atomic console writes

[John Ogness <john.ogness@linutronix.de>]

(added printk maintainers CC)

Hi Dick,

On 2019-12-19, Dick Hollenbeck <dick@softplc.com> wrote:
> Let's talk serially:
>
> When using serial port debug printing, I have always dedicated a
> serial port for that.  In contrast, I have never tried to dovetail
> debug messages with actual other use of the serial port.  I don't
> understand the use case for changing to polled mode on the fly and
> then changing back.  I could not disconnect the normal use (interrupt
> driven) serial cable and then rapidly attach another serial cable
> intended for capturing debug messages in time to see a polled debug
> message interleaved with normal port usage.  So I think the use case
> contemplated by the current code is rare if non-existent.  And the
> result is that the code is super ugly.

In Linux there are consoles, which I dare say is a common use case for
UARTs (i.e debug messages interleaved with normal port usage is not rare
and definitely exists).

> In fact the serial port code is getting uglier by the month. Its
> gotten too complex and bulkier for weird apparent requirements.  When
> those corner case requirements are met such that concurrent use is
> mandated, the solution is always more complex than if requirements can
> be considered mutually exclusive.  (e.g. Who prints debug statements
> onto a serial cable that is already in use for a MODBUS driver?  How
> would you see those print statements easily?)
>
> But for now, I drill down to a specific complaint.
>
> I am looking at branch linux-5.2.y-rt-rebase, file:
>   drivers/tty/serial/8250/8250_core.c
>   commit 82dfc28946eacceae by John Ogness.
>
>
> Here is a belated partial patch review, and would be some of the
> reasons I could not accept this patch if I was in charge of mainline:

Thanks. I've been waiting nearly a year for feedback on these patches.

> 1) The mutual exclusion used in set_eir() clear_ier() feels like the
> big kernel lock all over again.  All ports using the same lock....

Yes, this is a problem. Only consoles should be using the cpu-lock. I
will address this.

> 2) Those functions are in an 8250 specific area but do not have 8250
> specific names, and they are public.

So you would like to see them renamed to:

   serial8250_set_ier()
   serial8250_clear_ier()
   serial8250_restore_ier()

> 3) The lock used in set_eir() & clear_eir() puts the CPU into atomic
> mode and then calls serial_port_out().  This assumes that we know
> everything there will ever be to know about serial_port_out(), even
> though it is an abstraction, with multiple implementations now and
> more in the future.

Mainline serial8250_console_write() calls serial_port_out() in atomic
context as well. So this is already a requirement of serial_port_out(),
at least for consoles. And if the cpu-lock is only taken for consoles
(see my response to #1), then this should not introduce any new issues.

> In fact, the future is now.  I have expanded serial_port_out() to use
> a spinlock because my UART is in an FPGA, along with other peripherals
> which share a common FPGA interface.  The spinlock gets remapped to
> rt_mutex.  When there is a collision on that mutex somebody must get
> suspended, and then I see the kernel report of:
>
> BUG: scheduling while atomic: irq/56-ttyS5/592/0x00000002

If your FPGA-UART should be a console, then it is a bug in your
implementation (for mainline as well). I don't know if non-consoles also
have atomic requirements.

> fpga_write() is where the rt_mutex is, aka spinlock in true source
> representation.  I can run a couple million bytes through that
> function for UARTs and other peripherals, but you know RT, if can go
> wrong it will, and it eventually does.
>
> The atomic mode was entered in the common (shared_by_all_ports) serial
> lock in console_atomic_lock().  This is because the author was trying
> to provide mutual exclusion between debug messages and actual normal
> serial port use.  I think that is fool's gold.  I have no problem with
> serial port debug messages, but I don't share a port when I am using
> them.

You don't use your debug serial port as a console. That is a wise
choice that unfortunately most people will not make.

> So the next question is, do I go to a raw spin lock in my
> fpga_write(), or do I try and fix the usage of console_atomic_lock()
> in set_eir() and clear_eir()?

- I will change the functions to only take the cpu-lock if the 8250 is a
  console.

- I will rename the functions to serial8250_*.

I believe that will address your main points. However, if you want to
use your FPGA-UART as a console, you will need to change to a raw spin
lock (even for mainline).

Thanks for the feedback.

John Ogness