Hi Linus,
there is one spin-off from the kernel message patches I would still
like to get upstream: replacing all the different printk macros in the
s390 device drivers with dev_xyz and pr_xyz.
One thing the s390 printk macros have been doing which I miss with the
pr_xyz macros is a way to automatically prefix the messages with the
driver name. This is quite handy, saves some typing and shortens the
printk line. The pr_xyz macros could be taught about a prefix, would
something like the patch below be acceptable?
The idea is to add a '#define PR_PREFIX "driver"' to the start of the
source file and have the pr_xyz macros paste the prefix to each message.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
---
Subject: [PATCH] Add PR_PREFIX to pr_xyz macros.
From: Martin Schwidefsky <[email protected]>
A common reason for device drivers to implement their own printk macros
is the lack of a printk prefix with the standard pr_xyz macros. The most
common use of the prefix would be to add the name of the device driver to
all printks in a source file.
Signed-off-by: Martin Schwidefsky <[email protected]>
---
include/linux/kernel.h | 44 +++++++++++++++++++++++++-------------------
1 file changed, 25 insertions(+), 19 deletions(-)
diff -urpN linux-2.6/include/linux/kernel.h linux-2.6-patched/include/linux/kernel.h
--- linux-2.6/include/linux/kernel.h 2008-11-11 16:59:04.000000000 +0100
+++ linux-2.6-patched/include/linux/kernel.h 2008-11-11 17:19:04.000000000 +0100
@@ -318,32 +318,38 @@ static inline char *pack_hex_byte(char *
return buf;
}
-#define pr_emerg(fmt, arg...) \
- printk(KERN_EMERG fmt, ##arg)
-#define pr_alert(fmt, arg...) \
- printk(KERN_ALERT fmt, ##arg)
-#define pr_crit(fmt, arg...) \
- printk(KERN_CRIT fmt, ##arg)
-#define pr_err(fmt, arg...) \
- printk(KERN_ERR fmt, ##arg)
-#define pr_warning(fmt, arg...) \
- printk(KERN_WARNING fmt, ##arg)
-#define pr_notice(fmt, arg...) \
- printk(KERN_NOTICE fmt, ##arg)
-#define pr_info(fmt, arg...) \
- printk(KERN_INFO fmt, ##arg)
+#ifndef PR_PREFIX
+#define pr_fmt(format) format
+#else
+#define pr_fmt(format) PR_PREFIX ": " format
+#endif
+
+#define pr_emerg(fmt, ...) \
+ printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_alert(fmt, ...) \
+ printk(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_crit(fmt, ...) \
+ printk(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_err(fmt, ...) \
+ printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_warning(fmt, ...) \
+ printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_notice(fmt, ...) \
+ printk(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_info(fmt, ...) \
+ printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
/* If you are writing a driver, please use dev_dbg instead */
#if defined(CONFIG_DYNAMIC_PRINTK_DEBUG)
#define pr_debug(fmt, ...) do { \
- dynamic_pr_debug(fmt, ##__VA_ARGS__); \
+ dynamic_pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
} while (0)
#elif defined(DEBUG)
-#define pr_debug(fmt, arg...) \
- printk(KERN_DEBUG fmt, ##arg)
+#define pr_debug(fmt, ...) \
+ printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
#else
-#define pr_debug(fmt, arg...) \
- ({ if (0) printk(KERN_DEBUG fmt, ##arg); 0; })
+#define pr_debug(fmt, ...) \
+ ({ if (0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); 0; })
#endif
/*
On Wed, 12 Nov 2008, Martin Schwidefsky wrote:
>
> +#ifndef PR_PREFIX
> +#define pr_fmt(format) format
> +#else
> +#define pr_fmt(format) PR_PREFIX ": " format
> +#endif
> +
> +#define pr_emerg(fmt, ...) \
> + printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
So, why can't people just define their pr_fmt() macro directly, something
like
#define pr_fmt(fmt) \
"%s:%d " fmt, __FILE__, __LINE__
which seems to be much more useful and generic than forcing them to use
"PR_PREFIX"?
Linus
On Wed, 2008-11-12 at 11:48 -0800, Linus Torvalds wrote:
>
> On Wed, 12 Nov 2008, Martin Schwidefsky wrote:
> >
> > +#ifndef PR_PREFIX
> > +#define pr_fmt(format) format
> > +#else
> > +#define pr_fmt(format) PR_PREFIX ": " format
> > +#endif
> > +
> > +#define pr_emerg(fmt, ...) \
> > + printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
>
> So, why can't people just define their pr_fmt() macro directly, something
> like
>
> #define pr_fmt(fmt) \
> "%s:%d " fmt, __FILE__, __LINE__
>
> which seems to be much more useful and generic than forcing them to use
> "PR_PREFIX"?
Even better - much better. With that define a lot more printk wrappers
could be replaced. I'll come up with a patch.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
Subject: [PATCH] Add PR_PREFIX to pr_xyz macros.
From: Martin Schwidefsky <[email protected]>
A common reason for device drivers to implement their own printk macros
is the lack of a printk prefix with the standard pr_xyz macros. Introduce
a pr_fmt macro that is applied for every pr_xyz macro to the format string.
The most common use of the pr_fmt macro would be to add the name of the
device driver to all pr_xyz messages in a source file.
Signed-off-by: Martin Schwidefsky <[email protected]>
---
include/linux/kernel.h | 42 +++++++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 19 deletions(-)
diff -urpN linux-2.6/include/linux/kernel.h linux-2.6-patched/include/linux/kernel.h
--- linux-2.6/include/linux/kernel.h 2008-11-03 09:40:45.000000000 +0100
+++ linux-2.6-patched/include/linux/kernel.h 2008-11-12 21:07:21.000000000 +0100
@@ -318,32 +318,36 @@ static inline char *pack_hex_byte(char *
return buf;
}
-#define pr_emerg(fmt, arg...) \
- printk(KERN_EMERG fmt, ##arg)
-#define pr_alert(fmt, arg...) \
- printk(KERN_ALERT fmt, ##arg)
-#define pr_crit(fmt, arg...) \
- printk(KERN_CRIT fmt, ##arg)
-#define pr_err(fmt, arg...) \
- printk(KERN_ERR fmt, ##arg)
-#define pr_warning(fmt, arg...) \
- printk(KERN_WARNING fmt, ##arg)
-#define pr_notice(fmt, arg...) \
- printk(KERN_NOTICE fmt, ##arg)
-#define pr_info(fmt, arg...) \
- printk(KERN_INFO fmt, ##arg)
+#ifndef pr_fmt
+#define pr_fmt(fmt) fmt
+#endif
+
+#define pr_emerg(fmt, ...) \
+ printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_alert(fmt, ...) \
+ printk(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_crit(fmt, ...) \
+ printk(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_err(fmt, ...) \
+ printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_warning(fmt, ...) \
+ printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_notice(fmt, ...) \
+ printk(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_info(fmt, ...) \
+ printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
/* If you are writing a driver, please use dev_dbg instead */
#if defined(CONFIG_DYNAMIC_PRINTK_DEBUG)
#define pr_debug(fmt, ...) do { \
- dynamic_pr_debug(fmt, ##__VA_ARGS__); \
+ dynamic_pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
} while (0)
#elif defined(DEBUG)
-#define pr_debug(fmt, arg...) \
- printk(KERN_DEBUG fmt, ##arg)
+#define pr_debug(fmt, ...) \
+ printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
#else
-#define pr_debug(fmt, arg...) \
- ({ if (0) printk(KERN_DEBUG fmt, ##arg); 0; })
+#define pr_debug(fmt, ...) \
+ ({ if (0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); 0; })
#endif
/*
On Wed, 12 Nov 2008 21:16:43 +0100
Martin Schwidefsky <[email protected]> wrote:
> Subject: [PATCH] Add PR_PREFIX to pr_xyz macros.
>
> From: Martin Schwidefsky <[email protected]>
>
> A common reason for device drivers to implement their own printk macros
> is the lack of a printk prefix with the standard pr_xyz macros. Introduce
> a pr_fmt macro that is applied for every pr_xyz macro to the format string.
> The most common use of the pr_fmt macro would be to add the name of the
> device driver to all pr_xyz messages in a source file.
>
Seems sane. It would be nice to see one such driver converted over (hint;))
> ---
> include/linux/kernel.h | 42 +++++++++++++++++++++++-------------------
> 1 file changed, 23 insertions(+), 19 deletions(-)
>
> diff -urpN linux-2.6/include/linux/kernel.h linux-2.6-patched/include/linux/kernel.h
> --- linux-2.6/include/linux/kernel.h 2008-11-03 09:40:45.000000000 +0100
> +++ linux-2.6-patched/include/linux/kernel.h 2008-11-12 21:07:21.000000000 +0100
> @@ -318,32 +318,36 @@ static inline char *pack_hex_byte(char *
> return buf;
> }
>
> -#define pr_emerg(fmt, arg...) \
> - printk(KERN_EMERG fmt, ##arg)
> -#define pr_alert(fmt, arg...) \
> - printk(KERN_ALERT fmt, ##arg)
> -#define pr_crit(fmt, arg...) \
> - printk(KERN_CRIT fmt, ##arg)
> -#define pr_err(fmt, arg...) \
> - printk(KERN_ERR fmt, ##arg)
> -#define pr_warning(fmt, arg...) \
> - printk(KERN_WARNING fmt, ##arg)
> -#define pr_notice(fmt, arg...) \
> - printk(KERN_NOTICE fmt, ##arg)
> -#define pr_info(fmt, arg...) \
> - printk(KERN_INFO fmt, ##arg)
> +#ifndef pr_fmt
> +#define pr_fmt(fmt) fmt
> +#endif
> +
> +#define pr_emerg(fmt, ...) \
> + printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_alert(fmt, ...) \
> + printk(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_crit(fmt, ...) \
> + printk(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_err(fmt, ...) \
> + printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_warning(fmt, ...) \
> + printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_notice(fmt, ...) \
> + printk(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_info(fmt, ...) \
> + printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
>
> /* If you are writing a driver, please use dev_dbg instead */
> #if defined(CONFIG_DYNAMIC_PRINTK_DEBUG)
> #define pr_debug(fmt, ...) do { \
> - dynamic_pr_debug(fmt, ##__VA_ARGS__); \
> + dynamic_pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
> } while (0)
> #elif defined(DEBUG)
> -#define pr_debug(fmt, arg...) \
> - printk(KERN_DEBUG fmt, ##arg)
> +#define pr_debug(fmt, ...) \
> + printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> #else
> -#define pr_debug(fmt, arg...) \
> - ({ if (0) printk(KERN_DEBUG fmt, ##arg); 0; })
> +#define pr_debug(fmt, ...) \
> + ({ if (0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); 0; })
> #endif
gack, what a stupid mess we made with the pr_debug() special-case :(
On Fri, 2008-11-14 at 13:32 -0800, Andrew Morton wrote:
> > Subject: [PATCH] Add PR_PREFIX to pr_xyz macros.
> >
> > From: Martin Schwidefsky <[email protected]>
> >
> > A common reason for device drivers to implement their own printk macros
> > is the lack of a printk prefix with the standard pr_xyz macros. Introduce
> > a pr_fmt macro that is applied for every pr_xyz macro to the format string.
> > The most common use of the pr_fmt macro would be to add the name of the
> > device driver to all pr_xyz messages in a source file.
> >
>
> Seems sane. It would be nice to see one such driver converted over (hint;))
I now have about two dozen examples, I've created the patches after
Linus suggested to have the pr_fmt macro in the driver. One example is
below. The same will be done for all s390 device drivers.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
---
Subject: [PATCH] convert sclp printks to pr_xxx macros.
From: Martin Schwidefsky <[email protected]>
Signed-off-by: Martin Schwidefsky <[email protected]>
---
drivers/s390/char/sclp_cmd.c | 29 ++++++++++++++++-------------
drivers/s390/char/sclp_config.c | 10 ++++++----
drivers/s390/char/sclp_cpi_sys.c | 12 ++++++++----
drivers/s390/char/sclp_sdias.c | 18 +++++++++++-------
4 files changed, 41 insertions(+), 28 deletions(-)
diff -urpN linux-2.6/drivers/s390/char/sclp_cmd.c linux-2.6-patched/drivers/s390/char/sclp_cmd.c
--- linux-2.6/drivers/s390/char/sclp_cmd.c 2008-11-14 17:49:35.000000000 +0100
+++ linux-2.6-patched/drivers/s390/char/sclp_cmd.c 2008-11-14 17:50:21.000000000 +0100
@@ -6,6 +6,9 @@
* Peter Oberparleiter <[email protected]>
*/
+#define KMSG_COMPONENT "sclp_cmd"
+#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
+
#include <linux/completion.h>
#include <linux/init.h>
#include <linux/errno.h>
@@ -16,9 +19,8 @@
#include <linux/memory.h>
#include <asm/chpid.h>
#include <asm/sclp.h>
-#include "sclp.h"
-#define TAG "sclp_cmd: "
+#include "sclp.h"
#define SCLP_CMDW_READ_SCP_INFO 0x00020001
#define SCLP_CMDW_READ_SCP_INFO_FORCED 0x00120001
@@ -169,8 +171,8 @@ static int do_sync_request(sclp_cmdw_t c
/* Check response. */
if (request->status != SCLP_REQ_DONE) {
- printk(KERN_WARNING TAG "sync request failed "
- "(cmd=0x%08x, status=0x%02x)\n", cmd, request->status);
+ pr_warning("sync request failed (cmd=0x%08x, "
+ "status=0x%02x)\n", cmd, request->status);
rc = -EIO;
}
out:
@@ -224,8 +226,8 @@ int sclp_get_cpu_info(struct sclp_cpu_in
if (rc)
goto out;
if (sccb->header.response_code != 0x0010) {
- printk(KERN_WARNING TAG "readcpuinfo failed "
- "(response=0x%04x)\n", sccb->header.response_code);
+ pr_warning("readcpuinfo failed (response=0x%04x)\n",
+ sccb->header.response_code);
rc = -EIO;
goto out;
}
@@ -262,8 +264,9 @@ static int do_cpu_configure(sclp_cmdw_t
case 0x0120:
break;
default:
- printk(KERN_WARNING TAG "configure cpu failed (cmd=0x%08x, "
- "response=0x%04x)\n", cmd, sccb->header.response_code);
+ pr_warning("configure cpu failed (cmd=0x%08x, "
+ "response=0x%04x)\n", cmd,
+ sccb->header.response_code);
rc = -EIO;
break;
}
@@ -626,9 +629,9 @@ static int do_chp_configure(sclp_cmdw_t
case 0x0450:
break;
default:
- printk(KERN_WARNING TAG "configure channel-path failed "
- "(cmd=0x%08x, response=0x%04x)\n", cmd,
- sccb->header.response_code);
+ pr_warning("configure channel-path failed "
+ "(cmd=0x%08x, response=0x%04x)\n", cmd,
+ sccb->header.response_code);
rc = -EIO;
break;
}
@@ -695,8 +698,8 @@ int sclp_chp_read_info(struct sclp_chp_i
if (rc)
goto out;
if (sccb->header.response_code != 0x0010) {
- printk(KERN_WARNING TAG "read channel-path info failed "
- "(response=0x%04x)\n", sccb->header.response_code);
+ pr_warning("read channel-path info failed "
+ "(response=0x%04x)\n", sccb->header.response_code);
rc = -EIO;
goto out;
}
diff -urpN linux-2.6/drivers/s390/char/sclp_config.c linux-2.6-patched/drivers/s390/char/sclp_config.c
--- linux-2.6/drivers/s390/char/sclp_config.c 2008-10-10 00:13:53.000000000 +0200
+++ linux-2.6-patched/drivers/s390/char/sclp_config.c 2008-11-14 17:50:21.000000000 +0100
@@ -5,15 +5,17 @@
* Author(s): Heiko Carstens <[email protected]>
*/
+#define KMSG_COMPONENT "sclp_config"
+#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
+
#include <linux/init.h>
#include <linux/errno.h>
#include <linux/cpu.h>
#include <linux/sysdev.h>
#include <linux/workqueue.h>
#include <asm/smp.h>
-#include "sclp.h"
-#define TAG "sclp_config: "
+#include "sclp.h"
struct conf_mgm_data {
u8 reserved;
@@ -31,7 +33,7 @@ static void sclp_cpu_capability_notify(s
int cpu;
struct sys_device *sysdev;
- printk(KERN_WARNING TAG "cpu capability changed.\n");
+ pr_warning("cpu capability changed.\n");
get_online_cpus();
for_each_online_cpu(cpu) {
sysdev = get_cpu_sysdev(cpu);
@@ -78,7 +80,7 @@ static int __init sclp_conf_init(void)
return rc;
if (!(sclp_conf_register.sclp_send_mask & EVTYP_CONFMGMDATA_MASK)) {
- printk(KERN_WARNING TAG "no configuration management.\n");
+ pr_warning("no configuration management.\n");
sclp_unregister(&sclp_conf_register);
rc = -ENOSYS;
}
diff -urpN linux-2.6/drivers/s390/char/sclp_cpi_sys.c linux-2.6-patched/drivers/s390/char/sclp_cpi_sys.c
--- linux-2.6/drivers/s390/char/sclp_cpi_sys.c 2008-10-10 00:13:53.000000000 +0200
+++ linux-2.6-patched/drivers/s390/char/sclp_cpi_sys.c 2008-11-14 17:50:21.000000000 +0100
@@ -7,6 +7,9 @@
* Michael Ernst <[email protected]>
*/
+#define KMSG_COMPONENT "sclp_cpi"
+#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
+
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/stat.h>
@@ -20,6 +23,7 @@
#include <linux/completion.h>
#include <asm/ebcdic.h>
#include <asm/sclp.h>
+
#include "sclp.h"
#include "sclp_rw.h"
#include "sclp_cpi_sys.h"
@@ -150,16 +154,16 @@ static int cpi_req(void)
wait_for_completion(&completion);
if (req->status != SCLP_REQ_DONE) {
- printk(KERN_WARNING "cpi: request failed (status=0x%02x)\n",
- req->status);
+ pr_warning("request failed (status=0x%02x)\n",
+ req->status);
rc = -EIO;
goto out_free_req;
}
response = ((struct cpi_sccb *) req->sccb)->header.response_code;
if (response != 0x0020) {
- printk(KERN_WARNING "cpi: failed with "
- "response code 0x%x\n", response);
+ pr_warning("request failed with response code 0x%x\n",
+ response);
rc = -EIO;
}
diff -urpN linux-2.6/drivers/s390/char/sclp_sdias.c linux-2.6-patched/drivers/s390/char/sclp_sdias.c
--- linux-2.6/drivers/s390/char/sclp_sdias.c 2008-10-10 00:13:53.000000000 +0200
+++ linux-2.6-patched/drivers/s390/char/sclp_sdias.c 2008-11-14 17:50:21.000000000 +0100
@@ -5,15 +5,18 @@
* Author(s): Michael Holzheu
*/
+#define KMSG_COMPONENT "sclp_sdias"
+#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
+
#include <linux/sched.h>
#include <asm/sclp.h>
#include <asm/debug.h>
#include <asm/ipl.h>
+
#include "sclp.h"
#include "sclp_rw.h"
#define TRACE(x...) debug_sprintf_event(sdias_dbf, 1, x)
-#define ERROR_MSG(x...) printk ( KERN_ALERT "SDIAS: " x )
#define SDIAS_RETRIES 300
#define SDIAS_SLEEP_TICKS 50
@@ -131,7 +134,7 @@ int sclp_sdias_blk_count(void)
rc = sdias_sclp_send(&request);
if (rc) {
- ERROR_MSG("sclp_send failed for get_nr_blocks\n");
+ pr_err("sclp_send failed for get_nr_blocks\n");
goto out;
}
if (sccb.hdr.response_code != 0x0020) {
@@ -145,7 +148,8 @@ int sclp_sdias_blk_count(void)
rc = sccb.evbuf.blk_cnt;
break;
default:
- ERROR_MSG("SCLP error: %x\n", sccb.evbuf.event_status);
+ pr_err("SCLP error: %x\n",
+ sccb.evbuf.event_status);
rc = -EIO;
goto out;
}
@@ -201,7 +205,7 @@ int sclp_sdias_copy(void *dest, int star
rc = sdias_sclp_send(&request);
if (rc) {
- ERROR_MSG("sclp_send failed: %x\n", rc);
+ pr_err("sclp_send failed: %x\n", rc);
goto out;
}
if (sccb.hdr.response_code != 0x0020) {
@@ -219,9 +223,9 @@ int sclp_sdias_copy(void *dest, int star
case EVSTATE_NO_DATA:
TRACE("no data\n");
default:
- ERROR_MSG("Error from SCLP while copying hsa. "
- "Event status = %x\n",
- sccb.evbuf.event_status);
+ pr_err("Error from SCLP while copying hsa. "
+ "Event status = %x\n",
+ sccb.evbuf.event_status);
rc = -EIO;
}
out: