Re: [BUG] sleep in atomic in 8250 runtime PM code path

[Russell King - ARM Linux]

On Sat, Sep 29, 2018 at 01:20:36PM +0800, Jisheng Zhang wrote:
> Hi,
> 
> Recently I found I could trigger sleep in atomic bug on berlin after commit
> d76c74387e1c ("serial: 8250_dw: Fix runtime PM handling"). The path looks like:
> 
> dw8250_probe => serial850_register_8250_port => uart_add_one_port=>
> register_console => console_unlock => univ8250_console_write =>
> serial8250_console_write => serial8250_rpm_get => pm_runtime_get_sync
> 
> The irq is disabled by printk_safe_enter_irqsave() in console_unlock, but
> pm_runtime_get_sync can't be called in atomic context...
> 
> I guess the reason why we didn't notice it is due to the fact that
> only OMAP and DW sets UART_CAP_RPM currently, and DW set the flag in
> May 2018.
> 
> Per my understanding, the bug sits in the 8250 core driver rather than
> 8250_dw.c.

(Adding Tony and Sebastian, presumably CAP_RPM comes from OMAP since
that is the only other user, and this same bug is present there too.)

Correct.  printk() can be called from atomic contexts (consider what
happens when an oops or similar occurs - we can be in any context,
holding any locks etc.)  Plain printk() can also be used from within
spinlocked irqs-off regions.

This means the console's write function may be called in these contexts.
Since pm_runtime_get_sync() is may sleep, it means that its use in the
console path is _fundamentally_ wrong, and will lead to exactly this
problem.

I don't see a way around that other than to avoid RPM on consoles.
(which makes the presence of the RPM code in serial8250_console_write()
completely unnecessary.)

When I rewrote the serial drivers and created serial_core & 8250, this
is something that I realised, and I arranged the PM support at the time
to always maintain the console in active state (this is prior to RPM).

While I'm looking at commit d74d5d1b7288 ("tty: serial: 8250_core: add
run time pm"):

+static void serial8250_rpm_get_tx(struct uart_8250_port *p)
+{
+       unsigned char rpm_active;
+
+       if (!(p->capabilities & UART_CAP_RPM))
+               return;
+
+       rpm_active = xchg(&p->rpm_tx_active, 1);
+       if (rpm_active)
+               return;
+       pm_runtime_get_sync(p->port.dev);
+}

is particularly "interesting" - if this is called from sections of
code that allow it to be called concurrently from different contexts,
then we could have:

rpm_tx_active	thread 0			thread 1
0
		xchg(, 1)
1
						xchg(, 1)
						... goes on to use port ...
		pm_runtime_get_sync()

In other words, the port can be used _before_ pm_runtime_get_sync() is
called.

If, on the other hand, this can't race, then considering the
serial8250_rpm_put_tx() path as well, what stops this race from
happening:

rpm_tx_active	thread 0			thread 1
1
		serial8250_rpm_get_tx()
						serial8250_rpm_put_tx()
		xchg(, 1)
1
						xchg(, 0)
0
						pm_runtime_put_autosuspend()

Now to the real point about the above - if _neither_ race is possible,
then what is the point of the more expensive xchg() here rather than
simple test-and-assignment of rpm_tx_active?  Either these paths can't
race with each other and xchg() is unnecessary, or they can and they
_could_ fail as shown above.  My suspicion is that xchg() is an attempt
to reduce the likelyhood of one of these races being hit.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 14.8Mbps down 650kbps up
According to speedtest.net: 13Mbps down 490kbps up