[PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Joe Perches]

dev_<level> macros use a lot of repetitive string space.

Eliminate the string prefixes and function arguments from all the macro uses
and consolidate them in functions.

This patchset saves about 60K.

This implementation also adds the ability to use a struct va_format to
emit a format string along with va_list arguments.

This %pV implementation should not be used without a wrapper that
does printf argument verification like the dev_<level> functions.

Inspired a bit by Nick Andrew's patches and Linus' comments in December 2008
http://lkml.org/lkml/2008/12/6/15
http://lkml.org/lkml/2008/12/6/101

Joe Perches (2):
  vsprintf: Recursive vsnprintf: Add "%pV", struct va_format
  device.h drivers/base/core.c Convert dev_<level> macros to functions

 drivers/base/core.c    |  124 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  102 +++++++++++++++++++++++++++++----------
 include/linux/kernel.h |    5 ++
 lib/vsprintf.c         |    9 ++++
 4 files changed, 214 insertions(+), 26 deletions(-)

[PATCH 1/2] vsprintf: Recursive vsnprintf: Add "%pV", struct va_format

[Joe Perches]

Add the ability to print a format and va_list from a structure pointer

Allows __dev_printk to be implemented as a single printk while
minimizing string space duplication.

%pV should not be used without some mechanism to verify the
format and argument use ala __attribute__(format (printf(...))).

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/linux/kernel.h |    5 +++++
 lib/vsprintf.c         |    9 +++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 7f07074..0eae8e9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -169,6 +169,11 @@ static inline void might_fault(void)
 }
 #endif
 
+struct va_format {
+	const char *fmt;
+	va_list *va;
+};
+
 extern struct atomic_notifier_head panic_notifier_list;
 extern long (*panic_blink)(long time);
 NORET_TYPE void panic(const char * fmt, ...)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index af4aaa6..0fd7f8b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -950,6 +950,11 @@ static char *uuid_string(char *buf, char *end, const u8 *addr,
  *             [0][1][2][3]-[4][5]-[6][7]-[8][9]-[10][11][12][13][14][15]
  *           little endian output byte order is:
  *             [3][2][1][0]-[5][4]-[7][6]-[8][9]-[10][11][12][13][14][15]
+ * - 'V' For a struct va_format which contains a format string * and va_list *,
+ *       call vsnprintf(->format, *->va_list).
+ *       Implements a "recursive vsnprintf".
+ *       Do not use this feature without some mechanism to verify the
+ *       correctness of the format string and va_list arguments.
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
@@ -994,6 +999,10 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		break;
 	case 'U':
 		return uuid_string(buf, end, ptr, spec, fmt);
+	case 'V':
+		return buf + vsnprintf(buf, end - buf,
+				       ((struct va_format *)ptr)->fmt,
+				       *(((struct va_format *)ptr)->va));
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
-- 
1.7.0.14.g7e948

[PATCH 2/2] device.h drivers/base/core.c Convert dev_<level> macros to functions

[Joe Perches]

Save ~60k in a defconfig

Use %pV and struct va_format
Format arguments are verified before printk

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/base/core.c    |  124 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  102 +++++++++++++++++++++++++++++----------
 2 files changed, 200 insertions(+), 26 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2820257..6551640 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1745,3 +1745,127 @@ void device_shutdown(void)
 	}
 	async_synchronize_full();
 }
+
+/*
+ * Device logging functions
+ */
+
+#ifdef CONFIG_PRINTK
+
+static int __dev_printk(const char *level, const struct device *dev,
+			const char *fmt, va_list args)
+{
+	struct va_format vaf;
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+	return printk("%s%s %s: %pV",
+		      level, dev_driver_string(dev), dev_name(dev), &vaf);
+}
+
+int dev_printk(const char *level, const struct device *dev,
+	       const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(level, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_printk);
+
+int dev_emerg(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_EMERG, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_emerg);
+
+int dev_alert(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_ALERT, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_alert);
+
+int dev_crit(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_CRIT, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_crit);
+
+int dev_err(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_ERR, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_err);
+
+int dev_warn(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_WARNING, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_warn);
+
+int dev_notice(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_NOTICE, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_notice);
+
+int dev_info(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_INFO, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_info);
+
+#endif
diff --git a/include/linux/device.h b/include/linux/device.h
index b30527d..b4fb694 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -590,43 +590,93 @@ extern void sysdev_shutdown(void);
 
 /* debugging and troubleshooting/diagnostic helpers. */
 extern const char *dev_driver_string(const struct device *dev);
-#define dev_printk(level, dev, format, arg...)	\
-	printk(level "%s %s: " format , dev_driver_string(dev) , \
-	       dev_name(dev) , ## arg)
-
-#define dev_emerg(dev, format, arg...)		\
-	dev_printk(KERN_EMERG , dev , format , ## arg)
-#define dev_alert(dev, format, arg...)		\
-	dev_printk(KERN_ALERT , dev , format , ## arg)
-#define dev_crit(dev, format, arg...)		\
-	dev_printk(KERN_CRIT , dev , format , ## arg)
-#define dev_err(dev, format, arg...)		\
-	dev_printk(KERN_ERR , dev , format , ## arg)
-#define dev_warn(dev, format, arg...)		\
-	dev_printk(KERN_WARNING , dev , format , ## arg)
-#define dev_notice(dev, format, arg...)		\
-	dev_printk(KERN_NOTICE , dev , format , ## arg)
-#define dev_info(dev, format, arg...)		\
-	dev_printk(KERN_INFO , dev , format , ## arg)
+
+#ifdef CONFIG_PRINTK
+
+extern int dev_printk(const char *level, const struct device *dev,
+		      const char *fmt, ...)
+	__attribute__ ((format (printf, 3, 4)));
+extern int dev_emerg(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_alert(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_crit(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_err(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_warn(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_notice(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_info(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+
+#else
+
+static inline int dev_printk(const char *level, const struct device *dev,
+		      const char *fmt, ...)
+	__attribute__ ((format (printf, 3, 4)));
+static inline int dev_printk(const char *level, const struct device *dev,
+		      const char *fmt, ...)
+	 { return 0; }
+
+static inline int dev_emerg(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_emerg(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_crit(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_crit(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_alert(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_alert(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_err(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_err(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_warn(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_warn(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_notice(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_notice(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_info(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_info(const struct device *dev, const char *s, ...)
+	{ return 0; }
+
+#endif
 
 #if defined(DEBUG)
 #define dev_dbg(dev, format, arg...)		\
-	dev_printk(KERN_DEBUG , dev , format , ## arg)
+	dev_printk(KERN_DEBUG, dev, format, ##arg)
 #elif defined(CONFIG_DYNAMIC_DEBUG)
-#define dev_dbg(dev, format, ...) do { \
+#define dev_dbg(dev, format, ...)		     \
+do {						     \
 	dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
-	} while (0)
+} while (0)
 #else
-#define dev_dbg(dev, format, arg...)		\
-	({ if (0) dev_printk(KERN_DEBUG, dev, format, ##arg); 0; })
+#define dev_dbg(dev, format, arg...)				\
+({								\
+	if (0)							\
+		dev_printk(KERN_DEBUG, dev, format, ##arg);	\
+	0;							\
+})
 #endif
 
 #ifdef VERBOSE_DEBUG
 #define dev_vdbg	dev_dbg
 #else
-
-#define dev_vdbg(dev, format, arg...)		\
-	({ if (0) dev_printk(KERN_DEBUG, dev, format, ##arg); 0; })
+#define dev_vdbg(dev, format, arg...)				\
+({								\
+	if (0)							\
+		dev_printk(KERN_DEBUG, dev, format, ##arg);	\
+	0;							\
+})
 #endif
 
 /*
-- 
1.7.0.14.g7e948

[PATCH 1/2] vsprintf: Recursive vsnprintf: Add "%pV", struct va_format

[Joe Perches]

Add the ability to print a format and va_list from a structure pointer

Allows __dev_printk to be implemented as a single printk while
minimizing string space duplication.

%pV should not be used without some mechanism to verify the
format and argument use ala __attribute__(format (printf(...))).

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/linux/kernel.h |    5 +++++
 lib/vsprintf.c         |    9 +++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 7f07074..0eae8e9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -169,6 +169,11 @@ static inline void might_fault(void)
 }
 #endif
 
+struct va_format {
+	const char *fmt;
+	va_list *va;
+};
+
 extern struct atomic_notifier_head panic_notifier_list;
 extern long (*panic_blink)(long time);
 NORET_TYPE void panic(const char * fmt, ...)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index af4aaa6..0fd7f8b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -950,6 +950,11 @@ static char *uuid_string(char *buf, char *end, const u8 *addr,
  *             [0][1][2][3]-[4][5]-[6][7]-[8][9]-[10][11][12][13][14][15]
  *           little endian output byte order is:
  *             [3][2][1][0]-[5][4]-[7][6]-[8][9]-[10][11][12][13][14][15]
+ * - 'V' For a struct va_format which contains a format string * and va_list *,
+ *       call vsnprintf(->format, *->va_list).
+ *       Implements a "recursive vsnprintf".
+ *       Do not use this feature without some mechanism to verify the
+ *       correctness of the format string and va_list arguments.
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
@@ -994,6 +999,10 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		break;
 	case 'U':
 		return uuid_string(buf, end, ptr, spec, fmt);
+	case 'V':
+		return buf + vsnprintf(buf, end - buf,
+				       ((struct va_format *)ptr)->fmt,
+				       *(((struct va_format *)ptr)->va));
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
-- 
1.7.0.14.g7e948

[PATCH 2/2] device.h drivers/base/core.c Convert dev_<level> macros to functions

[Joe Perches]

Save ~60k in a defconfig

Use %pV and struct va_format
Format arguments are verified before printk

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/base/core.c    |  124 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  102 +++++++++++++++++++++++++++++----------
 2 files changed, 200 insertions(+), 26 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2820257..6551640 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1745,3 +1745,127 @@ void device_shutdown(void)
 	}
 	async_synchronize_full();
 }
+
+/*
+ * Device logging functions
+ */
+
+#ifdef CONFIG_PRINTK
+
+static int __dev_printk(const char *level, const struct device *dev,
+			const char *fmt, va_list args)
+{
+	struct va_format vaf;
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+	return printk("%s%s %s: %pV",
+		      level, dev_driver_string(dev), dev_name(dev), &vaf);
+}
+
+int dev_printk(const char *level, const struct device *dev,
+	       const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(level, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_printk);
+
+int dev_emerg(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_EMERG, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_emerg);
+
+int dev_alert(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_ALERT, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_alert);
+
+int dev_crit(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_CRIT, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_crit);
+
+int dev_err(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_ERR, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_err);
+
+int dev_warn(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_WARNING, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_warn);
+
+int dev_notice(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_NOTICE, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_notice);
+
+int dev_info(const struct device *dev, const char *fmt, ...)
+{
+	int r;
+	va_list args;
+
+	va_start(args, fmt);
+	r = __dev_printk(KERN_INFO, dev, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(dev_info);
+
+#endif
diff --git a/include/linux/device.h b/include/linux/device.h
index b30527d..b4fb694 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -590,43 +590,93 @@ extern void sysdev_shutdown(void);
 
 /* debugging and troubleshooting/diagnostic helpers. */
 extern const char *dev_driver_string(const struct device *dev);
-#define dev_printk(level, dev, format, arg...)	\
-	printk(level "%s %s: " format , dev_driver_string(dev) , \
-	       dev_name(dev) , ## arg)
-
-#define dev_emerg(dev, format, arg...)		\
-	dev_printk(KERN_EMERG , dev , format , ## arg)
-#define dev_alert(dev, format, arg...)		\
-	dev_printk(KERN_ALERT , dev , format , ## arg)
-#define dev_crit(dev, format, arg...)		\
-	dev_printk(KERN_CRIT , dev , format , ## arg)
-#define dev_err(dev, format, arg...)		\
-	dev_printk(KERN_ERR , dev , format , ## arg)
-#define dev_warn(dev, format, arg...)		\
-	dev_printk(KERN_WARNING , dev , format , ## arg)
-#define dev_notice(dev, format, arg...)		\
-	dev_printk(KERN_NOTICE , dev , format , ## arg)
-#define dev_info(dev, format, arg...)		\
-	dev_printk(KERN_INFO , dev , format , ## arg)
+
+#ifdef CONFIG_PRINTK
+
+extern int dev_printk(const char *level, const struct device *dev,
+		      const char *fmt, ...)
+	__attribute__ ((format (printf, 3, 4)));
+extern int dev_emerg(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_alert(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_crit(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_err(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_warn(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_notice(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+extern int dev_info(const struct device *dev, const char *fmt, ...)
+	__attribute__ ((format (printf, 2, 3)));
+
+#else
+
+static inline int dev_printk(const char *level, const struct device *dev,
+		      const char *fmt, ...)
+	__attribute__ ((format (printf, 3, 4)));
+static inline int dev_printk(const char *level, const struct device *dev,
+		      const char *fmt, ...)
+	 { return 0; }
+
+static inline int dev_emerg(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_emerg(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_crit(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_crit(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_alert(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_alert(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_err(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_err(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_warn(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_warn(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_notice(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_notice(const struct device *dev, const char *s, ...)
+	{ return 0; }
+static inline int dev_info(const struct device *dev, const char *s, ...)
+	__attribute__ ((format (printf, 2, 3)));
+static inline int dev_info(const struct device *dev, const char *s, ...)
+	{ return 0; }
+
+#endif
 
 #if defined(DEBUG)
 #define dev_dbg(dev, format, arg...)		\
-	dev_printk(KERN_DEBUG , dev , format , ## arg)
+	dev_printk(KERN_DEBUG, dev, format, ##arg)
 #elif defined(CONFIG_DYNAMIC_DEBUG)
-#define dev_dbg(dev, format, ...) do { \
+#define dev_dbg(dev, format, ...)		     \
+do {						     \
 	dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
-	} while (0)
+} while (0)
 #else
-#define dev_dbg(dev, format, arg...)		\
-	({ if (0) dev_printk(KERN_DEBUG, dev, format, ##arg); 0; })
+#define dev_dbg(dev, format, arg...)				\
+({								\
+	if (0)							\
+		dev_printk(KERN_DEBUG, dev, format, ##arg);	\
+	0;							\
+})
 #endif
 
 #ifdef VERBOSE_DEBUG
 #define dev_vdbg	dev_dbg
 #else
-
-#define dev_vdbg(dev, format, arg...)		\
-	({ if (0) dev_printk(KERN_DEBUG, dev, format, ##arg); 0; })
+#define dev_vdbg(dev, format, arg...)				\
+({								\
+	if (0)							\
+		dev_printk(KERN_DEBUG, dev, format, ##arg);	\
+	0;							\
+})
 #endif
 
 /*
-- 
1.7.0.14.g7e948

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Andrew Morton]

On Wed,  3 Mar 2010 22:27:21 -0800
Joe Perches <joe@perches.com> wrote:

> (Typo'ed Linus' email address, added Greg KH and netdev)

What is the networking significance here?

> dev_<level> macros use a lot of repetitive string space.
> 
> Eliminate the string prefixes and function arguments from all the macro uses
> and consolidate them in functions.
> 
> This patchset saves about 60K.
> 
> This implementation also adds the ability to use a struct va_format to
> emit a format string along with va_list arguments.
> 
> This %pV implementation should not be used without a wrapper that
> does printf argument verification like the dev_<level> functions.
> 
> Inspired a bit by Nick Andrew's patches and Linus' comments in December 2008
> http://lkml.org/lkml/2008/12/6/15
> http://lkml.org/lkml/2008/12/6/101
> 

Looks like a reasonable approach, although I didn't check how much
additional stack the recursion will take.  Bear in mind that printk()
can be called from super-deep contexts.

What would I need to do to make it recur more than once?  Include a %pV
in a string, like dev_printk("%s", %%pV")?

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Linus Torvalds]

On Thu, 4 Mar 2010, Andrew Morton wrote:
> 
> What would I need to do to make it recur more than once?  Include a %pV
> in a string, like dev_printk("%s", %%pV")?

I would argue that if somebody does that, they're just broken.

The single level of recursion is really nice - it solves a real annoyance. 
But if somebody starts trying to do multiple levels, that's just crazy.

Of course, I guess we don't relly have any way of sanely _counting_ the 
recursion level, so we'd have to just trust people to not do crazy things. 

And that's a big thing to take on trust ;)

			Linus

Re: [PATCH 2/2] device.h drivers/base/core.c Convert dev_<level> macros to functions

[Andrew Morton]

On Wed,  3 Mar 2010 22:27:23 -0800 Joe Perches <joe@perches.com> wrote:

> Save ~60k in a defconfig
> 
> Use %pV and struct va_format
> Format arguments are verified before printk

Well that doesn't work very well.

drivers/net/pcmcia/pcnet_cs.c:117: error: 'dev_info' redeclared as different kind of symbol
include/linux/device.h:645: error: previous declaration of 'dev_info' was here

there are lots of other dev_info's which will fail plus perhaps
dev_err's, etc.

Re: [PATCH 2/2] device.h drivers/base/core.c Convert dev_<level> macros to functions

[Andrew Morton]

On Thu, 4 Mar 2010 16:56:09 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed,  3 Mar 2010 22:27:23 -0800 Joe Perches <joe@perches.com> wrote:
> 
> > Save ~60k in a defconfig
> > 
> > Use %pV and struct va_format
> > Format arguments are verified before printk
> 
> Well that doesn't work very well.
> 
> drivers/net/pcmcia/pcnet_cs.c:117: error: 'dev_info' redeclared as different kind of symbol
> include/linux/device.h:645: error: previous declaration of 'dev_info' was here
> 
> there are lots of other dev_info's which will fail plus perhaps
> dev_err's, etc.
> 

btw, this may be kludgeable aroundable by doing

int _dev_info(const struct device *dev, const char *fmt, ...);

#define dev_info(...) _dev_info(...)

which will use the preprocessor's separation of `foo' from `foo()' to
fix things up.

But it would be better to rename all those dev_info's to device_info or
whatever, IMO.

Re: [PATCH 2/2] device.h drivers/base/core.c Convert dev_<level> macros to functions

[Andrew Morton]

On Thu, 4 Mar 2010 16:56:09 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed,  3 Mar 2010 22:27:23 -0800 Joe Perches <joe@perches.com> wrote:
> 
> > Save ~60k in a defconfig
> > 
> > Use %pV and struct va_format
> > Format arguments are verified before printk
> 
> Well that doesn't work very well.
> 
> drivers/net/pcmcia/pcnet_cs.c:117: error: 'dev_info' redeclared as different kind of symbol
> include/linux/device.h:645: error: previous declaration of 'dev_info' was here
> 
> there are lots of other dev_info's which will fail plus perhaps
> dev_err's, etc.
> 

btw, this may be kludgeable aroundable by doing

int _dev_info(const struct device *dev, const char *fmt, ...);

#define dev_info(...) _dev_info(...)

which will use the preprocessor's separation of `foo' from `foo()' to
fix things up.

But it would be better to rename all those dev_info's to device_info or
whatever, IMO.

Re: [PATCH 2/2] device.h drivers/base/core.c Convert dev_<level> macros to functions

[Joe Perches]

dOn Thu, 2010-03-04 at 17:00 -0800, Andrew Morton wrote:
> On Thu, 4 Mar 2010 16:56:09 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Wed,  3 Mar 2010 22:27:23 -0800 Joe Perches <joe@perches.com> wrote:
> > > Save ~60k in a defconfig
> > > Use %pV and struct va_format
> > > Format arguments are verified before printk
> > Well that doesn't work very well.
> > 
> > drivers/net/pcmcia/pcnet_cs.c:117: error: 'dev_info' redeclared as different kind of symbol
> > include/linux/device.h:645: error: previous declaration of 'dev_info' was here
> > 
> > there are lots of other dev_info's which will fail plus perhaps
> > dev_err's, etc.
> > 
> btw, this may be kludgeable aroundable by doing
> 
> int _dev_info(const struct device *dev, const char *fmt, ...);
> 
> #define dev_info(...) _dev_info(...)
> 
> which will use the preprocessor's separation of `foo' from `foo()' to
> fix things up.
> 
> But it would be better to rename all those dev_info's to device_info or
> whatever, IMO.

Perhaps using the function name _dev_info() and adding
the macro above is an acceptable workaround until the
dev_info struct names or local variable names could be
renamed if necessary.

dev_err and dev_warn exist in source comments only and
don't seem to be a problem.

David Miller?  Do you have an opinion on the renaming
of dev_info local variables and structs?

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Joe Perches]

On Thu, 2010-03-04 at 15:06 -0800, Linus Torvalds wrote:
> On Thu, 4 Mar 2010, Andrew Morton wrote:
> > What would I need to do to make it recur more than once?  Include a %pV
> > in a string, like dev_printk("%s", %%pV")?
> 
> I would argue that if somebody does that, they're just broken.
> 
> The single level of recursion is really nice - it solves a real annoyance. 
> But if somebody starts trying to do multiple levels, that's just crazy.
> 
> Of course, I guess we don't relly have any way of sanely _counting_ the 
> recursion level, so we'd have to just trust people to not do crazy things. 
> 
> And that's a big thing to take on trust ;)

Is that an ack, a nack or a get the hell out?

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Linus Torvalds]

On Sat, 6 Mar 2010, Joe Perches wrote:
> 
> Is that an ack, a nack or a get the hell out?

It's mostly an Ack. I think the concept is great. I'd love to have some 
way to limit recursion, and I'd also love to see some actual numbers of 
how deep the vsnprintf stack frame is, but I don't see how to do the 
first, and I'm hoping the second isn't too horrible.

		Linus

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Joe Perches]

On Sat, 2010-03-06 at 14:03 -0800, Linus Torvalds wrote:
> On Sat, 6 Mar 2010, Joe Perches wrote:
> > Is that an ack, a nack or a get the hell out?
> It's mostly an Ack. I think the concept is great. I'd love to have some 
> way to limit recursion,

Maybe limit the %pV recursion depth to 1 with something like:
(in vsprintf.c: pointer() )

	case 'V': {
		char *rtn;
		static int rcount++;

		if (rcount > 1) {	/* Don't recurse more than once */
			rcount--;
			break;		/* Use print the pointer */
		}
		rtn = buf + vsnprintf(buf, end - buf,
				      ((struct va_format *)ptr)->fmt,
				      *(((struct va_format *)ptr)->va));
		rcount--;
		return rtn;
	}
	
> and I'd also love to see some actual numbers of 
> how deep the vsnprintf stack frame is, but I don't see how to do the 
> first, and I'm hoping the second isn't too horrible.

I believe it's the arguments, a long long, a couple of pointers,
and a struct printf_spec.  Not too bad.

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Linus Torvalds]

On Sat, 6 Mar 2010, Joe Perches wrote:
> 
> Maybe limit the %pV recursion depth to 1 with something like:
> (in vsprintf.c: pointer() )

Nope. Think about concurrent users.

The thing is, you have to hide it in local storage. We could make it 
thread-local (not cpu-local), but even that interacts badly with 
interrupts.

The only really workable approach would be to have a stack slot that is 
created by the externally visible routines (and initialized to zero), and 
those then passe the address of that as an argument to the lower levels, 
and then the recursion happens entirely within those lower level functions 
that update the value.

So it's doable, it's just not pretty.

> > and I'd also love to see some actual numbers of > > how deep the vsnprintf stack frame is, but I don't see how to do the 
> > first, and I'm hoping the second isn't too horrible.
> 
> I believe it's the arguments, a long long, a couple of pointers,
> and a struct printf_spec.  Not too bad.

I'm not convinced. We pass that 'printf_spec' around a lot, including 
nesting. Not as a pointer, either.

(Bjorn Helgaas has a patch that gets rid of _some_ of the stack usage, but 
not nearly all).

		Linus

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Linus Torvalds]

On Sat, 6 Mar 2010, Linus Torvalds wrote:
> 
> I'm not convinced. We pass that 'printf_spec' around a lot, including 
> nesting. Not as a pointer, either.

Btw, I wonder if those fields could become bitfields, or 'unsigned char' 
etc. That printf_spec really is unnecessarily big. It should _easily_ fit 
in a single register on a 64-bit platform, possibly even a 32-bit one (we 
could limit field width and precision to 5 bits, for example)

			Linus

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Joe Perches]

On Sat, 2010-03-06 at 14:57 -0800, Linus Torvalds wrote:
> 
> On Sat, 6 Mar 2010, Linus Torvalds wrote:
> > 
> > I'm not convinced. We pass that 'printf_spec' around a lot, including 
> > nesting. Not as a pointer, either.
> 
> Btw, I wonder if those fields could become bitfields, or 'unsigned char' 
> etc. That printf_spec really is unnecessarily big. It should _easily_ fit 
> in a single register on a 64-bit platform, possibly even a 32-bit one (we 
> could limit field width and precision to 5 bits, for example)

I believe 32 bits isn't possible.

type:6
flags:8
width:8
base:6
precision:6
qualifier:8

So maybe something like this:

 lib/vsprintf.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)
---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index af4aaa6..fdee7f7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -408,12 +408,12 @@ enum format_type {
 };
 
 struct printf_spec {
-	enum format_type	type;
-	int			flags;		/* flags to number() */
-	int			field_width;	/* width of output field */
-	int			base;
-	int			precision;	/* # of digits/chars */
-	int			qualifier;
+	u16	type;
+	s16	field_width;	/* width of output field */
+	u8	flags;		/* flags to number() */
+	u8	base;
+	s8	precision;	/* # of digits/chars */
+	u8	qualifier;
 };
 
 static char *number(char *buf, char *end, unsigned long long num,
@@ -1333,7 +1333,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			break;
 
 		case FORMAT_TYPE_NRCHARS: {
-			int qualifier = spec.qualifier;
+			u8 qualifier = spec.qualifier;
 
 			if (qualifier == 'l') {
 				long *ip = va_arg(args, long *);
@@ -1619,7 +1619,7 @@ do {									\
 
 		case FORMAT_TYPE_NRCHARS: {
 			/* skip %n 's argument */
-			int qualifier = spec.qualifier;
+			u8 qualifier = spec.qualifier;
 			void *skip_arg;
 			if (qualifier == 'l')
 				skip_arg = va_arg(args, long *);
@@ -1885,7 +1885,9 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 	char *next;
 	char digit;
 	int num = 0;
-	int qualifier, base, field_width;
+	u8 qualifier;
+	u8 base;
+	s16 field_width;
 	bool is_sign;
 
 	while (*fmt && *str) {
@@ -1963,7 +1965,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		{
 			char *s = (char *)va_arg(args, char *);
 			if (field_width == -1)
-				field_width = INT_MAX;
+				field_width = SHORT_MAX;
 			/* first, skip leading white space in buffer */
 			str = skip_spaces(str);
 

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Linus Torvalds]

On Sat, 6 Mar 2010, Joe Perches wrote:
> 
> I believe 32 bits isn't possible.
> 
> type:6
> flags:8
> width:8
> base:6
> precision:6
> qualifier:8

type: 5
flags: 7
width: 5 (make it 6 bits and signed)
base: 5
precision: 5 (make it 6 bits, and signed)
qualifier: 3

Now, admittedly, right now 'qualifier' really has to be 8, but that's just 
because we use an ASCII character for it. But I don't think there are more 
than 8 actual legal qualifiers. And 'base' could be just 2 bits, since 
there are only a couple of legal bases, but I left room for them all 
anyway.

And the other really should fit in those numbers unchanged, I think. 

			Linus

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Linus Torvalds]

On Sat, 6 Mar 2010, Linus Torvalds wrote:
>
> width: 5 (make it 6 bits and signed)
> precision: 5 (make it 6 bits, and signed)

Oh, actually, no, we do that whole left-justification with the flags, we 
don't need that signed stuff. We do need one special value for the "no 
width, no precision", though.

		Linus

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Joe Perches]

On Sat, 2010-03-06 at 15:48 -0800, Linus Torvalds wrote:
> On Sat, 6 Mar 2010, Linus Torvalds wrote:
> > width: 5 (make it 6 bits and signed)
> > precision: 5 (make it 6 bits, and signed)
> Oh, actually, no, we do that whole left-justification with the flags, we 
> don't need that signed stuff. We do need one special value for the "no 
> width, no precision", though.

What I posted is pretty simple.
I think the added complexity isn't worth it.

If you or anyone else does, all yours.

Re: [RESEND PATCH 0/2] Make functions of dev_<level> macros, recursive vsnprintf

[Linus Torvalds]

On Sat, 6 Mar 2010, Linus Torvalds wrote:

> 
> 
> On Sat, 6 Mar 2010, Linus Torvalds wrote:
> >
> > width: 5 (make it 6 bits and signed)
> > precision: 5 (make it 6 bits, and signed)
> 
> Oh, actually, no, we do that whole left-justification with the flags, we 
> don't need that signed stuff. We do need one special value for the "no 
> width, no precision", though.

Btw, yeah, we do have a couple of "%.100s" etc, but we could fix those if 
we really wanted to do this.

And no, I guess we don't really need to make it 32-bit. Fitting in 64 bits 
would already be a big improvement over what we have now.

				Linus

[PATCH] vsprintf.c: Reduce sizeof struct printf_spec from 24 to 8 bytes

[Joe Perches]

On Sat, 2010-03-06 at 15:58 -0800, Linus Torvalds wrote:
> And no, I guess we don't really need to make it 32-bit. Fitting in 64 bits 
> would already be a big improvement over what we have now.

Reducing the size of struct printf_spec is a good thing
because multiple instances are commonly passed on stack.

It's possible for type to be u8 and field_width to be s8,
but this is likely small enough for now.

Signed-off-by: Joe Perches <joe@perches.com>
---
 lib/vsprintf.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index af4aaa6..fdee7f7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -408,12 +408,12 @@ enum format_type {
 };
 
 struct printf_spec {
-	enum format_type	type;
-	int			flags;		/* flags to number() */
-	int			field_width;	/* width of output field */
-	int			base;
-	int			precision;	/* # of digits/chars */
-	int			qualifier;
+	u16	type;
+	s16	field_width;	/* width of output field */
+	u8	flags;		/* flags to number() */
+	u8	base;
+	s8	precision;	/* # of digits/chars */
+	u8	qualifier;
 };
 
 static char *number(char *buf, char *end, unsigned long long num,
@@ -1333,7 +1333,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			break;
 
 		case FORMAT_TYPE_NRCHARS: {
-			int qualifier = spec.qualifier;
+			u8 qualifier = spec.qualifier;
 
 			if (qualifier == 'l') {
 				long *ip = va_arg(args, long *);
@@ -1619,7 +1619,7 @@ do {									\
 
 		case FORMAT_TYPE_NRCHARS: {
 			/* skip %n 's argument */
-			int qualifier = spec.qualifier;
+			u8 qualifier = spec.qualifier;
 			void *skip_arg;
 			if (qualifier == 'l')
 				skip_arg = va_arg(args, long *);
@@ -1885,7 +1885,9 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 	char *next;
 	char digit;
 	int num = 0;
-	int qualifier, base, field_width;
+	u8 qualifier;
+	u8 base;
+	s16 field_width;
 	bool is_sign;
 
 	while (*fmt && *str) {
@@ -1963,7 +1965,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		{
 			char *s = (char *)va_arg(args, char *);
 			if (field_width == -1)
-				field_width = INT_MAX;
+				field_width = SHORT_MAX;
 			/* first, skip leading white space in buffer */
 			str = skip_spaces(str);
 

Re: [PATCH] vsprintf.c: Reduce sizeof struct printf_spec from 24 to 8 bytes

[Linus Torvalds]

On Sat, 6 Mar 2010, Joe Perches wrote:
> 
> Reducing the size of struct printf_spec is a good thing
> because multiple instances are commonly passed on stack.

Sadly, this is not enough.

the 'pointer()' function has a 200+ byte stack footprint on x86-64. And 
vsnprintf itself is about 100+ bytes. So that stack depth is way bigger 
than I would have expected.

I'm not sure _why_ that stack footprint for 'pointer()' is so big, but I 
bet it's due to some simple inlining, and gcc (once more) sucking at it 
and not being able to combine stack frames. It's a damn shame.

I suspect it's mac_address_string() having 20-odd bytes of temporary data, 
ip6_compressed_string() having some more, ip6_addr_string() having 50 
bytes, uuid_string() adding thirty-odd bytes etc. Inline them all, suck up 
merging stack slots, and 200 bytes is easy.

So no, I don't think we can do the recursion as things stand. I've applied 
your cleanup patch, along with the two from Bjorn Helgaas (which he did 
for his pnp set), but they don't help this fundamental problem.

A few noinlines might be appropriate. As would a good gcc cluestick about 
inlining and stack usage. The latter is unlikely to materialize, I guess.

		Linus

Re: [PATCH] vsprintf.c: Reduce sizeof struct printf_spec from 24 to 8 bytes

[Linus Torvalds]

On Sat, 6 Mar 2010, Linus Torvalds wrote:

> the 'pointer()' function has a 200+ byte stack footprint on x86-64. And 
> vsnprintf itself is about 100+ bytes. So that stack depth is way bigger 
> than I would have expected.
> 
> I'm not sure _why_ that stack footprint for 'pointer()' is so big, but I 
> bet it's due to some simple inlining, and gcc (once more) sucking at it 
> and not being able to combine stack frames. It's a damn shame.

Yeah, a few noinline's gets 'pointer()' to just save registers on the 
stack, no need for any extra buffers (which then is ok for your recursion 
case - the other subfunctions it can call have their own buffers, of 
course, but they won't be in the recursive call-path except at the leaf.

vsnprintf() itself seems less obviously fixable. I'm not sure wht gcc 
decides it needs 88 bytes of temp-space there.

		Linus

[PATCH] vsprintf.c: Use noinline_for_stack

[Joe Perches]

On Sat, 2010-03-06 at 18:03 -0800, Linus Torvalds wrote:
> A few noinlines might be appropriate.

Mark static functions with noinline_for_stack

Signed-off-by: Joe Perches <joe@perches.com>
---
 lib/vsprintf.c |   67 +++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 42 insertions(+), 25 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 0d461c7..e9335a8 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -266,7 +266,8 @@ int strict_strtoll(const char *cp, unsigned int base, long long *res)
 }
 EXPORT_SYMBOL(strict_strtoll);
 
-static int skip_atoi(const char **s)
+static noinline_for_stack
+int skip_atoi(const char **s)
 {
 	int i = 0;
 
@@ -286,7 +287,8 @@ static int skip_atoi(const char **s)
 /* Formats correctly any integer in [0,99999].
  * Outputs from one to five digits depending on input.
  * On i386 gcc 4.1.2 -O2: ~250 bytes of code. */
-static char *put_dec_trunc(char *buf, unsigned q)
+static noinline_for_stack
+char *put_dec_trunc(char *buf, unsigned q)
 {
 	unsigned d3, d2, d1, d0;
 	d1 = (q>>4) & 0xf;
@@ -323,7 +325,8 @@ static char *put_dec_trunc(char *buf, unsigned q)
 	return buf;
 }
 /* Same with if's removed. Always emits five digits */
-static char *put_dec_full(char *buf, unsigned q)
+static noinline_for_stack
+char *put_dec_full(char *buf, unsigned q)
 {
 	/* BTW, if q is in [0,9999], 8-bit ints will be enough, */
 	/* but anyway, gcc produces better code with full-sized ints */
@@ -365,7 +368,8 @@ static char *put_dec_full(char *buf, unsigned q)
 	return buf;
 }
 /* No inlining helps gcc to use registers better */
-static noinline char *put_dec(char *buf, unsigned long long num)
+static noinline_for_stack
+char *put_dec(char *buf, unsigned long long num)
 {
 	while (1) {
 		unsigned rem;
@@ -416,8 +420,9 @@ struct printf_spec {
 	u8	qualifier;
 };
 
-static char *number(char *buf, char *end, unsigned long long num,
-			struct printf_spec spec)
+static noinline_for_stack
+char *number(char *buf, char *end, unsigned long long num,
+	     struct printf_spec spec)
 {
 	/* we are called with base 8, 10 or 16, only, thus don't need "G..."  */
 	static const char digits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */
@@ -536,7 +541,8 @@ static char *number(char *buf, char *end, unsigned long long num,
 	return buf;
 }
 
-static char *string(char *buf, char *end, const char *s, struct printf_spec spec)
+static noinline_for_stack
+char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 {
 	int len, i;
 
@@ -566,8 +572,9 @@ static char *string(char *buf, char *end, const char *s, struct printf_spec spec
 	return buf;
 }
 
-static char *symbol_string(char *buf, char *end, void *ptr,
-				struct printf_spec spec, char ext)
+static noinline_for_stack
+char *symbol_string(char *buf, char *end, void *ptr,
+		    struct printf_spec spec, char ext)
 {
 	unsigned long value = (unsigned long) ptr;
 #ifdef CONFIG_KALLSYMS
@@ -587,8 +594,9 @@ static char *symbol_string(char *buf, char *end, void *ptr,
 #endif
 }
 
-static char *resource_string(char *buf, char *end, struct resource *res,
-				struct printf_spec spec, const char *fmt)
+static noinline_for_stack
+char *resource_string(char *buf, char *end, struct resource *res,
+		      struct printf_spec spec, const char *fmt)
 {
 #ifndef IO_RSRC_PRINTK_SIZE
 #define IO_RSRC_PRINTK_SIZE	6
@@ -678,8 +686,9 @@ static char *resource_string(char *buf, char *end, struct resource *res,
 	return string(buf, end, sym, spec);
 }
 
-static char *mac_address_string(char *buf, char *end, u8 *addr,
-				struct printf_spec spec, const char *fmt)
+static noinline_for_stack
+char *mac_address_string(char *buf, char *end, u8 *addr,
+			 struct printf_spec spec, const char *fmt)
 {
 	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
 	char *p = mac_addr;
@@ -702,7 +711,8 @@ static char *mac_address_string(char *buf, char *end, u8 *addr,
 	return string(buf, end, mac_addr, spec);
 }
 
-static char *ip4_string(char *p, const u8 *addr, const char *fmt)
+static noinline_for_stack
+char *ip4_string(char *p, const u8 *addr, const char *fmt)
 {
 	int i;
 	bool leading_zeros = (fmt[0] == 'i');
@@ -751,7 +761,8 @@ static char *ip4_string(char *p, const u8 *addr, const char *fmt)
 	return p;
 }
 
-static char *ip6_compressed_string(char *p, const char *addr)
+static noinline_for_stack
+char *ip6_compressed_string(char *p, const char *addr)
 {
 	int i, j, range;
 	unsigned char zerolength[8];
@@ -831,7 +842,8 @@ static char *ip6_compressed_string(char *p, const char *addr)
 	return p;
 }
 
-static char *ip6_string(char *p, const char *addr, const char *fmt)
+static noinline_for_stack
+char *ip6_string(char *p, const char *addr, const char *fmt)
 {
 	int i;
 
@@ -846,8 +858,9 @@ static char *ip6_string(char *p, const char *addr, const char *fmt)
 	return p;
 }
 
-static char *ip6_addr_string(char *buf, char *end, const u8 *addr,
-			     struct printf_spec spec, const char *fmt)
+static noinline_for_stack
+char *ip6_addr_string(char *buf, char *end, const u8 *addr,
+		      struct printf_spec spec, const char *fmt)
 {
 	char ip6_addr[sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255")];
 
@@ -859,8 +872,9 @@ static char *ip6_addr_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, ip6_addr, spec);
 }
 
-static char *ip4_addr_string(char *buf, char *end, const u8 *addr,
-			     struct printf_spec spec, const char *fmt)
+static noinline_for_stack
+char *ip4_addr_string(char *buf, char *end, const u8 *addr,
+		      struct printf_spec spec, const char *fmt)
 {
 	char ip4_addr[sizeof("255.255.255.255")];
 
@@ -869,8 +883,9 @@ static char *ip4_addr_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, ip4_addr, spec);
 }
 
-static char *uuid_string(char *buf, char *end, const u8 *addr,
-			 struct printf_spec spec, const char *fmt)
+static noinline_for_stack
+char *uuid_string(char *buf, char *end, const u8 *addr,
+		  struct printf_spec spec, const char *fmt)
 {
 	char uuid[sizeof("xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx")];
 	char *p = uuid;
@@ -958,8 +973,9 @@ static char *uuid_string(char *buf, char *end, const u8 *addr,
  * function pointers are really function descriptors, which contain a
  * pointer to the real address.
  */
-static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
-			struct printf_spec spec)
+static noinline_for_stack
+char *pointer(const char *fmt, char *buf, char *end, void *ptr,
+	      struct printf_spec spec)
 {
 	if (!ptr)
 		return string(buf, end, "(null)", spec);
@@ -1028,7 +1044,8 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
  * @precision: precision of a number
  * @qualifier: qualifier of a number (long, size_t, ...)
  */
-static int format_decode(const char *fmt, struct printf_spec *spec)
+static noinline_for_stack
+int format_decode(const char *fmt, struct printf_spec *spec)
 {
 	const char *start = fmt;
 

Re: [PATCH] vsprintf.c: Use noinline_for_stack

[Joe Perches]

On Sat, 2010-03-06 at 18:33 -0800, Joe Perches wrote:
> On Sat, 2010-03-06 at 18:03 -0800, Linus Torvalds wrote:
> > A few noinlines might be appropriate.
> Mark static functions with noinline_for_stack

It's fine by me that the vsnprintf recursion and
(pr|dev|netdev)_<level> text reduction patches didn't make .34.

I'd like to know what you think necessary to get them into .35.

Perhaps it'd be useful if they could go into -next as-is for a
bit of wider testing.

http://patchwork.kernel.org/patch/83940/
http://patchwork.kernel.org/patch/83724/
http://patchwork.kernel.org/patch/83726/
http://patchwork.kernel.org/patch/83725/

Re: [PATCH] vsprintf.c: Use noinline_for_stack

[Andrew Morton]

On Sat, 06 Mar 2010 18:33:35 -0800
Joe Perches <joe@perches.com> wrote:

> On Sat, 2010-03-06 at 18:03 -0800, Linus Torvalds wrote:
> > A few noinlines might be appropriate.
> 
> Mark static functions with noinline_for_stack
> 

-ENOTESTINGRESULTS.



Before:

akpm:/usr/src/25> objdump -d lib/vsprintf.o | perl scripts/checkstack.pl
0x00000e82 pointer [vsprintf.o]:                        344
0x0000198c pointer [vsprintf.o]:                        344
0x000025d6 scnprintf [vsprintf.o]:                      216
0x00002648 scnprintf [vsprintf.o]:                      216
0x00002565 snprintf [vsprintf.o]:                       208
0x0000267c sprintf [vsprintf.o]:                        208
0x000030a3 bprintf [vsprintf.o]:                        208
0x00003b1e sscanf [vsprintf.o]:                         208
0x00000608 number [vsprintf.o]:                         136
0x00000937 number [vsprintf.o]:                         136

After:

akpm:/usr/src/25> objdump -d lib/vsprintf.o | perl scripts/checkstack.pl  
0x00000a7c symbol_string [vsprintf.o]:                  248
0x00000ae8 symbol_string [vsprintf.o]:                  248
0x00002310 scnprintf [vsprintf.o]:                      216
0x00002382 scnprintf [vsprintf.o]:                      216
0x0000229f snprintf [vsprintf.o]:                       208
0x000023b6 sprintf [vsprintf.o]:                        208
0x00002ddd bprintf [vsprintf.o]:                        208
0x00003858 sscanf [vsprintf.o]:                         208
0x00000625 number [vsprintf.o]:                         136
0x00000954 number [vsprintf.o]:                         136

nice.

Re: [PATCH] vsprintf.c: Use noinline_for_stack

[Linus Torvalds]

On Fri, 12 Mar 2010, Andrew Morton wrote:
> 
> -ENOTESTINGRESULTS.
> 
> Before:
> 
> akpm:/usr/src/25> objdump -d lib/vsprintf.o | perl scripts/checkstack.pl
> 0x00000e82 pointer [vsprintf.o]:                        344
> 0x0000198c pointer [vsprintf.o]:                        344
> 0x000025d6 scnprintf [vsprintf.o]:                      216
> 0x00002648 scnprintf [vsprintf.o]:                      216
> 0x00002565 snprintf [vsprintf.o]:                       208
> 0x0000267c sprintf [vsprintf.o]:                        208
> 0x000030a3 bprintf [vsprintf.o]:                        208
> 0x00003b1e sscanf [vsprintf.o]:                         208
> 0x00000608 number [vsprintf.o]:                         136
> 0x00000937 number [vsprintf.o]:                         136
> 
> After:
> 
> akpm:/usr/src/25> objdump -d lib/vsprintf.o | perl scripts/checkstack.pl  
> 0x00000a7c symbol_string [vsprintf.o]:                  248
> 0x00000ae8 symbol_string [vsprintf.o]:                  248
> 0x00002310 scnprintf [vsprintf.o]:                      216
> 0x00002382 scnprintf [vsprintf.o]:                      216
> 0x0000229f snprintf [vsprintf.o]:                       208
> 0x000023b6 sprintf [vsprintf.o]:                        208
> 0x00002ddd bprintf [vsprintf.o]:                        208
> 0x00003858 sscanf [vsprintf.o]:                         208
> 0x00000625 number [vsprintf.o]:                         136
> 0x00000954 number [vsprintf.o]:                         136
> 
> nice.

Note that the fact that the numbers are smaller is to some degree less 
important than _where_ the numbers are.

In the "before" side, it's the "pointer()" function that has a big stack 
depth. And the recursion that is going to happen is very much about 
vsnprintf -> pointer -> vsnprintf, so that is bad.

Now it's the new non-inlined leaf functions that still have a big stack 
footprint, and that's much better, because they wouldn't be part of any 
recursive behavior.

Not that I think it's wonderful even now. Especially that whole 
'symbol_string()' thing is not only a big stack user, it ends up calling 
down a fair number of other functions. Non-recursively, but still.

That, in turn, is due to this:

 - include/linux/kallsyms.h:
	#define KSYM_NAME_LEN 128
	#define KSYM_SYMBOL_LEN (sizeof("%s+%#lx/%#lx [%s]") + (KSYM_NAME_LEN - 1) + \

 - symbol_string():
	char sym[KSYM_SYMBOL_LEN];

ie we "need" about 150 bytes for just that silly symbol expansion (rounded 
up etc). Which is ridiculous, since we could/should limit it to something 
sane. But the kallsyms_lookup()/sprint_symbol() functions don't take a 
length parameter, so we have to do the worst-case thing (which itself has 
tons of unnecessary padding).

Gaah. We do _not_ want a kmalloc() or something like that in this path, 
since its' very much used for oopses (which in turn may be due to various 
slab bugs etc).

		Linus

Re: [PATCH] vsprintf.c: Use noinline_for_stack

[Joe Perches]

On Sat, 2010-03-13 at 07:35 -0800, Linus Torvalds wrote:
> On Fri, 12 Mar 2010, Andrew Morton wrote:
> > nice.
> But the kallsyms_lookup()/sprint_symbol() functions don't take a 
> length parameter, so we have to do the worst-case thing (which itself has 
> tons of unnecessary padding).

I sent a patch once about that using a struct
because I didn't like the unbounded sprint
http://lkml.org/lkml/2009/4/15/16

> Gaah. We do _not_ want a kmalloc() or something like that in this path, 
> since its' very much used for oopses (which in turn may be due to various 
> slab bugs etc).

Perhaps a new snprint_symbol function with the
other kallsyms... functions changed as necessary.

thoughts?

[PATCH] vsprintf.c: remove stack variable ksym from

[Joe Perches]

On Sat, 2010-03-13 at 09:44 -0800, Joe Perches wrote:
> On Sat, 2010-03-13 at 07:35 -0800, Linus Torvalds wrote:
> > On Fri, 12 Mar 2010, Andrew Morton wrote:
> > > nice.
> > But the kallsyms_lookup()/sprint_symbol() functions don't take a 
> > length parameter, so we have to do the worst-case thing (which itself has 
> > tons of unnecessary padding).
> Perhaps a new snprint_symbol function with the
> other kallsyms... functions changed as necessary.

Perhaps something like this:

Functions added:

kallsyms_lookup_n	length limited kallsyms_lookup
snprint_symbol		length limited sprint_symbol
module_address_lookup_n	length limited module_address_lookup

Changes to vsprintf.c, function symbol_string:

remove stack variable ksym, just use output buffer
and length limited functions.

Compiled, untested

Signed-off-by: Joe Perches
---
 include/linux/kallsyms.h |    7 ++++
 include/linux/module.h   |   15 ++++++++
 kernel/kallsyms.c        |   91 ++++++++++++++++++++++++++++++++++++++++------
 kernel/module.c          |   33 +++++++++++++++++
 lib/vsprintf.c           |   14 ++++---
 5 files changed, 143 insertions(+), 17 deletions(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index d8e9b3d..b1729b5 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -34,8 +34,15 @@ const char *kallsyms_lookup(unsigned long addr,
 			    unsigned long *offset,
 			    char **modname, char *namebuf);
 
+const char *kallsyms_lookup_n(unsigned long addr,
+			      unsigned long *symbolsize,
+			      unsigned long *offset,
+			      char **modname, char *namebuf, size_t size);
+
 /* Look up a kernel symbol and return it in a text buffer. */
 extern int sprint_symbol(char *buffer, unsigned long address);
+/* Look up a kernel symbol and return it in a text buffer. */
+extern int snprint_symbol(char *buffer, size_t size, unsigned long address);
 
 /* Look up a kernel symbol and print it to the kernel messages. */
 extern void __print_symbol(const char *fmt, unsigned long address);
diff --git a/include/linux/module.h b/include/linux/module.h
index 5e869ff..7a65fad 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -520,6 +520,11 @@ const char *module_address_lookup(unsigned long addr,
 			    unsigned long *offset,
 			    char **modname,
 			    char *namebuf);
+const char *module_address_lookup_n(unsigned long addr,
+				    unsigned long *symbolsize,
+				    unsigned long *offset,
+				    char **modname,
+				    char *namebuf, size_t size);
 int lookup_module_symbol_name(unsigned long addr, char *symname);
 int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);
 
@@ -598,6 +603,16 @@ static inline const char *module_address_lookup(unsigned long addr,
 	return NULL;
 }
 
+/* For kallsyms to ask for address resolution.  NULL means not found. */
+static inline const char *module_address_lookup_n(unsigned long addr,
+						  unsigned long *symbolsize,
+						  unsigned long *offset,
+						  char **modname,
+						  char *namebuf, size_t size)
+{
+	return NULL;
+}
+
 static inline int lookup_module_symbol_name(unsigned long addr, char *symname)
 {
 	return -ERANGE;
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 8e5288a..0516d23 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -84,9 +84,11 @@ static int is_ksym_addr(unsigned long addr)
  * Expand a compressed symbol data into the resulting uncompressed string,
  * given the offset to where the symbol is in the compressed stream.
  */
-static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
+static unsigned int kallsyms_expand_symbol(unsigned int off,
+					   char *result, size_t size)
 {
-	int len, skipped_first = 0;
+	int len;
+	bool skipped_first = false;
 	const u8 *tptr, *data;
 
 	/* Get the compressed symbol length from the first symbol byte. */
@@ -110,16 +112,18 @@ static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
 		len--;
 
 		while (*tptr) {
-			if (skipped_first) {
+			if (skipped_first && size > 1) {
 				*result = *tptr;
 				result++;
+				size--;
 			} else
-				skipped_first = 1;
+				skipped_first = true;
 			tptr++;
 		}
 	}
 
-	*result = '\0';
+	if (size)
+		*result = '\0';
 
 	/* Return to offset to the next symbol. */
 	return off;
@@ -174,7 +178,7 @@ unsigned long kallsyms_lookup_name(const char *name)
 	unsigned int off;
 
 	for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
-		off = kallsyms_expand_symbol(off, namebuf);
+		off = kallsyms_expand_symbol(off, namebuf, sizeof(namebuf));
 
 		if (strcmp(namebuf, name) == 0)
 			return kallsyms_addresses[i];
@@ -193,7 +197,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
 	int ret;
 
 	for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
-		off = kallsyms_expand_symbol(off, namebuf);
+		off = kallsyms_expand_symbol(off, namebuf, sizeof(namebuf));
 		ret = fn(data, namebuf, NULL, kallsyms_addresses[i]);
 		if (ret != 0)
 			return ret;
@@ -292,7 +296,8 @@ const char *kallsyms_lookup(unsigned long addr,
 
 		pos = get_symbol_pos(addr, symbolsize, offset);
 		/* Grab name */
-		kallsyms_expand_symbol(get_symbol_offset(pos), namebuf);
+		kallsyms_expand_symbol(get_symbol_offset(pos),
+				       namebuf, KSYM_NAME_LEN);
 		if (modname)
 			*modname = NULL;
 		return namebuf;
@@ -303,6 +308,38 @@ const char *kallsyms_lookup(unsigned long addr,
 				     namebuf);
 }
 
+/*
+ * Lookup an address (length_limited)
+ * - modname is set to NULL if it's in the kernel.
+ * - We guarantee that the returned name is valid until we reschedule even if.
+ *   It resides in a module.
+ * - We also guarantee that modname will be valid until rescheduled.
+ */
+const char *kallsyms_lookup_n(unsigned long addr,
+			      unsigned long *symbolsize,
+			      unsigned long *offset,
+			      char **modname, char *namebuf, size_t size)
+{
+	if (size)
+		namebuf[size - 1] = 0;
+	namebuf[0] = 0;
+
+	if (is_ksym_addr(addr)) {
+		unsigned long pos;
+
+		pos = get_symbol_pos(addr, symbolsize, offset);
+		/* Grab name */
+		kallsyms_expand_symbol(get_symbol_offset(pos), namebuf, size);
+		if (modname)
+			*modname = NULL;
+		return namebuf;
+	}
+
+	/* See if it's in a module. */
+	return module_address_lookup_n(addr, symbolsize, offset, modname,
+				       namebuf, size);
+}
+
 int lookup_symbol_name(unsigned long addr, char *symname)
 {
 	symname[0] = '\0';
@@ -313,7 +350,8 @@ int lookup_symbol_name(unsigned long addr, char *symname)
 
 		pos = get_symbol_pos(addr, NULL, NULL);
 		/* Grab name */
-		kallsyms_expand_symbol(get_symbol_offset(pos), symname);
+		kallsyms_expand_symbol(get_symbol_offset(pos),
+				       symname, KSYM_NAME_LEN);
 		return 0;
 	}
 	/* See if it's in a module. */
@@ -331,7 +369,8 @@ int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
 
 		pos = get_symbol_pos(addr, size, offset);
 		/* Grab name */
-		kallsyms_expand_symbol(get_symbol_offset(pos), name);
+		kallsyms_expand_symbol(get_symbol_offset(pos),
+				       name, KSYM_NAME_LEN);
 		modname[0] = '\0';
 		return 0;
 	}
@@ -340,6 +379,36 @@ int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
 }
 
 /* Look up a kernel symbol and return it in a text buffer. */
+int snprint_symbol(char *buffer, size_t size, unsigned long address)
+{
+	char *modname;
+	const char *name;
+	unsigned long offset, ksize;
+	int len;
+
+	name = kallsyms_lookup_n(address, &ksize, &offset, &modname,
+				 buffer, size);
+	if (!name)
+		return snprintf(buffer, size, "0x%lx", address);
+
+	if (name != buffer)
+		strncpy(buffer, name, size);
+	len = strlen(buffer);
+	buffer += len;
+
+	if (modname)
+		len += snprintf(buffer, size - len,
+				"+%#lx/%#lx [%s]", offset, ksize, modname);
+	else
+		len += snprintf(buffer, size - len,
+				"+%#lx/%#lx", offset, ksize);
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(snprint_symbol);
+
+
+/* Look up a kernel symbol and return it in a text buffer. */
 int sprint_symbol(char *buffer, unsigned long address)
 {
 	char *modname;
@@ -407,7 +476,7 @@ static unsigned long get_ksymbol_core(struct kallsym_iter *iter)
 
 	iter->type = kallsyms_get_symbol_type(off);
 
-	off = kallsyms_expand_symbol(off, iter->name);
+	off = kallsyms_expand_symbol(off, iter->name, sizeof(iter->name));
 
 	return off - iter->nameoff;
 }
diff --git a/kernel/module.c b/kernel/module.c
index c968d36..17a879a 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2607,6 +2607,39 @@ const char *module_address_lookup(unsigned long addr,
 	return ret;
 }
 
+/* For kallsyms to ask for address resolution.  NULL means not found.  Careful
+ * not to lock to avoid deadlock on oopses, simply disable preemption. */
+const char *module_address_lookup_n(unsigned long addr,
+				    unsigned long *ksize,
+				    unsigned long *offset,
+				    char **modname,
+				    char *namebuf, size_t size)
+{
+	struct module *mod;
+	const char *ret = NULL;
+
+	preempt_disable();
+	list_for_each_entry_rcu(mod, &modules, list) {
+		if (within_module_init(addr, mod) ||
+		    within_module_core(addr, mod)) {
+			if (modname)
+				*modname = mod->name;
+			ret = get_ksymbol(mod, addr, ksize, offset);
+			break;
+		}
+	}
+	/* Make a copy in here where it's safe */
+	if (ret) {
+		if (size) {
+			strncpy(namebuf, ret, size - 1);
+			namebuf[size - 1] = 0;
+		}
+		ret = namebuf;
+	}
+	preempt_enable();
+	return ret;
+}
+
 int lookup_module_symbol_name(unsigned long addr, char *symname)
 {
 	struct module *mod;
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 0d461c7..dab6354 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -571,13 +571,15 @@ static char *symbol_string(char *buf, char *end, void *ptr,
 {
 	unsigned long value = (unsigned long) ptr;
 #ifdef CONFIG_KALLSYMS
-	char sym[KSYM_SYMBOL_LEN];
-	if (ext != 'f' && ext != 's')
-		sprint_symbol(sym, value);
-	else
-		kallsyms_lookup(value, NULL, NULL, NULL, sym);
+	/* Cap ending position to spec.field_width if specified */
+	if (spec.field_width > 0 && (end - buf) > spec.field_width)
+		end = buf + spec.field_width;
 
-	return string(buf, end, sym, spec);
+	if (ext != 'f' && ext != 's')
+		return buf + snprint_symbol(buf, end - buf, value);
+		
+	kallsyms_lookup_n(value, NULL, NULL, NULL, buf, end - buf);
+	return buf + strlen(buf);
 #else
 	spec.field_width = 2 * sizeof(void *);
 	spec.flags |= SPECIAL | SMALL | ZEROPAD;

Re: [PATCH] vsprintf.c: remove stack variable ksym from

[Paulo Marques]

Joe Perches wrote:
> On Sat, 2010-03-13 at 09:44 -0800, Joe Perches wrote:
>> On Sat, 2010-03-13 at 07:35 -0800, Linus Torvalds wrote:
>>> On Fri, 12 Mar 2010, Andrew Morton wrote:
>>>> nice.
>>> But the kallsyms_lookup()/sprint_symbol() functions don't take a 
>>> length parameter, so we have to do the worst-case thing (which itself has 
>>> tons of unnecessary padding).
>> Perhaps a new snprint_symbol function with the
>> other kallsyms... functions changed as necessary.
> 
> Perhaps something like this:

Just one minor nit:

[...]
>  
> -	*result = '\0';
> +	if (size)
> +		*result = '\0';

This test seems to be here to handle the "size == 0" case, but

>[...]
> +const char *kallsyms_lookup_n(unsigned long addr,
> +			      unsigned long *symbolsize,
> +			      unsigned long *offset,
> +			      char **modname, char *namebuf, size_t size)
> +{
> +	if (size)
> +		namebuf[size - 1] = 0;
> +	namebuf[0] = 0;

here we seem to write the namebuf[0] even if "size == 0". So maybe both
assignments in this function should be inside the "if (size)" test.

Other than that, the patch looks good:

Reviewed-by: Paulo Marques <pmarques@grupopie.com>

-- 
Paulo Marques - www.grupopie.com

"As far as we know, our computer has never had an undetected error."
Weisert

Re: [PATCH] vsprintf.c: remove stack variable ksym from

[Paulo Marques]

Joe Perches wrote:
> On Sat, 2010-03-13 at 09:44 -0800, Joe Perches wrote:
>> On Sat, 2010-03-13 at 07:35 -0800, Linus Torvalds wrote:
>>> On Fri, 12 Mar 2010, Andrew Morton wrote:
>>>> nice.
>>> But the kallsyms_lookup()/sprint_symbol() functions don't take a 
>>> length parameter, so we have to do the worst-case thing (which itself has 
>>> tons of unnecessary padding).
>> Perhaps a new snprint_symbol function with the
>> other kallsyms... functions changed as necessary.
> 
> Perhaps something like this:

Just one minor nit:

[...]
>  
> -	*result = '\0';
> +	if (size)
> +		*result = '\0';

This test seems to be here to handle the "size == 0" case, but

>[...]
> +const char *kallsyms_lookup_n(unsigned long addr,
> +			      unsigned long *symbolsize,
> +			      unsigned long *offset,
> +			      char **modname, char *namebuf, size_t size)
> +{
> +	if (size)
> +		namebuf[size - 1] = 0;
> +	namebuf[0] = 0;

here we seem to write the namebuf[0] even if "size == 0". So maybe both
assignments in this function should be inside the "if (size)" test.

Other than that, the patch looks good:

Reviewed-by: Paulo Marques <pmarques@grupopie.com>

-- 
Paulo Marques - www.grupopie.com

"As far as we know, our computer has never had an undetected error."
Weisert