On Tue, 10 Mar 2009, Stephen Rothwell wrote:
> Today's linux-next build (x86_64 allmodconfig) failed like this:
>
> crypto/zlib.c: In function 'zlib_compress_update':
> crypto/zlib.c:148: warning: initialization makes integer from pointer without a cast
> crypto/zlib.c:148: error: initializer element is not computable at load time
> crypto/zlib.c:148: error: (near initialization for 'descriptor.primary_hash')
> crypto/zlib.c:148: warning: excess elements in struct initializer
> crypto/zlib.c:148: warning: (near initialization for 'descriptor')
>
> And many more similar. This line is a pr_debug() statement, so the
> finger points at commit 25b67b75587d43ff3f09ad88c03c70a38372d95d
> ("dynamic debug: combine dprintk and dynamic printk") from the
> driver-core tree.
>
> The preprocessed code looks like this:
>
> static int zlib_compress_update(struct crypto_pcomp *tfm,
> struct comp_request *req)
> {
> int ret;
> struct zlib_ctx *dctx = crypto_tfm_ctx(crypto_pcomp_tfm(tfm));
> struct z_stream_s *stream = &dctx->comp_stream;
>
> do { do { static struct _ddebug descriptor __attribute__((__used__)) __attribute__((section("__verbose"), aligned(8))) = { "zlib", __func__, "/scratch/sfr/next/crypto/zlib.c", "%s: " "avail_in %u, avail_out %u\n", __func__, 55, 33, 148, 0 }; if (({ int __ret = 0; if (__builtin_expect(!!((dynamic_debug_enabled & (1LL << 55)) && (dynamic_debug_enabled2 & (1LL << 33))), 0)) if (__builtin_expect(!!(descriptor.flags), 0)) __ret = 1; __ret; })) printk("<7>" "zlib" ":" "%s: " "avail_in %u, avail_out %u\n", __func__, req->avail_in, req->avail_out); } while (0); } while (0);
>
> The problem is the line:
>
> #define pr_fmt(fmt) "%s: " fmt, __func__
>
> in crypto/zlib.c which was introduced by commit
> bf68e65ec9ea61e32ab71bef59aa5d24d255241f ("crypto: zlib - New zlib crypto
> module, using pcomp") from the crypto tree.
>
> For today, I have removed the above line from crypto/zlib.c, but
> something better needs to be done for tomorrow!
I had a closer look. It happens on all archs, if CONFIG_DYNAMIC_DEBUG=y.
crypto/zlib.c has:
#define pr_fmt(fmt) "%s: " fmt, __func__
If CONFIG_DYNAMIC_DEBUG is set, include/linux/kernel.h has:
#define pr_debug(fmt, ...) do { \
dynamic_pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
} while (0)
include/linux/dynamic_debug.h has:
#define dynamic_pr_debug(fmt, ...) do { \
static struct _ddebug descriptor \
__used \
__attribute__((section("__verbose"), aligned(8))) = \
{ KBUILD_MODNAME, __func__, __FILE__, fmt, DEBUG_HASH, \
DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \
if (__dynamic_dbg_enabled(descriptor)) \
printk(KERN_DEBUG KBUILD_MODNAME ":" fmt, \
##__VA_ARGS__); \
} while (0)
So in crypto/zlib.c,
| pr_debug("avail_in %u, avail_out %u\n", req->avail_in, req->avail_out);
is expanded to
| do {
| do {
| static struct _ddebug descriptor
| __attribute__((__used__))
| __attribute__((section("__verbose"), aligned(8))) = {
| "zlib",
| __func__,
| "crypto/zlib.c",
| "%s: " "avail_in %u, avail_out %u\n",
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is the actual format string
| __func__,
^^^^^^^^^
But this is the first parameter, which should not be here!!!
| 55,
| 33,
| 150,
| 0
| };
| if (({
| int __ret = 0;
| if (__builtin_expect(!!((dynamic_debug_enabled & (1LL << 55)) &&
| (dynamic_debug_enabled2 & (1LL << 33))), 0))
| if (__builtin_expect(!!(descriptor.flags), 0))
| __ret = 1;
| __ret;
| }))
| printk("<7>" "zlib" ":" "%s: " "avail_in %u, avail_out %u\n", __func__,
| req->avail_in, req->avail_out);
| } while (0);
| } while (0);
Due the the superfluous `__func__', all field members are shifted by one
position, and compilation breaks.
Apparently inside dynamic_pr_debug(), `fmt' is:
"avail_in %u, avail_out %u\n", __func__
instead of only the part before the comma:
"avail_in %u, avail_out %u\n"
For the non-CONFIG_DYNAMIC_DEBUG case, pr_debug() is expanded correctly:
DEBUG is defined:
#define pr_debug(fmt, ...) \
printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
and the line is expanded to:
| printk("<7>" "%s: " "avail_in %u, avail_out %u\n", __func__, req->avail_in, req->avail_out);
DEBUG is not defined:
#define pr_debug(fmt, ...) \
({ if (0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); 0; })
and the line is expanded to:
| ({ if (0) printk("<7>" "%s: " "avail_in %u, avail_out %u\n", __func__, req->avail_in, req->avail_out); 0; });
Why doesn't it work for dynamic_pr_debug()?
BTW, Martin: Is `#define pr_fmt(fmt) "%s: " fmt, __func__' a valid and
intended usage of your pr_fmt() infrastructure?
Thanks!
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village ? Da Vincilaan 7-D1 ? B-1935 Zaventem ? Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 ? RPR Brussels
Fortis ? BIC GEBABEBB ? IBAN BE41293037680010
On Tue, Mar 10, 2009 at 02:31:17PM +0100, Geert Uytterhoeven wrote:
>
> Why doesn't it work for dynamic_pr_debug()?
Because fmt is expanded twice?
> BTW, Martin: Is `#define pr_fmt(fmt) "%s: " fmt, __func__' a valid and
> intended usage of your pr_fmt() infrastructure?
I never liked this in the first place :) If anyone wants to debug
this they can add whatever printks they want.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Tue, 10 Mar 2009 14:31:17 +0100 (CET)
Geert Uytterhoeven <[email protected]> wrote:
> crypto/zlib.c has:
>
> #define pr_fmt(fmt) "%s: " fmt, __func__
>
> If CONFIG_DYNAMIC_DEBUG is set, include/linux/kernel.h has:
>
> #define pr_debug(fmt, ...) do { \
> dynamic_pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
> } while (0)
>
> include/linux/dynamic_debug.h has:
>
> #define dynamic_pr_debug(fmt, ...) do { \
> static struct _ddebug descriptor \
> __used \
> __attribute__((section("__verbose"), aligned(8))) = \
> { KBUILD_MODNAME, __func__, __FILE__, fmt, DEBUG_HASH, \
> DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \
> if (__dynamic_dbg_enabled(descriptor)) \
> printk(KERN_DEBUG KBUILD_MODNAME ":" fmt, \
> ##__VA_ARGS__); \
> } while (0)
The dynamic_pr_debug macro currently works only with pr_fmt definitions
that do not add additional parameters. The way how we use the pr_fmt
macro is:
#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
The same could be done with the problematic pr_fmt definition:
#define pr_fmt(fmt) __func__ ": " fmt
> BTW, Martin: Is `#define pr_fmt(fmt) "%s: " fmt, __func__' a valid and
> intended usage of your pr_fmt() infrastructure?
The indended use is a simple prefix to the format string. To paste an
additional parameter is an interesting use of the pr_fmt macro ..
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Tue, 10 Mar 2009, Martin Schwidefsky wrote:
> On Tue, 10 Mar 2009 14:31:17 +0100 (CET)
> Geert Uytterhoeven <[email protected]> wrote:
>
> > crypto/zlib.c has:
> >
> > #define pr_fmt(fmt) "%s: " fmt, __func__
> >
> > If CONFIG_DYNAMIC_DEBUG is set, include/linux/kernel.h has:
> >
> > #define pr_debug(fmt, ...) do { \
> > dynamic_pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
> > } while (0)
> >
> > include/linux/dynamic_debug.h has:
> >
> > #define dynamic_pr_debug(fmt, ...) do { \
> > static struct _ddebug descriptor \
> > __used \
> > __attribute__((section("__verbose"), aligned(8))) = \
> > { KBUILD_MODNAME, __func__, __FILE__, fmt, DEBUG_HASH, \
> > DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \
> > if (__dynamic_dbg_enabled(descriptor)) \
> > printk(KERN_DEBUG KBUILD_MODNAME ":" fmt, \
> > ##__VA_ARGS__); \
> > } while (0)
>
> The dynamic_pr_debug macro currently works only with pr_fmt definitions
> that do not add additional parameters. The way how we use the pr_fmt
> macro is:
>
> #define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
>
> The same could be done with the problematic pr_fmt definition:
>
> #define pr_fmt(fmt) __func__ ": " fmt
No, that also doesn't work:
| crypto/zlib.c:150: error: expected '}' before string constant
| crypto/zlib.c:150: error: expected ')' before '__func__'
| crypto/zlib.c:162: error: expected '}' before string constant
| crypto/zlib.c:162: error: expected ')' before '__func__'
| crypto/zlib.c:166: error: expected '}' before string constant
| crypto/zlib.c:166: error: expected ')' before '__func__'
| crypto/zlib.c:170: error: expected '}' before string constant
| crypto/zlib.c:170: error: expected ')' before '__func__'
> > BTW, Martin: Is `#define pr_fmt(fmt) "%s: " fmt, __func__' a valid and
> > intended usage of your pr_fmt() infrastructure?
>
> The indended use is a simple prefix to the format string. To paste an
> additional parameter is an interesting use of the pr_fmt macro ..
Bummer, I was so happy I could do things like
| #define pr_fmt(fmt) "%s:%u: " fmt, __func__, __LINE__
...
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village ? Da Vincilaan 7-D1 ? B-1935 Zaventem ? Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 ? RPR Brussels
Fortis ? BIC GEBABEBB ? IBAN BE41293037680010
On Tue, 10 Mar 2009 16:29:50 +0100 (CET)
Geert Uytterhoeven <[email protected]> wrote:
> > The same could be done with the problematic pr_fmt definition:
> >
> > #define pr_fmt(fmt) __func__ ": " fmt
>
> No, that also doesn't work:
>
> | crypto/zlib.c:150: error: expected '}' before string constant
> | crypto/zlib.c:150: error: expected ')' before '__func__'
> | crypto/zlib.c:162: error: expected '}' before string constant
> | crypto/zlib.c:162: error: expected ')' before '__func__'
> | crypto/zlib.c:166: error: expected '}' before string constant
> | crypto/zlib.c:166: error: expected ')' before '__func__'
> | crypto/zlib.c:170: error: expected '}' before string constant
> | crypto/zlib.c:170: error: expected ')' before '__func__'
>
> > > BTW, Martin: Is `#define pr_fmt(fmt) "%s: " fmt, __func__' a valid and
> > > intended usage of your pr_fmt() infrastructure?
> >
> > The indended use is a simple prefix to the format string. To paste an
> > additional parameter is an interesting use of the pr_fmt macro ..
>
> Bummer, I was so happy I could do things like
>
> | #define pr_fmt(fmt) "%s:%u: " fmt, __func__, __LINE__
Actually that seem like a nice thing to have. With the upstream version of
dynamic_pr_debug this works, there the format string is used only on a printk.
git commit 25b67b75587d43ff3f09ad88c03c70a38372d95d introduces the code
that pastes the format string to the _ddebug structure.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Tue, Mar 10, 2009 at 05:08:41PM +0100, Martin Schwidefsky wrote:
> On Tue, 10 Mar 2009 16:29:50 +0100 (CET)
> Geert Uytterhoeven <[email protected]> wrote:
>
> > > The same could be done with the problematic pr_fmt definition:
> > >
> > > #define pr_fmt(fmt) __func__ ": " fmt
> >
> > No, that also doesn't work:
> >
> > | crypto/zlib.c:150: error: expected '}' before string constant
> > | crypto/zlib.c:150: error: expected ')' before '__func__'
> > | crypto/zlib.c:162: error: expected '}' before string constant
> > | crypto/zlib.c:162: error: expected ')' before '__func__'
> > | crypto/zlib.c:166: error: expected '}' before string constant
> > | crypto/zlib.c:166: error: expected ')' before '__func__'
> > | crypto/zlib.c:170: error: expected '}' before string constant
> > | crypto/zlib.c:170: error: expected ')' before '__func__'
> >
> > > > BTW, Martin: Is `#define pr_fmt(fmt) "%s: " fmt, __func__' a valid and
> > > > intended usage of your pr_fmt() infrastructure?
> > >
> > > The indended use is a simple prefix to the format string. To paste an
> > > additional parameter is an interesting use of the pr_fmt macro ..
> >
> > Bummer, I was so happy I could do things like
> >
> > | #define pr_fmt(fmt) "%s:%u: " fmt, __func__, __LINE__
>
> Actually that seem like a nice thing to have. With the upstream version of
> dynamic_pr_debug this works, there the format string is used only on a printk.
> git commit 25b67b75587d43ff3f09ad88c03c70a38372d95d introduces the code
> that pastes the format string to the _ddebug structure.
>
hmmm...yeah, some macro magic in include/linux/dynamic_debug.h converts
the 'fmt' arg into a series of strings. It doesn't look as pretty in the
dynamic debug control file:
crypto/zlib.c:333 [zlib]zlib_decompress_final - "\042%s: \042
\042avail_in %u, avail_out %u (consumed %u, produced %u)\n\042,
__func__"
with all those '\042' there, which are the '"' characters, but we
probably could live with it.
patch below.
thanks,
-Jason
Signed-off-by: Jason Baron <[email protected]>
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 07781aa..16cf212 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -50,28 +50,30 @@ extern int ddebug_remove_module(char *mod_name);
__ret = 1; \
__ret; })
-#define dynamic_pr_debug(fmt, ...) do { \
- static struct _ddebug descriptor \
- __used \
- __attribute__((section("__verbose"), aligned(8))) = \
- { KBUILD_MODNAME, __func__, __FILE__, fmt, DEBUG_HASH, \
- DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \
- if (__dynamic_dbg_enabled(descriptor)) \
- printk(KERN_DEBUG KBUILD_MODNAME ":" fmt, \
- ##__VA_ARGS__); \
+#define STRINGIFY(args...) #args
+
+#define dynamic_pr_debug(fmt, ...) do { \
+ static struct _ddebug descriptor \
+ __used \
+ __attribute__((section("__verbose"), aligned(8))) = \
+ { KBUILD_MODNAME, __func__, __FILE__, STRINGIFY(fmt), DEBUG_HASH, \
+ DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \
+ if (__dynamic_dbg_enabled(descriptor)) \
+ printk(KERN_DEBUG KBUILD_MODNAME ":" fmt, \
+ ##__VA_ARGS__); \
} while (0)
-#define dynamic_dev_dbg(dev, fmt, ...) do { \
- static struct _ddebug descriptor \
- __used \
- __attribute__((section("__verbose"), aligned(8))) = \
- { KBUILD_MODNAME, __func__, __FILE__, fmt, DEBUG_HASH, \
- DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \
- if (__dynamic_dbg_enabled(descriptor)) \
- dev_printk(KERN_DEBUG, dev, \
- KBUILD_MODNAME ": " fmt, \
- ##__VA_ARGS__); \
+#define dynamic_dev_dbg(dev, fmt, ...) do { \
+ static struct _ddebug descriptor \
+ __used \
+ __attribute__((section("__verbose"), aligned(8))) = \
+ { KBUILD_MODNAME, __func__, __FILE__, STRINGIFY(fmt), DEBUG_HASH, \
+ DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \
+ if (__dynamic_dbg_enabled(descriptor)) \
+ dev_printk(KERN_DEBUG, dev, \
+ KBUILD_MODNAME ": " fmt, \
+ ##__VA_ARGS__); \
} while (0)
#else
On Tue, Mar 10, 2009 at 04:02:00PM -0400, Jason Baron wrote:
> On Tue, Mar 10, 2009 at 05:08:41PM +0100, Martin Schwidefsky wrote:
> > On Tue, 10 Mar 2009 16:29:50 +0100 (CET)
> > Geert Uytterhoeven <[email protected]> wrote:
> >
> > > > The same could be done with the problematic pr_fmt definition:
> > > >
> > > > #define pr_fmt(fmt) __func__ ": " fmt
> > >
> > > No, that also doesn't work:
> > >
> > > | crypto/zlib.c:150: error: expected '}' before string constant
> > > | crypto/zlib.c:150: error: expected ')' before '__func__'
> > > | crypto/zlib.c:162: error: expected '}' before string constant
> > > | crypto/zlib.c:162: error: expected ')' before '__func__'
> > > | crypto/zlib.c:166: error: expected '}' before string constant
> > > | crypto/zlib.c:166: error: expected ')' before '__func__'
> > > | crypto/zlib.c:170: error: expected '}' before string constant
> > > | crypto/zlib.c:170: error: expected ')' before '__func__'
> > >
> > > > > BTW, Martin: Is `#define pr_fmt(fmt) "%s: " fmt, __func__' a valid and
> > > > > intended usage of your pr_fmt() infrastructure?
> > > >
> > > > The indended use is a simple prefix to the format string. To paste an
> > > > additional parameter is an interesting use of the pr_fmt macro ..
> > >
> > > Bummer, I was so happy I could do things like
> > >
> > > | #define pr_fmt(fmt) "%s:%u: " fmt, __func__, __LINE__
> >
> > Actually that seem like a nice thing to have. With the upstream version of
> > dynamic_pr_debug this works, there the format string is used only on a printk.
> > git commit 25b67b75587d43ff3f09ad88c03c70a38372d95d introduces the code
> > that pastes the format string to the _ddebug structure.
> >
>
> hmmm...yeah, some macro magic in include/linux/dynamic_debug.h converts
> the 'fmt' arg into a series of strings. It doesn't look as pretty in the
> dynamic debug control file:
>
> crypto/zlib.c:333 [zlib]zlib_decompress_final - "\042%s: \042
> \042avail_in %u, avail_out %u (consumed %u, produced %u)\n\042,
> __func__"
>
> with all those '\042' there, which are the '"' characters, but we
> probably could live with it.
>
> patch below.
Can you resend this with a description of what the patch does so I can
apply it?
thanks,
greg k-h
On Tue, 10 Mar 2009, Jason Baron wrote:
> On Tue, Mar 10, 2009 at 05:08:41PM +0100, Martin Schwidefsky wrote:
> > On Tue, 10 Mar 2009 16:29:50 +0100 (CET)
> > Geert Uytterhoeven <[email protected]> wrote:
> >
> > > > The same could be done with the problematic pr_fmt definition:
> > > >
> > > > #define pr_fmt(fmt) __func__ ": " fmt
> > >
> > > No, that also doesn't work:
> > >
> > > | crypto/zlib.c:150: error: expected '}' before string constant
> > > | crypto/zlib.c:150: error: expected ')' before '__func__'
> > > | crypto/zlib.c:162: error: expected '}' before string constant
> > > | crypto/zlib.c:162: error: expected ')' before '__func__'
> > > | crypto/zlib.c:166: error: expected '}' before string constant
> > > | crypto/zlib.c:166: error: expected ')' before '__func__'
> > > | crypto/zlib.c:170: error: expected '}' before string constant
> > > | crypto/zlib.c:170: error: expected ')' before '__func__'
> > >
> > > > > BTW, Martin: Is `#define pr_fmt(fmt) "%s: " fmt, __func__' a valid and
> > > > > intended usage of your pr_fmt() infrastructure?
> > > >
> > > > The indended use is a simple prefix to the format string. To paste an
> > > > additional parameter is an interesting use of the pr_fmt macro ..
> > >
> > > Bummer, I was so happy I could do things like
> > >
> > > | #define pr_fmt(fmt) "%s:%u: " fmt, __func__, __LINE__
> >
> > Actually that seem like a nice thing to have. With the upstream version of
> > dynamic_pr_debug this works, there the format string is used only on a printk.
> > git commit 25b67b75587d43ff3f09ad88c03c70a38372d95d introduces the code
> > that pastes the format string to the _ddebug structure.
> >
>
> hmmm...yeah, some macro magic in include/linux/dynamic_debug.h converts
> the 'fmt' arg into a series of strings. It doesn't look as pretty in the
> dynamic debug control file:
>
> crypto/zlib.c:333 [zlib]zlib_decompress_final - "\042%s: \042
> \042avail_in %u, avail_out %u (consumed %u, produced %u)\n\042,
> __func__"
>
> with all those '\042' there, which are the '"' characters, but we
> probably could live with it.
Ugh, not only that, it also contains the __func__.
BTW, why do you store this string? The only thing you can do with it later is
to print it verbatim (without % format parameter expansion), as it has
swallowed the first parameter (__func__), but still contains the %s.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village ? Da Vincilaan 7-D1 ? B-1935 Zaventem ? Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 ? RPR Brussels
Fortis ? BIC GEBABEBB ? IBAN BE41293037680010
Jason Baron wrote:
> On Tue, Mar 10, 2009 at 05:08:41PM +0100, Martin Schwidefsky wrote:
>
>> On Tue, 10 Mar 2009 16:29:50 +0100 (CET)
>> Geert Uytterhoeven <[email protected]> wrote:
>>
>>
>>>> The same could be done with the problematic pr_fmt definition:
>>>>
>>>> #define pr_fmt(fmt) __func__ ": " fmt
>>>>
>>> No, that also doesn't work:
>>>
>>> | crypto/zlib.c:150: error: expected '}' before string constant
>>> | crypto/zlib.c:150: error: expected ')' before '__func__'
>>> | crypto/zlib.c:162: error: expected '}' before string constant
>>> | crypto/zlib.c:162: error: expected ')' before '__func__'
>>> | crypto/zlib.c:166: error: expected '}' before string constant
>>> | crypto/zlib.c:166: error: expected ')' before '__func__'
>>> | crypto/zlib.c:170: error: expected '}' before string constant
>>> | crypto/zlib.c:170: error: expected ')' before '__func__'
>>>
>>>
>>>>> BTW, Martin: Is `#define pr_fmt(fmt) "%s: " fmt, __func__' a valid and
>>>>> intended usage of your pr_fmt() infrastructure?
>>>>>
>>>> The indended use is a simple prefix to the format string. To paste an
>>>> additional parameter is an interesting use of the pr_fmt macro ..
>>>>
>>> Bummer, I was so happy I could do things like
>>>
>>> | #define pr_fmt(fmt) "%s:%u: " fmt, __func__, __LINE__
>>>
>> Actually that seem like a nice thing to have. With the upstream version of
>> dynamic_pr_debug this works, there the format string is used only on a printk.
>> git commit 25b67b75587d43ff3f09ad88c03c70a38372d95d introduces the code
>> that pastes the format string to the _ddebug structure.
>>
>>
>
> hmmm...yeah, some macro magic in include/linux/dynamic_debug.h converts
> the 'fmt' arg into a series of strings. It doesn't look as pretty in the
> dynamic debug control file:
>
> crypto/zlib.c:333 [zlib]zlib_decompress_final - "\042%s: \042
> \042avail_in %u, avail_out %u (consumed %u, produced %u)\n\042,
> __func__"
>
> with all those '\042' there, which are the '"' characters, but we
> probably could live with it.
>
> patch below.
>
>
I think this patch does the same thing more cleanly.
When CONFIG_DYNAMIC_DEBUG is enabled, allow callers of pr_debug()
to provide their own definition of pr_fmt() even if that definition
uses tricks like
#define pr_fmt(fmt) "%s:" fmt, __func__
Signed-off-by: Greg Banks <[email protected]>
---
include/linux/dynamic_debug.h | 4 ++--
include/linux/kernel.h | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
Index: linux-git/include/linux/dynamic_debug.h
===================================================================
--- linux-git.orig/include/linux/dynamic_debug.h
+++ linux-git/include/linux/dynamic_debug.h
@@ -57,7 +57,7 @@ extern int ddebug_remove_module(char *mo
{ KBUILD_MODNAME, __func__, __FILE__, fmt, DEBUG_HASH, \
DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \
if (__dynamic_dbg_enabled(descriptor)) \
- printk(KERN_DEBUG KBUILD_MODNAME ":" fmt, \
+ printk(KERN_DEBUG KBUILD_MODNAME ":" pr_fmt(fmt), \
##__VA_ARGS__); \
} while (0)
@@ -70,7 +70,7 @@ extern int ddebug_remove_module(char *mo
DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \
if (__dynamic_dbg_enabled(descriptor)) \
dev_printk(KERN_DEBUG, dev, \
- KBUILD_MODNAME ": " fmt, \
+ KBUILD_MODNAME ": " pr_fmt(fmt),\
##__VA_ARGS__); \
} while (0)
Index: linux-git/include/linux/kernel.h
===================================================================
--- linux-git.orig/include/linux/kernel.h
+++ linux-git/include/linux/kernel.h
@@ -359,8 +359,9 @@ static inline char *pack_hex_byte(char *
#define pr_debug(fmt, ...) \
printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
#elif defined(CONFIG_DYNAMIC_DEBUG)
+/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
#define pr_debug(fmt, ...) do { \
- dynamic_pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
+ dynamic_pr_debug(fmt, ##__VA_ARGS__); \
} while (0)
#else
#define pr_debug(fmt, ...) \
--
Greg Banks, P.Engineer, SGI Australian Software Group.
the brightly coloured sporks of revolution.
I don't speak for SGI.
On Wed, 11 Mar 2009, Greg Banks wrote:
> Jason Baron wrote:
> > On Tue, Mar 10, 2009 at 05:08:41PM +0100, Martin Schwidefsky wrote:
> >
> >> On Tue, 10 Mar 2009 16:29:50 +0100 (CET)
> >> Geert Uytterhoeven <[email protected]> wrote:
> >>
> >>
> >>>> The same could be done with the problematic pr_fmt definition:
> >>>>
> >>>> #define pr_fmt(fmt) __func__ ": " fmt
> >>>>
> >>> No, that also doesn't work:
> >>>
> >>> | crypto/zlib.c:150: error: expected '}' before string constant
> >>> | crypto/zlib.c:150: error: expected ')' before '__func__'
> >>> | crypto/zlib.c:162: error: expected '}' before string constant
> >>> | crypto/zlib.c:162: error: expected ')' before '__func__'
> >>> | crypto/zlib.c:166: error: expected '}' before string constant
> >>> | crypto/zlib.c:166: error: expected ')' before '__func__'
> >>> | crypto/zlib.c:170: error: expected '}' before string constant
> >>> | crypto/zlib.c:170: error: expected ')' before '__func__'
> >>>
> >>>
> >>>>> BTW, Martin: Is `#define pr_fmt(fmt) "%s: " fmt, __func__' a valid and
> >>>>> intended usage of your pr_fmt() infrastructure?
> >>>>>
> >>>> The indended use is a simple prefix to the format string. To paste an
> >>>> additional parameter is an interesting use of the pr_fmt macro ..
> >>>>
> >>> Bummer, I was so happy I could do things like
> >>>
> >>> | #define pr_fmt(fmt) "%s:%u: " fmt, __func__, __LINE__
> >>>
> >> Actually that seem like a nice thing to have. With the upstream version of
> >> dynamic_pr_debug this works, there the format string is used only on a printk.
> >> git commit 25b67b75587d43ff3f09ad88c03c70a38372d95d introduces the code
> >> that pastes the format string to the _ddebug structure.
> >>
> >>
> >
> > hmmm...yeah, some macro magic in include/linux/dynamic_debug.h converts
> > the 'fmt' arg into a series of strings. It doesn't look as pretty in the
> > dynamic debug control file:
> >
> > crypto/zlib.c:333 [zlib]zlib_decompress_final - "\042%s: \042
> > \042avail_in %u, avail_out %u (consumed %u, produced %u)\n\042,
> > __func__"
> >
> > with all those '\042' there, which are the '"' characters, but we
> > probably could live with it.
> >
> > patch below.
> >
> >
>
> I think this patch does the same thing more cleanly.
>
> When CONFIG_DYNAMIC_DEBUG is enabled, allow callers of pr_debug()
> to provide their own definition of pr_fmt() even if that definition
> uses tricks like
>
> #define pr_fmt(fmt) "%s:" fmt, __func__
>
> Signed-off-by: Greg Banks <[email protected]>
Thanks, much cleaner!
Acked-by: Geert Uytterhoeven <[email protected]>
> ---
>
> include/linux/dynamic_debug.h | 4 ++--
> include/linux/kernel.h | 3 ++-
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> Index: linux-git/include/linux/dynamic_debug.h
> ===================================================================
> --- linux-git.orig/include/linux/dynamic_debug.h
> +++ linux-git/include/linux/dynamic_debug.h
> @@ -57,7 +57,7 @@ extern int ddebug_remove_module(char *mo
> { KBUILD_MODNAME, __func__, __FILE__, fmt, DEBUG_HASH, \
> DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \
> if (__dynamic_dbg_enabled(descriptor)) \
> - printk(KERN_DEBUG KBUILD_MODNAME ":" fmt, \
> + printk(KERN_DEBUG KBUILD_MODNAME ":" pr_fmt(fmt), \
> ##__VA_ARGS__); \
> } while (0)
>
> @@ -70,7 +70,7 @@ extern int ddebug_remove_module(char *mo
> DEBUG_HASH2, __LINE__, _DPRINTK_FLAGS_DEFAULT }; \
> if (__dynamic_dbg_enabled(descriptor)) \
> dev_printk(KERN_DEBUG, dev, \
> - KBUILD_MODNAME ": " fmt, \
> + KBUILD_MODNAME ": " pr_fmt(fmt),\
> ##__VA_ARGS__); \
> } while (0)
>
> Index: linux-git/include/linux/kernel.h
> ===================================================================
> --- linux-git.orig/include/linux/kernel.h
> +++ linux-git/include/linux/kernel.h
> @@ -359,8 +359,9 @@ static inline char *pack_hex_byte(char *
> #define pr_debug(fmt, ...) \
> printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> #elif defined(CONFIG_DYNAMIC_DEBUG)
> +/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
> #define pr_debug(fmt, ...) do { \
> - dynamic_pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
> + dynamic_pr_debug(fmt, ##__VA_ARGS__); \
> } while (0)
> #else
> #define pr_debug(fmt, ...) \
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village ? Da Vincilaan 7-D1 ? B-1935 Zaventem ? Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 ? RPR Brussels
Fortis ? BIC GEBABEBB ? IBAN BE41293037680010
On Wed, Mar 11, 2009 at 09:07:28PM +1100, Greg Banks wrote:
>
> I think this patch does the same thing more cleanly.
>
> When CONFIG_DYNAMIC_DEBUG is enabled, allow callers of pr_debug()
> to provide their own definition of pr_fmt() even if that definition
> uses tricks like
>
> #define pr_fmt(fmt) "%s:" fmt, __func__
>
patch looks good. I agree its simpler than what I proposed. However, I
don't think we want to add pr_fmt() in the dynamic_dev_dbg() path, since
dev_dbg() isn't doing that to start with. Other than that, I ack it.
thanks,
-Jason