On 01/30/15 at 05:43pm, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
> Date: Mon, 26 Jan 2015 19:49:59 +0100
> Subject: [PATCH] efi, x86: Add a "debug" option to the efi= cmdline
>
> ... and hide the memory regions dump behind it. Make it default-off.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> ---
> arch/x86/platform/efi/efi.c | 5 ++++-
> include/linux/efi.h | 1 +
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index dbc8627a5cdf..e859d56ce9f8 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -491,7 +491,8 @@ void __init efi_init(void)
> if (efi_memmap_init())
> return;
>
> - print_efi_memmap();
> + if (efi_enabled(EFI_DBG))
> + print_efi_memmap();
> }
>
> void __init efi_late_init(void)
> @@ -939,6 +940,8 @@ static int __init arch_parse_efi_cmdline(char *str)
> {
> if (parse_option_str(str, "old_map"))
> set_bit(EFI_OLD_MEMMAP, &efi.flags);
> + if (parse_option_str(str, "debug"))
> + set_bit(EFI_DBG, &efi.flags);
>
> return 0;
> }
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 0238d612750e..14cec75d7e74 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -940,6 +940,7 @@ extern int __init efi_setup_pcdp_console(char *);
> #define EFI_64BIT 5 /* Is the firmware 64-bit? */
> #define EFI_PARAVIRT 6 /* Access is via a paravirt interface */
> #define EFI_ARCH_1 7 /* First arch-specific bit */
> +#define EFI_DBG 8 /* Print additional debug info at runtime */
Boris, a nickpick about alignment of the second column..
Thanks
Dave
On Thu, Feb 05, 2015 at 11:18:46AM +0800, Dave Young wrote:
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 0238d612750e..14cec75d7e74 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -940,6 +940,7 @@ extern int __init efi_setup_pcdp_console(char *);
> > #define EFI_64BIT 5 /* Is the firmware 64-bit? */
> > #define EFI_PARAVIRT 6 /* Access is via a paravirt interface */
> > #define EFI_ARCH_1 7 /* First arch-specific bit */
> > +#define EFI_DBG 8 /* Print additional debug info at runtime */
>
> Boris, a nickpick about alignment of the second column..
This is due to how diffs show tabs. See EFI_PARAVIRT above, it appears
misaligned too. If you switch to list mode in vim, for example, it'll
show you how many tabs are being used:
#define EFI_PARAVIRT^I^I6^I/* Access is via a paravirt interface */$
#define EFI_ARCH_1^I^I7^I/* First arch-specific bit */$
#define EFI_DBG^I^I^I8^I/* Print additional debug info at runtime */$
and EFI_DBG has an additional tab. However, the vertical alignment is
correct. You can apply the patch and see for yourself :-)
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On 02/05/15 at 09:11am, Borislav Petkov wrote:
> On Thu, Feb 05, 2015 at 11:18:46AM +0800, Dave Young wrote:
> > > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > > index 0238d612750e..14cec75d7e74 100644
> > > --- a/include/linux/efi.h
> > > +++ b/include/linux/efi.h
> > > @@ -940,6 +940,7 @@ extern int __init efi_setup_pcdp_console(char *);
> > > #define EFI_64BIT 5 /* Is the firmware 64-bit? */
> > > #define EFI_PARAVIRT 6 /* Access is via a paravirt interface */
> > > #define EFI_ARCH_1 7 /* First arch-specific bit */
> > > +#define EFI_DBG 8 /* Print additional debug info at runtime */
> >
> > Boris, a nickpick about alignment of the second column..
>
> This is due to how diffs show tabs. See EFI_PARAVIRT above, it appears
> misaligned too. If you switch to list mode in vim, for example, it'll
> show you how many tabs are being used:
>
> #define EFI_PARAVIRT^I^I6^I/* Access is via a paravirt interface */$
> #define EFI_ARCH_1^I^I7^I/* First arch-specific bit */$
> #define EFI_DBG^I^I^I8^I/* Print additional debug info at runtime */$
>
> and EFI_DBG has an additional tab. However, the vertical alignment is
> correct. You can apply the patch and see for yourself :-)
Interesting, you are right :)
Acked-by: Dave Young <[email protected]>
Thanks
Dave
From: Borislav Petkov <[email protected]>
Subject: [PATCH v2] efi, x86: Add a "debug" option to the efi= cmdline
... and hide the memory regions dump behind it. Make it default-off.
Signed-off-by: Borislav Petkov <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Acked-by: Laszlo Ersek <[email protected]>
Acked-by: Dave Young <[email protected]>
---
Documentation/kernel-parameters.txt | 3 ++-
arch/x86/platform/efi/efi.c | 5 ++++-
include/linux/efi.h | 1 +
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 176d4fe4f076..101ab5601c2d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1024,7 +1024,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
Format: {"off" | "on" | "skip[mbr]"}
efi= [EFI]
- Format: { "old_map", "nochunk", "noruntime" }
+ Format: { "old_map", "nochunk", "noruntime", "debug" }
old_map [X86-64]: switch to the old ioremap-based EFI
runtime services mapping. 32-bit still uses this one by
default.
@@ -1032,6 +1032,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
boot stub, as chunking can cause problems with some
firmware implementations.
noruntime : disable EFI runtime services support
+ debug: enable misc debug output
efi_no_storage_paranoia [EFI; X86]
Using this parameter you can use more than 50% of
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index dbc8627a5cdf..e859d56ce9f8 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -491,7 +491,8 @@ void __init efi_init(void)
if (efi_memmap_init())
return;
- print_efi_memmap();
+ if (efi_enabled(EFI_DBG))
+ print_efi_memmap();
}
void __init efi_late_init(void)
@@ -939,6 +940,8 @@ static int __init arch_parse_efi_cmdline(char *str)
{
if (parse_option_str(str, "old_map"))
set_bit(EFI_OLD_MEMMAP, &efi.flags);
+ if (parse_option_str(str, "debug"))
+ set_bit(EFI_DBG, &efi.flags);
return 0;
}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 0238d612750e..14cec75d7e74 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -940,6 +940,7 @@ extern int __init efi_setup_pcdp_console(char *);
#define EFI_64BIT 5 /* Is the firmware 64-bit? */
#define EFI_PARAVIRT 6 /* Access is via a paravirt interface */
#define EFI_ARCH_1 7 /* First arch-specific bit */
+#define EFI_DBG 8 /* Print additional debug info at runtime */
#ifdef CONFIG_EFI
/*
--
2.2.0.33.gc18b867
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
It is better if we add some printk from efifb messages.
Please review the below patch.
>From 7fbac896ab87f1b37646ac2f49bb8216ec330642 Mon Sep 17 00:00:00 2001
From: Parmeshwr Prasad <[email protected]>
Date: Wed, 4 Feb 2015 06:50:32 -0500
Subject: [PATCH] efi, x86: Add a debug option to the efi= cmdline
Signed-off-by: Parmeshwr Prasad <[email protected]>
---
drivers/video/fbdev/efifb.c | 49 +++++++++++++++++++++++++--------------------
1 file changed, 27 insertions(+), 22 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 4bfff34..505bc56 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -145,7 +145,8 @@ static int efifb_probe(struct platform_device *dev)
printk(KERN_DEBUG "efifb: invalid framebuffer address\n");
return -ENODEV;
}
- printk(KERN_INFO "efifb: probing for efifb\n");
+ if (efi_enabled(EFI_DBG))
+ printk(KERN_INFO "efifb: probing for efifb\n");
/* just assume they're all unset if any are */
if (!screen_info.blue_size) {
@@ -224,20 +225,22 @@ static int efifb_probe(struct platform_device *dev)
err = -EIO;
goto err_release_fb;
}
-
- printk(KERN_INFO "efifb: framebuffer at 0x%lx, mapped to 0x%p, "
- "using %dk, total %dk\n",
- efifb_fix.smem_start, info->screen_base,
- size_remap/1024, size_total/1024);
- printk(KERN_INFO "efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
- efifb_defined.xres, efifb_defined.yres,
- efifb_defined.bits_per_pixel, efifb_fix.line_length,
- screen_info.pages);
+ if (efi_enabled(EFI_DBG)){
+ printk(KERN_INFO "efifb: framebuffer at 0x%lx, mapped to 0x%p, "
+ "using %dk, total %dk\n",
+ efifb_fix.smem_start, info->screen_base,
+ size_remap/1024, size_total/1024);
+ printk(KERN_INFO "efifb: mode is %dx%dx%d, linelength=%d, pages=%d\n",
+ efifb_defined.xres, efifb_defined.yres,
+ efifb_defined.bits_per_pixel, efifb_fix.line_length,
+ screen_info.pages);
+ }
efifb_defined.xres_virtual = efifb_defined.xres;
efifb_defined.yres_virtual = efifb_fix.smem_len /
efifb_fix.line_length;
- printk(KERN_INFO "efifb: scrolling: redraw\n");
+ if (efi_enabled(EFI_DBG))
+ printk(KERN_INFO "efifb: scrolling: redraw\n");
efifb_defined.yres_virtual = efifb_defined.yres;
/* some dummy values for timing to make fbset happy */
@@ -255,17 +258,19 @@ static int efifb_probe(struct platform_device *dev)
efifb_defined.transp.offset = screen_info.rsvd_pos;
efifb_defined.transp.length = screen_info.rsvd_size;
- printk(KERN_INFO "efifb: %s: "
- "size=%d:%d:%d:%d, shift=%d:%d:%d:%d\n",
- "Truecolor",
- screen_info.rsvd_size,
- screen_info.red_size,
- screen_info.green_size,
- screen_info.blue_size,
- screen_info.rsvd_pos,
- screen_info.red_pos,
- screen_info.green_pos,
- screen_info.blue_pos);
+ if (efi_enabled(EFI_DBG)){
+ printk(KERN_INFO "efifb: %s: "
+ "size=%d:%d:%d:%d, shift=%d:%d:%d:%d\n",
+ "Truecolor",
+ screen_info.rsvd_size,
+ screen_info.red_size,
+ screen_info.green_size,
+ screen_info.blue_size,
+ screen_info.rsvd_pos,
+ screen_info.red_pos,
+ screen_info.green_pos,
+ screen_info.blue_pos);
+ }
efifb_fix.ypanstep = 0;
efifb_fix.ywrapstep = 0;
--
1.9.3
On Thu, Feb 05, 2015 at 04:44:41AM -0600, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
> Subject: [PATCH v2] efi, x86: Add a "debug" option to the efi= cmdline
>
> ... and hide the memory regions dump behind it. Make it default-off.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Acked-by: Laszlo Ersek <[email protected]>
> Acked-by: Dave Young <[email protected]>
> ---
> Documentation/kernel-parameters.txt | 3 ++-
> arch/x86/platform/efi/efi.c | 5 ++++-
> include/linux/efi.h | 1 +
> 3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 176d4fe4f076..101ab5601c2d 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1024,7 +1024,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> Format: {"off" | "on" | "skip[mbr]"}
>
> efi= [EFI]
> - Format: { "old_map", "nochunk", "noruntime" }
> + Format: { "old_map", "nochunk", "noruntime", "debug" }
> old_map [X86-64]: switch to the old ioremap-based EFI
> runtime services mapping. 32-bit still uses this one by
> default.
> @@ -1032,6 +1032,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> boot stub, as chunking can cause problems with some
> firmware implementations.
> noruntime : disable EFI runtime services support
> + debug: enable misc debug output
>
> efi_no_storage_paranoia [EFI; X86]
> Using this parameter you can use more than 50% of
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index dbc8627a5cdf..e859d56ce9f8 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -491,7 +491,8 @@ void __init efi_init(void)
> if (efi_memmap_init())
> return;
>
> - print_efi_memmap();
> + if (efi_enabled(EFI_DBG))
> + print_efi_memmap();
> }
>
> void __init efi_late_init(void)
> @@ -939,6 +940,8 @@ static int __init arch_parse_efi_cmdline(char *str)
> {
> if (parse_option_str(str, "old_map"))
> set_bit(EFI_OLD_MEMMAP, &efi.flags);
> + if (parse_option_str(str, "debug"))
> + set_bit(EFI_DBG, &efi.flags);
>
> return 0;
> }
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 0238d612750e..14cec75d7e74 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -940,6 +940,7 @@ extern int __init efi_setup_pcdp_console(char *);
> #define EFI_64BIT 5 /* Is the firmware 64-bit? */
> #define EFI_PARAVIRT 6 /* Access is via a paravirt interface */
> #define EFI_ARCH_1 7 /* First arch-specific bit */
> +#define EFI_DBG 8 /* Print additional debug info at runtime */
>
> #ifdef CONFIG_EFI
> /*
> --
> 2.2.0.33.gc18b867
>
> --
> Regards/Gruss,
> Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 05, 2015 at 07:45:10AM -0500, Parmeshwr Prasad wrote:
> It is better if we add some printk from efifb messages.
> Please review the below patch.
First of all, we saw you patch.
Then,
> From 7fbac896ab87f1b37646ac2f49bb8216ec330642 Mon Sep 17 00:00:00 2001
> From: Parmeshwr Prasad <[email protected]>
> Date: Wed, 4 Feb 2015 06:50:32 -0500
> Subject: [PATCH] efi, x86: Add a debug option to the efi= cmdline
yours has the same Subject as mine. This is not how we do patches.
Consult Documentation/SubmittingPatches about how to do them properly.
What is more, your patch doesn't have a commit message. But it needs one.
Fix all that, send it as a *separate* patch after mine has been applied
and people will take a look then.
>
> Signed-off-by: Parmeshwr Prasad <[email protected]>
> ---
> drivers/video/fbdev/efifb.c | 49 +++++++++++++++++++++++++--------------------
> 1 file changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 4bfff34..505bc56 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -145,7 +145,8 @@ static int efifb_probe(struct platform_device *dev)
> printk(KERN_DEBUG "efifb: invalid framebuffer address\n");
> return -ENODEV;
> }
> - printk(KERN_INFO "efifb: probing for efifb\n");
> + if (efi_enabled(EFI_DBG))
> + printk(KERN_INFO "efifb: probing for efifb\n");
If we're going to use the "efifb" prefix, change those to pr_info and
define pr_fmt - lotsa examples in the kernel sources.
More importantly, you'd need a consensus from people that the printks in
efifb are really not interesting and should be behind efi=debug.
HTH.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Thu, Feb 05, 2015 at 08:28:58AM -0600, Borislav Petkov wrote:
> On Thu, Feb 05, 2015 at 07:45:10AM -0500, Parmeshwr Prasad wrote:
> > It is better if we add some printk from efifb messages.
> > Please review the below patch.
>
> First of all, we saw you patch.
>
> Then,
>
> > From 7fbac896ab87f1b37646ac2f49bb8216ec330642 Mon Sep 17 00:00:00 2001
> > From: Parmeshwr Prasad <[email protected]>
> > Date: Wed, 4 Feb 2015 06:50:32 -0500
> > Subject: [PATCH] efi, x86: Add a debug option to the efi= cmdline
>
> yours has the same Subject as mine. This is not how we do patches.
> Consult Documentation/SubmittingPatches about how to do them properly.
>
> What is more, your patch doesn't have a commit message. But it needs one.
>
> Fix all that, send it as a *separate* patch after mine has been applied
> and people will take a look then.
I am really sorry that I could not mentioned for what purpose this patch is .
I wanted this patch to be included along with your patch. As your patch is
To add “debug” cmdline in efi=”<option>”.
There are some messages in efifb which appears while booting. I thought of
Putting that messages under efi=”debug” cmdline. When efi=debug is given
Then only efifb messages should appear.
>
> >
> > Signed-off-by: Parmeshwr Prasad <[email protected]>
> > ---
> > drivers/video/fbdev/efifb.c | 49 +++++++++++++++++++++++++--------------------
> > 1 file changed, 27 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> > index 4bfff34..505bc56 100644
> > --- a/drivers/video/fbdev/efifb.c
> > +++ b/drivers/video/fbdev/efifb.c
> > @@ -145,7 +145,8 @@ static int efifb_probe(struct platform_device *dev)
> > printk(KERN_DEBUG "efifb: invalid framebuffer address\n");
> > return -ENODEV;
> > }
> > - printk(KERN_INFO "efifb: probing for efifb\n");
> > + if (efi_enabled(EFI_DBG))
> > + printk(KERN_INFO "efifb: probing for efifb\n");
>
> If we're going to use the "efifb" prefix, change those to pr_info and
> define pr_fmt - lotsa examples in the kernel sources.
>
> More importantly, you'd need a consensus from people that the printks in
> efifb are really not interesting and should be behind efi=debug.
>
> HTH.
>
> --
> Regards/Gruss,
> Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 06, 2015 at 01:00:12AM -0500, Parmeshwr Prasad wrote:
> I am really sorry that I could not mentioned for what purpose this patch is .
> I wanted this patch to be included along with your patch. As your patch is
> To add “debug” cmdline in efi=”<option>”.
>
> There are some messages in efifb which appears while booting. I thought of
> Putting that messages under efi=”debug” cmdline. When efi=debug is given
> Then only efifb messages should appear.
This is not how we do patches. If you feel that those printks in efifb
are too verbose and should be behind a debug switch or, even removed
completely, you can do a separate patch as I explained to you and write
in that commit message what needs to be done and *why* you think it
should be done.
HTH.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Thu, 05 Feb, at 11:44:41AM, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
> Subject: [PATCH v2] efi, x86: Add a "debug" option to the efi= cmdline
>
> ... and hide the memory regions dump behind it. Make it default-off.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Acked-by: Laszlo Ersek <[email protected]>
> Acked-by: Dave Young <[email protected]>
> ---
> Documentation/kernel-parameters.txt | 3 ++-
> arch/x86/platform/efi/efi.c | 5 ++++-
> include/linux/efi.h | 1 +
> 3 files changed, 7 insertions(+), 2 deletions(-)
Thanks Borislav, applied!
--
Matt Fleming, Intel Open Source Technology Center