| Message ID | 20180504163041.28726-1-wagi@monom.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On Fri, May 4, 2018 at 6:30 PM, Daniel Wagner <wagi@monom.org> wrote: > From: Daniel Wagner <daniel.wagner@siemens.com> > > Commit 40f70c03e33a ("serial: sh-sci: add locking to console write > function to avoid SMP lockup") copied the strategy to avoid locking > problems in conjuncture with the console from the UART8250 > driver. Instead using directly spin_{try}lock_irqsave(), > local_irq_save() followed by spin_{try}lock() was used. While this is > correct on mainline, for -rt it is a problem. spin_{try}lock() will > check if it is running in a valid context. Since the local_irq_save() > has already been executed, the context has changed and > spin_{try}lock() will complain. The reason why spin_{try}lock() > complains is that on -rt the spin locks are turned into mutexes and > therefore can sleep. Sleeping with interrupts disabled is not valid. [...] > --- > > changes since v1: > - Ported to current mainline (initial version was against v4.4.y) > - Left local_irq_save() in place when spinlocks are not used as suggested > by Geert. Thanks for the update! > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -2890,16 +2890,16 @@ static void serial_console_write(struct console *co, const char *s, > unsigned long flags; > int locked = 1; > > - local_irq_save(flags); > #if defined(SUPPORT_SYSRQ) > - if (port->sysrq) > + if (port->sysrq) { > locked = 0; > - else > + local_irq_save(flags); > + } else > #endif > if (oops_in_progress) > - locked = spin_trylock(&port->lock); > + locked = spin_trylock_irqsave(&port->lock, flags); If the spinlock could not be taken, interrupts are re-enabled: include/linux/spinlock.h: #define raw_spin_trylock_irqsave(lock, flags) \ ({ \ local_irq_save(flags); \ raw_spin_trylock(lock) ? \ 1 : ({ local_irq_restore(flags); 0; }); \ }) hence I think you need to check for this and disable interrupts again: if (!locked) local_irq_save(flags); > else > - spin_lock(&port->lock); > + spin_lock_irqsave(&port->lock, flags); > > /* first save SCSCR then disable interrupts, keep clock source */ > ctrl = serial_port_in(port, SCSCR); > @@ -2919,8 +2919,9 @@ static void serial_console_write(struct console *co, const char *s, > serial_port_out(port, SCSCR, ctrl); > > if (locked) > - spin_unlock(&port->lock); > - local_irq_restore(flags); > + spin_unlock_irqrestore(&port->lock, flags); > + else > + local_irq_restore(flags); > } > > static int serial_console_setup(struct console *co, char *options) Gr{oetje,eeting}s, Geert
On 2018-05-04 18:30:41 [+0200], Daniel Wagner wrote: > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -2890,16 +2890,16 @@ static void serial_console_write(struct console *co, const char *s, > unsigned long flags; > int locked = 1; > > - local_irq_save(flags); > #if defined(SUPPORT_SYSRQ) > - if (port->sysrq) > + if (port->sysrq) { > locked = 0; > - else > + local_irq_save(flags); how is this helping? You should see a splat after a sysrq request. > + } else > #endif > if (oops_in_progress) > - locked = spin_trylock(&port->lock); > + locked = spin_trylock_irqsave(&port->lock, flags); > else > - spin_lock(&port->lock); > + spin_lock_irqsave(&port->lock, flags); > > /* first save SCSCR then disable interrupts, keep clock source */ > ctrl = serial_port_in(port, SCSCR); Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sebastian, On 05/07/2018 02:51 PM, Sebastian Andrzej Siewior wrote: > On 2018-05-04 18:30:41 [+0200], Daniel Wagner wrote: >> --- a/drivers/tty/serial/sh-sci.c >> +++ b/drivers/tty/serial/sh-sci.c >> @@ -2890,16 +2890,16 @@ static void serial_console_write(struct console *co, const char *s, >> unsigned long flags; >> int locked = 1; >> >> - local_irq_save(flags); >> #if defined(SUPPORT_SYSRQ) >> - if (port->sysrq) >> + if (port->sysrq) { >> locked = 0; >> - else >> + local_irq_save(flags); > > how is this helping? You should see a splat after a sysrq request. You are right, I didn't really think this through. Should 'echo t > /proc/sysrq' trigger the splat? At least I was so naive that think it would be enough. Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-05-08 09:18:44 [+0200], Daniel Wagner wrote: > Hi Sebastian, Hi, > Should 'echo t > /proc/sysrq' trigger the splat? At least I was so naive > that think it would be enough. No, this is a different interface. The thing is you send the BREAK command and then, the next character (within a timeout) that comes over the UART is the MAGIC request. While that character is being received the sysrq request is answered and output is written to the console which is probably the UART and so you can't acquire the lock again. So you can't acquire the lock. I added a try_lock once to at least try to acquire the lock because this may race against a printk() from another CPU. But then someone complained about a failed try_lock on UP (this can't happen on UP unless in a scenario like this where the lock is already acquired) so this change to the 8250 was reverted. So the whole situation of the console interface is not perfect and this is the duct tape we have. It would good to have the same duct tape in all drivers or come up with a different interface to cover corner cases :) To reproduce the sysrq code path: You need to the UART as a console and send a BREAK (CTRL-A F in minicom) followed by `t' to match your example. > Thanks, > Daniel Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index fdbbff547106..ec7417be9e08 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2890,16 +2890,16 @@ static void serial_console_write(struct console *co, const char *s, unsigned long flags; int locked = 1; - local_irq_save(flags); #if defined(SUPPORT_SYSRQ) - if (port->sysrq) + if (port->sysrq) { locked = 0; - else + local_irq_save(flags); + } else #endif if (oops_in_progress) - locked = spin_trylock(&port->lock); + locked = spin_trylock_irqsave(&port->lock, flags); else - spin_lock(&port->lock); + spin_lock_irqsave(&port->lock, flags); /* first save SCSCR then disable interrupts, keep clock source */ ctrl = serial_port_in(port, SCSCR); @@ -2919,8 +2919,9 @@ static void serial_console_write(struct console *co, const char *s, serial_port_out(port, SCSCR, ctrl); if (locked) - spin_unlock(&port->lock); - local_irq_restore(flags); + spin_unlock_irqrestore(&port->lock, flags); + else + local_irq_restore(flags); } static int serial_console_setup(struct console *co, char *options)