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

[Sean Anderson <seanga2@gmail.com>]

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
>>>
>>
>