(added printk maintainers CC) Hi Dick, On 2019-12-19, Dick Hollenbeck <dick@xxxxxxxxxxx> 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
