[PATCH 0/5] log: convert pr_*() to logging

[PATCH 0/5] log: convert pr_*() to logging

[Heinrich Schuchardt]

In drivers we use a family of printing functions including pr_err() and
pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
via printf().

Our logging functions allow finer grained control of output. So replace
printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
remains unchanged.

The first patch changes the k3-j721e RAM driver to avoid a duplicate use of
the macro name BIT_MASK().

The second patch ensures that debug_cond() can be used like a function.

The third patch provides macros for logging at levels EMERGENCY, ALERT,
CRITICAL and for message continuation.

The fourth patch uses the logging macros to implement the pr_*() macros.

The fifth patch provices a unit test.

Heinrich Schuchardt (5):
  ram: k3-j721e: rename BIT_MASK()
  log: make debug_cond() function like
  log: provide missing macros
  log: convert pr_*() to logging
  test: unit test for pr_err(), pr_cont()

 drivers/ram/k3-j721e/lpddr4.c         | 14 ++---
 drivers/ram/k3-j721e/lpddr4_private.h | 20 ++++---
 include/linux/bitops.h                |  4 +-
 include/linux/printk.h                | 82 +++++++++++++++------------
 include/log.h                         | 25 +++++---
 test/log/Makefile                     |  1 +
 test/log/pr_cont_test.c               | 45 +++++++++++++++
 7 files changed, 129 insertions(+), 62 deletions(-)
 create mode 100644 test/log/pr_cont_test.c

--
2.29.2

[PATCH 1/5] ram: k3-j721e: rename BIT_MASK()

[Heinrich Schuchardt]

The macro BIT_MASK is already defined in include/linux/bitops.h. To avoid
name collisions rename BIT_MASK() in drivers/ram/k3-j721e/lpddr4_private.h
to LPDDR4_BIT_MASK().

Remove superfluous parantheses.
Remove superfluous comparison to 0.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 drivers/ram/k3-j721e/lpddr4.c         | 14 +++++++-------
 drivers/ram/k3-j721e/lpddr4_private.h | 20 ++++++++++++--------
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/ram/k3-j721e/lpddr4.c b/drivers/ram/k3-j721e/lpddr4.c
index fc80fb1e2c..68043d7cb6 100644
--- a/drivers/ram/k3-j721e/lpddr4.c
+++ b/drivers/ram/k3-j721e/lpddr4.c
@@ -719,7 +719,7 @@ uint32_t lpddr4_checkctlinterrupt(const lpddr4_privatedata * pd,

 		/* MISRA compliance (Shifting operation) check */
 		if (fieldshift < WORD_SHIFT) {
-			if (((ctlirqstatus >> fieldshift) & BIT_MASK) > 0U) {
+			if ((ctlirqstatus >> fieldshift) & LPDDR4_BIT_MASK) {
 				*irqstatus = true;
 			} else {
 				*irqstatus = false;
@@ -746,11 +746,11 @@ uint32_t lpddr4_ackctlinterrupt(const lpddr4_privatedata * pd,
 		if (localinterrupt > WORD_SHIFT) {
 			localinterrupt =
 			    (localinterrupt - (uint32_t) WORD_SHIFT);
-			regval = ((uint32_t) BIT_MASK << localinterrupt);
+			regval = (uint32_t)LPDDR4_BIT_MASK << localinterrupt;
 			CPS_REG_WRITE(&(ctlregbase->LPDDR4__INT_ACK_1__REG),
 				      regval);
 		} else {
-			regval = ((uint32_t) BIT_MASK << localinterrupt);
+			regval = (uint32_t)LPDDR4_BIT_MASK << localinterrupt;
 			CPS_REG_WRITE(&(ctlregbase->LPDDR4__INT_ACK_0__REG),
 				      regval);
 		}
@@ -823,7 +823,7 @@ uint32_t lpddr4_checkphyindepinterrupt(const lpddr4_privatedata * pd,
 		phyindepirqstatus =
 		    CPS_REG_READ(&(ctlregbase->LPDDR4__PI_INT_STATUS__REG));
 		*irqstatus =
-		    (((phyindepirqstatus >> (uint32_t) intr) & BIT_MASK) > 0U);
+		    !!((phyindepirqstatus >> (uint32_t)intr) & LPDDR4_BIT_MASK);
 	}
 	return result;
 }
@@ -841,7 +841,7 @@ uint32_t lpddr4_ackphyindepinterrupt(const lpddr4_privatedata * pd,
 		lpddr4_ctlregs *ctlregbase = (lpddr4_ctlregs *) pd->ctlbase;

 		/* Write 1 to the requested bit to ACk the interrupt */
-		regval = ((uint32_t) BIT_MASK << ui32shiftinterrupt);
+		regval = (uint32_t)LPDDR4_BIT_MASK << ui32shiftinterrupt;
 		CPS_REG_WRITE(&(ctlregbase->LPDDR4__PI_INT_ACK__REG), regval);
 	}

@@ -894,7 +894,7 @@ static void lpddr4_checkwrlvlerror(lpddr4_ctlregs * ctlregbase,
 	    (volatile uint32_t
 	     *)(&(ctlregbase->LPDDR4__PHY_WRLVL_ERROR_OBS_0__REG));
 	/* PHY_WRLVL_ERROR_OBS_X[1:0] should be zero */
-	errbitmask = (BIT_MASK << 1) | (BIT_MASK);
+	errbitmask = (LPDDR4_BIT_MASK << 1) | LPDDR4_BIT_MASK;
 	for (snum = 0U; snum < DSLICE_NUM; snum++) {
 		regval = CPS_REG_READ(regaddress);
 		if ((regval & errbitmask) != 0U) {
@@ -1054,7 +1054,7 @@ static void lpddr4_seterrors(lpddr4_ctlregs * ctlregbase,
 			     lpddr4_debuginfo * debuginfo, bool * errfoundptr)
 {

-	uint32_t errbitmask = (BIT_MASK << 0x1U) | (BIT_MASK);
+	uint32_t errbitmask = (LPDDR4_BIT_MASK << 0x1U) | LPDDR4_BIT_MASK;
 	/* Check PLL observation registers for PLL lock errors */

 	debuginfo->pllerror =
diff --git a/drivers/ram/k3-j721e/lpddr4_private.h b/drivers/ram/k3-j721e/lpddr4_private.h
index 42c923464a..3d5017ea47 100644
--- a/drivers/ram/k3-j721e/lpddr4_private.h
+++ b/drivers/ram/k3-j721e/lpddr4_private.h
@@ -14,9 +14,9 @@
 #define VERSION_0  (0x54d5da40U)
 #define VERSION_1  (0xc1865a1U)

-#define BIT_MASK    (0x1U)
-#define BYTE_MASK   (0xffU)
-#define NIBBLE_MASK (0xfU)
+#define LPDDR4_BIT_MASK	(0x1U)
+#define BYTE_MASK	(0xffU)
+#define NIBBLE_MASK	(0xfU)

 #define WORD_SHIFT (32U)
 #define WORD_MASK (0xffffffffU)
@@ -46,11 +46,15 @@
 #define IO_CALIB_DONE ((uint32_t)0x1U << 23U)
 #define IO_CALIB_FIELD ((uint32_t)NIBBLE_MASK << 28U)
 #define IO_CALIB_STATE ((uint32_t)0xBU << 28U)
-#define RX_CAL_DONE ((uint32_t)BIT_MASK << 4U)
-#define CA_TRAIN_RL (((uint32_t)BIT_MASK << 5U) | ((uint32_t)BIT_MASK << 4U))
+#define RX_CAL_DONE ((uint32_t)LPDDR4_BIT_MASK << 4U)
+#define CA_TRAIN_RL (((uint32_t)LPDDR4_BIT_MASK << 5U) | \
+		     ((uint32_t)LPDDR4_BIT_MASK << 4U))
 #define WR_LVL_STATE (((uint32_t)NIBBLE_MASK) << 13U)
-#define GATE_LVL_ERROR_FIELDS (((uint32_t)BIT_MASK << 7U) | ((uint32_t)BIT_MASK << 6U))
-#define READ_LVL_ERROR_FIELDS ((((uint32_t)NIBBLE_MASK) << 28U) | (((uint32_t)BYTE_MASK) << 16U))
-#define DQ_LVL_STATUS (((uint32_t)BIT_MASK << 26U) | (((uint32_t)BYTE_MASK) << 18U))
+#define GATE_LVL_ERROR_FIELDS (((uint32_t)LPDDR4_BIT_MASK << 7U) | \
+			       ((uint32_t)LPDDR4_BIT_MASK << 6U))
+#define READ_LVL_ERROR_FIELDS ((((uint32_t)NIBBLE_MASK) << 28U) | \
+			       (((uint32_t)BYTE_MASK) << 16U))
+#define DQ_LVL_STATUS (((uint32_t)LPDDR4_BIT_MASK << 26U) | \
+		       (((uint32_t)BYTE_MASK) << 18U))

 #endif  /* LPDDR4_PRIV_H */
--
2.29.2

[PATCH 1/5] ram: k3-j721e: rename BIT_MASK()

[Simon Glass]

On Mon, 4 Jan 2021 at 00:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> The macro BIT_MASK is already defined in include/linux/bitops.h. To avoid
> name collisions rename BIT_MASK() in drivers/ram/k3-j721e/lpddr4_private.h
> to LPDDR4_BIT_MASK().
>
> Remove superfluous parantheses.
> Remove superfluous comparison to 0.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/ram/k3-j721e/lpddr4.c         | 14 +++++++-------
>  drivers/ram/k3-j721e/lpddr4_private.h | 20 ++++++++++++--------
>  2 files changed, 19 insertions(+), 15 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 1/5] ram: k3-j721e: rename BIT_MASK()

[Tom Rini]

On Mon, Jan 04, 2021 at 08:02:52AM +0100, Heinrich Schuchardt wrote:

> The macro BIT_MASK is already defined in include/linux/bitops.h. To avoid
> name collisions rename BIT_MASK() in drivers/ram/k3-j721e/lpddr4_private.h
> to LPDDR4_BIT_MASK().
> 
> Remove superfluous parantheses.
> Remove superfluous comparison to 0.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210118/f26c83f6/attachment.sig>

[PATCH 2/5] log: make debug_cond() function like

[Heinrich Schuchardt]

Change debug_cond() such that it can be used instead of a function like
debug().

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/log.h | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/include/log.h b/include/log.h
index 29f18a82dc..34cea802a1 100644
--- a/include/log.h
+++ b/include/log.h
@@ -217,10 +217,9 @@ static inline int _log_nop(enum log_category_t cat, enum log_level_t level,
 #if !_DEBUG && CONFIG_IS_ENABLED(LOG)

 #define debug_cond(cond, fmt, args...)			\
-	do {						\
-		if (1)					\
-			log(LOG_CATEGORY, LOGL_DEBUG, fmt, ##args); \
-	} while (0)
+({							\
+	log(LOG_CATEGORY, LOGL_DEBUG, fmt, ##args);	\
+})

 #else /* _DEBUG */

@@ -229,11 +228,11 @@ static inline int _log_nop(enum log_category_t cat, enum log_level_t level,
  * computed by a preprocessor in the best case, allowing for the best
  * optimization.
  */
-#define debug_cond(cond, fmt, args...)			\
-	do {						\
-		if (cond)				\
-			printf(pr_fmt(fmt), ##args);	\
-	} while (0)
+#define debug_cond(cond, fmt, args...)		\
+({						\
+	if (cond)				\
+		printf(pr_fmt(fmt), ##args);	\
+})

 #endif /* _DEBUG */

--
2.29.2

[PATCH 2/5] log: make debug_cond() function like

[Simon Glass]

On Mon, 4 Jan 2021 at 00:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Change debug_cond() such that it can be used instead of a function like
> debug().
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/log.h | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 2/5] log: make debug_cond() function like

[Tom Rini]

On Mon, Jan 04, 2021 at 08:02:53AM +0100, Heinrich Schuchardt wrote:

> Change debug_cond() such that it can be used instead of a function like
> debug().
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210118/b11f801d/attachment.sig>

[PATCH 3/5] log: provide missing macros

[Heinrich Schuchardt]

With commit d094a0734cee ("log: allow for message continuation") we have
defined a special log level and category for message continuation. Let's
have a macro for using these.

If logging is enabled log_cont() will create a continuation log output with
the same logging level and category as the previous message.

If logging is not enabled, log_cont() will print like printf().

Provide macros for logging levels LOG_EMERG, LOG_ALERT, LOG_CRIT.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/log.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/log.h b/include/log.h
index 34cea802a1..830707ef3d 100644
--- a/include/log.h
+++ b/include/log.h
@@ -156,6 +156,9 @@ static inline int _log_nop(enum log_category_t cat, enum log_level_t level,
  */
 #if CONFIG_IS_ENABLED(LOG)
 #define _LOG_MAX_LEVEL CONFIG_VAL(LOG_MAX_LEVEL)
+#define log_emer(_fmt...)	log(LOG_CATEGORY, LOGL_EMERG, ##_fmt)
+#define log_alert(_fmt...)	log(LOG_CATEGORY, LOGL_ALERT, ##_fmt)
+#define log_crit(_fmt...)	log(LOG_CATEGORY, LOGL_CRIT, ##_fmt)
 #define log_err(_fmt...)	log(LOG_CATEGORY, LOGL_ERR, ##_fmt)
 #define log_warning(_fmt...)	log(LOG_CATEGORY, LOGL_WARNING, ##_fmt)
 #define log_notice(_fmt...)	log(LOG_CATEGORY, LOGL_NOTICE, ##_fmt)
@@ -163,12 +166,17 @@ static inline int _log_nop(enum log_category_t cat, enum log_level_t level,
 #define log_debug(_fmt...)	log(LOG_CATEGORY, LOGL_DEBUG, ##_fmt)
 #define log_content(_fmt...)	log(LOG_CATEGORY, LOGL_DEBUG_CONTENT, ##_fmt)
 #define log_io(_fmt...)		log(LOG_CATEGORY, LOGL_DEBUG_IO, ##_fmt)
+#define log_cont(_fmt...)	log(LOGC_CONT, LOGL_CONT, ##_fmt)
 #else
 #define _LOG_MAX_LEVEL LOGL_INFO
+#define log_emerg(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
+#define log_alert(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
+#define log_crit(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
 #define log_err(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
 #define log_warning(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
 #define log_notice(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
 #define log_info(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
+#define log_cont(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
 #define log_debug(_fmt, ...)	debug(_fmt, ##__VA_ARGS__)
 #define log_content(_fmt...)	log_nop(LOG_CATEGORY, \
 					LOGL_DEBUG_CONTENT, ##_fmt)
--
2.29.2

[PATCH 3/5] log: provide missing macros

[Simon Glass]

On Mon, 4 Jan 2021 at 00:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> With commit d094a0734cee ("log: allow for message continuation") we have
> defined a special log level and category for message continuation. Let's
> have a macro for using these.
>
> If logging is enabled log_cont() will create a continuation log output with
> the same logging level and category as the previous message.
>
> If logging is not enabled, log_cont() will print like printf().
>
> Provide macros for logging levels LOG_EMERG, LOG_ALERT, LOG_CRIT.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/log.h | 8 ++++++++
>  1 file changed, 8 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 3/5] log: provide missing macros

[Tom Rini]

On Mon, Jan 04, 2021 at 08:02:54AM +0100, Heinrich Schuchardt wrote:

> With commit d094a0734cee ("log: allow for message continuation") we have
> defined a special log level and category for message continuation. Let's
> have a macro for using these.
> 
> If logging is enabled log_cont() will create a continuation log output with
> the same logging level and category as the previous message.
> 
> If logging is not enabled, log_cont() will print like printf().
> 
> Provide macros for logging levels LOG_EMERG, LOG_ALERT, LOG_CRIT.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210118/2ee5fcc6/attachment.sig>

[PATCH 4/5] log: convert pr_*() to logging

[Heinrich Schuchardt]

In drivers we use a family of printing functions including pr_err() and
pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
via printf().

Our logging functions allow finer grained control of output. So replace
printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
remains unchanged.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/linux/bitops.h |  4 ++-
 include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
 2 files changed, 48 insertions(+), 38 deletions(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 16f28993f5..d2e5ca026e 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -6,7 +6,6 @@
 #include <asm/types.h>
 #include <asm-generic/bitsperlong.h>
 #include <linux/compiler.h>
-#include <linux/kernel.h>

 #ifdef	__KERNEL__
 #define BIT(nr)			(1UL << (nr))
@@ -19,6 +18,9 @@
 #define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
 #endif

+/* kernel.h includes log.h which include bitops.h */
+#include <linux/kernel.h>
+
 /*
  * Create a contiguous bitmask starting at bit position @l and ending at
  * position @h. For example
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 088513ad29..5e85513853 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -1,6 +1,7 @@
 #ifndef __KERNEL_PRINTK__
 #define __KERNEL_PRINTK__

+#include <log.h>
 #include <stdio.h>
 #include <linux/compiler.h>

@@ -28,49 +29,56 @@
 	0;						\
 })

-#define __printk(level, fmt, ...)					\
-({									\
-	level < CONFIG_LOGLEVEL ? printk(fmt, ##__VA_ARGS__) : 0;	\
-})
-
 #ifndef pr_fmt
 #define pr_fmt(fmt) fmt
 #endif

-#define pr_emerg(fmt, ...) \
-	__printk(0, pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_alert(fmt, ...) \
-	__printk(1, pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_crit(fmt, ...) \
-	__printk(2, pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_err(fmt, ...) \
-	__printk(3, pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_warning(fmt, ...) \
-	__printk(4, pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_warn pr_warning
-#define pr_notice(fmt, ...) \
-	__printk(5, pr_fmt(fmt), ##__VA_ARGS__)
-#define pr_info(fmt, ...) \
-	__printk(6, pr_fmt(fmt), ##__VA_ARGS__)
-
-#define pr_cont(fmt, ...) \
-	printk(fmt, ##__VA_ARGS__)
-
-/* pr_devel() should produce zero code unless DEBUG is defined */
-#ifdef DEBUG
-#define pr_devel(fmt, ...) \
-	__printk(7, pr_fmt(fmt), ##__VA_ARGS__)
-#else
-#define pr_devel(fmt, ...) \
-	no_printk(pr_fmt(fmt), ##__VA_ARGS__)
-#endif
+#define pr_emerg(fmt, ...)						\
+({									\
+	CONFIG_LOGLEVEL > 0 ? log_emerg(fmt, ##__VA_ARGS__) : 0;	\
+})
+#define pr_alert(fmt, ...)						\
+({									\
+	CONFIG_LOGLEVEL > 1 ? log_alert(fmt, ##__VA_ARGS__) : 0;	\
+})
+#define pr_crit(fmt, ...)						\
+({									\
+	CONFIG_LOGLEVEL > 2 ? log_crit(fmt, ##__VA_ARGS__) : 0;		\
+})
+#define pr_err(fmt, ...)						\
+({									\
+	CONFIG_LOGLEVEL > 3 ? log_err(fmt, ##__VA_ARGS__) : 0;		\
+})
+#define pr_warn(fmt, ...)						\
+({									\
+	CONFIG_LOGLEVEL > 4 ? log_warning(fmt, ##__VA_ARGS__) : 0;	\
+})
+#define pr_notice(fmt, ...)						\
+({									\
+	CONFIG_LOGLEVEL > 5 ? log_notice(fmt, ##__VA_ARGS__) : 0;	\
+})
+#define pr_info(fmt, ...)						\
+({									\
+	CONFIG_LOGLEVEL > 6 ? log_info(fmt, ##__VA_ARGS__) : 0;		\
+})
+#define pr_debug(fmt, ...)						\
+({									\
+	CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;	\
+})
+#define pr_devel(fmt, ...)						\
+({									\
+	CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;	\
+})

-#ifdef DEBUG
-#define pr_debug(fmt, ...) \
-	__printk(7, pr_fmt(fmt), ##__VA_ARGS__)
+#ifdef CONFIG_LOG
+#define pr_cont(fmt, ...)						\
+({									\
+	gd->logl_prev < CONFIG_LOGLEVEL ?				\
+		log_cont(fmt, ##__VA_ARGS__) : 0;			\
+})
 #else
-#define pr_debug(fmt, ...) \
-	no_printk(pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_cont(fmt, ...)						\
+	printk(fmt, ##__VA_ARGS__)
 #endif

 #define printk_once(fmt, ...) \
--
2.29.2

[PATCH 4/5] log: convert pr_*() to logging

[Simon Glass]

Hi Heinrich,

On Mon, 4 Jan 2021 at 00:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> In drivers we use a family of printing functions including pr_err() and
> pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
> via printf().
>
> Our logging functions allow finer grained control of output. So replace
> printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
> remains unchanged.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/linux/bitops.h |  4 ++-
>  include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
>  2 files changed, 48 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index 16f28993f5..d2e5ca026e 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -6,7 +6,6 @@
>  #include <asm/types.h>
>  #include <asm-generic/bitsperlong.h>
>  #include <linux/compiler.h>
> -#include <linux/kernel.h>
>
>  #ifdef __KERNEL__
>  #define BIT(nr)                        (1UL << (nr))
> @@ -19,6 +18,9 @@
>  #define BITS_TO_LONGS(nr)      DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
>  #endif
>
> +/* kernel.h includes log.h which include bitops.h */
> +#include <linux/kernel.h>
> +
>  /*
>   * Create a contiguous bitmask starting at bit position @l and ending at
>   * position @h. For example
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 088513ad29..5e85513853 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -1,6 +1,7 @@
>  #ifndef __KERNEL_PRINTK__
>  #define __KERNEL_PRINTK__
>
> +#include <log.h>
>  #include <stdio.h>
>  #include <linux/compiler.h>
>
> @@ -28,49 +29,56 @@
>         0;                                              \
>  })
>
> -#define __printk(level, fmt, ...)                                      \
> -({                                                                     \
> -       level < CONFIG_LOGLEVEL ? printk(fmt, ##__VA_ARGS__) : 0;       \
> -})
> -
>  #ifndef pr_fmt
>  #define pr_fmt(fmt) fmt
>  #endif
>
> -#define pr_emerg(fmt, ...) \
> -       __printk(0, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_alert(fmt, ...) \
> -       __printk(1, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_crit(fmt, ...) \
> -       __printk(2, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_err(fmt, ...) \
> -       __printk(3, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_warning(fmt, ...) \
> -       __printk(4, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_warn pr_warning
> -#define pr_notice(fmt, ...) \
> -       __printk(5, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_info(fmt, ...) \
> -       __printk(6, pr_fmt(fmt), ##__VA_ARGS__)
> -
> -#define pr_cont(fmt, ...) \
> -       printk(fmt, ##__VA_ARGS__)
> -
> -/* pr_devel() should produce zero code unless DEBUG is defined */
> -#ifdef DEBUG
> -#define pr_devel(fmt, ...) \
> -       __printk(7, pr_fmt(fmt), ##__VA_ARGS__)
> -#else
> -#define pr_devel(fmt, ...) \
> -       no_printk(pr_fmt(fmt), ##__VA_ARGS__)
> -#endif
> +#define pr_emerg(fmt, ...)                                             \
> +({                                                                     \
> +       CONFIG_LOGLEVEL > 0 ? log_emerg(fmt, ##__VA_ARGS__) : 0;        \

Should this use CONFIG_VAL(LOGLEVEL) ?

Also I don't suppose there is a way to implement this in the __printk() macro?

> +})
> +#define pr_alert(fmt, ...)                                             \
> +({                                                                     \
> +       CONFIG_LOGLEVEL > 1 ? log_alert(fmt, ##__VA_ARGS__) : 0;        \
> +})
> +#define pr_crit(fmt, ...)                                              \
> +({                                                                     \
> +       CONFIG_LOGLEVEL > 2 ? log_crit(fmt, ##__VA_ARGS__) : 0;         \
> +})
> +#define pr_err(fmt, ...)                                               \
> +({                                                                     \
> +       CONFIG_LOGLEVEL > 3 ? log_err(fmt, ##__VA_ARGS__) : 0;          \
> +})
> +#define pr_warn(fmt, ...)                                              \
> +({                                                                     \
> +       CONFIG_LOGLEVEL > 4 ? log_warning(fmt, ##__VA_ARGS__) : 0;      \
> +})
> +#define pr_notice(fmt, ...)                                            \
> +({                                                                     \
> +       CONFIG_LOGLEVEL > 5 ? log_notice(fmt, ##__VA_ARGS__) : 0;       \
> +})
> +#define pr_info(fmt, ...)                                              \
> +({                                                                     \
> +       CONFIG_LOGLEVEL > 6 ? log_info(fmt, ##__VA_ARGS__) : 0;         \
> +})
> +#define pr_debug(fmt, ...)                                             \
> +({                                                                     \
> +       CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;        \
> +})
> +#define pr_devel(fmt, ...)                                             \
> +({                                                                     \
> +       CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;        \
> +})
>
> -#ifdef DEBUG
> -#define pr_debug(fmt, ...) \
> -       __printk(7, pr_fmt(fmt), ##__VA_ARGS__)
> +#ifdef CONFIG_LOG
> +#define pr_cont(fmt, ...)                                              \
> +({                                                                     \
> +       gd->logl_prev < CONFIG_LOGLEVEL ?                               \
> +               log_cont(fmt, ##__VA_ARGS__) : 0;                       \
> +})
>  #else
> -#define pr_debug(fmt, ...) \
> -       no_printk(pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_cont(fmt, ...)                                              \
> +       printk(fmt, ##__VA_ARGS__)
>  #endif
>
>  #define printk_once(fmt, ...) \
> --
> 2.29.2
>

Regards,
Simon

[PATCH 4/5] log: convert pr_*() to logging

[Tom Rini]

On Mon, Jan 04, 2021 at 08:02:55AM +0100, Heinrich Schuchardt wrote:

> In drivers we use a family of printing functions including pr_err() and
> pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
> via printf().
> 
> Our logging functions allow finer grained control of output. So replace
> printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
> remains unchanged.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/linux/bitops.h |  4 ++-
>  include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
>  2 files changed, 48 insertions(+), 38 deletions(-)

This causes some fairly massive growth in various subsystems such as ubi
and we might want to look at what, if anything, we can do, before
growing some platforms by 15KiB (xilinx_zynqmp_virt) due to strings.
Thanks.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210116/b35a374b/attachment.sig>

[PATCH 4/5] log: convert pr_*() to logging

[Heinrich Schuchardt]

On 1/17/21 1:16 AM, Tom Rini wrote:
> On Mon, Jan 04, 2021 at 08:02:55AM +0100, Heinrich Schuchardt wrote:
>
>> In drivers we use a family of printing functions including pr_err() and
>> pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
>> via printf().
>>
>> Our logging functions allow finer grained control of output. So replace
>> printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
>> remains unchanged.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   include/linux/bitops.h |  4 ++-
>>   include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
>>   2 files changed, 48 insertions(+), 38 deletions(-)
>
> This causes some fairly massive growth in various subsystems such as ubi
> and we might want to look at what, if anything, we can do, before
> growing some platforms by 15KiB (xilinx_zynqmp_virt) due to strings.

xilinx_zynqmp_virt has CONFIG_LOG enabled. Switching from printf() to
log() incurs size growth. Did you observe a size grows on platforms with
CONFIG_LOG=n?

Best regards

Heinrich

> Thanks.
>

[PATCH 4/5] log: convert pr_*() to logging

[Tom Rini]

On Sun, Jan 17, 2021 at 08:37:15AM +0100, Heinrich Schuchardt wrote:
> On 1/17/21 1:16 AM, Tom Rini wrote:
> > On Mon, Jan 04, 2021 at 08:02:55AM +0100, Heinrich Schuchardt wrote:
> > 
> > > In drivers we use a family of printing functions including pr_err() and
> > > pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
> > > via printf().
> > > 
> > > Our logging functions allow finer grained control of output. So replace
> > > printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
> > > remains unchanged.
> > > 
> > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > ---
> > >   include/linux/bitops.h |  4 ++-
> > >   include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
> > >   2 files changed, 48 insertions(+), 38 deletions(-)
> > 
> > This causes some fairly massive growth in various subsystems such as ubi
> > and we might want to look at what, if anything, we can do, before
> > growing some platforms by 15KiB (xilinx_zynqmp_virt) due to strings.
> 
> xilinx_zynqmp_virt has CONFIG_LOG enabled. Switching from printf() to
> log() incurs size growth. Did you observe a size grows on platforms with
> CONFIG_LOG=n?

Yes, it has logging enabled, and we're converting a large number of
things that were before compile-time discarded to no longer be so.  This
is, in general, good and what I've asked for.  But when seeing very
large growth in doing so, I think we need to maybe take a step back and
look at the UBI subsystem for example and see if we can't/shouldn't
tweak things more.

So, I'm going to run a size test with just this patch as the change, so
we can have more concrete numbers to look at.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210118/c297eb6e/attachment.sig>

[PATCH 4/5] log: convert pr_*() to logging

[Tom Rini]

On Mon, Jan 18, 2021 at 08:02:41AM -0500, Tom Rini wrote:
> On Sun, Jan 17, 2021 at 08:37:15AM +0100, Heinrich Schuchardt wrote:
> > On 1/17/21 1:16 AM, Tom Rini wrote:
> > > On Mon, Jan 04, 2021 at 08:02:55AM +0100, Heinrich Schuchardt wrote:
> > > 
> > > > In drivers we use a family of printing functions including pr_err() and
> > > > pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
> > > > via printf().
> > > > 
> > > > Our logging functions allow finer grained control of output. So replace
> > > > printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
> > > > remains unchanged.
> > > > 
> > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > ---
> > > >   include/linux/bitops.h |  4 ++-
> > > >   include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
> > > >   2 files changed, 48 insertions(+), 38 deletions(-)
> > > 
> > > This causes some fairly massive growth in various subsystems such as ubi
> > > and we might want to look at what, if anything, we can do, before
> > > growing some platforms by 15KiB (xilinx_zynqmp_virt) due to strings.
> > 
> > xilinx_zynqmp_virt has CONFIG_LOG enabled. Switching from printf() to
> > log() incurs size growth. Did you observe a size grows on platforms with
> > CONFIG_LOG=n?
> 
> Yes, it has logging enabled, and we're converting a large number of
> things that were before compile-time discarded to no longer be so.  This
> is, in general, good and what I've asked for.  But when seeing very
> large growth in doing so, I think we need to maybe take a step back and
> look at the UBI subsystem for example and see if we can't/shouldn't
> tweak things more.
> 
> So, I'm going to run a size test with just this patch as the change, so
> we can have more concrete numbers to look at.

OK, so the build is done and interesting output starts at:
https://gist.github.com/trini/53e7da62c6c9d18c189e6baffd01ff00#file-2021-01-18-0800-txt-L239

Most of the time we're well under 1KiB, which is great.  UBI has some
huge growth, but it's a very few platforms, and I've added the
custodians here so they can object, or not, to such size growth.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210118/c35d6714/attachment.sig>

[PATCH 4/5] log: convert pr_*() to logging

[Heinrich Schuchardt]

On 18.01.21 16:30, Tom Rini wrote:
> On Mon, Jan 18, 2021 at 08:02:41AM -0500, Tom Rini wrote:
>> On Sun, Jan 17, 2021 at 08:37:15AM +0100, Heinrich Schuchardt wrote:
>>> On 1/17/21 1:16 AM, Tom Rini wrote:
>>>> On Mon, Jan 04, 2021 at 08:02:55AM +0100, Heinrich Schuchardt wrote:
>>>>
>>>>> In drivers we use a family of printing functions including pr_err() and
>>>>> pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
>>>>> via printf().
>>>>>
>>>>> Our logging functions allow finer grained control of output. So replace
>>>>> printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
>>>>> remains unchanged.
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> ---
>>>>>   include/linux/bitops.h |  4 ++-
>>>>>   include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
>>>>>   2 files changed, 48 insertions(+), 38 deletions(-)
>>>>
>>>> This causes some fairly massive growth in various subsystems such as ubi
>>>> and we might want to look at what, if anything, we can do, before
>>>> growing some platforms by 15KiB (xilinx_zynqmp_virt) due to strings.
>>>
>>> xilinx_zynqmp_virt has CONFIG_LOG enabled. Switching from printf() to
>>> log() incurs size growth. Did you observe a size grows on platforms with
>>> CONFIG_LOG=n?
>>
>> Yes, it has logging enabled, and we're converting a large number of
>> things that were before compile-time discarded to no longer be so.  This
>> is, in general, good and what I've asked for.  But when seeing very
>> large growth in doing so, I think we need to maybe take a step back and
>> look at the UBI subsystem for example and see if we can't/shouldn't
>> tweak things more.
>>
>> So, I'm going to run a size test with just this patch as the change, so
>> we can have more concrete numbers to look at.
>
> OK, so the build is done and interesting output starts at:
> https://gist.github.com/trini/53e7da62c6c9d18c189e6baffd01ff00#file-2021-01-18-0800-txt-L239
>
> Most of the time we're well under 1KiB, which is great.  UBI has some
> huge growth, but it's a very few platforms, and I've added the
> custodians here so they can object, or not, to such size growth.
>

Hello Tom,

How shall we proceed?

Best regards

Heinrich

[PATCH 4/5] log: convert pr_*() to logging

[Tom Rini]

On Thu, Feb 18, 2021 at 10:16:58AM +0100, Heinrich Schuchardt wrote:
> On 18.01.21 16:30, Tom Rini wrote:
> > On Mon, Jan 18, 2021 at 08:02:41AM -0500, Tom Rini wrote:
> >> On Sun, Jan 17, 2021 at 08:37:15AM +0100, Heinrich Schuchardt wrote:
> >>> On 1/17/21 1:16 AM, Tom Rini wrote:
> >>>> On Mon, Jan 04, 2021 at 08:02:55AM +0100, Heinrich Schuchardt wrote:
> >>>>
> >>>>> In drivers we use a family of printing functions including pr_err() and
> >>>>> pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
> >>>>> via printf().
> >>>>>
> >>>>> Our logging functions allow finer grained control of output. So replace
> >>>>> printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
> >>>>> remains unchanged.
> >>>>>
> >>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>> ---
> >>>>>   include/linux/bitops.h |  4 ++-
> >>>>>   include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
> >>>>>   2 files changed, 48 insertions(+), 38 deletions(-)
> >>>>
> >>>> This causes some fairly massive growth in various subsystems such as ubi
> >>>> and we might want to look at what, if anything, we can do, before
> >>>> growing some platforms by 15KiB (xilinx_zynqmp_virt) due to strings.
> >>>
> >>> xilinx_zynqmp_virt has CONFIG_LOG enabled. Switching from printf() to
> >>> log() incurs size growth. Did you observe a size grows on platforms with
> >>> CONFIG_LOG=n?
> >>
> >> Yes, it has logging enabled, and we're converting a large number of
> >> things that were before compile-time discarded to no longer be so.  This
> >> is, in general, good and what I've asked for.  But when seeing very
> >> large growth in doing so, I think we need to maybe take a step back and
> >> look at the UBI subsystem for example and see if we can't/shouldn't
> >> tweak things more.
> >>
> >> So, I'm going to run a size test with just this patch as the change, so
> >> we can have more concrete numbers to look at.
> >
> > OK, so the build is done and interesting output starts at:
> > https://gist.github.com/trini/53e7da62c6c9d18c189e6baffd01ff00#file-2021-01-18-0800-txt-L239
> >
> > Most of the time we're well under 1KiB, which is great.  UBI has some
> > huge growth, but it's a very few platforms, and I've added the
> > custodians here so they can object, or not, to such size growth.
> >
> 
> Hello Tom,
> 
> How shall we proceed?

Michal? Patrice or Patrick?  Since this patch causes noticeable growth
on your platforms I'm waiting for your input here, to be clear.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210218/3498a918/attachment.sig>

[PATCH 4/5] log: convert pr_*() to logging

[Patrice CHOTARD]

Hi Tom

On 2/18/21 2:05 PM, Tom Rini wrote:
> On Thu, Feb 18, 2021 at 10:16:58AM +0100, Heinrich Schuchardt wrote:
>> On 18.01.21 16:30, Tom Rini wrote:
>>> On Mon, Jan 18, 2021 at 08:02:41AM -0500, Tom Rini wrote:
>>>> On Sun, Jan 17, 2021 at 08:37:15AM +0100, Heinrich Schuchardt wrote:
>>>>> On 1/17/21 1:16 AM, Tom Rini wrote:
>>>>>> On Mon, Jan 04, 2021 at 08:02:55AM +0100, Heinrich Schuchardt wrote:
>>>>>>
>>>>>>> In drivers we use a family of printing functions including pr_err() and
>>>>>>> pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
>>>>>>> via printf().
>>>>>>>
>>>>>>> Our logging functions allow finer grained control of output. So replace
>>>>>>> printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
>>>>>>> remains unchanged.
>>>>>>>
>>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>> ---
>>>>>>>   include/linux/bitops.h |  4 ++-
>>>>>>>   include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
>>>>>>>   2 files changed, 48 insertions(+), 38 deletions(-)
>>>>>>
>>>>>> This causes some fairly massive growth in various subsystems such as ubi
>>>>>> and we might want to look at what, if anything, we can do, before
>>>>>> growing some platforms by 15KiB (xilinx_zynqmp_virt) due to strings.
>>>>>
>>>>> xilinx_zynqmp_virt has CONFIG_LOG enabled. Switching from printf() to
>>>>> log() incurs size growth. Did you observe a size grows on platforms with
>>>>> CONFIG_LOG=n?
>>>>
>>>> Yes, it has logging enabled, and we're converting a large number of
>>>> things that were before compile-time discarded to no longer be so.  This
>>>> is, in general, good and what I've asked for.  But when seeing very
>>>> large growth in doing so, I think we need to maybe take a step back and
>>>> look at the UBI subsystem for example and see if we can't/shouldn't
>>>> tweak things more.
>>>>
>>>> So, I'm going to run a size test with just this patch as the change, so
>>>> we can have more concrete numbers to look at.
>>>
>>> OK, so the build is done and interesting output starts at:
>>> https://gist.github.com/trini/53e7da62c6c9d18c189e6baffd01ff00#file-2021-01-18-0800-txt-L239
>>>
>>> Most of the time we're well under 1KiB, which is great.  UBI has some
>>> huge growth, but it's a very few platforms, and I've added the
>>> custodians here so they can object, or not, to such size growth.
>>>
>>
>> Hello Tom,
>>
>> How shall we proceed?
> 
> Michal? Patrice or Patrick?  Since this patch causes noticeable growth
> on your platforms I'm waiting for your input here, to be clear.  Thanks!
> 

No objection for stm32 platforms, ok for us ;-)

Patrice

[PATCH 4/5] log: convert pr_*() to logging

[Sean Anderson]

On 1/4/21 2:02 AM, Heinrich Schuchardt wrote:
> In drivers we use a family of printing functions including pr_err() and
> pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
> via printf().
> 
> Our logging functions allow finer grained control of output. So replace
> printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
> remains unchanged.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   include/linux/bitops.h |  4 ++-
>   include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
>   2 files changed, 48 insertions(+), 38 deletions(-)
> 
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index 16f28993f5..d2e5ca026e 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -6,7 +6,6 @@
>   #include <asm/types.h>
>   #include <asm-generic/bitsperlong.h>
>   #include <linux/compiler.h>
> -#include <linux/kernel.h>
> 
>   #ifdef	__KERNEL__
>   #define BIT(nr)			(1UL << (nr))
> @@ -19,6 +18,9 @@
>   #define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
>   #endif
> 
> +/* kernel.h includes log.h which include bitops.h */
> +#include <linux/kernel.h>
> +
>   /*
>    * Create a contiguous bitmask starting at bit position @l and ending at
>    * position @h. For example
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 088513ad29..5e85513853 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -1,6 +1,7 @@
>   #ifndef __KERNEL_PRINTK__
>   #define __KERNEL_PRINTK__
> 
> +#include <log.h>
>   #include <stdio.h>
>   #include <linux/compiler.h>
> 
> @@ -28,49 +29,56 @@
>   	0;						\
>   })
> 
> -#define __printk(level, fmt, ...)					\
> -({									\
> -	level < CONFIG_LOGLEVEL ? printk(fmt, ##__VA_ARGS__) : 0;	\

Couldn't we just do

#define __printk(level, fmt, ...) log(LOG_CATEGORY, level, fmt, ##__VA_ARGS__)

> -})
> -
>   #ifndef pr_fmt
>   #define pr_fmt(fmt) fmt
>   #endif
> 
> -#define pr_emerg(fmt, ...) \
> -	__printk(0, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_alert(fmt, ...) \
> -	__printk(1, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_crit(fmt, ...) \
> -	__printk(2, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_err(fmt, ...) \
> -	__printk(3, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_warning(fmt, ...) \
> -	__printk(4, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_warn pr_warning
> -#define pr_notice(fmt, ...) \
> -	__printk(5, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_info(fmt, ...) \
> -	__printk(6, pr_fmt(fmt), ##__VA_ARGS__)
> -
> -#define pr_cont(fmt, ...) \
> -	printk(fmt, ##__VA_ARGS__)
> -
> -/* pr_devel() should produce zero code unless DEBUG is defined */
> -#ifdef DEBUG
> -#define pr_devel(fmt, ...) \
> -	__printk(7, pr_fmt(fmt), ##__VA_ARGS__)
> -#else
> -#define pr_devel(fmt, ...) \
> -	no_printk(pr_fmt(fmt), ##__VA_ARGS__)
> -#endif
> +#define pr_emerg(fmt, ...)						\
> +({									\
> +	CONFIG_LOGLEVEL > 0 ? log_emerg(fmt, ##__VA_ARGS__) : 0;	\
> +})

There is also an off-by-one mismatch between the numbers here and the
log level constants. E.g. LOGL_INFO is 6, but pr_info only gets emitted
if CONFIG_LOGLEVEL is >= 7.

--Sean

> +#define pr_alert(fmt, ...)						\
> +({									\
> +	CONFIG_LOGLEVEL > 1 ? log_alert(fmt, ##__VA_ARGS__) : 0;	\
> +})
> +#define pr_crit(fmt, ...)						\
> +({									\
> +	CONFIG_LOGLEVEL > 2 ? log_crit(fmt, ##__VA_ARGS__) : 0;		\
> +})
> +#define pr_err(fmt, ...)						\
> +({									\
> +	CONFIG_LOGLEVEL > 3 ? log_err(fmt, ##__VA_ARGS__) : 0;		\
> +})
> +#define pr_warn(fmt, ...)						\
> +({									\
> +	CONFIG_LOGLEVEL > 4 ? log_warning(fmt, ##__VA_ARGS__) : 0;	\
> +})
> +#define pr_notice(fmt, ...)						\
> +({									\
> +	CONFIG_LOGLEVEL > 5 ? log_notice(fmt, ##__VA_ARGS__) : 0;	\
> +})
> +#define pr_info(fmt, ...)						\
> +({									\
> +	CONFIG_LOGLEVEL > 6 ? log_info(fmt, ##__VA_ARGS__) : 0;		\
> +})
> +#define pr_debug(fmt, ...)						\
> +({									\
> +	CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;	\
> +})
> +#define pr_devel(fmt, ...)						\
> +({									\
> +	CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;	\
> +})
> 
> -#ifdef DEBUG
> -#define pr_debug(fmt, ...) \
> -	__printk(7, pr_fmt(fmt), ##__VA_ARGS__)
> +#ifdef CONFIG_LOG
> +#define pr_cont(fmt, ...)						\
> +({									\
> +	gd->logl_prev < CONFIG_LOGLEVEL ?				\
> +		log_cont(fmt, ##__VA_ARGS__) : 0;			\
> +})
>   #else
> -#define pr_debug(fmt, ...) \
> -	no_printk(pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_cont(fmt, ...)						\
> +	printk(fmt, ##__VA_ARGS__)
>   #endif
> 
>   #define printk_once(fmt, ...) \
> --
> 2.29.2
> 

[PATCH 4/5] log: convert pr_*() to logging

[Heinrich Schuchardt]

On 1/17/21 1:37 AM, Sean Anderson wrote:
> On 1/4/21 2:02 AM, Heinrich Schuchardt wrote:
>> In drivers we use a family of printing functions including pr_err() and
>> pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to 
>> output
>> via printf().
>>
>> Our logging functions allow finer grained control of output. So replace
>> printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
>> remains unchanged.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> ? include/linux/bitops.h |? 4 ++-
>> ? include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
>> ? 2 files changed, 48 insertions(+), 38 deletions(-)
>>
>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
>> index 16f28993f5..d2e5ca026e 100644
>> --- a/include/linux/bitops.h
>> +++ b/include/linux/bitops.h
>> @@ -6,7 +6,6 @@
>> ? #include <asm/types.h>
>> ? #include <asm-generic/bitsperlong.h>
>> ? #include <linux/compiler.h>
>> -#include <linux/kernel.h>
>>
>> ? #ifdef??? __KERNEL__
>> ? #define BIT(nr)??????????? (1UL << (nr))
>> @@ -19,6 +18,9 @@
>> ? #define BITS_TO_LONGS(nr)??? DIV_ROUND_UP(nr, BITS_PER_BYTE * 
>> sizeof(long))
>> ? #endif
>>
>> +/* kernel.h includes log.h which include bitops.h */
>> +#include <linux/kernel.h>
>> +
>> ? /*
>> ?? * Create a contiguous bitmask starting at bit position @l and 
>> ending at
>> ?? * position @h. For example
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index 088513ad29..5e85513853 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -1,6 +1,7 @@
>> ? #ifndef __KERNEL_PRINTK__
>> ? #define __KERNEL_PRINTK__
>>
>> +#include <log.h>
>> ? #include <stdio.h>
>> ? #include <linux/compiler.h>
>>
>> @@ -28,49 +29,56 @@
>> ????? 0;??????????????????????? \
>> ? })
>>
>> -#define __printk(level, fmt, ...)??????????????????? \
>> -({??????????????????????????????????? \
>> -??? level < CONFIG_LOGLEVEL ? printk(fmt, ##__VA_ARGS__) : 0;??? \
> 
> Couldn't we just do
> 
> #define __printk(level, fmt, ...) log(LOG_CATEGORY, level, fmt, 
> ##__VA_ARGS__)

Dear Sean,

Thanks for reviewing.

As of today log() does not print anything if CONFIG_LOG is not enabled.

A patch by Simon to change this behavior is pending. If it gets merged, 
we can do the suggested simplification.

log: Convert log values to printf() if not enabled
https://patchwork.ozlabs.org/project/uboot/patch/20210114033051.483560-5-sjg at chromium.org/

> 
>> -})
>> -
>> ? #ifndef pr_fmt
>> ? #define pr_fmt(fmt) fmt
>> ? #endif
>>
>> -#define pr_emerg(fmt, ...) \
>> -??? __printk(0, pr_fmt(fmt), ##__VA_ARGS__)
>> -#define pr_alert(fmt, ...) \
>> -??? __printk(1, pr_fmt(fmt), ##__VA_ARGS__)
>> -#define pr_crit(fmt, ...) \
>> -??? __printk(2, pr_fmt(fmt), ##__VA_ARGS__)
>> -#define pr_err(fmt, ...) \
>> -??? __printk(3, pr_fmt(fmt), ##__VA_ARGS__)
>> -#define pr_warning(fmt, ...) \
>> -??? __printk(4, pr_fmt(fmt), ##__VA_ARGS__)
>> -#define pr_warn pr_warning
>> -#define pr_notice(fmt, ...) \
>> -??? __printk(5, pr_fmt(fmt), ##__VA_ARGS__)
>> -#define pr_info(fmt, ...) \
>> -??? __printk(6, pr_fmt(fmt), ##__VA_ARGS__)
>> -
>> -#define pr_cont(fmt, ...) \
>> -??? printk(fmt, ##__VA_ARGS__)
>> -
>> -/* pr_devel() should produce zero code unless DEBUG is defined */
>> -#ifdef DEBUG
>> -#define pr_devel(fmt, ...) \
>> -??? __printk(7, pr_fmt(fmt), ##__VA_ARGS__)
>> -#else
>> -#define pr_devel(fmt, ...) \
>> -??? no_printk(pr_fmt(fmt), ##__VA_ARGS__)
>> -#endif
>> +#define pr_emerg(fmt, ...)??????????????????????? \
>> +({??????????????????????????????????? \
>> +??? CONFIG_LOGLEVEL > 0 ? log_emerg(fmt, ##__VA_ARGS__) : 0;??? \
>> +})
> 
> There is also an off-by-one mismatch between the numbers here and the
> log level constants. E.g. LOGL_INFO is 6, but pr_info only gets emitted
> if CONFIG_LOGLEVEL is >= 7.

The Kconfig for CONFIG_LOGLEVEL description reads:

"All Messages with a loglevel *smaller than* the console loglevel will 
be compiled in."

I did not intend to change this definition.

Best regards

Heinrich

> 
> --Sean
> 
>> +#define pr_alert(fmt, ...)??????????????????????? \
>> +({??????????????????????????????????? \
>> +??? CONFIG_LOGLEVEL > 1 ? log_alert(fmt, ##__VA_ARGS__) : 0;??? \
>> +})
>> +#define pr_crit(fmt, ...)??????????????????????? \
>> +({??????????????????????????????????? \
>> +??? CONFIG_LOGLEVEL > 2 ? log_crit(fmt, ##__VA_ARGS__) : 0;??????? \
>> +})
>> +#define pr_err(fmt, ...)??????????????????????? \
>> +({??????????????????????????????????? \
>> +??? CONFIG_LOGLEVEL > 3 ? log_err(fmt, ##__VA_ARGS__) : 0;??????? \
>> +})
>> +#define pr_warn(fmt, ...)??????????????????????? \
>> +({??????????????????????????????????? \
>> +??? CONFIG_LOGLEVEL > 4 ? log_warning(fmt, ##__VA_ARGS__) : 0;??? \
>> +})
>> +#define pr_notice(fmt, ...)??????????????????????? \
>> +({??????????????????????????????????? \
>> +??? CONFIG_LOGLEVEL > 5 ? log_notice(fmt, ##__VA_ARGS__) : 0;??? \
>> +})
>> +#define pr_info(fmt, ...)??????????????????????? \
>> +({??????????????????????????????????? \
>> +??? CONFIG_LOGLEVEL > 6 ? log_info(fmt, ##__VA_ARGS__) : 0;??????? \
>> +})
>> +#define pr_debug(fmt, ...)??????????????????????? \
>> +({??????????????????????????????????? \
>> +??? CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;??? \
>> +})
>> +#define pr_devel(fmt, ...)??????????????????????? \
>> +({??????????????????????????????????? \
>> +??? CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;??? \
>> +})
>>
>> -#ifdef DEBUG
>> -#define pr_debug(fmt, ...) \
>> -??? __printk(7, pr_fmt(fmt), ##__VA_ARGS__)
>> +#ifdef CONFIG_LOG
>> +#define pr_cont(fmt, ...)??????????????????????? \
>> +({??????????????????????????????????? \
>> +??? gd->logl_prev < CONFIG_LOGLEVEL ???????????????? \
>> +??????? log_cont(fmt, ##__VA_ARGS__) : 0;??????????? \
>> +})
>> ? #else
>> -#define pr_debug(fmt, ...) \
>> -??? no_printk(pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_cont(fmt, ...)??????????????????????? \
>> +??? printk(fmt, ##__VA_ARGS__)
>> ? #endif
>>
>> ? #define printk_once(fmt, ...) \
>> -- 
>> 2.29.2
>>
> 

[PATCH 4/5] log: convert pr_*() to logging

[Sean Anderson]

On 1/17/21 2:26 AM, Heinrich Schuchardt wrote:
> On 1/17/21 1:37 AM, Sean Anderson wrote:
>> On 1/4/21 2:02 AM, Heinrich Schuchardt wrote:
>>> In drivers we use a family of printing functions including pr_err() and
>>> pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
>>> via printf().
>>>
>>> Our logging functions allow finer grained control of output. So replace
>>> printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
>>> remains unchanged.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>   include/linux/bitops.h |  4 ++-
>>>   include/linux/printk.h | 82 +++++++++++++++++++++++-------------------
>>>   2 files changed, 48 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
>>> index 16f28993f5..d2e5ca026e 100644
>>> --- a/include/linux/bitops.h
>>> +++ b/include/linux/bitops.h
>>> @@ -6,7 +6,6 @@
>>>   #include <asm/types.h>
>>>   #include <asm-generic/bitsperlong.h>
>>>   #include <linux/compiler.h>
>>> -#include <linux/kernel.h>
>>>
>>>   #ifdef    __KERNEL__
>>>   #define BIT(nr)            (1UL << (nr))
>>> @@ -19,6 +18,9 @@
>>>   #define BITS_TO_LONGS(nr)    DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
>>>   #endif
>>>
>>> +/* kernel.h includes log.h which include bitops.h */
>>> +#include <linux/kernel.h>
>>> +
>>>   /*
>>>    * Create a contiguous bitmask starting at bit position @l and ending at
>>>    * position @h. For example
>>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>>> index 088513ad29..5e85513853 100644
>>> --- a/include/linux/printk.h
>>> +++ b/include/linux/printk.h
>>> @@ -1,6 +1,7 @@
>>>   #ifndef __KERNEL_PRINTK__
>>>   #define __KERNEL_PRINTK__
>>>
>>> +#include <log.h>
>>>   #include <stdio.h>
>>>   #include <linux/compiler.h>
>>>
>>> @@ -28,49 +29,56 @@
>>>       0;                        \
>>>   })
>>>
>>> -#define __printk(level, fmt, ...)                    \
>>> -({                                    \
>>> -    level < CONFIG_LOGLEVEL ? printk(fmt, ##__VA_ARGS__) : 0;    \
>>
>> Couldn't we just do
>>
>> #define __printk(level, fmt, ...) log(LOG_CATEGORY, level, fmt, ##__VA_ARGS__)
> 
> Dear Sean,
> 
> Thanks for reviewing.
> 
> As of today log() does not print anything if CONFIG_LOG is not enabled.

> #if CONFIG_IS_ENABLED(LOG)
> #define _LOG_MAX_LEVEL CONFIG_VAL(LOG_MAX_LEVEL)
> #define log_err(_fmt...)	log(LOG_CATEGORY, LOGL_ERR, ##_fmt)
> #define log_warning(_fmt...)	log(LOG_CATEGORY, LOGL_WARNING, ##_fmt)
> #define log_notice(_fmt...)	log(LOG_CATEGORY, LOGL_NOTICE, ##_fmt)
> #define log_info(_fmt...)	log(LOG_CATEGORY, LOGL_INFO, ##_fmt)
> #define log_debug(_fmt...)	log(LOG_CATEGORY, LOGL_DEBUG, ##_fmt)
> #define log_content(_fmt...)	log(LOG_CATEGORY, LOGL_DEBUG_CONTENT, ##_fmt)
> #define log_io(_fmt...)		log(LOG_CATEGORY, LOGL_DEBUG_IO, ##_fmt)
> #else
> #define _LOG_MAX_LEVEL LOGL_INFO
> #define log_err(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
> #define log_warning(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
> #define log_notice(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
> #define log_info(_fmt, ...)	printf(_fmt, ##__VA_ARGS__)
> #define log_debug(_fmt, ...)	debug(_fmt, ##__VA_ARGS__)
> #define log_content(_fmt...)	log_nop(LOG_CATEGORY, \
> 					LOGL_DEBUG_CONTENT, ##_fmt)
> #define log_io(_fmt...)		log_nop(LOG_CATEGORY, LOGL_DEBUG_IO, ##_fmt)
> #endif

If CONFIG_LOG is not enabled, then log statements are converted to
debug and printf.

> 
> A patch by Simon to change this behavior is pending. If it gets merged, we can do the suggested simplification.
> 
> log: Convert log values to printf() if not enabled
> https://patchwork.ozlabs.org/project/uboot/patch/20210114033051.483560-5-sjg at chromium.org/
> 
>>
>>> -})
>>> -
>>>   #ifndef pr_fmt
>>>   #define pr_fmt(fmt) fmt
>>>   #endif
>>>
>>> -#define pr_emerg(fmt, ...) \
>>> -    __printk(0, pr_fmt(fmt), ##__VA_ARGS__)
>>> -#define pr_alert(fmt, ...) \
>>> -    __printk(1, pr_fmt(fmt), ##__VA_ARGS__)
>>> -#define pr_crit(fmt, ...) \
>>> -    __printk(2, pr_fmt(fmt), ##__VA_ARGS__)
>>> -#define pr_err(fmt, ...) \
>>> -    __printk(3, pr_fmt(fmt), ##__VA_ARGS__)
>>> -#define pr_warning(fmt, ...) \
>>> -    __printk(4, pr_fmt(fmt), ##__VA_ARGS__)
>>> -#define pr_warn pr_warning
>>> -#define pr_notice(fmt, ...) \
>>> -    __printk(5, pr_fmt(fmt), ##__VA_ARGS__)
>>> -#define pr_info(fmt, ...) \
>>> -    __printk(6, pr_fmt(fmt), ##__VA_ARGS__)
>>> -
>>> -#define pr_cont(fmt, ...) \
>>> -    printk(fmt, ##__VA_ARGS__)
>>> -
>>> -/* pr_devel() should produce zero code unless DEBUG is defined */
>>> -#ifdef DEBUG
>>> -#define pr_devel(fmt, ...) \
>>> -    __printk(7, pr_fmt(fmt), ##__VA_ARGS__)
>>> -#else
>>> -#define pr_devel(fmt, ...) \
>>> -    no_printk(pr_fmt(fmt), ##__VA_ARGS__)
>>> -#endif
>>> +#define pr_emerg(fmt, ...)                        \
>>> +({                                    \
>>> +    CONFIG_LOGLEVEL > 0 ? log_emerg(fmt, ##__VA_ARGS__) : 0;    \
>>> +})
>>
>> There is also an off-by-one mismatch between the numbers here and the
>> log level constants. E.g. LOGL_INFO is 6, but pr_info only gets emitted
>> if CONFIG_LOGLEVEL is >= 7.
> 
> The Kconfig for CONFIG_LOGLEVEL description reads:
> 
> "All Messages with a loglevel *smaller than* the console loglevel will be compiled in."

Ok then, perhaps that should be rectified.

--Sean

> 
> I did not intend to change this definition.
> 
> Best regards
> 
> Heinrich
> 
>>
>> --Sean
>>
>>> +#define pr_alert(fmt, ...)                        \
>>> +({                                    \
>>> +    CONFIG_LOGLEVEL > 1 ? log_alert(fmt, ##__VA_ARGS__) : 0;    \
>>> +})
>>> +#define pr_crit(fmt, ...)                        \
>>> +({                                    \
>>> +    CONFIG_LOGLEVEL > 2 ? log_crit(fmt, ##__VA_ARGS__) : 0;        \
>>> +})
>>> +#define pr_err(fmt, ...)                        \
>>> +({                                    \
>>> +    CONFIG_LOGLEVEL > 3 ? log_err(fmt, ##__VA_ARGS__) : 0;        \
>>> +})
>>> +#define pr_warn(fmt, ...)                        \
>>> +({                                    \
>>> +    CONFIG_LOGLEVEL > 4 ? log_warning(fmt, ##__VA_ARGS__) : 0;    \
>>> +})
>>> +#define pr_notice(fmt, ...)                        \
>>> +({                                    \
>>> +    CONFIG_LOGLEVEL > 5 ? log_notice(fmt, ##__VA_ARGS__) : 0;    \
>>> +})
>>> +#define pr_info(fmt, ...)                        \
>>> +({                                    \
>>> +    CONFIG_LOGLEVEL > 6 ? log_info(fmt, ##__VA_ARGS__) : 0;        \
>>> +})
>>> +#define pr_debug(fmt, ...)                        \
>>> +({                                    \
>>> +    CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;    \
>>> +})
>>> +#define pr_devel(fmt, ...)                        \
>>> +({                                    \
>>> +    CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;    \
>>> +})
>>>
>>> -#ifdef DEBUG
>>> -#define pr_debug(fmt, ...) \
>>> -    __printk(7, pr_fmt(fmt), ##__VA_ARGS__)
>>> +#ifdef CONFIG_LOG
>>> +#define pr_cont(fmt, ...)                        \
>>> +({                                    \
>>> +    gd->logl_prev < CONFIG_LOGLEVEL ?                \
>>> +        log_cont(fmt, ##__VA_ARGS__) : 0;            \
>>> +})
>>>   #else
>>> -#define pr_debug(fmt, ...) \
>>> -    no_printk(pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_cont(fmt, ...)                        \
>>> +    printk(fmt, ##__VA_ARGS__)
>>>   #endif
>>>
>>>   #define printk_once(fmt, ...) \
>>> -- 
>>> 2.29.2
>>>
>>
> 

[PATCH 4/5] log: convert pr_*() to logging

[Heinrich Schuchardt]

On 1/17/21 11:27 PM, Sean Anderson wrote:
> On 1/17/21 2:26 AM, Heinrich Schuchardt wrote:
>> On 1/17/21 1:37 AM, Sean Anderson wrote:
>>> On 1/4/21 2:02 AM, Heinrich Schuchardt wrote:
>>>> In drivers we use a family of printing functions including pr_err() and
>>>> pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to 
>>>> output
>>>> via printf().
>>>>
>>>> Our logging functions allow finer grained control of output. So replace
>>>> printf() by the matching logging functions. The usage of 
>>>> CONFIG_LOGLEVEL
>>>> remains unchanged.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>> ? include/linux/bitops.h |? 4 ++-
>>>> ? include/linux/printk.h | 82 
>>>> +++++++++++++++++++++++-------------------
>>>> ? 2 files changed, 48 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
>>>> index 16f28993f5..d2e5ca026e 100644
>>>> --- a/include/linux/bitops.h
>>>> +++ b/include/linux/bitops.h
>>>> @@ -6,7 +6,6 @@
>>>> ? #include <asm/types.h>
>>>> ? #include <asm-generic/bitsperlong.h>
>>>> ? #include <linux/compiler.h>
>>>> -#include <linux/kernel.h>
>>>>
>>>> ? #ifdef??? __KERNEL__
>>>> ? #define BIT(nr)??????????? (1UL << (nr))
>>>> @@ -19,6 +18,9 @@
>>>> ? #define BITS_TO_LONGS(nr)??? DIV_ROUND_UP(nr, BITS_PER_BYTE * 
>>>> sizeof(long))
>>>> ? #endif
>>>>
>>>> +/* kernel.h includes log.h which include bitops.h */
>>>> +#include <linux/kernel.h>
>>>> +
>>>> ? /*
>>>> ?? * Create a contiguous bitmask starting at bit position @l and 
>>>> ending at
>>>> ?? * position @h. For example
>>>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>>>> index 088513ad29..5e85513853 100644
>>>> --- a/include/linux/printk.h
>>>> +++ b/include/linux/printk.h
>>>> @@ -1,6 +1,7 @@
>>>> ? #ifndef __KERNEL_PRINTK__
>>>> ? #define __KERNEL_PRINTK__
>>>>
>>>> +#include <log.h>
>>>> ? #include <stdio.h>
>>>> ? #include <linux/compiler.h>
>>>>
>>>> @@ -28,49 +29,56 @@
>>>> ????? 0;??????????????????????? \
>>>> ? })
>>>>
>>>> -#define __printk(level, fmt, ...)??????????????????? \
>>>> -({??????????????????????????????????? \
>>>> -??? level < CONFIG_LOGLEVEL ? printk(fmt, ##__VA_ARGS__) : 0;??? \
>>>
>>> Couldn't we just do
>>>
>>> #define __printk(level, fmt, ...) log(LOG_CATEGORY, level, fmt, 
>>> ##__VA_ARGS__)
>>
>> Dear Sean,
>>
>> Thanks for reviewing.
>>
>> As of today log() does not print anything if CONFIG_LOG is not enabled.
> 
>> #if CONFIG_IS_ENABLED(LOG)
>> #define _LOG_MAX_LEVEL CONFIG_VAL(LOG_MAX_LEVEL)
>> #define log_err(_fmt...)??? log(LOG_CATEGORY, LOGL_ERR, ##_fmt)
>> #define log_warning(_fmt...)??? log(LOG_CATEGORY, LOGL_WARNING, ##_fmt)
>> #define log_notice(_fmt...)??? log(LOG_CATEGORY, LOGL_NOTICE, ##_fmt)
>> #define log_info(_fmt...)??? log(LOG_CATEGORY, LOGL_INFO, ##_fmt)
>> #define log_debug(_fmt...)??? log(LOG_CATEGORY, LOGL_DEBUG, ##_fmt)
>> #define log_content(_fmt...)??? log(LOG_CATEGORY, LOGL_DEBUG_CONTENT, 
>> ##_fmt)
>> #define log_io(_fmt...)??????? log(LOG_CATEGORY, LOGL_DEBUG_IO, ##_fmt)
>> #else
>> #define _LOG_MAX_LEVEL LOGL_INFO
>> #define log_err(_fmt, ...)??? printf(_fmt, ##__VA_ARGS__)
>> #define log_warning(_fmt, ...)??? printf(_fmt, ##__VA_ARGS__)
>> #define log_notice(_fmt, ...)??? printf(_fmt, ##__VA_ARGS__)
>> #define log_info(_fmt, ...)??? printf(_fmt, ##__VA_ARGS__)
>> #define log_debug(_fmt, ...)??? debug(_fmt, ##__VA_ARGS__)
>> #define log_content(_fmt...)??? log_nop(LOG_CATEGORY, \
>> ??????????????????? LOGL_DEBUG_CONTENT, ##_fmt)
>> #define log_io(_fmt...)??????? log_nop(LOG_CATEGORY, LOGL_DEBUG_IO, 
>> ##_fmt)
>> #endif
> 
> If CONFIG_LOG is not enabled, then log statements are converted to
> debug and printf.

Only the ones you cited, not the function log() referenced in your proposal.

> 
>>
>> A patch by Simon to change this behavior is pending. If it gets 
>> merged, we can do the suggested simplification.
>>
>> log: Convert log values to printf() if not enabled
>> https://patchwork.ozlabs.org/project/uboot/patch/20210114033051.483560-5-sjg at chromium.org/ 
>>
>>
>>>
>>>> -})
>>>> -
>>>> ? #ifndef pr_fmt
>>>> ? #define pr_fmt(fmt) fmt
>>>> ? #endif
>>>>
>>>> -#define pr_emerg(fmt, ...) \
>>>> -??? __printk(0, pr_fmt(fmt), ##__VA_ARGS__)
>>>> -#define pr_alert(fmt, ...) \
>>>> -??? __printk(1, pr_fmt(fmt), ##__VA_ARGS__)
>>>> -#define pr_crit(fmt, ...) \
>>>> -??? __printk(2, pr_fmt(fmt), ##__VA_ARGS__)
>>>> -#define pr_err(fmt, ...) \
>>>> -??? __printk(3, pr_fmt(fmt), ##__VA_ARGS__)
>>>> -#define pr_warning(fmt, ...) \
>>>> -??? __printk(4, pr_fmt(fmt), ##__VA_ARGS__)
>>>> -#define pr_warn pr_warning
>>>> -#define pr_notice(fmt, ...) \
>>>> -??? __printk(5, pr_fmt(fmt), ##__VA_ARGS__)
>>>> -#define pr_info(fmt, ...) \
>>>> -??? __printk(6, pr_fmt(fmt), ##__VA_ARGS__)
>>>> -
>>>> -#define pr_cont(fmt, ...) \
>>>> -??? printk(fmt, ##__VA_ARGS__)
>>>> -
>>>> -/* pr_devel() should produce zero code unless DEBUG is defined */
>>>> -#ifdef DEBUG
>>>> -#define pr_devel(fmt, ...) \
>>>> -??? __printk(7, pr_fmt(fmt), ##__VA_ARGS__)
>>>> -#else
>>>> -#define pr_devel(fmt, ...) \
>>>> -??? no_printk(pr_fmt(fmt), ##__VA_ARGS__)
>>>> -#endif
>>>> +#define pr_emerg(fmt, ...)??????????????????????? \
>>>> +({??????????????????????????????????? \
>>>> +??? CONFIG_LOGLEVEL > 0 ? log_emerg(fmt, ##__VA_ARGS__) : 0;??? \
>>>> +})
>>>
>>> There is also an off-by-one mismatch between the numbers here and the
>>> log level constants. E.g. LOGL_INFO is 6, but pr_info only gets emitted
>>> if CONFIG_LOGLEVEL is >= 7.
>>
>> The Kconfig for CONFIG_LOGLEVEL description reads:
>>
>> "All Messages with a loglevel *smaller than* the console loglevel will 
>> be compiled in."
> 
> Ok then, perhaps that should be rectified.

The current definition allows to disable log level 0. So you better 
don't change it.

Best regards

Heinrich

> 
> --Sean
> 
>>
>> I did not intend to change this definition.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> --Sean
>>>
>>>> +#define pr_alert(fmt, ...)??????????????????????? \
>>>> +({??????????????????????????????????? \
>>>> +??? CONFIG_LOGLEVEL > 1 ? log_alert(fmt, ##__VA_ARGS__) : 0;??? \
>>>> +})
>>>> +#define pr_crit(fmt, ...)??????????????????????? \
>>>> +({??????????????????????????????????? \
>>>> +??? CONFIG_LOGLEVEL > 2 ? log_crit(fmt, ##__VA_ARGS__) : 0;??????? \
>>>> +})
>>>> +#define pr_err(fmt, ...)??????????????????????? \
>>>> +({??????????????????????????????????? \
>>>> +??? CONFIG_LOGLEVEL > 3 ? log_err(fmt, ##__VA_ARGS__) : 0;??????? \
>>>> +})
>>>> +#define pr_warn(fmt, ...)??????????????????????? \
>>>> +({??????????????????????????????????? \
>>>> +??? CONFIG_LOGLEVEL > 4 ? log_warning(fmt, ##__VA_ARGS__) : 0;??? \
>>>> +})
>>>> +#define pr_notice(fmt, ...)??????????????????????? \
>>>> +({??????????????????????????????????? \
>>>> +??? CONFIG_LOGLEVEL > 5 ? log_notice(fmt, ##__VA_ARGS__) : 0;??? \
>>>> +})
>>>> +#define pr_info(fmt, ...)??????????????????????? \
>>>> +({??????????????????????????????????? \
>>>> +??? CONFIG_LOGLEVEL > 6 ? log_info(fmt, ##__VA_ARGS__) : 0;??????? \
>>>> +})
>>>> +#define pr_debug(fmt, ...)??????????????????????? \
>>>> +({??????????????????????????????????? \
>>>> +??? CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;??? \
>>>> +})
>>>> +#define pr_devel(fmt, ...)??????????????????????? \
>>>> +({??????????????????????????????????? \
>>>> +??? CONFIG_LOGLEVEL > 7 ? log_debug(fmt, ##__VA_ARGS__) : 0;??? \
>>>> +})
>>>>
>>>> -#ifdef DEBUG
>>>> -#define pr_debug(fmt, ...) \
>>>> -??? __printk(7, pr_fmt(fmt), ##__VA_ARGS__)
>>>> +#ifdef CONFIG_LOG
>>>> +#define pr_cont(fmt, ...)??????????????????????? \
>>>> +({??????????????????????????????????? \
>>>> +??? gd->logl_prev < CONFIG_LOGLEVEL ???????????????? \
>>>> +??????? log_cont(fmt, ##__VA_ARGS__) : 0;??????????? \
>>>> +})
>>>> ? #else
>>>> -#define pr_debug(fmt, ...) \
>>>> -??? no_printk(pr_fmt(fmt), ##__VA_ARGS__)
>>>> +#define pr_cont(fmt, ...)??????????????????????? \
>>>> +??? printk(fmt, ##__VA_ARGS__)
>>>> ? #endif
>>>>
>>>> ? #define printk_once(fmt, ...) \
>>>> -- 
>>>> 2.29.2
>>>>
>>>
>>
> 

[PATCH 4/5] log: convert pr_*() to logging

[Tom Rini]

On Mon, Jan 04, 2021 at 08:02:55AM +0100, Heinrich Schuchardt wrote:

> In drivers we use a family of printing functions including pr_err() and
> pr_cont(). CONFIG_LOGLEVEL is used to control which of these lead to output
> via printf().
> 
> Our logging functions allow finer grained control of output. So replace
> printf() by the matching logging functions. The usage of CONFIG_LOGLEVEL
> remains unchanged.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210301/54316570/attachment.sig>

[PATCH 5/5] test: unit test for pr_err(), pr_cont()

[Heinrich Schuchardt]

Provide a unit test for printing via pr_err() and pr_cont().

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 test/log/Makefile       |  1 +
 test/log/pr_cont_test.c | 45 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 test/log/pr_cont_test.c

diff --git a/test/log/Makefile b/test/log/Makefile
index 88bc573e9f..afdafa502a 100644
--- a/test/log/Makefile
+++ b/test/log/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_CMD_LOG) += log_filter.o
 ifdef CONFIG_UT_LOG

 obj-y += test-main.o
+obj-y += pr_cont_test.o

 ifdef CONFIG_SANDBOX
 obj-$(CONFIG_LOG_SYSLOG) += syslog_test.o
diff --git a/test/log/pr_cont_test.c b/test/log/pr_cont_test.c
new file mode 100644
index 0000000000..236eff4b33
--- /dev/null
+++ b/test/log/pr_cont_test.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2021, Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * Test continuation of log messages using pr_cont().
+ */
+
+#include <common.h>
+#include <console.h>
+#include <test/log.h>
+#include <test/test.h>
+#include <test/suites.h>
+#include <test/ut.h>
+#include <linux/printk.h>
+
+#define BUFFSIZE 64
+
+#undef CONFIG_LOGLEVEL
+#define CONFIG_LOGLEVEL 4
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static int log_test_pr_cont(struct unit_test_state *uts)
+{
+	int log_fmt;
+	int log_level;
+
+	log_fmt = gd->log_fmt;
+	log_level = gd->default_log_level;
+
+	/* Write two messages, the second continuing the first */
+	gd->log_fmt = BIT(LOGF_MSG);
+	gd->default_log_level = LOGL_INFO;
+	console_record_reset_enable();
+	pr_err("ea%d ", 1);
+	pr_cont("cc%d\n", 2);
+	gd->default_log_level = log_level;
+	gd->log_fmt = log_fmt;
+	gd->flags &= ~GD_FLG_RECORD;
+	ut_assertok(ut_check_console_line(uts, "ea1 cc2"));
+	ut_assertok(ut_check_console_end(uts));
+
+	return 0;
+}
+LOG_TEST(log_test_pr_cont);
--
2.29.2

[PATCH 5/5] test: unit test for pr_err(), pr_cont()

[Simon Glass]

On Mon, 4 Jan 2021 at 00:03, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Provide a unit test for printing via pr_err() and pr_cont().
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  test/log/Makefile       |  1 +
>  test/log/pr_cont_test.c | 45 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
>  create mode 100644 test/log/pr_cont_test.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 5/5] test: unit test for pr_err(), pr_cont()

[Tom Rini]

On Mon, Jan 04, 2021 at 08:02:56AM +0100, Heinrich Schuchardt wrote:

> Provide a unit test for printing via pr_err() and pr_cont().
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210118/53107fbc/attachment.sig>