| Message ID | 20221118145512.509950-1-gsomlo@gmail.com |
|---|---|
| Headers | show |
| Series | serial: liteuart: add IRQ support | expand |
On 21. 11. 22, 19:50, Gabriel L. Somlo wrote: >>> static void liteuart_timer(struct timer_list *t) >>> { >>> struct liteuart_port *uart = from_timer(uart, t, timer); >>> struct uart_port *port = &uart->port; >>> - liteuart_rx_chars(port); >>> - >>> + liteuart_interrupt(0, port); >> >> Are you sure spin_lock() is safe from this path? I assume so, but have you >> thought about it? > > I checked and at that point `in_serving_softirq()` is true. > > *However*, after studying spin_lock() and friends for a while, I'm > not quite clear on whether that unequivocally translates > to "yes, we're safe" :) Depends whether some hard irq context is grabbing the port lock too. If it does, it will spin forever waiting for this soft irq to finish (never happens as it was interrupted). > As such, I'm inclined to switch to `spin_lock_irqsave()` and > `spin_unlock_irqrestore()` even in the interrupt handler, which is > explicitly stated to be "safe from any context": > https://www.kernel.org/doc/html/v4.15/kernel-hacking/locking.html#cheat-sheet-for-locking > The alternative could be to set `TIMER_IRQSAFE` in `timer_setup()`, > but no other tty driver seems to be doing that, so I'd be a bit off > the beaten path there... :) Ah, no. > Please do let me know what you think about this, particularly if you > consider going the spin_lock_irqsave-everywhere-just-to-be-safe route > overkill... :) If you are unsure about the other contexts, irqsave/restore is the way to go. It can be lifted later, if someone investigates harder. thanks,
On Tue, Nov 22, 2022 at 8:44 AM Jiri Slaby <jirislaby@kernel.org> wrote: > On 21. 11. 22, 19:50, Gabriel L. Somlo wrote: > >>> static void liteuart_timer(struct timer_list *t) > >>> { > >>> struct liteuart_port *uart = from_timer(uart, t, timer); > >>> struct uart_port *port = &uart->port; > >>> - liteuart_rx_chars(port); > >>> - > >>> + liteuart_interrupt(0, port); > >> > >> Are you sure spin_lock() is safe from this path? I assume so, but have you > >> thought about it? > > > > I checked and at that point `in_serving_softirq()` is true. > > > > *However*, after studying spin_lock() and friends for a while, I'm > > not quite clear on whether that unequivocally translates > > to "yes, we're safe" :) > > Depends whether some hard irq context is grabbing the port lock too. If > it does, it will spin forever waiting for this soft irq to finish (never > happens as it was interrupted). > > > As such, I'm inclined to switch to `spin_lock_irqsave()` and > > `spin_unlock_irqrestore()` even in the interrupt handler, which is > > explicitly stated to be "safe from any context": > > https://www.kernel.org/doc/html/v4.15/kernel-hacking/locking.html#cheat-sheet-for-locking > > > > > The alternative could be to set `TIMER_IRQSAFE` in `timer_setup()`, > > but no other tty driver seems to be doing that, so I'd be a bit off > > the beaten path there... :) > > Ah, no. > > > Please do let me know what you think about this, particularly if you > > consider going the spin_lock_irqsave-everywhere-just-to-be-safe route > > overkill... :) > > If you are unsure about the other contexts, irqsave/restore is the way > to go. It can be lifted later, if someone investigates harder. Inside the interrupt handler, plain spin_lock() should be OK. Inside the timer handler, I think you need to use spin_lock_irqsave(), as e.g. liteuart_set_termios() (and lots of code in serial_core.c) already uses spin_lock_irqsave(). Besides, on non-SMP, spin_lock() doesn't do anything, so you need to rely on disabling the local interrupt. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Nov 22, 2022 at 10:54:45AM +0100, Geert Uytterhoeven wrote: > On Tue, Nov 22, 2022 at 8:44 AM Jiri Slaby <jirislaby@kernel.org> wrote: > > On 21. 11. 22, 19:50, Gabriel L. Somlo wrote: > > >>> static void liteuart_timer(struct timer_list *t) > > >>> { > > >>> struct liteuart_port *uart = from_timer(uart, t, timer); > > >>> struct uart_port *port = &uart->port; > > >>> - liteuart_rx_chars(port); > > >>> - > > >>> + liteuart_interrupt(0, port); > > >> > > >> Are you sure spin_lock() is safe from this path? I assume so, but have you > > >> thought about it? > > > > > > I checked and at that point `in_serving_softirq()` is true. > > > > > > *However*, after studying spin_lock() and friends for a while, I'm > > > not quite clear on whether that unequivocally translates > > > to "yes, we're safe" :) > > > > Depends whether some hard irq context is grabbing the port lock too. If > > it does, it will spin forever waiting for this soft irq to finish (never > > happens as it was interrupted). > > > > > As such, I'm inclined to switch to `spin_lock_irqsave()` and > > > `spin_unlock_irqrestore()` even in the interrupt handler, which is > > > explicitly stated to be "safe from any context": > > > https://www.kernel.org/doc/html/v4.15/kernel-hacking/locking.html#cheat-sheet-for-locking > > > > > > > > > The alternative could be to set `TIMER_IRQSAFE` in `timer_setup()`, > > > but no other tty driver seems to be doing that, so I'd be a bit off > > > the beaten path there... :) > > > > Ah, no. > > > > > Please do let me know what you think about this, particularly if you > > > consider going the spin_lock_irqsave-everywhere-just-to-be-safe route > > > overkill... :) > > > > If you are unsure about the other contexts, irqsave/restore is the way > > to go. It can be lifted later, if someone investigates harder. > > Inside the interrupt handler, plain spin_lock() should be OK. > Inside the timer handler, I think you need to use spin_lock_irqsave(), > as e.g. liteuart_set_termios() (and lots of code in serial_core.c) > already uses spin_lock_irqsave(). > Besides, on non-SMP, spin_lock() doesn't do anything, so you need > to rely on disabling the local interrupt. Thanks Geert for the clarification! I could write two wrappers around the actual code doing the interrupt handler work, one with spin_lock() for the actual irq handler and another with spin_lock_irqsave() for the timer codepath. But to keep things simple I'm inclined to simply use spin_lock_irqsave() and add a comment saying it's because we need it when polling and it's not actually harmful when using IRQ. Thanks, --Gabriel > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Add IRQ support to the LiteX LiteUART serial interface Changes from v4: - picked up r/b Ilpo Järvinen on 12/14 and 13/14 - 12/14 ("add IRQ support for the RX path"): using dev_err() instead of combining pr_err() and pr_fmt(); also, remove superfluous comment > Changes from v3: > - picked up r/b Ilpo Järvinen on select subset of commits > - rebased entire series on top of tty-next tree > - 2/14 ("use bit number macros"): explicitly include <linux/bits.h> > - 3/14 ("remove unused uart_ops stubs"): don't add gratuitous comment > removed later on in the series > - 12/14 ("add IRQ support for the RX path"): add shadow irq register > to support polling mode and avoid reading hardware mmio irq register > to learn which irq flags are enabled > - this also simplifies both liteuart_interrupt() and liteuart_startup() > - 13/14 ("add IRQ support for the TX path"): > - removed superfluous curly braces from liteuart_interrupt() > - simplified [start|stop]_tx() by using shadow irq register from 12/14 > - simplified liteuart_tx_chars() by rebasing on top of tty-next tree > > Changes from v2: > - further split out "separate RX loop from poll timer" into > dedicated patches highlighting additional changes explicitly: > - factoring out tty_flip_buffer_push() (6/14) > - ack only RX events in RX loop (7/14) > - pass constant flag to uart_insert_char() directly (8/14) > - fix variable types in rx loop (9/14) > - separating RX loop from poll timer (10/14) > - added patch (11/14) to move function definitions to a more > convenient location, making subsequent changes easier to read > in diff patch form. > - fixes and clarifications for RX path IRQ support patch (now 12/14): > - only return IRQ_HANDLED for RX events > - use pr_fmt() to improve style of irq setup error message > - remove unnecessary locking from around single-register access > - modify TX path to support both IRQ-mode and polling (13/14) > - move polling-only liteuart_putchar() behind same conditional > (CONFIG_SERIAL_LITEUART_CONSOLE) as the rest of the code that's > still using it. > > Changes from v1: > - split minor cosmetic changes out into individual patches > (1/3 became 1..5/7) > - patches 6/7 and 7/7 unchanged (used to be 2/3 and 3/3) Gabriel Somlo (14): serial: liteuart: use KBUILD_MODNAME as driver name serial: liteuart: use bit number macros serial: liteuart: remove unused uart_ops stubs serial: liteuart: don't set unused port fields serial: liteuart: minor style fix in liteuart_init() serial: liteuart: move tty_flip_buffer_push() out of rx loop serial: liteuart: rx loop should only ack rx events serial: liteuart: simplify passing of uart_insert_char() flag serial: liteuart: fix rx loop variable types serial: liteuart: separate rx loop from poll timer serial: liteuart: move function definitions serial: liteuart: add IRQ support for the RX path serial: liteuart: add IRQ support for the TX path serial: liteuart: move polling putchar() function drivers/tty/serial/liteuart.c | 210 ++++++++++++++++++++-------------- 1 file changed, 126 insertions(+), 84 deletions(-)