Re: v4.4.12-rt20 build: 0 failures 5 warnings (v4.4.12-rt20)

[Arnd Bergmann <arnd@arndb.de>]

On Friday, July 15, 2016 3:23:05 PM CEST Sebastian Andrzej Siewior wrote:
> * Arnd Bergmann | 2016-07-15 14:29:31 [+0200]:
> 
> >Agreed, but it's hard for the compiler to figure that out. On mainline,
> 
> we have the same thing in drivers/tty/serial/8250/8250_port.c btw. Don't
> see the warning there.

Interesting, I also see no substantial difference, my best guess is
that the warning appears or disappears based on some inlining
choices that can vary.

I also notice that your "tty: serial: 8250: don't take the trylock
during oops" patch would apply on the pl011 driver as well.

> >we have this instead:
> >
> >       local_irq_save(flags);
> >       if (uap->port.sysrq)
> >               locked = 0;
> >       else if (oops_in_progress)
> >               locked = spin_trylock(&uap->port.lock);
> >       else
> >               spin_lock(&uap->port.lock);
> >…
> >       if (locked)
> >               spin_unlock(&uap->port.lock);
> >       local_irq_restore(flags);
> >
> >which looks like it's intentionally written to avoid the warning
> >and was changed by Thomas in "tty/serial/pl011: Make the locking work on RT":
> >
> >http://git.kernel.org/cgit/linux/kernel/git/rt/linux-stable-rt.git/commit/drivers/tty/serial/amba-pl011.c?h=v4.4-rt-rebase&id=7b537b66fcb12c40eb1b10eb352d570a9d34a657
> >
> >Maybe there is another way to write this upstream that avoids the
> >warning.
> 
> I don't see any other way around except for initializing flags upfront:

Sure, that always works, it's just a bit ugly since the flags word
should never be zero when it gets written back to the hardware irq
state, at least in portable code.

Maybe something like the version below?

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8a9e213387a7..676fcfa7fce4 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2158,22 +2158,9 @@ static void pl011_console_putchar(struct uart_port *port, int ch)
 }
 
 static void
-pl011_console_write(struct console *co, const char *s, unsigned int count)
+__pl011_console_write(struct uart_amba_port *uap, const char *s, unsigned int count)
 {
-	struct uart_amba_port *uap = amba_ports[co->index];
 	unsigned int old_cr = 0, new_cr;
-	unsigned long flags;
-	int locked = 1;
-
-	clk_enable(uap->clk);
-
-	local_irq_save(flags);
-	if (uap->port.sysrq)
-		locked = 0;
-	else if (oops_in_progress)
-		locked = spin_trylock(&uap->port.lock);
-	else
-		spin_lock(&uap->port.lock);
 
 	/*
 	 *	First save the CR then disable the interrupts
@@ -2195,10 +2182,24 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)
 		cpu_relax();
 	if (!uap->vendor->always_enabled)
 		pl011_write(old_cr, uap, REG_CR);
+}
 
-	if (locked)
-		spin_unlock(&uap->port.lock);
-	local_irq_restore(flags);
+
+static void
+pl011_console_write(struct console *co, const char *s, unsigned int count)
+{
+	struct uart_amba_port *uap = amba_ports[co->index];
+	unsigned long flags;
+
+	clk_enable(uap->clk);
+
+	if (uap->port.sysrq || oops_in_progress) {
+		__pl011_console_write(uap, s, count);
+	} else {
+		spin_lock_irqsave(&uap->port.lock, flags);
+		__pl011_console_write(uap, s, count);
+		spin_unlock_irqrestore(&uap->port.lock, flags);
+	}
 
 	clk_disable(uap->clk);
 }