| Message ID | 20180430080918.16763-1-wagi@monom.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On Mon, Apr 30, 2018 at 10:09 AM, 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. > Cc: Shinya Kuribayashi <shinya.kuribayashi.px@renesas.com> > Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com> Thanks for your patch! > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -2516,13 +2516,12 @@ static void serial_console_write(struct console *co, const char *s, > unsigned long flags; > int locked = 1; > > - local_irq_save(flags); Hence the below now runs with local interrupts enabled. For checking port->sysrq or oops_in_progress that probably isn't an issue. If oops_in_progress is set, you have other problems, and the race condition between checking the flag and calling spin_lock{,_irqsave}() existed before, and is hard to avoid. For actual console printing, I think you want to keep interrupts disabled. > if (port->sysrq) > locked = 0; > else 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); Add if (!locked local_irq_save(flags) here? > /* first save the SCSCR then disable the interrupts */ > ctrl = serial_port_in(port, SCSCR); > @@ -2539,8 +2538,7 @@ 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); > } Gr{oetje,eeting}s, Geert
On 2018-05-03 09:43:33 [+0200], Geert Uytterhoeven wrote: > > --- a/drivers/tty/serial/sh-sci.c > > +++ b/drivers/tty/serial/sh-sci.c > > @@ -2516,13 +2516,12 @@ static void serial_console_write(struct console *co, const char *s, > > unsigned long flags; > > int locked = 1; > > > > - local_irq_save(flags); > > Hence the below now runs with local interrupts enabled. > > For checking port->sysrq or oops_in_progress that probably isn't an issue. > If oops_in_progress is set, you have other problems, and the race condition > between checking the flag and calling spin_lock{,_irqsave}() existed before, > and is hard to avoid. while oops_in_progress is an issue of its own, the port->sysrq isn't avoided by by local_irq_save(). On SMP systems you can still receive a `break' signal on the UART and have a `printk()' issued on another CPU. > For actual console printing, I think you want to keep interrupts disabled. why? They should be disabled as part of getting the lock and not for any other reason. > > if (port->sysrq) > > locked = 0; > > else 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); > > Add > > if (!locked > local_irq_save(flags) > > here? So for oops_in_progress you get here with interrupts disabled. And if not, I don't see the point in disabling the interrupts without any kind of locking. > Gr{oetje,eeting}s, > > Geert > 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
On 05/07/2018 02:47 PM, Sebastian Andrzej Siewior wrote: > On 2018-05-03 09:43:33 [+0200], Geert Uytterhoeven wrote: >>> --- a/drivers/tty/serial/sh-sci.c >>> +++ b/drivers/tty/serial/sh-sci.c >>> @@ -2516,13 +2516,12 @@ static void serial_console_write(struct console *co, const char *s, >>> unsigned long flags; >>> int locked = 1; >>> >>> - local_irq_save(flags); >> >> Hence the below now runs with local interrupts enabled. >> >> For checking port->sysrq or oops_in_progress that probably isn't an issue. >> If oops_in_progress is set, you have other problems, and the race condition >> between checking the flag and calling spin_lock{,_irqsave}() existed before, >> and is hard to avoid. > > while oops_in_progress is an issue of its own, the port->sysrq isn't > avoided by by local_irq_save(). On SMP systems you can still receive a > `break' signal on the UART and have a `printk()' issued on another CPU. > >> For actual console printing, I think you want to keep interrupts disabled. > > why? They should be disabled as part of getting the lock and not for any > other reason. > >>> if (port->sysrq) >>> locked = 0; >>> else 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); >> >> Add >> >> if (!locked >> local_irq_save(flags) >> >> here? > > So for oops_in_progress you get here with interrupts disabled. And if > not, I don't see the point in disabling the interrupts without any kind > of locking. So I understand, the initial version of this patch was correct. @Geert if you don't object I'll send a v3 (v1 ported to mainline). 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
Hi Daniel, On Tue, May 8, 2018 at 9:23 AM, Daniel Wagner <wagi@monom.org> wrote: > On 05/07/2018 02:47 PM, Sebastian Andrzej Siewior wrote: >> On 2018-05-03 09:43:33 [+0200], Geert Uytterhoeven wrote: >>>> --- a/drivers/tty/serial/sh-sci.c >>>> +++ b/drivers/tty/serial/sh-sci.c >>>> @@ -2516,13 +2516,12 @@ static void serial_console_write(struct console >>>> *co, const char *s, >>>> unsigned long flags; >>>> int locked = 1; >>>> >>>> - local_irq_save(flags); >>> >>> >>> Hence the below now runs with local interrupts enabled. >>> >>> For checking port->sysrq or oops_in_progress that probably isn't an >>> issue. >>> If oops_in_progress is set, you have other problems, and the race >>> condition >>> between checking the flag and calling spin_lock{,_irqsave}() existed >>> before, >>> and is hard to avoid. >> >> while oops_in_progress is an issue of its own, the port->sysrq isn't >> avoided by by local_irq_save(). On SMP systems you can still receive a >> `break' signal on the UART and have a `printk()' issued on another CPU. >> >>> For actual console printing, I think you want to keep interrupts >>> disabled. >> >> why? They should be disabled as part of getting the lock and not for any >> other reason. >> >>>> if (port->sysrq) >>>> locked = 0; >>>> else 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); >>> >>> >>> Add >>> >>> if (!locked >>> local_irq_save(flags) >>> >>> here? >> >> >> So for oops_in_progress you get here with interrupts disabled. And if >> not, I don't see the point in disabling the interrupts without any kind >> of locking. > > > So I understand, the initial version of this patch was correct. > > @Geert if you don't object I'll send a v3 (v1 ported to mainline). Please go ahead, thanks! Gr{oetje,eeting}s, Geert
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index cf8d0955657d..3900711b60fc 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2516,13 +2516,12 @@ static void serial_console_write(struct console *co, const char *s, unsigned long flags; int locked = 1; - local_irq_save(flags); if (port->sysrq) locked = 0; else 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 the SCSCR then disable the interrupts */ ctrl = serial_port_in(port, SCSCR); @@ -2539,8 +2538,7 @@ 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); } static int serial_console_setup(struct console *co, char *options)