From: Daniel Wagner <[email protected]>
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.
BUG: sleeping function called from invalid context at /home/wagi/work/rt/v4.4-cip-rt/kernel/locking/rtmutex.c:995
in_atomic(): 0, irqs_disabled(): 128, pid: 778, name: irq/76-eth0
CPU: 0 PID: 778 Comm: irq/76-eth0 Not tainted 4.4.126-test-cip22-rt14-00403-gcd03665c8318 #12
Hardware name: Generic RZ/G1 (Flattened Device Tree)
Backtrace:
[<c00140a0>] (dump_backtrace) from [<c001424c>] (show_stack+0x18/0x1c)
r7:c06b01f0 r6:60010193 r5:00000000 r4:c06b01f0
[<c0014234>] (show_stack) from [<c01d3c94>] (dump_stack+0x78/0x94)
[<c01d3c1c>] (dump_stack) from [<c004c134>] (___might_sleep+0x134/0x194)
r7:60010113 r6:c06d3559 r5:00000000 r4:ffffe000
[<c004c000>] (___might_sleep) from [<c04ded60>] (rt_spin_lock+0x20/0x74)
r5:c06f4d60 r4:c06f4d60
[<c04ded40>] (rt_spin_lock) from [<c02577e4>] (serial_console_write+0x100/0x118)
r5:c06f4d60 r4:c06f4d60
[<c02576e4>] (serial_console_write) from [<c0061060>] (call_console_drivers.constprop.15+0x10c/0x124)
r10:c06d2894 r9:c04e18b0 r8:00000028 r7:00000000 r6:c06d3559 r5:c06d2798
r4:c06b9914 r3:c02576e4
[<c0060f54>] (call_console_drivers.constprop.15) from [<c0062984>] (console_unlock+0x32c/0x430)
r10:c06d30d8 r9:00000028 r8:c06dd518 r7:00000005 r6:00000000 r5:c06d2798
r4:c06d2798 r3:00000028
[<c0062658>] (console_unlock) from [<c0062e1c>] (vprintk_emit+0x394/0x4f0)
r10:c06d2798 r9:c06d30ee r8:00000006 r7:00000005 r6:c06a78fc r5:00000027
r4:00000003
[<c0062a88>] (vprintk_emit) from [<c0062fa0>] (vprintk+0x28/0x30)
r10:c060bd46 r9:00001000 r8:c06b9a90 r7:c06b9a90 r6:c06b994c r5:c06b9a3c
r4:c0062fa8
[<c0062f78>] (vprintk) from [<c0062fb8>] (vprintk_default+0x10/0x14)
[<c0062fa8>] (vprintk_default) from [<c009cd30>] (printk+0x78/0x84)
[<c009ccbc>] (printk) from [<c025afdc>] (credit_entropy_bits+0x17c/0x2cc)
r3:00000001 r2:decade60 r1:c061a5ee r0:c061a523
r4:00000006
[<c025ae60>] (credit_entropy_bits) from [<c025bf74>] (add_interrupt_randomness+0x160/0x178)
r10:466e7196 r9:1f536000 r8:fffeef74 r7:00000000 r6:c06b9a60 r5:c06b9a3c
r4:dfbcf680
[<c025be14>] (add_interrupt_randomness) from [<c006536c>] (irq_thread+0x1e8/0x248)
r10:c006537c r9:c06cdf21 r8:c0064fcc r7:df791c24 r6:df791c00 r5:ffffe000
r4:df525180
[<c0065184>] (irq_thread) from [<c003fba4>] (kthread+0x108/0x11c)
r10:00000000 r9:00000000 r8:c0065184 r7:df791c00 r6:00000000 r5:df791d00
r4:decac000
[<c003fa9c>] (kthread) from [<c00101b8>] (ret_from_fork+0x14/0x3c)
r8:00000000 r7:00000000 r6:00000000 r5:c003fa9c r4:df791d00
Cc: Shinya Kuribayashi <[email protected]>
Signed-off-by: Daniel Wagner <[email protected]>
---
drivers/tty/serial/sh-sci.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
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)
--
2.14.3
Hi Daniel,
On Mon, Apr 30, 2018 at 10:09 AM, Daniel Wagner <[email protected]> wrote:
> From: Daniel Wagner <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Daniel Wagner <[email protected]>
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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 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
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
Hi Daniel,
On Tue, May 8, 2018 at 9:23 AM, Daniel Wagner <[email protected]> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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