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

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

[Arnd Bergmann]

On Friday, July 15, 2016 5:25:17 AM CEST Build bot for Mark Brown wrote:
> Tree/Branch: v4.4.12-rt20
> Git describe: v4.4.12-rt20
> Commit: b4059f165a Linux 4.4.12-rt20
> 
> Build Time: 79 min 2 sec
> 
> Passed:    9 / 9   (100.00 %)
> Failed:    0 / 9   (  0.00 %)
> 
> Errors: 0
> Warnings: 5
> Section Mismatches: 0
> 
> -------------------------------------------------------------------------------
> defconfigs with issues (other than build errors):
>       1 warnings    0 mismatches  : arm64-allnoconfig
>       2 warnings    0 mismatches  : arm64-allmodconfig
>       1 warnings    0 mismatches  : arm-multi_v5_defconfig
>       2 warnings    0 mismatches  : arm-multi_v7_defconfig
>       1 warnings    0 mismatches  : x86_64-defconfig
>       3 warnings    0 mismatches  : arm-allmodconfig
>       1 warnings    0 mismatches  : arm-allnoconfig
>       1 warnings    0 mismatches  : x86_64-allnoconfig
>       2 warnings    0 mismatches  : arm64-defconfig
> 
> -------------------------------------------------------------------------------

Cc linux-rt-users@vger.kernel.org, let's have a look at the individual warnings:

> Warnings Summary: 5
> 	  9 ../kernel/sched/core.c:3473:12: warning: 'preemptible_lazy' defined but not used [-Wunused-function]

This was introduced by the rt patchset, I'd suggest adding a fix to the next
v4.4-rt release. This is almost certainly harmless.

> 	  2 ../include/linux/spinlock.h:246:3: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]

I'm missing context here, so I don't know what caused it, but the warning is
not present in v4.4.12 (without -rt), so it's worth looking into.

It happens for both arm-multi_v7_defconfig and arm64-defconfig.

> 	  1 ../lib/lz4/lz4hc_compress.c:514:1: warning: the frame size of 1472 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 	  1 ../drivers/xen/balloon.c:155:13: warning: 'release_memory_resource' declared 'static' but never defined [-Wunused-function]
> 	  1 ../drivers/mtd/chips/cfi_cmdset_0020.c:651:1: warning: the frame size of 1032 bytes is larger than 1024 bytes [-Wframe-larger-than=]

These three are present in v4.4.12 and only got fixed upstream later.
I have some hope that the fixes will make it into stable kernels
eventually. The "frame size" warnings are a side-effect of a gcc
problem with gcov profiling, and the xen warning was introduced by
a stable backport patch.

	Arnd

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

[Sebastian Andrzej Siewior]

* Arnd Bergmann | 2016-07-15 09:07:20 [+0200]:

>Cc linux-rt-users@vger.kernel.org, let's have a look at the individual warnings:
thanks.

>
>> Warnings Summary: 5
>> 	  9 ../kernel/sched/core.c:3473:12: warning: 'preemptible_lazy' defined but not used [-Wunused-function]
>
>This was introduced by the rt patchset, I'd suggest adding a fix to the next
>v4.4-rt release. This is almost certainly harmless.

has been adressed in 4.6.4-rt8 and is harmless.

>> 	  2 ../include/linux/spinlock.h:246:3: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
>
>I'm missing context here, so I don't know what caused it, but the warning is
>not present in v4.4.12 (without -rt), so it's worth looking into.

the full log:

|In file included from include/linux/seqlock.h:35:0,
|                 from include/linux/time.h:5,
|                 from include/uapi/linux/timex.h:56,
|                 from include/linux/timex.h:56,
|                 from include/linux/sched.h:19,
|                 from arch/arm64/include/asm/compat.h:25,
|                 from arch/arm64/include/asm/stat.h:23,
|                 from include/linux/stat.h:5,
|                 from include/linux/module.h:10,
|                 from drivers/tty/serial/amba-pl011.c:37:
|drivers/tty/serial/amba-pl011.c: In function ‘pl011_console_write’:
|include/linux/spinlock.h:246:3: warning: ‘flags’ may be used uninitialized in this function [-Wmaybe-uninitialized]
|   _raw_spin_unlock_irqrestore(lock, flags); \
|   ^
|drivers/tty/serial/amba-pl011.c:2065:16: note: ‘flags’ was declared here
|  unsigned long flags;
|                ^

and relevant part of the source:
|pl011_console_write(struct console *co, const char *s, unsigned int count)
|{
…
|        unsigned long flags;
|        int locked = 1;
|
…
|        if (uap->port.sysrq)
|                locked = 0;
|        else if (oops_in_progress)
|                locked = spin_trylock_irqsave(&uap->port.lock, flags);
|        else
|                spin_lock_irqsave(&uap->port.lock, flags);
…
|        if (locked)
|                spin_unlock_irqrestore(&uap->port.lock, flags);
…
|}

looks like a false positive to me.

>	Arnd

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

On Friday, July 15, 2016 2:05:27 PM CEST Sebastian Andrzej Siewior wrote:
> * Arnd Bergmann | 2016-07-15 09:07:20 [+0200]:
> 
> >Cc linux-rt-users@vger.kernel.org, let's have a look at the individual warnings:
> thanks.
> 
> >
> >> Warnings Summary: 5
> >> 	  9 ../kernel/sched/core.c:3473:12: warning: 'preemptible_lazy' defined but not used [-Wunused-function]
> >
> >This was introduced by the rt patchset, I'd suggest adding a fix to the next
> >v4.4-rt release. This is almost certainly harmless.
> 
> has been adressed in 4.6.4-rt8 and is harmless.

Ok.

> |drivers/tty/serial/amba-pl011.c: In function ‘pl011_console_write’:
> |include/linux/spinlock.h:246:3: warning: ‘flags’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> |   _raw_spin_unlock_irqrestore(lock, flags); \
> |   ^
> |drivers/tty/serial/amba-pl011.c:2065:16: note: ‘flags’ was declared here
> |  unsigned long flags;
> |                ^
> 
> and relevant part of the source:
> |pl011_console_write(struct console *co, const char *s, unsigned int count)
> |{
> …
> |        unsigned long flags;
> |        int locked = 1;
> |
> …
> |        if (uap->port.sysrq)
> |                locked = 0;
> |        else if (oops_in_progress)
> |                locked = spin_trylock_irqsave(&uap->port.lock, flags);
> |        else
> |                spin_lock_irqsave(&uap->port.lock, flags);
> …
> |        if (locked)
> |                spin_unlock_irqrestore(&uap->port.lock, flags);
> …
> |}
> 
> looks like a false positive to me.

Agreed, but it's hard for the compiler to figure that out. On mainline,
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.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sebastian Andrzej Siewior]

* 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.

>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:

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2161,18 +2161,17 @@ pl011_console_write(struct console *co, 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;
+	unsigned long flags = 0;
 	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);
+		locked = spin_trylock_irqsave(&uap->port.lock, flags);
 	else
-		spin_lock(&uap->port.lock);
+		spin_lock_irqsave(&uap->port.lock, flags);
 
 	/*
 	 *	First save the CR then disable the interrupts
@@ -2196,8 +2195,7 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)
 		pl011_write(old_cr, uap, REG_CR);
 
 	if (locked)
-		spin_unlock(&uap->port.lock);
-	local_irq_restore(flags);
+		spin_unlock_irqrestore(&uap->port.lock, flags);
 
 	clk_disable(uap->clk);
 }

>	Arnd

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

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);
 }

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

[Sebastian Andrzej Siewior]

* Arnd Bergmann | 2016-07-15 21:48:55 [+0200]:

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

That one. That is something I am not really sure about in the long run. On
-RT we can't try_lock() with IRQs off and that is why I removed it.
You could do the same with pl011 but you are screwed anyway because
clk_enable() will take a sleeping lock and that is a no no.

So you could stay with the try_lock because it does not solve anything.
In the long run I though about a console flag which denotes the console
as RT-IRQ save which is the case for 8250 but not for pl011 (due to
clk_enable()) so should not get on -RT into this case where it matters.

On the other hand if you oops on !RT+UP in your uart driver while
holding the lock then the try_lock will fail resulting in a lockdep
splat (because try_lock should not fail on UP). So might want take it as
a procation in that case :)

>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.

yes :)

>Maybe something like the version below?

sure.

Sebastian