At the beginning of setup_64.c, it has,
#ifdef DEBUG
#define DBG(fmt...) udbg_printf(fmt)
#else
#define DBG(fmt...)
#endif
where DBG() could be compiled away, and generate warnings,
arch/powerpc/kernel/setup_64.c: In function 'initialize_cache_info':
arch/powerpc/kernel/setup_64.c:579:49: warning: suggest braces around
empty body in an 'if' statement [-Wempty-body]
DBG("Argh, can't find dcache properties !\n");
^
arch/powerpc/kernel/setup_64.c:582:49: warning: suggest braces around
empty body in an 'if' statement [-Wempty-body]
DBG("Argh, can't find icache properties !\n");
Fix it by using the suggestions from Michael:
"Neither of those sites should use DBG(), that's not really early boot
code, they should just use pr_warn().
And the other uses of DBG() in initialize_cache_info() should just be
removed.
In smp_release_cpus() the entry/exit DBG's should just be removed, and
the spinning_secondaries line should just be pr_debug().
That would just leave the two calls in early_setup(). If we taught
udbg_printf() to return early when udbg_putc is NULL, then we could just
call udbg_printf() unconditionally and get rid of the DBG macro
entirely."
Suggested-by: Michael Ellerman <[email protected]>
Signed-off-by: Qian Cai <[email protected]>
---
v4: Use the suggestions from Michael and __func__ per checkpatch.
v3: Use no_printk() macro, and make sure that format and argument are always
verified by the compiler using a more generic form ##__VA_ARGS__ per Joe.
v2: Fix it by using a NOP while loop per Tyrel.
arch/powerpc/kernel/setup_64.c | 26 ++++++--------------------
arch/powerpc/kernel/udbg.c | 14 ++++++++------
2 files changed, 14 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 44b4c432a273..d2af4c228970 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -68,12 +68,6 @@
#include "setup.h"
-#ifdef DEBUG
-#define DBG(fmt...) udbg_printf(fmt)
-#else
-#define DBG(fmt...)
-#endif
-
int spinning_secondaries;
u64 ppc64_pft_size;
@@ -305,7 +299,7 @@ void __init early_setup(unsigned long dt_ptr)
/* Enable early debugging if any specified (see udbg.h) */
udbg_early_init();
- DBG(" -> early_setup(), dt_ptr: 0x%lx\n", dt_ptr);
+ udbg_printf(" -> %s(), dt_ptr: 0x%lx\n", __func__, dt_ptr);
/*
* Do early initialization using the flattened device
@@ -362,11 +356,11 @@ void __init early_setup(unsigned long dt_ptr)
*/
this_cpu_enable_ftrace();
- DBG(" <- early_setup()\n");
+ udbg_printf(" <- %s()\n", __func__);
#ifdef CONFIG_PPC_EARLY_DEBUG_BOOTX
/*
- * This needs to be done *last* (after the above DBG() even)
+ * This needs to be done *last* (after the above udbg_printf() even)
*
* Right after we return from this function, we turn on the MMU
* which means the real-mode access trick that btext does will
@@ -436,8 +430,6 @@ void smp_release_cpus(void)
if (!use_spinloop())
return;
- DBG(" -> smp_release_cpus()\n");
-
/* All secondary cpus are spinning on a common spinloop, release them
* all now so they can start to spin on their individual paca
* spinloops. For non SMP kernels, the secondary cpus never get out
@@ -456,9 +448,7 @@ void smp_release_cpus(void)
break;
udelay(1);
}
- DBG("spinning_secondaries = %d\n", spinning_secondaries);
-
- DBG(" <- smp_release_cpus()\n");
+ pr_debug("spinning_secondaries = %d\n", spinning_secondaries);
}
#endif /* CONFIG_SMP || CONFIG_KEXEC_CORE */
@@ -551,8 +541,6 @@ void __init initialize_cache_info(void)
struct device_node *cpu = NULL, *l2, *l3 = NULL;
u32 pvr;
- DBG(" -> initialize_cache_info()\n");
-
/*
* All shipping POWER8 machines have a firmware bug that
* puts incorrect information in the device-tree. This will
@@ -576,10 +564,10 @@ void __init initialize_cache_info(void)
*/
if (cpu) {
if (!parse_cache_info(cpu, false, &ppc64_caches.l1d))
- DBG("Argh, can't find dcache properties !\n");
+ pr_warn("Argh, can't find dcache properties !\n");
if (!parse_cache_info(cpu, true, &ppc64_caches.l1i))
- DBG("Argh, can't find icache properties !\n");
+ pr_warn("Argh, can't find icache properties !\n");
/*
* Try to find the L2 and L3 if any. Assume they are
@@ -604,8 +592,6 @@ void __init initialize_cache_info(void)
cur_cpu_spec->dcache_bsize = dcache_bsize;
cur_cpu_spec->icache_bsize = icache_bsize;
-
- DBG(" <- initialize_cache_info()\n");
}
/*
diff --git a/arch/powerpc/kernel/udbg.c b/arch/powerpc/kernel/udbg.c
index a384e7c8b01c..01595e8cafe7 100644
--- a/arch/powerpc/kernel/udbg.c
+++ b/arch/powerpc/kernel/udbg.c
@@ -120,13 +120,15 @@ int udbg_write(const char *s, int n)
#define UDBG_BUFSIZE 256
void udbg_printf(const char *fmt, ...)
{
- char buf[UDBG_BUFSIZE];
- va_list args;
+ if (udbg_putc) {
+ char buf[UDBG_BUFSIZE];
+ va_list args;
- va_start(args, fmt);
- vsnprintf(buf, UDBG_BUFSIZE, fmt, args);
- udbg_puts(buf);
- va_end(args);
+ va_start(args, fmt);
+ vsnprintf(buf, UDBG_BUFSIZE, fmt, args);
+ udbg_puts(buf);
+ va_end(args);
+ }
}
void __init udbg_progress(char *s, unsigned short hex)
--
1.8.3.1
Michael, ping in case that you might forget this one forever as well.
On Mon, 2019-07-15 at 14:32 -0400, Qian Cai wrote:
> At the beginning of setup_64.c, it has,
>
> #ifdef DEBUG
> #define DBG(fmt...) udbg_printf(fmt)
> #else
> #define DBG(fmt...)
> #endif
>
> where DBG() could be compiled away, and generate warnings,
>
> arch/powerpc/kernel/setup_64.c: In function 'initialize_cache_info':
> arch/powerpc/kernel/setup_64.c:579:49: warning: suggest braces around
> empty body in an 'if' statement [-Wempty-body]
> DBG("Argh, can't find dcache properties !\n");
> ^
> arch/powerpc/kernel/setup_64.c:582:49: warning: suggest braces around
> empty body in an 'if' statement [-Wempty-body]
> DBG("Argh, can't find icache properties !\n");
>
> Fix it by using the suggestions from Michael:
>
> "Neither of those sites should use DBG(), that's not really early boot
> code, they should just use pr_warn().
>
> And the other uses of DBG() in initialize_cache_info() should just be
> removed.
>
> In smp_release_cpus() the entry/exit DBG's should just be removed, and
> the spinning_secondaries line should just be pr_debug().
>
> That would just leave the two calls in early_setup(). If we taught
> udbg_printf() to return early when udbg_putc is NULL, then we could just
> call udbg_printf() unconditionally and get rid of the DBG macro
> entirely."
>
> Suggested-by: Michael Ellerman <[email protected]>
> Signed-off-by: Qian Cai <[email protected]>
> ---
>
> v4: Use the suggestions from Michael and __func__ per checkpatch.
> v3: Use no_printk() macro, and make sure that format and argument are always
> verified by the compiler using a more generic form ##__VA_ARGS__ per Joe.
> v2: Fix it by using a NOP while loop per Tyrel.
>
> arch/powerpc/kernel/setup_64.c | 26 ++++++--------------------
> arch/powerpc/kernel/udbg.c | 14 ++++++++------
> 2 files changed, 14 insertions(+), 26 deletions(-)
>
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index 44b4c432a273..d2af4c228970 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -68,12 +68,6 @@
>
> #include "setup.h"
>
> -#ifdef DEBUG
> -#define DBG(fmt...) udbg_printf(fmt)
> -#else
> -#define DBG(fmt...)
> -#endif
> -
> int spinning_secondaries;
> u64 ppc64_pft_size;
>
> @@ -305,7 +299,7 @@ void __init early_setup(unsigned long dt_ptr)
> /* Enable early debugging if any specified (see udbg.h) */
> udbg_early_init();
>
> - DBG(" -> early_setup(), dt_ptr: 0x%lx\n", dt_ptr);
> + udbg_printf(" -> %s(), dt_ptr: 0x%lx\n", __func__, dt_ptr);
>
> /*
> * Do early initialization using the flattened device
> @@ -362,11 +356,11 @@ void __init early_setup(unsigned long dt_ptr)
> */
> this_cpu_enable_ftrace();
>
> - DBG(" <- early_setup()\n");
> + udbg_printf(" <- %s()\n", __func__);
>
> #ifdef CONFIG_PPC_EARLY_DEBUG_BOOTX
> /*
> - * This needs to be done *last* (after the above DBG() even)
> + * This needs to be done *last* (after the above udbg_printf() even)
> *
> * Right after we return from this function, we turn on the MMU
> * which means the real-mode access trick that btext does will
> @@ -436,8 +430,6 @@ void smp_release_cpus(void)
> if (!use_spinloop())
> return;
>
> - DBG(" -> smp_release_cpus()\n");
> -
> /* All secondary cpus are spinning on a common spinloop, release them
> * all now so they can start to spin on their individual paca
> * spinloops. For non SMP kernels, the secondary cpus never get out
> @@ -456,9 +448,7 @@ void smp_release_cpus(void)
> break;
> udelay(1);
> }
> - DBG("spinning_secondaries = %d\n", spinning_secondaries);
> -
> - DBG(" <- smp_release_cpus()\n");
> + pr_debug("spinning_secondaries = %d\n", spinning_secondaries);
> }
> #endif /* CONFIG_SMP || CONFIG_KEXEC_CORE */
>
> @@ -551,8 +541,6 @@ void __init initialize_cache_info(void)
> struct device_node *cpu = NULL, *l2, *l3 = NULL;
> u32 pvr;
>
> - DBG(" -> initialize_cache_info()\n");
> -
> /*
> * All shipping POWER8 machines have a firmware bug that
> * puts incorrect information in the device-tree. This will
> @@ -576,10 +564,10 @@ void __init initialize_cache_info(void)
> */
> if (cpu) {
> if (!parse_cache_info(cpu, false, &ppc64_caches.l1d))
> - DBG("Argh, can't find dcache properties !\n");
> + pr_warn("Argh, can't find dcache properties !\n");
>
> if (!parse_cache_info(cpu, true, &ppc64_caches.l1i))
> - DBG("Argh, can't find icache properties !\n");
> + pr_warn("Argh, can't find icache properties !\n");
>
> /*
> * Try to find the L2 and L3 if any. Assume they are
> @@ -604,8 +592,6 @@ void __init initialize_cache_info(void)
>
> cur_cpu_spec->dcache_bsize = dcache_bsize;
> cur_cpu_spec->icache_bsize = icache_bsize;
> -
> - DBG(" <- initialize_cache_info()\n");
> }
>
> /*
> diff --git a/arch/powerpc/kernel/udbg.c b/arch/powerpc/kernel/udbg.c
> index a384e7c8b01c..01595e8cafe7 100644
> --- a/arch/powerpc/kernel/udbg.c
> +++ b/arch/powerpc/kernel/udbg.c
> @@ -120,13 +120,15 @@ int udbg_write(const char *s, int n)
> #define UDBG_BUFSIZE 256
> void udbg_printf(const char *fmt, ...)
> {
> - char buf[UDBG_BUFSIZE];
> - va_list args;
> + if (udbg_putc) {
> + char buf[UDBG_BUFSIZE];
> + va_list args;
>
> - va_start(args, fmt);
> - vsnprintf(buf, UDBG_BUFSIZE, fmt, args);
> - udbg_puts(buf);
> - va_end(args);
> + va_start(args, fmt);
> + vsnprintf(buf, UDBG_BUFSIZE, fmt, args);
> + udbg_puts(buf);
> + va_end(args);
> + }
> }
>
> void __init udbg_progress(char *s, unsigned short hex)
On Mon, 2019-07-15 at 18:32:32 UTC, Qian Cai wrote:
> At the beginning of setup_64.c, it has,
>
> #ifdef DEBUG
> #define DBG(fmt...) udbg_printf(fmt)
> #else
> #define DBG(fmt...)
> #endif
>
> where DBG() could be compiled away, and generate warnings,
>
> arch/powerpc/kernel/setup_64.c: In function 'initialize_cache_info':
> arch/powerpc/kernel/setup_64.c:579:49: warning: suggest braces around
> empty body in an 'if' statement [-Wempty-body]
> DBG("Argh, can't find dcache properties !\n");
> ^
> arch/powerpc/kernel/setup_64.c:582:49: warning: suggest braces around
> empty body in an 'if' statement [-Wempty-body]
> DBG("Argh, can't find icache properties !\n");
>
> Fix it by using the suggestions from Michael:
>
> "Neither of those sites should use DBG(), that's not really early boot
> code, they should just use pr_warn().
>
> And the other uses of DBG() in initialize_cache_info() should just be
> removed.
>
> In smp_release_cpus() the entry/exit DBG's should just be removed, and
> the spinning_secondaries line should just be pr_debug().
>
> That would just leave the two calls in early_setup(). If we taught
> udbg_printf() to return early when udbg_putc is NULL, then we could just
> call udbg_printf() unconditionally and get rid of the DBG macro
> entirely."
>
> Suggested-by: Michael Ellerman <[email protected]>
> Signed-off-by: Qian Cai <[email protected]>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/3b9176e9a874a848afa7eb2f6943639eb18b7a17
cheers