Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1333505 > unrolled thread
| Started by | Borislav Petkov <bp@alien8.de> |
|---|---|
| First post | 2016-02-14 15:10 +0100 |
| Last post | 2016-02-16 10:20 +0100 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v2] x86/kernel: use pr_<level>() and dev_<level> Borislav Petkov <bp@alien8.de> - 2016-02-14 15:10 +0100
Re: [PATCH v2] x86/kernel: use pr_<level>() and dev_<level> Ingo Molnar <mingo@kernel.org> - 2016-02-16 09:30 +0100
Re: [PATCH v2] x86/kernel: use pr_<level>() and dev_<level> Borislav Petkov <bp@alien8.de> - 2016-02-16 10:20 +0100
| From | Borislav Petkov <bp@alien8.de> |
|---|---|
| Date | 2016-02-14 15:10 +0100 |
| Subject | Re: [PATCH v2] x86/kernel: use pr_<level>() and dev_<level> |
| Message-ID | <r25x7-3ED-7@gated-at.bofh.it> |
On Sun, Feb 14, 2016 at 12:10:47PM +0800, Chen Yucong wrote:
> arch/x86/kernel/* use a mixture of printk(KERN_<level> ) and pr_<level>().
> This patch converts the bulk of printk(KERN_<level> ) to pr_<level>() and
> uses dev_dbg() instead of the dev_printk(KERN_DEBUG,). All pr_warning()
> calls have been replaced with pr_warn().
>
> Not sure what to do about the printk(KERN_DEFAULT) and printk() without a
> log level.
...
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 25f9093..0ecb579 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -59,7 +59,7 @@ __setup("noreplace-paravirt", setup_noreplace_paravirt);
> #define DPRINTK(fmt, args...) \
> do { \
> if (debug_alternative) \
> - printk(KERN_DEBUG "%s: " fmt "\n", __func__, ##args); \
> + pr_debug("%s: " fmt "\n", __func__, ##args); \
> } while (0)
>
> #define DUMP_BYTES(buf, len, fmt, args...) \
> @@ -70,10 +70,10 @@ do { \
> if (!(len)) \
> break; \
> \
> - printk(KERN_DEBUG fmt, ##args); \
> + pr_debug(fmt, ##args); \
> for (j = 0; j < (len) - 1; j++) \
> - printk(KERN_CONT "%02hhx ", buf[j]); \
> - printk(KERN_CONT "%02hhx\n", buf[j]); \
> + pr_cont("%02hhx ", buf[j]); \
> + pr_cont("%02hhx\n", buf[j]); \
> } \
> } while (0)
>
NAK the hell out of that hunk!
Did you actually look at how pr_debug() is defined?
Yeah, I don't think so. With your change, when I boot with
"debug-alternative" I get:
...
[ 0.064005] Last level dTLB entries: 4KB 512, 2MB 255, 4MB 127, 1GB 0
[ 0.068005] e9 d5 3e d3 00
[ 0.072004] e9 e8 92 21 ff
[ 0.075003] eb 11 0f 1f 00
[ 0.077906] e8 c5 b6 30 00
[ 0.084009] f3 48 0f b8 c7
[ 0.091241] f3 48 0f b8 c7
[ 0.094259] e8 d9 b5 30 00
[ 0.097611] f3 48 0f b8 c7
[ 0.100004] f3 48 0f b8 c7
[ 0.103067] 90 90 90
[ 0.105575] 0f ae f0
[ 0.108004] 0f ae f0
[ 0.112007] 90 90 90
[ 0.114331] 0f ae f0
[ 0.116004] 0f ae f0
[ 0.118365] e8 07 21 62 ff
How is that useful?!
Please stop for a second with those senseless conversions and think
first. Try the change you're doing in kvm, take a look at what it
affects and think hard whether it makes any sense at all. Only if it
does, *then* send out the patch.
I'm willing to bet that *all* pr_debug* conversions below are wrong too.
Geez :(
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
[toc] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2016-02-16 09:30 +0100 |
| Message-ID | <r2Jbc-5ii-23@gated-at.bofh.it> |
| In reply to | #1333505 |
* Borislav Petkov <bp@alien8.de> wrote:
> > @@ -70,10 +70,10 @@ do { \
> > if (!(len)) \
> > break; \
> > \
> > - printk(KERN_DEBUG fmt, ##args); \
> > + pr_debug(fmt, ##args); \
> > for (j = 0; j < (len) - 1; j++) \
> > - printk(KERN_CONT "%02hhx ", buf[j]); \
> > - printk(KERN_CONT "%02hhx\n", buf[j]); \
> > + pr_cont("%02hhx ", buf[j]); \
> > + pr_cont("%02hhx\n", buf[j]); \
> > } \
> > } while (0)
> >
>
> NAK the hell out of that hunk!
>
> Did you actually look at how pr_debug() is defined?
>
> Yeah, I don't think so. With your change, when I boot with
> "debug-alternative" I get:
>
> ...
> [ 0.064005] Last level dTLB entries: 4KB 512, 2MB 255, 4MB 127, 1GB 0
> [ 0.068005] e9 d5 3e d3 00
> [ 0.072004] e9 e8 92 21 ff
> [ 0.075003] eb 11 0f 1f 00
> [ 0.077906] e8 c5 b6 30 00
> [ 0.084009] f3 48 0f b8 c7
Hm, so I suspect the other patch I already applied is buggy as well?
triton:~/tip> git log -1 -p 1b74dde7c47c | grep KERN_DEBUG
- printk(KERN_DEBUG "tseg: %010llx\n", tseg);
- printk(KERN_DEBUG "CPU%d: Thermal monitoring handled by SMI\n", cpu);
-#define Dprintk(x...) do { if (debug_print) printk(KERN_DEBUG x); } while (0)
- printk(KERN_DEBUG "After WB checking\n");
- printk(KERN_DEBUG "MTRR MAP PFN: %016llx - %016llx\n",
- printk(KERN_DEBUG "After UC checking\n");
- printk(KERN_DEBUG "MTRR MAP PFN: %016llx - %016llx\n",
- printk(KERN_DEBUG "After sorting\n");
- printk(KERN_DEBUG "MTRR MAP PFN: %016llx - %016llx\n",
- printk(KERN_DEBUG "reg %d, base: %ld%cB, range: %ld%cB, type %s\n",
- printk(KERN_DEBUG "original variable MTRRs\n");
- printk(KERN_DEBUG "New variable MTRRs\n");
- printk(KERN_DEBUG "New variable MTRRs\n");
- printk(KERN_DEBUG "Failed to setup IBS LVT offset, "
- printk(KERN_DEBUG "No CPU node configured for IBS\n");
- printk(KERN_DEBUG "No EILVT entry available\n");
Thanks,
Ingo
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@alien8.de> |
|---|---|
| Date | 2016-02-16 10:20 +0100 |
| Message-ID | <r2JXA-5TU-13@gated-at.bofh.it> |
| In reply to | #1335137 |
On Tue, Feb 16, 2016 at 09:24:31AM +0100, Ingo Molnar wrote:
> Hm, so I suspect the other patch I already applied is buggy as well?
Yap.
pr_debug() is ugly to use:
/* If you are writing a driver, please use dev_dbg instead */
#if defined(CONFIG_DYNAMIC_DEBUG)
/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
#define pr_debug(fmt, ...) \
dynamic_pr_debug(fmt, ##__VA_ARGS__)
#elif defined(DEBUG)
#define pr_debug(fmt, ...) \
printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
#else
#define pr_debug(fmt, ...) \
no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
#endif
so in the !CONFIG_DYNAMIC_DEBUG case, you want to boot with "ignore_loglevel"
and see all those debug statements but that can't work because you have to
rebuild and slap a
#define DEBUG
somewhere *before* all includes.
Which is clearly dumb.
FWIW, that pr_debug() should have been defined without the
defined(DEBUG) conditional in order to be an equivalent conversion like
the rest of the pr_* gunk.
I propose we add
pr_debug_i_mean_it()
and use *that* instead. :-)
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web