aufs vs. m68k conflict, please advice

aufs vs. m68k conflict, please advice

[Thorsten Glaser]

Hi,

a build of linux-2.6 (3.2~rc4-1~experimental.1) with gcc-4.6 (to
check whether we can switch to it for the kernel, too) fails:

[…]
  LD [M]  fs/affs/affs.o
  LD      fs/aufs/built-in.o
  CC [M]  fs/aufs/module.o
In file included from /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/include/linux/hardirq.h:7:0,
                 from /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/arch/m68k/include/asm/irqflags.h:6,
                 from /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/include/linux/irqflags.h:15,
                 from /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/include/linux/spinlock.h:53,
                 from /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/include/linux/seqlock.h:29,
                 from /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/include/linux/time.h:8,
                 from /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/include/linux/stat.h:60,
                 from /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/include/linux/module.h:10,
                 from /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/fs/aufs/module.c:23:
/tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/arch/m68k/include/asm/hardirq.h: In function 'ack_bad_irq':
/tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/arch/m68k/include/asm/hardirq.h:23:2: error: expected ')' before 'AUFS_NAME'
make[7]: *** [fs/aufs/module.o] Error 1
[…]

The cause isn’t hard to figure out:

    I /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/arch/m68k R21  <40   C1  384 |180 115|73 14:39
   21 static inline void ack_bad_irq(unsigned int irq)
   22 {
   23         pr_crit("unexpected IRQ trap at vector %02x\n", irq);
   24 }

(pbuild26252)root@ara5:/ # fgrep AUFS_NAME /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/fs/aufs/*
/tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/fs/aufs/Makefile:ccflags-y += -D'pr_fmt(fmt)=AUFS_NAME"\040%s:%d:%s[%d]:\040"fmt,__func__,__LINE__,current->comm,current->pid'

This, to me, looks like cpp abuse in aufs, but I’m not a kernel
programmer (Linux or otherwise).

bye,
//mirabilos
-- 
Support mksh as /bin/sh and RoQA dash NOW!
‣ src:bash (240 (258) bugs: 0 RC, 167 (181) I&N, 73 (77) M&W, 0 F&P)
‣ src:dash (72 (82) bugs: 3 RC, 27 (30) I&N, 42 (49) M&W, 0 F&P)
‣ src:mksh (1 bug: 0 RC, 0 I&N, 1 M&W, 0 F&P)
http://qa.debian.org/data/bts/graphs/d/dash.png is pretty red, innit?

Re: aufs vs. m68k conflict, please advice

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On Fri, 2011-12-16 at 14:42 +0000, Thorsten Glaser wrote:
> Hi,
> 
> a build of linux-2.6 (3.2~rc4-1~experimental.1) with gcc-4.6 (to
> check whether we can switch to it for the kernel, too) fails:
[...]
> /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/arch/m68k/include/asm/hardirq.h: In function 'ack_bad_irq':
> /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/arch/m68k/include/asm/hardirq.h:23:2: error: expected ')' before 'AUFS_NAME'
> make[7]: *** [fs/aufs/module.o] Error 1
> […]
> 
> The cause isn’t hard to figure out:
> 
>     I /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/arch/m68k R21  <40   C1  384 |180 115|73 14:39
>    21 static inline void ack_bad_irq(unsigned int irq)
>    22 {
>    23         pr_crit("unexpected IRQ trap at vector %02x\n", irq);
>    24 }
> 
> (pbuild26252)root@ara5:/ # fgrep AUFS_NAME /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/fs/aufs/*
> /tmp/buildd/linux-2.6-3.2~rc4/debian/build/source_m68k_none/fs/aufs/Makefile:ccflags-y += -D'pr_fmt(fmt)=AUFS_NAME"\040%s:%d:%s[%d]:\040"fmt,__func__,__LINE__,current->comm,current->pid'
> 
> This, to me, looks like cpp abuse in aufs, but I’m not a kernel
> programmer (Linux or otherwise).

Maybe, but it should work just as long as AUFS_NAME is also defined in
advance.  Not sure why that's not also provided on the command line, or
why other architectures get away with it.  Maybe they just don't use
pr_*() in headers.

Ben.

-- 
Ben Hutchings
Computers are not intelligent.	They only think they are.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: aufs vs. m68k conflict, please advice

[Thorsten Glaser]

Ben Hutchings dixit:

>Maybe, but it should work just as long as AUFS_NAME is also defined in

Right, looks like it.

>advance.  Not sure why that's not also provided on the command line, or

Hrm. Can you forward this to the aufs people then, and maybe get
us some fix?

>why other architectures get away with it.  Maybe they just don't use
>pr_*() in headers.

*shrug* Like I said, no idea. I’m “just” the build “dæmon” ☺
(ok, with a bit of knowledge but nothing really learned)

bye,
//mirabilos
-- 
<ch> you introduced a merge commit        │<mika> % g rebase -i HEAD^^
<mika> sorry, no idea and rebasing just fscked │<mika> Segmentation
<ch> should have cloned into a clean repo      │  fault (core dumped)
<ch> if I rebase that now, it's really ugh     │<mika:#grml> wuahhhhhh

Re: aufs vs. m68k conflict, please advice

[Thorsten Glaser]

Ben Hutchings dixit:

>why other architectures get away with it.  Maybe they just don't use
>pr_*() in headers.

Maybe something like this?

#define ack_bad_irq(irq) do {					\
	pr_crit("unexpected IRQ trap at vector %02x\n",		\
	    (unsigned int)(irq));				\
} while (/* CONSTCOND */ 0)

This would defer pr_crit expansion to when the static inline
function was actually used.

Just an idea of the moment,
//mirabilos
-- 
  “Having a smoking section in a restaurant is like having
          a peeing section in a swimming pool.”
						-- Edward Burr

Re: aufs vs. m68k conflict, please advice

[Thorsten Glaser]

Dixi quod…

>Maybe something like this?

With that, aufs indeed compiles and module-links.

bye,
//mirabilos
-- 
  “Having a smoking section in a restaurant is like having
          a peeing section in a swimming pool.”
						-- Edward Burr

Re: aufs vs. m68k conflict, please advice

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Sat, 2011-12-17 at 14:28 +0000, Thorsten Glaser wrote:
> Ben Hutchings dixit:
> 
> >why other architectures get away with it.  Maybe they just don't use
> >pr_*() in headers.
> 
> Maybe something like this?
> 
> #define ack_bad_irq(irq) do {					\
> 	pr_crit("unexpected IRQ trap at vector %02x\n",		\
> 	    (unsigned int)(irq));				\
> } while (/* CONSTCOND */ 0)
> 
> This would defer pr_crit expansion to when the static inline
> function was actually used.

No, I don't think this is a bug in the header.

Ben.

-- 
Ben Hutchings
Computers are not intelligent.	They only think they are.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: aufs vs. m68k conflict, please advice

[Uwe Kleine-König]

On Sat, Dec 17, 2011 at 02:28:35PM +0000, Thorsten Glaser wrote:
> Ben Hutchings dixit:
> 
> >why other architectures get away with it.  Maybe they just don't use
> >pr_*() in headers.
> 
> Maybe something like this?
> 
> #define ack_bad_irq(irq) do {					\
> 	pr_crit("unexpected IRQ trap at vector %02x\n",		\
> 	    (unsigned int)(irq));				\
> } while (/* CONSTCOND */ 0)
> 
> This would defer pr_crit expansion to when the static inline
> function was actually used.
> 
> Just an idea of the moment,
IMHO the problem is that aufs provides an incomplete definition of
pr_fmt. Either it should define AUFS_NAME on the commandline, too, or
should define pr_fmt in an aufs header (or a .c file) #included after
all other headers and only when AUFS_NAME is defined, too.

The ugly thing about aufs' pr_fmt being already there when ack_bad_irq
is defined is, that the message printed by the pr_crit suddenly looks
aufs specific which it clearly isn't. So it should better make sure that
the definition isn't available to ack_bad_irq.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: aufs vs. m68k conflict, please advice

[Thorsten Glaser]

Uwe Kleine-K�nig dixit:

>On Sat, Dec 17, 2011 at 02:28:35PM +0000, Thorsten Glaser wrote:

>> Maybe something like this?
[…]
>> Just an idea of the moment,

Well, it does make the thing compile with minimal effort.

>IMHO the problem is that aufs provides an incomplete definition of
>pr_fmt. Either it should define AUFS_NAME on the commandline, too, or
>should define pr_fmt in an aufs header (or a .c file) #included after
>all other headers and only when AUFS_NAME is defined, too.

My initial thoughts, too.

>The ugly thing about aufs' pr_fmt being already there when ack_bad_irq
>is defined is, that the message printed by the pr_crit suddenly looks
>aufs specific which it clearly isn't. So it should better make sure that
>the definition isn't available to ack_bad_irq.

True, but looking at the actual changes, it doesn’t look too aufs
specific to me. (If the function ack_bad_irq is instantiated in
the aufs code at all, which is debatable; a quick fgrep -r doesn’t
find anything.)

Anyway, will please someone communicate that to the aufs developers
and include some sort of fix in the next upload? (My build is still
compiling, 132 bogomips is fast; will communicate success or failure
of the kernel in general.)

Thanks,
//mirabilos
-- 
<dileks> ch: good, you corrected yourself. ppl tend to tweet such news
immediately, sth. like "grml devs seem to be buyable"    <ch> dileks: we
_are_. if you throw enough money in our direction, things will happen
<mika> everyone is buyable, it's just a matter of price   <mrud> and now
comes [mira] and uses this as a signature ;0	   -- they asked for it…

Re: aufs vs. m68k conflict, please advice

[Uwe Kleine-König]

On Sat, Dec 17, 2011 at 07:00:00PM +0000, Thorsten Glaser wrote:
> Uwe Kleine-K�nig dixit:
Your mailer is broken. And it added:

	X-Message-Flag: Your mailer is broken. Get an update at http://www.washington.edu/pine/getpine/pcpine.html for free.

to your mail :-) I guess it means you not me?

> >On Sat, Dec 17, 2011 at 02:28:35PM +0000, Thorsten Glaser wrote:
> 
> >> Maybe something like this?
> […]
> >> Just an idea of the moment,
> 
> Well, it does make the thing compile with minimal effort.
> 
> >IMHO the problem is that aufs provides an incomplete definition of
> >pr_fmt. Either it should define AUFS_NAME on the commandline, too, or
> >should define pr_fmt in an aufs header (or a .c file) #included after
> >all other headers and only when AUFS_NAME is defined, too.
> 
> My initial thoughts, too.
> 
> >The ugly thing about aufs' pr_fmt being already there when ack_bad_irq
> >is defined is, that the message printed by the pr_crit suddenly looks
> >aufs specific which it clearly isn't. So it should better make sure that
> >the definition isn't available to ack_bad_irq.
> 
> True, but looking at the actual changes, it doesn’t look too aufs
> specific to me. (If the function ack_bad_irq is instantiated in
> the aufs code at all, which is debatable; a quick fgrep -r doesn’t
> find anything.)
Right, probably pr_something just shouldn't be used in headers I think.

The easiest fix would be:

-ccflags-y += -D'pr_fmt(fmt)=AUFS_NAME"\040%s:%d:%s[%d]:\040"fmt,__func__,__LINE__,current->comm,current->pid'
+ccflags-y += -D'pr_fmt(fmt)="aufs\040%s:%d:%s[%d]:\040"fmt,__func__,__LINE__,current->comm,current->pid'

But as it is ugly to have that in a Makefile, you can also try the patch
below.

Best regards
Uwe

From ad5d7fd2630feea17a1a6fffac74fdcb0505b2ee Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Date: Sat, 17 Dec 2011 20:45:21 +0100
Subject: [PATCH] new patch to not leak pr_fmt to foreign headers

---
 patches/features/all/aufs3/dont-leak-pr_fmt.patch |  211 +++++++++++++++++++++
 patches/series/base                               |    1 +
 2 files changed, 212 insertions(+), 0 deletions(-)
 create mode 100644 patches/features/all/aufs3/dont-leak-pr_fmt.patch

diff --git a/patches/features/all/aufs3/dont-leak-pr_fmt.patch b/patches/features/all/aufs3/dont-leak-pr_fmt.patch
new file mode 100644
index 0000000..93e05ca
--- /dev/null
+++ b/patches/features/all/aufs3/dont-leak-pr_fmt.patch
@@ -0,0 +1,211 @@
+From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
+Subject: [PATCH] don't leak incomplete definition of pr_fmt to headers
+
+This fixes a build problem on m68k, more details can be found at:
+http://lists.debian.org/debian-kernel/2011/12/msg00460.html
+---
+ fs/aufs/Makefile  |    2 --
+ fs/aufs/aufs.h    |    4 ++++
+ fs/aufs/branch.h  |    1 -
+ fs/aufs/cpup.h    |    1 -
+ fs/aufs/dbgaufs.h |    1 -
+ fs/aufs/debug.h   |    1 -
+ fs/aufs/dentry.h  |    1 -
+ fs/aufs/dir.h     |    1 -
+ fs/aufs/dynop.h   |    1 -
+ fs/aufs/file.h    |    1 -
+ fs/aufs/fstype.h  |    1 -
+ fs/aufs/inode.h   |    1 -
+ fs/aufs/opts.h    |    1 -
+ fs/aufs/rdu.c     |    1 -
+ fs/aufs/super.h   |    1 -
+ fs/aufs/sysaufs.h |    1 -
+ fs/aufs/whout.h   |    1 -
+ fs/aufs/wkq.h     |    1 -
+ 18 files changed, 4 insertions(+), 18 deletions(-)
+
+--- source_amd64_none.orig/fs/aufs/Makefile
++++ source_amd64_none/fs/aufs/Makefile
+@@ -8,8 +8,6 @@
+ # cf. include/linux/kernel.h
+ # enable pr_debug
+ ccflags-y += -DDEBUG
+-# sparse doesn't allow spaces
+-ccflags-y += -D'pr_fmt(fmt)=AUFS_NAME"\040%s:%d:%s[%d]:\040"fmt,__func__,__LINE__,current->comm,current->pid'
+ 
+ obj-$(CONFIG_AUFS_FS) += aufs.o
+ aufs-y := module.o sbinfo.o super.o branch.o xino.o sysaufs.o opts.o \
+--- source_amd64_none.orig/fs/aufs/aufs.h
++++ source_amd64_none/fs/aufs/aufs.h
+@@ -33,6 +33,11 @@
+ #define AuStubInt0(name, ...) \
+ 	AuStub(int, name, return 0, __VA_ARGS__)
+ 
++#include <linux/aufs_type.h>
++
++#undef pr_fmt
++#define pr_fmt(fmt) AUFS_NAME " %s:%d:%s[%d]: " fmt, __func__, __LINE__, current->comm, current->pid
++
+ #include "debug.h"
+ 
+ #include "branch.h"
+--- source_amd64_none.orig/fs/aufs/debug.h
++++ source_amd64_none/fs/aufs/debug.h
+@@ -35,7 +35,6 @@
+ #include <linux/delay.h>
+ /* #include <linux/kd.h> */
+ #include <linux/sysrq.h>
+-#include <linux/aufs_type.h>
+ 
+ #include <asm/system.h>
+ 
+--- source_amd64_none.orig/fs/aufs/file.h
++++ source_amd64_none/fs/aufs/file.h
+@@ -28,7 +28,6 @@
+ #include <linux/file.h>
+ #include <linux/fs.h>
+ #include <linux/poll.h>
+-#include <linux/aufs_type.h>
+ #include "rwsem.h"
+ 
+ struct au_branch;
+--- source_amd64_none.orig/fs/aufs/inode.h
++++ source_amd64_none/fs/aufs/inode.h
+@@ -27,7 +27,6 @@
+ 
+ #include <linux/fs.h>
+ #include <linux/fsnotify.h>
+-#include <linux/aufs_type.h>
+ #include "rwsem.h"
+ 
+ struct vfsmount;
+--- source_amd64_none.orig/fs/aufs/rdu.c
++++ source_amd64_none/fs/aufs/rdu.c
+@@ -24,7 +24,6 @@
+ #include <linux/fs_stack.h>
+ #include <linux/security.h>
+ #include <linux/uaccess.h>
+-#include <linux/aufs_type.h>
+ #include "aufs.h"
+ 
+ /* bits for struct aufs_rdu.flags */
+--- source_amd64_none.orig/fs/aufs/branch.h
++++ source_amd64_none/fs/aufs/branch.h
+@@ -27,7 +27,6 @@
+ 
+ #include <linux/fs.h>
+ #include <linux/mount.h>
+-#include <linux/aufs_type.h>
+ #include "dynop.h"
+ #include "rwsem.h"
+ #include "super.h"
+--- source_amd64_none.orig/fs/aufs/cpup.h
++++ source_amd64_none/fs/aufs/cpup.h
+@@ -27,7 +27,6 @@
+ 
+ #include <linux/path.h>
+ #include <linux/time.h>
+-#include <linux/aufs_type.h>
+ 
+ struct inode;
+ struct file;
+--- source_amd64_none.orig/fs/aufs/dbgaufs.h
++++ source_amd64_none/fs/aufs/dbgaufs.h
+@@ -26,7 +26,6 @@
+ #ifdef __KERNEL__
+ 
+ #include <linux/init.h>
+-#include <linux/aufs_type.h>
+ 
+ struct super_block;
+ struct au_sbinfo;
+--- source_amd64_none.orig/fs/aufs/dentry.h
++++ source_amd64_none/fs/aufs/dentry.h
+@@ -26,7 +26,6 @@
+ #ifdef __KERNEL__
+ 
+ #include <linux/dcache.h>
+-#include <linux/aufs_type.h>
+ #include "rwsem.h"
+ 
+ struct au_hdentry {
+--- source_amd64_none.orig/fs/aufs/dir.h
++++ source_amd64_none/fs/aufs/dir.h
+@@ -26,7 +26,6 @@
+ #ifdef __KERNEL__
+ 
+ #include <linux/fs.h>
+-#include <linux/aufs_type.h>
+ 
+ /* ---------------------------------------------------------------------- */
+ 
+--- source_amd64_none.orig/fs/aufs/dynop.h
++++ source_amd64_none/fs/aufs/dynop.h
+@@ -28,7 +28,6 @@
+ #include <linux/fs.h>
+ #include <linux/mm.h>
+ #include <linux/rcupdate.h>
+-#include <linux/aufs_type.h>
+ #include "inode.h"
+ 
+ enum {AuDy_AOP, AuDyLast};
+--- source_amd64_none.orig/fs/aufs/fstype.h
++++ source_amd64_none/fs/aufs/fstype.h
+@@ -28,7 +28,6 @@
+ #include <linux/fs.h>
+ #include <linux/magic.h>
+ #include <linux/romfs_fs.h>
+-#include <linux/aufs_type.h>
+ 
+ static inline int au_test_aufs(struct super_block *sb)
+ {
+--- source_amd64_none.orig/fs/aufs/opts.h
++++ source_amd64_none/fs/aufs/opts.h
+@@ -26,7 +26,6 @@
+ #ifdef __KERNEL__
+ 
+ #include <linux/path.h>
+-#include <linux/aufs_type.h>
+ 
+ struct file;
+ struct super_block;
+--- source_amd64_none.orig/fs/aufs/super.h
++++ source_amd64_none/fs/aufs/super.h
+@@ -26,7 +26,6 @@
+ #ifdef __KERNEL__
+ 
+ #include <linux/fs.h>
+-#include <linux/aufs_type.h>
+ #include "rwsem.h"
+ #include "spl.h"
+ #include "wkq.h"
+--- source_amd64_none.orig/fs/aufs/sysaufs.h
++++ source_amd64_none/fs/aufs/sysaufs.h
+@@ -26,7 +26,6 @@
+ #ifdef __KERNEL__
+ 
+ #include <linux/sysfs.h>
+-#include <linux/aufs_type.h>
+ #include "module.h"
+ 
+ struct super_block;
+--- source_amd64_none.orig/fs/aufs/whout.h
++++ source_amd64_none/fs/aufs/whout.h
+@@ -25,7 +25,6 @@
+ 
+ #ifdef __KERNEL__
+ 
+-#include <linux/aufs_type.h>
+ #include "dir.h"
+ 
+ /* whout.c */
+--- source_amd64_none.orig/fs/aufs/wkq.h
++++ source_amd64_none/fs/aufs/wkq.h
+@@ -28,7 +28,6 @@
+ 
+ #include <linux/sched.h>
+ #include <linux/wait.h>
+-#include <linux/aufs_type.h>
+ 
+ struct super_block;
+ 
diff --git a/patches/series/base b/patches/series/base
index dc8b037..98916b1 100644
--- a/patches/series/base
+++ b/patches/series/base
@@ -14,6 +14,7 @@
 + features/all/aufs3/aufs3-add.patch
 # mark as staging/crap
 + features/all/aufs3/mark-as-staging.patch
++ features/all/aufs3/dont-leak-pr_fmt.patch
 
 + bugfix/ia64/hardcode-arch-script-output.patch
 + bugfix/mips/disable-advansys.patch
-- 
1.7.7.3


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] m68k/irq: don't use pr_crit in an header

[Uwe Kleine-König]

Using pr_crit in an header results in funny messages. Consider

	#define pr_fmt(fmt) "mydriver: " fmt
	#include <linux/hardirq.h>

which makes the message from ack_bad_irq

	mydriver: unexpected IRQ trap...

so better use plain printk with KERN_CRIT directly.

This fixes a build problem on m68k with aufs3 en passant because the
latter builds with

	ccflags-y += -D'pr_fmt(fmt)=AUFS_NAME"\040%s:%d:%s[%d]:\040"fmt,__func__,__LINE__,current->comm,current->pid'

without providing AUFS_NAME early enough for ack_bad_irq (which is the
problem of aufs).

Cc: Thorsten Glaser <tg@debian.org>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/m68k/include/asm/hardirq.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/m68k/include/asm/hardirq.h b/arch/m68k/include/asm/hardirq.h
index db30ed2..1f652e0 100644
--- a/arch/m68k/include/asm/hardirq.h
+++ b/arch/m68k/include/asm/hardirq.h
@@ -20,7 +20,7 @@
 
 static inline void ack_bad_irq(unsigned int irq)
 {
-	pr_crit("unexpected IRQ trap at vector %02x\n", irq);
+	printk(KERN_CRIT "unexpected IRQ trap at vector %02x\n", irq);
 }
 
 /* entry.S is sensitive to the offsets of these fields */
-- 
1.7.7.3

Re: [PATCH] m68k/irq: don't use pr_crit in an header

[Thorsten Glaser]

Uwe Kleine-König dixit:

>so better use plain printk with KERN_CRIT directly.

Wasn’t it considered good style to switch f̲r̲o̲m̲ that t̲o̲ pr_crit?

In that case, my other patch from Message-ID
<Pine.BSM.4.64L.1112171426140.856@herc.mirbsd.org> can still
be used. Feel free to assume a Signed-off on that by me, if
this is the desired direction of change.

(Of course, aufs changes notwithstanding.)

bye,
//mirabilos
-- 
Support mksh as /bin/sh and RoQA dash NOW!
‣ src:bash (240 (258) bugs: 0 RC, 167 (181) I&N, 73 (77) M&W, 0 F&P)
‣ src:dash (72 (82) bugs: 3 RC, 27 (30) I&N, 42 (49) M&W, 0 F&P)
‣ src:mksh (1 bug: 0 RC, 0 I&N, 1 M&W, 0 F&P)
http://qa.debian.org/data/bts/graphs/d/dash.png is pretty red, innit?

Re: [PATCH] m68k/irq: don't use pr_crit in an header

[Geert Uytterhoeven]

2011/12/17 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Using pr_crit in an header results in funny messages. Consider
>
>        #define pr_fmt(fmt) "mydriver: " fmt
>        #include <linux/hardirq.h>
>
> which makes the message from ack_bad_irq
>
>        mydriver: unexpected IRQ trap...
>
> so better use plain printk with KERN_CRIT directly.

Yep, that's expected behavior, as defining pr_fmt() modifies all kernel messages
generated from that module.

> This fixes a build problem on m68k with aufs3 en passant because the
> latter builds with
>
>        ccflags-y += -D'pr_fmt(fmt)=AUFS_NAME"\040%s:%d:%s[%d]:\040"fmt,__func__,__LINE__,current->comm,current->pid'
>
> without providing AUFS_NAME early enough for ack_bad_irq (which is the
> problem of aufs).

Isn't this a problem with (out of tree) aufs?
Why does it put a define that relies on an (apparently sometimes still
undefined)
variable on the build command line?
Any header may contain calls to pr_*().

> Cc: Thorsten Glaser <tg@debian.org>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/m68k/include/asm/hardirq.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/m68k/include/asm/hardirq.h b/arch/m68k/include/asm/hardirq.h
> index db30ed2..1f652e0 100644
> --- a/arch/m68k/include/asm/hardirq.h
> +++ b/arch/m68k/include/asm/hardirq.h
> @@ -20,7 +20,7 @@
>
>  static inline void ack_bad_irq(unsigned int irq)
>  {
> -       pr_crit("unexpected IRQ trap at vector %02x\n", irq);
> +       printk(KERN_CRIT "unexpected IRQ trap at vector %02x\n", irq);

Nack. Nowadays pr_crit(...) is recommended over "printk(KERN_CRIT ...)".

Besides, there are (albeit not that many yet) other callers of pr_*() in
header files. Do you plan to revert them to printk(), too?

Please fix aufs instead. Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH] m68k/irq: don't use pr_crit in an header

[Uwe Kleine-König]

Hi Geert,

(thanks for adding Joe to Cc:, I noticed that when I wanted to add him
myself :-)

On Sun, Dec 18, 2011 at 11:32:21AM +0100, Geert Uytterhoeven wrote:
> 2011/12/17 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > Using pr_crit in an header results in funny messages. Consider
> >
> >        #define pr_fmt(fmt) "mydriver: " fmt
> >        #include <linux/hardirq.h>
> >
> > which makes the message from ack_bad_irq
> >
> >        mydriver: unexpected IRQ trap...
> >
> > so better use plain printk with KERN_CRIT directly.
> 
> Yep, that's expected behavior, as defining pr_fmt() modifies all kernel messages
> generated from that module.
I'm aware it is expected, I only wondered if it is also desirable to
have messages in headers modified depending on the module the header is
included in.
 
> > This fixes a build problem on m68k with aufs3 en passant because the
> > latter builds with
> >
> >        ccflags-y += -D'pr_fmt(fmt)=AUFS_NAME"\040%s:%d:%s[%d]:\040"fmt,__func__,__LINE__,current->comm,current->pid'
> >
> > without providing AUFS_NAME early enough for ack_bad_irq (which is the
> > problem of aufs).
> 
> Isn't this a problem with (out of tree) aufs?
> Why does it put a define that relies on an (apparently sometimes still
> undefined)
> variable on the build command line?
This is definitily a bug in aufs that needs fixing independant of the
issue of using or not using pr_... in headers.

> Any header may contain calls to pr_*().
> 
> > Cc: Thorsten Glaser <tg@debian.org>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  arch/m68k/include/asm/hardirq.h |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/m68k/include/asm/hardirq.h b/arch/m68k/include/asm/hardirq.h
> > index db30ed2..1f652e0 100644
> > --- a/arch/m68k/include/asm/hardirq.h
> > +++ b/arch/m68k/include/asm/hardirq.h
> > @@ -20,7 +20,7 @@
> >
> >  static inline void ack_bad_irq(unsigned int irq)
> >  {
> > -       pr_crit("unexpected IRQ trap at vector %02x\n", irq);
> > +       printk(KERN_CRIT "unexpected IRQ trap at vector %02x\n", irq);
> 
> Nack. Nowadays pr_crit(...) is recommended over "printk(KERN_CRIT ...)".
I know that, I just wonder if the proponents of this recommendation are
aware of the issue when using pr_* in headers. Joe?
 
> Besides, there are (albeit not that many yet) other callers of pr_*() in
> header files. Do you plan to revert them to printk(), too?
That depends on the outcome of this discussion.

> Please fix aufs instead. Thanks!
I already provided a patch for that, too. (Currently only on the Debian
kernel ML.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |


-- 
To UNSUBSCRIBE, email to debian-kernel-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: http://lists.debian.org/20111218104248.GS24496@pengutronix.de

Re: [PATCH] m68k/irq: don't use pr_crit in an header

[Joe Perches]

On Sun, 2011-12-18 at 11:42 +0100, Uwe Kleine-König wrote:
> On Sun, Dec 18, 2011 at 11:32:21AM +0100, Geert Uytterhoeven wrote:
> > 2011/12/17 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > > Using pr_crit in an header results in funny messages. Consider
> > >        #define pr_fmt(fmt) "mydriver: " fmt
> > >        #include <linux/hardirq.h>
> > > which makes the message from ack_bad_irq
> > >        mydriver: unexpected IRQ trap...
> > > so better use plain printk with KERN_CRIT directly.

Why or when is that inappropriate?

> I only wondered if it is also desirable to
> have messages in headers modified depending on the module the header is
> included in.
[]
> > Nack. Nowadays pr_crit(...) is recommended over "printk(KERN_CRIT ...)".
> I know that, I just wonder if the proponents of this recommendation are
> aware of the issue when using pr_* in headers. Joe?

I believe it to be a feature rather than a defect.

For instance:  commit 256ee435b9a9ee9cca69602fe8046b27ca99fbee

    netdevice: Convert printk to pr_info in netif_tx_stop_queue
    
    This allows any caller to be prefaced by any specific
    pr_fmt to better identify which device driver is using
    this function inappropriately. 

cheers, Joe