From: Matt Fleming <matt@codeblueprint.co.uk> To: "Elliott, Robert (Persistent Memory)" <elliott@hpe.com> Cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org, linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Leif Lindholm <leif.lindholm@linaro.org> Subject: Re: [PATCH 1/4] x86/efi: show actual ending addresses in efi_print_memmap Date: Wed, 23 Dec 2015 12:47:11 +0000 [thread overview] Message-ID: <20151223124711.GB2471@codeblueprint.co.uk> (raw) In-Reply-To: <94D0CD8314A33A4D9D801C0FE68B40295BEC3C9C@G9W0745.americas.hpqcorp.net> On Mon, 21 Dec, at 04:44:19PM, Elliott, Robert (Persistent Memory) wrote: > > The format of that line is architecture-specific and only appears > under the efi=debug kernel parameter, so I don't know how much > anyone relies on the specific format. Good question for the lists. Both kernel and non-kernel developers are consumers of these debug statements, and people do come to rely on the format of the text. I have certainly become acustomed to the current format we have, particularly when debugging issues via other users, i.e. when I'm not the person running the kernel being debugged. Just because it's a debug option doesn't mean it's completely open to modification. Reasonable Justification must be provided. Having said that, I think you provide a good argument in your other email, https://lkml.kernel.org/r/94D0CD8314A33A4D9D801C0FE68B40295BEC74CF@G9W0745.americas.hpqcorp.net > arch/ia64/kernel/efi.c shares the range=[...) format, but prints > the range after the bitmask rather than before: > printk("mem%02d: %s " > "range=[0x%016lx-0x%016lx) (%4lu%s)\n", > i, efi_md_typeattr_format(buf, sizeof(buf), md), > md->phys_addr, > md->phys_addr + efi_md_size(md), size, unit); > > arch/arm64/kernel/efi.c has no mem prefix, or range=[...) text > surrounding the range: > pr_info(" 0x%012llx-0x%012llx %s", > paddr, paddr + (npages << EFI_PAGE_SHIFT) - 1, > efi_md_typeattr_format(buf, sizeof(buf), md)); > pr_cont("*"); > pr_cont("\n"); > > The x86 code is inside ifdef EFI_DEBUG, which is always > defined in that file. I wonder if it was supposed to change > to efi_enabled(EFI_DBG) to be based off the efi=debug kernel > parameter? efi_init() qualified the call to this function > based on that: > if (efi_enabled(EFI_DBG)) > efi_print_memmap(); The EFI_DEBUG symbol should just be deleted. It's been enabled since forever and really provides no value, because as you point out, printing of the memory map is guarded by EFI_DBG anyway. I'll send out a patch for ripping that out. > In arch/ia64/kernel/efi.c efi_init(), EFI_DEBUG is set to 0 > so the print doesn't happen at all without editing the > source code. It doesn't use efi_enabled(EFI_DBG). > > arch/arm64/kernel/efi.c uses efi_enabled(EFI_DBG) exclusively.
WARNING: multiple messages have this Message-ID (diff)
From: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> To: "Elliott, Robert (Persistent Memory)" <elliott-ZPxbGqLxI0U@public.gmane.org> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>, x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> Subject: Re: [PATCH 1/4] x86/efi: show actual ending addresses in efi_print_memmap Date: Wed, 23 Dec 2015 12:47:11 +0000 [thread overview] Message-ID: <20151223124711.GB2471@codeblueprint.co.uk> (raw) In-Reply-To: <94D0CD8314A33A4D9D801C0FE68B40295BEC3C9C-W1gbDvblbosSZAcGdq5asR6epYMZPwEe5NbjCUgZEJk@public.gmane.org> On Mon, 21 Dec, at 04:44:19PM, Elliott, Robert (Persistent Memory) wrote: > > The format of that line is architecture-specific and only appears > under the efi=debug kernel parameter, so I don't know how much > anyone relies on the specific format. Good question for the lists. Both kernel and non-kernel developers are consumers of these debug statements, and people do come to rely on the format of the text. I have certainly become acustomed to the current format we have, particularly when debugging issues via other users, i.e. when I'm not the person running the kernel being debugged. Just because it's a debug option doesn't mean it's completely open to modification. Reasonable Justification must be provided. Having said that, I think you provide a good argument in your other email, https://lkml.kernel.org/r/94D0CD8314A33A4D9D801C0FE68B40295BEC74CF-W1gbDvblbosSZAcGdq5asR6epYMZPwEe5NbjCUgZEJk@public.gmane.org > arch/ia64/kernel/efi.c shares the range=[...) format, but prints > the range after the bitmask rather than before: > printk("mem%02d: %s " > "range=[0x%016lx-0x%016lx) (%4lu%s)\n", > i, efi_md_typeattr_format(buf, sizeof(buf), md), > md->phys_addr, > md->phys_addr + efi_md_size(md), size, unit); > > arch/arm64/kernel/efi.c has no mem prefix, or range=[...) text > surrounding the range: > pr_info(" 0x%012llx-0x%012llx %s", > paddr, paddr + (npages << EFI_PAGE_SHIFT) - 1, > efi_md_typeattr_format(buf, sizeof(buf), md)); > pr_cont("*"); > pr_cont("\n"); > > The x86 code is inside ifdef EFI_DEBUG, which is always > defined in that file. I wonder if it was supposed to change > to efi_enabled(EFI_DBG) to be based off the efi=debug kernel > parameter? efi_init() qualified the call to this function > based on that: > if (efi_enabled(EFI_DBG)) > efi_print_memmap(); The EFI_DEBUG symbol should just be deleted. It's been enabled since forever and really provides no value, because as you point out, printing of the memory map is guarded by EFI_DBG anyway. I'll send out a patch for ripping that out. > In arch/ia64/kernel/efi.c efi_init(), EFI_DEBUG is set to 0 > so the print doesn't happen at all without editing the > source code. It doesn't use efi_enabled(EFI_DBG). > > arch/arm64/kernel/efi.c uses efi_enabled(EFI_DBG) exclusively.
next prev parent reply other threads:[~2015-12-23 12:47 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-12-18 1:28 Robert Elliott 2015-12-18 1:28 ` (unknown), Robert Elliott 2015-12-18 1:28 ` [PATCH 1/4] x86/efi: show actual ending addresses in efi_print_memmap Robert Elliott 2015-12-21 15:50 ` Matt Fleming 2015-12-21 16:06 ` Matt Fleming 2015-12-21 16:06 ` Matt Fleming 2015-12-22 20:08 ` Elliott, Robert (Persistent Memory) 2015-12-22 20:08 ` Elliott, Robert (Persistent Memory) 2015-12-23 12:44 ` Matt Fleming 2015-12-21 16:44 ` Elliott, Robert (Persistent Memory) 2015-12-23 12:47 ` Matt Fleming [this message] 2015-12-23 12:47 ` Matt Fleming 2015-12-24 1:07 ` [PATCH v2 " Robert Elliott 2015-12-24 1:07 ` Robert Elliott 2016-01-08 12:04 ` Matt Fleming 2015-12-18 1:28 ` [PATCH 2/4] efi: add NV memory attribute Robert Elliott 2015-12-21 15:54 ` Matt Fleming 2015-12-21 15:54 ` Matt Fleming 2015-12-18 1:28 ` [PATCH 3/4] efi: add Persistent Memory type name Robert Elliott 2016-01-08 12:20 ` Matt Fleming 2016-01-08 12:20 ` Matt Fleming 2015-12-18 1:28 ` [PATCH 4/4] x86/efi: print size and base in binary units in efi_print_memmap Robert Elliott 2015-12-18 1:28 ` Robert Elliott 2015-12-21 16:16 ` Matt Fleming 2015-12-21 16:16 ` Matt Fleming 2015-12-23 0:11 ` Elliott, Robert (Persistent Memory) 2015-12-23 0:11 ` Elliott, Robert (Persistent Memory) 2015-12-23 15:52 ` Matt Fleming 2015-12-23 15:52 ` Matt Fleming 2015-12-27 14:35 ` Andy Shevchenko 2015-12-27 14:35 ` Andy Shevchenko 2016-01-08 12:19 ` Matt Fleming 2016-01-08 16:38 ` Elliott, Robert (Persistent Memory) 2016-01-08 16:38 ` Elliott, Robert (Persistent Memory) 2016-01-08 16:44 ` Andy Shevchenko 2016-01-08 16:39 ` Andy Shevchenko 2016-01-08 16:39 ` Andy Shevchenko 2016-01-11 14:09 ` Matt Fleming 2016-01-12 13:17 ` Andy Shevchenko 2016-01-12 13:17 ` Andy Shevchenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20151223124711.GB2471@codeblueprint.co.uk \ --to=matt@codeblueprint.co.uk \ --cc=ard.biesheuvel@linaro.org \ --cc=elliott@hpe.com \ --cc=hpa@zytor.com \ --cc=leif.lindholm@linaro.org \ --cc=linux-efi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mingo@kernel.org \ --cc=tglx@linutronix.de \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.