Skip to contentSkip to conversation

include/linux/kernel.h: Add config option for pr_fmt(fmt)

RFC PATCH In review Joe Perches 3 participants 15 messages Oct 22, 2010 – Nov 9, 2010 1 message not in the archive

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 include/linux/kernel.h: Add config option for pr_fmt(fmt)

Change the default #define pr_fmt(fmt) from:
- #define pr_fmt(fmt) fmt
to:
- #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

This will standard use of prefixes and prevent the addition of new #defines when using pr_<level>.

Adds a config option to use the old style if desired.

This adds prefixes to 326 strings in an x86 defconfig. Broken out by subsystem/module, the prefixes are:

     10 acpi                
      2 act_api             
      2 af_inet             
      6 af_packet           
     22 apic                
      1 apic_noop           
      2 blktrace            
      1 boot                
      4 calibrate           
      1 centaur             
      2 cleanup             
      1 common              
      2 dev                 
      1 devinet             
     17 edac_mce_amd        
      1 ehci_hcd            
      1 fb                  
     13 generic             
      9 hibernate           
      2 i2c_algo_bit        
      1 io_delay            
      1 ip6table_filter     
      1 iptable_filter      
      2 iptable_nat         
      3 ipv6                
      2 irq                 
      8 kexec               
      5 libphy              
     19 main                
      2 manage              
     11 mce                 
      2 mount               
      1 msr                 
      5 nf_conntrack_ipv4   
      7 nf_conntrack_ipv6   
      4 nf_conntrack_netlink
      2 nfnetlink           
      2 nfnetlink_log       
      1 ohci_hcd            
      5 oom_kill            
      1 pcieportdrv         
      9 percpu              
     21 perf_event          
      1 pid                 
      2 platform            
      2 probe_32            
      1 rtc_cmos            
      2 setup               
      1 setup_bus           
      1 skbuff              
      1 sleep               
      8 smpboot             
      4 snapshot            
      3 suspend             
      6 swap                
      1 tcp_ipv4            
      8 trace               
     14 trace_events        
     26 trace_kprobe        
      3 trace_sched_switch  
      1 trace_stat          
      3 tsc_sync            
     10 usbcore             
      2 usb_storage         
     10 vgaarb              
      1 xt_state 

Some of these add useful info, others are redundant and can and will be trivially fixed by adding either a new line to #define pr_fmt(fmt) fmt to the sources or by removing the internal prefix strings in the existing pr_<level> calls.

---
 include/linux/kernel.h |    4 ++++
 init/Kconfig           |   10 ++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)
include/linux/kernel.h +0 -0
index 1759ba5..9cb2e8f 100644
@@ -409,8 +409,12 @@ static inline char *pack_hex_byte(char *buf, u8

byte)
extern int hex_to_bin(char ch);

#ifndef pr_fmt
+#ifdef CONFIG_PR_FMT_IS_KBUILD_MODNAME +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt +#else
#define pr_fmt(fmt) fmt
#endif
+#endif

 #define pr_emerg(fmt, ...) \
         printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
init/Kconfig +10 -0
index 7b920aa..a406d1c 100644
@@ -897,6 +897,16 @@ config PRINTK
 	  very difficult to diagnose system problems, saying N here is
 	  strongly discouraged.
 
+config PR_FMT_IS_KBUILD_MODNAME
+	default y
+	depends on PRINTK
+	bool "Use KBUILD_MODNAME as prefix for pr_<level>"
+	help
+	  This option sets the default pr_fmt to KBUILD_MODNAME.
+	  This will prefix all pr_<level> logging message calls with the
+	  build system defined value unless there is an existing define
+	  of pr_fmt in the source code.
+
 config BUG
 	bool "BUG() support" if EMBEDDED
 	default y

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw

PATCH 1/1 drivers/acpi: Add and use pr_fmt(fmt)

Use #define pr_fmt(fmt) with appropriate prefix. Remove use PREFIX/<foo>_PFX from pr_<level>. Remove #defines of PREFIX and <foo>_PFX.
Coalesce some formats.

This changes the output of messages that use FW_WARN/FW_BUG so that the prefix is first then FW_<LEVEL>.

---
 drivers/acpi/apei/apei-base.c |   37 ++++++++++++------------
 drivers/acpi/apei/einj.c      |   26 ++++++++---------
 drivers/acpi/apei/erst.c      |   42 ++++++++++++---------------
 drivers/acpi/apei/ghes.c      |   50 ++++++++++++++++----------------
 drivers/acpi/apei/hest.c      |   22 +++++++-------
 drivers/acpi/ec.c             |   62 ++++++++++++++++++++---------------------
 6 files changed, 115 insertions(+), 124 deletions(-)
drivers/acpi/apei/apei-base.c +18 -19
index 4a904a4..d32b05a 100644
@@ -30,6 +30,8 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#define pr_fmt(fmt) "apei: " fmt
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -44,8 +46,6 @@
 
 #include "apei-internal.h"
 
-#define APEI_PFX "APEI: "
-
 /*
  * APEI ERST (Error Record Serialization Table) and EINJ (Error
  * INJection) interpreter framework.
@@ -181,9 +181,9 @@ rewind:
 		if (ip == ctx->ip) {
 			if (entry->instruction >= ctx->instructions ||
 			    !ctx->ins_table[entry->instruction].run) {
-				pr_warning(FW_WARN APEI_PFX
+				pr_warn(FW_WARN
 			"Invalid action table, unknown instruction type: %d\n",
-					   entry->instruction);
+					entry->instruction);
 				return -EINVAL;
 			}
 			run = ctx->ins_table[entry->instruction].run;
@@ -222,9 +222,9 @@ static int apei_exec_for_each_entry(struct apei_exec_context *ctx,
 		if (end)
 			*end = i;
 		if (ins >= ctx->instructions || !ins_table[ins].run) {
-			pr_warning(FW_WARN APEI_PFX
+			pr_warn(FW_WARN
 			"Invalid action table, unknown instruction type: %d\n",
-				   ins);
+				ins);
 			return -EINVAL;
 		}
 		rc = func(ctx, entry, data);
@@ -458,7 +458,7 @@ int apei_resources_request(struct apei_resources *resources,
 		r = request_mem_region(res->start, res->end - res->start,
 				       desc);
 		if (!r) {
-			pr_err(APEI_PFX
+			pr_err(
 		"Can not request iomem region <%016llx-%016llx> for GARs.\n",
 			       (unsigned long long)res->start,
 			       (unsigned long long)res->end);
@@ -470,7 +470,7 @@ int apei_resources_request(struct apei_resources *resources,
 	list_for_each_entry(res, &resources->ioport, list) {
 		r = request_region(res->start, res->end - res->start, desc);
 		if (!r) {
-			pr_err(APEI_PFX
+			pr_err(
 		"Can not request ioport region <%016llx-%016llx> for GARs.\n",
 			       (unsigned long long)res->start,
 			       (unsigned long long)res->end);
@@ -481,7 +481,7 @@ int apei_resources_request(struct apei_resources *resources,
 
 	rc = apei_resources_merge(&apei_resources_all, resources);
 	if (rc) {
-		pr_err(APEI_PFX "Fail to merge resources!\n");
+		pr_err("Fail to merge resources!\n");
 		goto err_unmap_ioport;
 	}
 
@@ -515,7 +515,7 @@ void apei_resources_release(struct apei_resources *resources)
 
 	rc = apei_resources_sub(&apei_resources_all, resources);
 	if (rc)
-		pr_err(APEI_PFX "Fail to sub resources!\n");
+		pr_err("Fail to sub resources!\n");
 }
 EXPORT_SYMBOL_GPL(apei_resources_release);
 
@@ -528,24 +528,23 @@ static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr)
 	/* Handle possible alignment issues */
 	memcpy(paddr, &reg->address, sizeof(*paddr));
 	if (!*paddr) {
-		pr_warning(FW_BUG APEI_PFX
-			   "Invalid physical address in GAR [0x%llx/%u/%u]\n",
-			   *paddr, width, space_id);
+		pr_warn(FW_BUG
+			"Invalid physical address in GAR [0x%llx/%u/%u]\n",
+			*paddr, width, space_id);
 		return -EINVAL;
 	}
 
 	if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
-		pr_warning(FW_BUG APEI_PFX
-			   "Invalid bit width in GAR [0x%llx/%u/%u]\n",
-			   *paddr, width, space_id);
+		pr_warn(FW_BUG "Invalid bit width in GAR [0x%llx/%u/%u]\n",
+			*paddr, width, space_id);
 		return -EINVAL;
 	}
 
 	if (space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
 	    space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
-		pr_warning(FW_BUG APEI_PFX
-			   "Invalid address space type in GAR [0x%llx/%u/%u]\n",
-			   *paddr, width, space_id);
+		pr_warn(FW_BUG
+			"Invalid address space type in GAR [0x%llx/%u/%u]\n",
+			*paddr, width, space_id);
 		return -EINVAL;
 	}
 
drivers/acpi/apei/einj.c +12 -14
index cf29df6..5b60e6d 100644
@@ -24,6 +24,8 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -36,8 +38,6 @@
 
 #include "apei-internal.h"
 
-#define EINJ_PFX "EINJ: "
-
 #define SPIN_UNIT		100			/* 100ns */
 /* Firmware should respond within 1 miliseconds */
 #define FIRMWARE_TIMEOUT	(1 * NSEC_PER_MSEC)
@@ -136,8 +136,7 @@ static int einj_get_available_error_type(u32 *type)
 static int einj_timedout(u64 *t)
 {
 	if ((s64)*t < SPIN_UNIT) {
-		pr_warning(FW_WARN EINJ_PFX
-			   "Firmware does not respond in time\n");
+		pr_warn(FW_WARN "Firmware does not respond in time\n");
 		return 1;
 	}
 	*t -= SPIN_UNIT;
@@ -196,7 +195,7 @@ static int __einj_error_trigger(u64 trigger_paddr)
 	r = request_mem_region(trigger_paddr, sizeof(*trigger_tab),
 			       "APEI EINJ Trigger Table");
 	if (!r) {
-		pr_err(EINJ_PFX
+		pr_err(
 	"Can not request iomem region <%016llx-%016llx> for Trigger table.\n",
 		       (unsigned long long)trigger_paddr,
 		       (unsigned long long)trigger_paddr+sizeof(*trigger_tab));
@@ -204,13 +203,12 @@ static int __einj_error_trigger(u64 trigger_paddr)
 	}
 	trigger_tab = ioremap_cache(trigger_paddr, sizeof(*trigger_tab));
 	if (!trigger_tab) {
-		pr_err(EINJ_PFX "Failed to map trigger table!\n");
+		pr_err("Failed to map trigger table!\n");
 		goto out_rel_header;
 	}
 	rc = einj_check_trigger_header(trigger_tab);
 	if (rc) {
-		pr_warning(FW_BUG EINJ_PFX
-			   "The trigger error action table is invalid\n");
+		pr_warn(FW_BUG "The trigger error action table is invalid\n");
 		goto out_rel_header;
 	}
 	rc = -EIO;
@@ -219,7 +217,7 @@ static int __einj_error_trigger(u64 trigger_paddr)
 			       table_size - sizeof(*trigger_tab),
 			       "APEI EINJ Trigger Table");
 	if (!r) {
-		pr_err(EINJ_PFX
+		pr_err(
 "Can not request iomem region <%016llx-%016llx> for Trigger Table Entry.\n",
 		       (unsigned long long)trigger_paddr+sizeof(*trigger_tab),
 		       (unsigned long long)trigger_paddr + table_size);
@@ -228,7 +226,7 @@ static int __einj_error_trigger(u64 trigger_paddr)
 	iounmap(trigger_tab);
 	trigger_tab = ioremap_cache(trigger_paddr, table_size);
 	if (!trigger_tab) {
-		pr_err(EINJ_PFX "Failed to map trigger table!\n");
+		pr_err("Failed to map trigger table!\n");
 		goto out_rel_entry;
 	}
 	trigger_entry = (struct acpi_whea_header *)
@@ -454,17 +452,17 @@ static int __init einj_init(void)
 	status = acpi_get_table(ACPI_SIG_EINJ, 0,
 				(struct acpi_table_header **)&einj_tab);
 	if (status == AE_NOT_FOUND) {
-		pr_info(EINJ_PFX "Table is not found!\n");
+		pr_info("Table is not found!\n");
 		return -ENODEV;
 	} else if (ACPI_FAILURE(status)) {
 		const char *msg = acpi_format_exception(status);
-		pr_err(EINJ_PFX "Failed to get table, %s\n", msg);
+		pr_err("Failed to get table, %s\n", msg);
 		return -EINVAL;
 	}
 
 	rc = einj_check_table(einj_tab);
 	if (rc) {
-		pr_warning(FW_BUG EINJ_PFX "EINJ table is invalid\n");
+		pr_warn(FW_BUG "EINJ table is invalid\n");
 		return -EINVAL;
 	}
 
@@ -513,7 +511,7 @@ static int __init einj_init(void)
 			goto err_unmap;
 	}
 
-	pr_info(EINJ_PFX "Error INJection is initialized.\n");
+	pr_info("Error INJection is initialized.\n");
 
 	return 0;
 
drivers/acpi/apei/erst.c +19 -23

128 lines not on this page · raw message

drivers/acpi/apei/ghes.c +25 -25

112 lines not on this page · raw message

drivers/acpi/apei/hest.c +11 -11
index 1a3508a..7b92833 100644
@@ -27,6 +27,8 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -39,8 +41,6 @@
 
 #include "apei-internal.h"
 
-#define HEST_PFX "HEST: "
-
 int hest_disable;
 EXPORT_SYMBOL_GPL(hest_disable);
 
@@ -96,15 +96,15 @@ int apei_hest_parse(apei_hest_func_t func, void *data)
 	for (i = 0; i < hest_tab->error_source_count; i++) {
 		len = hest_esrc_len(hest_hdr);
 		if (!len) {
-			pr_warning(FW_WARN HEST_PFX
-				   "Unknown or unused hardware error source "
-				   "type: %d for hardware error source: %d.\n",
-				   hest_hdr->type, hest_hdr->source_id);
+			pr_warn(FW_WARN
+				"Unknown or unused hardware error source "
+				"type: %d for hardware error source: %d.\n",
+				hest_hdr->type, hest_hdr->source_id);
 			return -EINVAL;
 		}
 		if ((void *)hest_hdr + len >
 		    (void *)hest_tab + hest_tab->header.length) {
-			pr_warning(FW_BUG HEST_PFX
+			pr_warn(FW_BUG
 		"Table contents overflow for hardware error source: %d.\n",
 				hest_hdr->source_id);
 			return -EINVAL;
@@ -205,18 +205,18 @@ static int __init hest_init(void)
 		goto err;
 
 	if (hest_disable) {
-		pr_info(HEST_PFX "HEST tabling parsing is disabled.\n");
+		pr_info("HEST tabling parsing is disabled.\n");
 		goto err;
 	}
 
 	status = acpi_get_table(ACPI_SIG_HEST, 0,
 				(struct acpi_table_header **)&hest_tab);
 	if (status == AE_NOT_FOUND) {
-		pr_info(HEST_PFX "Table is not found!\n");
+		pr_info("Table is not found!\n");
 		goto err;
 	} else if (ACPI_FAILURE(status)) {
 		const char *msg = acpi_format_exception(status);
-		pr_err(HEST_PFX "Failed to get table, %s\n", msg);
+		pr_err("Failed to get table, %s\n", msg);
 		rc = -EINVAL;
 		goto err;
 	}
@@ -229,7 +229,7 @@ static int __init hest_init(void)
 	if (rc)
 		goto err;
 
-	pr_info(HEST_PFX "HEST table parsing is initialized.\n");
+	pr_info("HEST table parsing is initialized.\n");
 
 	return 0;
 err:
drivers/acpi/ec.c +30 -32

191 lines not on this page · raw message

#3 lore raw
In reply to Joe Perches

Hi Joe,

Quoted 7 lines · Joe Perches

Change the default #define pr_fmt(fmt) from:
- #define pr_fmt(fmt) fmt
to:
- #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

This will standard use of prefixes and prevent the addition of new #defines when using pr_<level>.

I'm all for it!

Quoted 1 line

Adds a config option to use the old style if desired.

Not sure what the idea is. Once pr_fmt() includes the module name, we will drop hard-coded prefixes in all log messages throughout the kernel tree. Once this is done, a kernel built with PR_FMT_IS_KBUILD_MODNAME=n would become horribly confusing.

So I wouldn't add any configuration option. Doing so is likely to add to confusion more than help.

Quoted 1 line

This adds prefixes to 326 strings in an x86 defconfig.

How relevant is the x86 defconfig? It doesn't include any hardware-specific driver, does it? This is where I would expect the greater count of pr_*() calls. For example, I have 15 hits in drivers/hwmon, and 8 in drivers/i2c.

I've used the following grep to find them: grep -I 'pr_[a-z]*([^"]'

Let me know if you have anything better.

Quoted 69 lines

Broken out by subsystem/module, the prefixes are:

     10 acpi                
      2 act_api             
      2 af_inet             
      6 af_packet           
     22 apic                
      1 apic_noop           
      2 blktrace            
      1 boot                
      4 calibrate           
      1 centaur             
      2 cleanup             
      1 common              
      2 dev                 
      1 devinet             
     17 edac_mce_amd        
      1 ehci_hcd            
      1 fb                  
     13 generic             
      9 hibernate           
      2 i2c_algo_bit        
      1 io_delay            
      1 ip6table_filter     
      1 iptable_filter      
      2 iptable_nat         
      3 ipv6                
      2 irq                 
      8 kexec               
      5 libphy              
     19 main                
      2 manage              
     11 mce                 
      2 mount               
      1 msr                 
      5 nf_conntrack_ipv4   
      7 nf_conntrack_ipv6   
      4 nf_conntrack_netlink
      2 nfnetlink           
      2 nfnetlink_log       
      1 ohci_hcd            
      5 oom_kill            
      1 pcieportdrv         
      9 percpu              
     21 perf_event          
      1 pid                 
      2 platform            
      2 probe_32            
      1 rtc_cmos            
      2 setup               
      1 setup_bus           
      1 skbuff              
      1 sleep               
      8 smpboot             
      4 snapshot            
      3 suspend             
      6 swap                
      1 tcp_ipv4            
      8 trace               
     14 trace_events        
     26 trace_kprobe        
      3 trace_sched_switch  
      1 trace_stat          
      3 tsc_sync            
     10 usbcore             
      2 usb_storage         
     10 vgaarb              
      1 xt_state 
Quoted 5 lines

Some of these add useful info, others are redundant and can and will be trivially fixed by adding either a new line to #define pr_fmt(fmt) fmt to the sources or by removing the internal prefix strings in the existing pr_<level> calls.

Hopefully the latter, otherwise the effort to standardize the log messages will fail.

Quoted 46 lines
---
 include/linux/kernel.h |    4 ++++
 init/Kconfig           |   10 ++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)
include/linux/kernel.h +4 -0
index 1759ba5..9cb2e8f 100644
@@ -409,8 +409,12 @@ static inline char *pack_hex_byte(char *buf, u8
 byte)
 extern int hex_to_bin(char ch);
 
 #ifndef pr_fmt
+#ifdef CONFIG_PR_FMT_IS_KBUILD_MODNAME
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#else
 #define pr_fmt(fmt) fmt
 #endif
+#endif
 
 #define pr_emerg(fmt, ...) \
         printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
init/Kconfig +10 -0
index 7b920aa..a406d1c 100644
@@ -897,6 +897,16 @@ config PRINTK
 	  very difficult to diagnose system problems, saying N here is
 	  strongly discouraged.
 
+config PR_FMT_IS_KBUILD_MODNAME
+	default y
+	depends on PRINTK
+	bool "Use KBUILD_MODNAME as prefix for pr_<level>"
+	help
+	  This option sets the default pr_fmt to KBUILD_MODNAME.
+	  This will prefix all pr_<level> logging message calls with the
+	  build system defined value unless there is an existing define
+	  of pr_fmt in the source code.
+
 config BUG
 	bool "BUG() support" if EMBEDDED
 	default y
Signature · 1 line
Jean Delvare
#4 lore raw
Quoted 7 lines · Jean Delvare
Quoted 6 lines · Joe Perches

Change the default #define pr_fmt(fmt) from:
- #define pr_fmt(fmt) fmt
to:
- #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
This will standard use of prefixes and prevent the addition of new #defines when using pr_<level>.

Quoted 6 lines

I'm all for it!

Quoted 1 line

Adds a config option to use the old style if desired.

Not sure what the idea is. Once pr_fmt() includes the module name, we will drop hard-coded prefixes in all log messages throughout the kernel tree. Once this is done, a kernel built with PR_FMT_IS_KBUILD_MODNAME=n would become horribly confusing.

True. The idea is to allow a transition period and remove this PR_FMT_IS_KBUILD_MODNAME config option later.

Quoted 2 lines

How relevant is the x86 defconfig?
It doesn't include any hardware-specific driver, does it?

It adds lots of x86 specific drivers...

Quoted 2 lines

I've used the following grep to find them: grep -I 'pr_[a-z]*([^"]' Let me know if you have anything better.

Perhaps use -P "\bpr_(emerg|alert|crit|err|warning|warn|notice|info|cont|debug)\s*\(\s*\"\w+:"

Another way is to use:
strings <vmlinux|other> | grep -P "^<.>\w+:"

#5 lore raw
Quoted 9 lines · Joe Perches
Quoted 8 lines · Jean Delvare
Quoted 6 lines · Joe Perches

Change the default #define pr_fmt(fmt) from:
- #define pr_fmt(fmt) fmt
to:
- #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
This will standard use of prefixes and prevent the addition of new #defines when using pr_<level>.

I'm all for it!

Quoted 9 lines
Quoted 5 lines
Quoted 1 line

Adds a config option to use the old style if desired.

Not sure what the idea is. Once pr_fmt() includes the module name, we will drop hard-coded prefixes in all log messages throughout the kernel tree. Once this is done, a kernel built with PR_FMT_IS_KBUILD_MODNAME=n would become horribly confusing.

True. The idea is to allow a transition period and remove this PR_FMT_IS_KBUILD_MODNAME config option later.

Personally I would prefer to just make the change without config option.

However, if that is not acceptable, a config option which is enabled by default would still be better than nothing - and much better than hundreds of pr_fmt() spread throughout the code. Feel free to add my Acked-by, whatever that may help.

Guenter

#6 lore raw
In reply to Joe Perches
Quoted 9 lines · Joe Perches
Quoted 8 lines · Jean Delvare
Quoted 6 lines · Joe Perches

Change the default #define pr_fmt(fmt) from:
- #define pr_fmt(fmt) fmt
to:
- #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
This will standard use of prefixes and prevent the addition of new #defines when using pr_<level>.

I'm all for it!

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Adds a config option to use the old style if desired.

Not sure what the idea is. Once pr_fmt() includes the module name, we will drop hard-coded prefixes in all log messages throughout the kernel tree. Once this is done, a kernel built with PR_FMT_IS_KBUILD_MODNAME=n would become horribly confusing.

True. The idea is to allow a transition period and remove this PR_FMT_IS_KBUILD_MODNAME config option later.

I don't buy this, sorry. During the "transition period", neither value of this option will produce good results. One will lead to duplicate prefixes and the other will lead to missing prefixes.

I think it's preferable to drop this option, and prepare patches for all drivers, so that the change happens all at once and we're done with it. You seem to have all the tools to produce such patches, right?

Of course there will be 1% of all messages which we won't get right, and they will have t be fixed afterwards, but I don't think this is a problem.

Quoted 10 lines
Quoted 2 lines

How relevant is the x86 defconfig?
It doesn't include any hardware-specific driver, does it?

It adds lots of x86 specific drivers...

Quoted 2 lines

I've used the following grep to find them: grep -I 'pr_[a-z]*([^"]' Let me know if you have anything better.

Perhaps use -P "\bpr_(emerg|alert|crit|err|warning|warn|notice|info|cont|debug)\s*\(\s*\"\w+:"

Ah, right. This finds the hard-coded prefixes, my approach only caught the constant prefixes. So we want to run both.

Quoted 2 lines

Another way is to use:
strings <vmlinux|other> | grep -P "^<.>\w+:"

This catches everything, but you don't always know where the string comes from, so it isn't as convenient.

Signature · 1 line
Jean Delvare
#7 lore raw
Quoted 16 lines · Jean Delvare
Quoted 15 lines · Joe Perches
Quoted 13 lines · Jean Delvare
Quoted 6 lines · Joe Perches

Change the default #define pr_fmt(fmt) from:
- #define pr_fmt(fmt) fmt
to:
- #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
This will standard use of prefixes and prevent the addition of new #defines when using pr_<level>.

I'm all for it!

Quoted 1 line

Adds a config option to use the old style if desired.

Not sure what the idea is. Once pr_fmt() includes the module name, we will drop hard-coded prefixes in all log messages throughout the kernel tree. Once this is done, a kernel built with PR_FMT_IS_KBUILD_MODNAME=n would become horribly confusing.

Quoted 6 lines
Quoted 2 lines

True. The idea is to allow a transition period and remove this PR_FMT_IS_KBUILD_MODNAME config option later.

I don't buy this, sorry. During the "transition period", neither value of this option will produce good results. One will lead to duplicate prefixes and the other will lead to missing prefixes.

So why don't you change it in your kernel.h and include that in linux-next and see how many complaints you get.

#8 lore raw
Quoted 20 lines · Joe Perches
Quoted 19 lines · Jean Delvare
Quoted 17 lines · Joe Perches
Quoted 7 lines · Jean Delvare
Quoted 6 lines · Joe Perches

Change the default #define pr_fmt(fmt) from:
- #define pr_fmt(fmt) fmt
to:
- #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
This will standard use of prefixes and prevent the addition of new #defines when using pr_<level>.

Quoted 6 lines

I'm all for it!

Quoted 1 line

Adds a config option to use the old style if desired.

Not sure what the idea is. Once pr_fmt() includes the module name, we will drop hard-coded prefixes in all log messages throughout the kernel tree. Once this is done, a kernel built with PR_FMT_IS_KBUILD_MODNAME=n would become horribly confusing.

True. The idea is to allow a transition period and remove this PR_FMT_IS_KBUILD_MODNAME config option later.

Quoted 6 lines
Quoted 3 lines

I don't buy this, sorry. During the "transition period", neither value of this option will produce good results. One will lead to duplicate prefixes and the other will lead to missing prefixes.

So why don't you change it in your kernel.h and include that in linux-next and see how many complaints you get.

It would seem awkward that the i2c or hwmon tree would touch kernel.h.

Besides, linux-next is meant for integration testing. We already know that the change will integrate fine, in that it won't cause a build failure or runtime crash. We also know that, without the tree-wide cleanup of many driver, the change will cause duplicate prefixes in many messages.

There's little point in testing something we know will not be good enough. Better prepare all the driver patches, and test the whole thing when it's ready. I know it will be a very large and intrusive patchset, but this can certainly be done with Andrew's support.

Signature · 1 line
Jean Delvare
#9 lore raw
Quoted 32 lines · Jean Delvare
Quoted 16 lines · Joe Perches
Quoted 15 lines · Jean Delvare
Quoted 14 lines · Joe Perches
Quoted 13 lines · Jean Delvare
Quoted 6 lines · Joe Perches

Change the default #define pr_fmt(fmt) from:
- #define pr_fmt(fmt) fmt
to:
- #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
This will standard use of prefixes and prevent the addition of new #defines when using pr_<level>.

I'm all for it!

Quoted 1 line

Adds a config option to use the old style if desired.

Not sure what the idea is. Once pr_fmt() includes the module name, we will drop hard-coded prefixes in all log messages throughout the kernel tree. Once this is done, a kernel built with PR_FMT_IS_KBUILD_MODNAME=n would become horribly confusing.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

True. The idea is to allow a transition period and remove this PR_FMT_IS_KBUILD_MODNAME config option later.

I don't buy this, sorry. During the "transition period", neither value of this option will produce good results. One will lead to duplicate prefixes and the other will lead to missing prefixes.

So why don't you change it in your kernel.h and include that in linux-next and see how many complaints you get.

It would seem awkward that the i2c or hwmon tree would touch kernel.h.

Besides, linux-next is meant for integration testing. We already know that the change will integrate fine, in that it won't cause a build failure or runtime crash. We also know that, without the tree-wide cleanup of many driver, the change will cause duplicate prefixes in many messages.

Quoted 4 lines

There's little point in testing something we know will not be good enough. Better prepare all the driver patches, and test the whole thing when it's ready. I know it will be a very large and intrusive patchset, but this can certainly be done with Andrew's support.

I think you underestimate the time, effort and acceptance levels by the various arches and maintainers required.

Also, it's not just drivers, it's arch, lib, and kernel.

A short term issue might be .text expansion of embedded platforms that use CONFIG_PRINTK.

It doesn't look as if too many people care though.

#10 lore raw

Hi Joe,

Sorry for the late answer.

Quoted 12 lines · Joe Perches
Quoted 10 lines · Jean Delvare

Besides, linux-next is meant for integration testing. We already know that the change will integrate fine, in that it won't cause a build failure or runtime crash. We also know that, without the tree-wide cleanup of many driver, the change will cause duplicate prefixes in many messages.

There's little point in testing something we know will not be good enough. Better prepare all the driver patches, and test the whole thing when it's ready. I know it will be a very large and intrusive patchset, but this can certainly be done with Andrew's support.

Quoted 5 lines

I think you underestimate the time, effort and acceptance levels by the various arches and maintainers required.

Also, it's not just drivers, it's arch, lib, and kernel. (...)

I've had time to think about it all some more, and I have to admit that my counter-proposal doesn't really fly. Changing everything at once throughout the whole kernel tree is simply too difficult.

So I hate to admit it, but your initial proposal was certainly better, because it can be done one subsystem at a time. So I think we should forget about my objections and go on with your first patchset.

That being said, to avoid messing up the kernel tree completely, I think we need a clearly defined plan before we start. This plan should include:
* A clear statement of goal.
* An explanation of the steps we have to go through to reach it.
* An rough schedule of when it will happen (in either time or kernel versions) with a deadline after which changing the default definition of pr_fmt() will be considered OK.

And the plan should be made known to all subsystem maintainers, with publicly visible progress tracking. Otherwise I fear it will take forever to reach your goal.

Thanks,

Signature · 1 line
Jean Delvare
#11 lore raw
Quoted 23 lines · Jean Delvare

Hi Joe,

Sorry for the late answer.

Quoted 12 lines · Joe Perches
Quoted 10 lines · Jean Delvare

Besides, linux-next is meant for integration testing. We already know that the change will integrate fine, in that it won't cause a build failure or runtime crash. We also know that, without the tree-wide cleanup of many driver, the change will cause duplicate prefixes in many messages.

There's little point in testing something we know will not be good enough. Better prepare all the driver patches, and test the whole thing when it's ready. I know it will be a very large and intrusive patchset, but this can certainly be done with Andrew's support.

Quoted 5 lines

I think you underestimate the time, effort and acceptance levels by the various arches and maintainers required.

Also, it's not just drivers, it's arch, lib, and kernel. (...)

Quoted 8 lines

I've had time to think about it all some more, and I have to admit that my counter-proposal doesn't really fly. Changing everything at once throughout the whole kernel tree is simply too difficult.

So I hate to admit it, but your initial proposal was certainly better, because it can be done one subsystem at a time. So I think we should forget about my objections and go on with your first patchset.

I pretty much came to the same conclusion. No objections here anymore either.

Guenter

#12 lore raw
Quoted 4 lines · Guenter Roeck
Quoted 3 lines · Jean Delvare

Changing everything at once
throughout the whole kernel tree is simply too difficult. So I hate to admit it,

I try to value my own opinion rather less highly than I used to so I can discount it better when I'm proven wrong.

That must come from being married...

Quoted 2 lines

go on with your first patchset.
I pretty much came to the same conclusion. No objections here anymore either.

Is there anything you need from me?

#13 lore raw
Quoted 11 lines · Joe Perches
Quoted 4 lines · Guenter Roeck
Quoted 3 lines · Jean Delvare

Changing everything at once
throughout the whole kernel tree is simply too difficult. So I hate to admit it,

I try to value my own opinion rather less highly than I used to so I can discount it better when I'm proven wrong.

That must come from being married...

;)

Quoted 5 lines
Quoted 2 lines

go on with your first patchset.
I pretty much came to the same conclusion. No objections here anymore either.

Is there anything you need from me?

I'll start applying the patches to my next tree. I'll get back to you if I run into trouble.

Jean, do you want to take some of the patches, or should I take them all ?

Guenter

#14 lore raw

Hi Guenter,

Quoted 4 lines · Guenter Roeck

I'll start applying the patches to my next tree. I'll get back to you if I run into trouble.

Jean, do you want to take some of the patches, or should I take them all ?

I'll take the it87, lm78, pc87360, pc87427 and w83781d patches. Please take all the rest. Thanks!

Signature · 1 line
Jean Delvare
#15 lore raw
Quoted 11 lines · Jean Delvare

Hi Guenter,

Quoted 4 lines · Guenter Roeck

I'll start applying the patches to my next tree. I'll get back to you if I run into trouble.

Jean, do you want to take some of the patches, or should I take them all ?

I'll take the it87, lm78, pc87360, pc87427 and w83781d patches. Please take all the rest. Thanks!

Ok.

Guenter