[PATCH] efi earlyprintk fix

[PATCH] efi earlyprintk fix

[Dave Young]

there's below one line shift problem:

                     ACPI=0xdabfe000  ACPI 2.0=0xdabfe014  SMBIOS=0xdaa9e000
[    0.000000] efi:  

In fact check efi_y and the lfb_height should be compared at the begin of the
loop of early_efi_write

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 arch/x86/platform/efi/early_printk.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6.orig/arch/x86/platform/efi/early_printk.c
+++ linux-2.6/arch/x86/platform/efi/early_printk.c
@@ -106,6 +106,15 @@ early_efi_write(struct console *con, con
 		if (count > linemax)
 			count = linemax;
 
+		if (efi_y + font->height >= si->lfb_height) {
+			u32 i;
+
+			efi_y -= font->height;
+			early_efi_scroll_up();
+
+			for (i = 0; i < font->height; i++)
+				early_efi_clear_scanline(efi_y + i);
+		}
 		for (h = 0; h < font->height; h++) {
 			unsigned int n, x;
 
@@ -142,15 +151,6 @@ early_efi_write(struct console *con, con
 			efi_y += font->height;
 		}
 
-		if (efi_y + font->height >= si->lfb_height) {
-			u32 i;
-
-			efi_y -= font->height;
-			early_efi_scroll_up();
-
-			for (i = 0; i < font->height; i++)
-				early_efi_clear_scanline(efi_y + i);
-		}
 	}
 }
 

Re: [PATCH] efi earlyprintk fix

[Matt Fleming]

On Sun, 03 Nov, at 08:16:47PM, Dave Young wrote:
> 
> there's below one line shift problem:
> 
>                      ACPI=0xdabfe000  ACPI 2.0=0xdabfe014  SMBIOS=0xdaa9e000
> [    0.000000] efi:  
> 
> In fact check efi_y and the lfb_height should be compared at the begin of the
> loop of early_efi_write
 
Hmm... this is interesting. I can't produce this on any of my machines.
Where did you see this? What hardware?

> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  arch/x86/platform/efi/early_printk.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- linux-2.6.orig/arch/x86/platform/efi/early_printk.c
> +++ linux-2.6/arch/x86/platform/efi/early_printk.c
> @@ -106,6 +106,15 @@ early_efi_write(struct console *con, con
>  		if (count > linemax)
>  			count = linemax;
>  
> +		if (efi_y + font->height >= si->lfb_height) {
> +			u32 i;
> +
> +			efi_y -= font->height;
> +			early_efi_scroll_up();
> +
> +			for (i = 0; i < font->height; i++)
> +				early_efi_clear_scanline(efi_y + i);
> +		}
>  		for (h = 0; h < font->height; h++) {
>  			unsigned int n, x;
>  
> @@ -142,15 +151,6 @@ early_efi_write(struct console *con, con
>  			efi_y += font->height;
>  		}
>  
> -		if (efi_y + font->height >= si->lfb_height) {
> -			u32 i;
> -
> -			efi_y -= font->height;
> -			early_efi_scroll_up();
> -
> -			for (i = 0; i < font->height; i++)
> -				early_efi_clear_scanline(efi_y + i);
> -		}
>  	}
>  }

Notice how we figure out the initial value of efi_y...

static __init int early_efi_setup(struct console *con, char *options)
{
	struct screen_info *si;
	u16 xres, yres;
	u32 i;

	si = &boot_params.screen_info;
	xres = si->lfb_width;
	yres = si->lfb_height;

	....

	efi_y = rounddown(yres, font->height) - font->height;

Every time we enter early_efi_write(), we should have enough space to
write one line, so I'm not at all sure what's going wrong with your
display.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efi earlyprintk fix

[Dave Young]

On 11/04/13 at 10:37am, Matt Fleming wrote:
> On Sun, 03 Nov, at 08:16:47PM, Dave Young wrote:
> > 
> > there's below one line shift problem:
> > 
> >                      ACPI=0xdabfe000  ACPI 2.0=0xdabfe014  SMBIOS=0xdaa9e000
> > [    0.000000] efi:  
> > 
> > In fact check efi_y and the lfb_height should be compared at the begin of the
> > loop of early_efi_write
>  
> Hmm... this is interesting. I can't produce this on any of my machines.
> Where did you see this? What hardware?

It's my laptop Thinkpad T420, the screen scolls very fast, I use boot_delay=500
to verify it with my patch for moving boot_delay param an early param.

Even without boot_delay, there's always a line at bottom with only the prefix:
"efi:"

> 
> > Signed-off-by: Dave Young <dyoung@redhat.com>
> > ---
> >  arch/x86/platform/efi/early_printk.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- linux-2.6.orig/arch/x86/platform/efi/early_printk.c
> > +++ linux-2.6/arch/x86/platform/efi/early_printk.c
> > @@ -106,6 +106,15 @@ early_efi_write(struct console *con, con
> >  		if (count > linemax)
> >  			count = linemax;
> >  
> > +		if (efi_y + font->height >= si->lfb_height) {
> > +			u32 i;
> > +
> > +			efi_y -= font->height;
> > +			early_efi_scroll_up();
> > +
> > +			for (i = 0; i < font->height; i++)
> > +				early_efi_clear_scanline(efi_y + i);
> > +		}
> >  		for (h = 0; h < font->height; h++) {
> >  			unsigned int n, x;
> >  
> > @@ -142,15 +151,6 @@ early_efi_write(struct console *con, con
> >  			efi_y += font->height;
> >  		}
> >  
> > -		if (efi_y + font->height >= si->lfb_height) {
> > -			u32 i;
> > -
> > -			efi_y -= font->height;
> > -			early_efi_scroll_up();
> > -
> > -			for (i = 0; i < font->height; i++)
> > -				early_efi_clear_scanline(efi_y + i);
> > -		}
> >  	}
> >  }
> 
> Notice how we figure out the initial value of efi_y...
> 
> static __init int early_efi_setup(struct console *con, char *options)
> {
> 	struct screen_info *si;
> 	u16 xres, yres;
> 	u32 i;
> 
> 	si = &boot_params.screen_info;
> 	xres = si->lfb_width;
> 	yres = si->lfb_height;
> 
> 	....
> 
> 	efi_y = rounddown(yres, font->height) - font->height;
> 
> Every time we enter early_efi_write(), we should have enough space to
> write one line, so I'm not at all sure what's going wrong with your
> display.
> 
> -- 
> Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efi earlyprintk fix

[Matt Fleming]

On Mon, 04 Nov, at 08:58:53PM, Dave Young wrote:
> On 11/04/13 at 10:37am, Matt Fleming wrote:
> > On Sun, 03 Nov, at 08:16:47PM, Dave Young wrote:
> > > 
> > > there's below one line shift problem:
> > > 
> > >                      ACPI=0xdabfe000  ACPI 2.0=0xdabfe014  SMBIOS=0xdaa9e000
> > > [    0.000000] efi:  
> > > 
> > > In fact check efi_y and the lfb_height should be compared at the begin of the
> > > loop of early_efi_write
> >  
> > Hmm... this is interesting. I can't produce this on any of my machines.
> > Where did you see this? What hardware?
> 
> It's my laptop Thinkpad T420, the screen scolls very fast, I use boot_delay=500
> to verify it with my patch for moving boot_delay param an early param.

Dave, how are you booting your laptop? Are you using grub? Does the
earlyprintk output start at the bottom of the screen? Do you set any
parameters for efifb?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efi earlyprintk fix

[Dave Young]

On 11/04/13 at 08:58pm, Dave Young wrote:
> On 11/04/13 at 10:37am, Matt Fleming wrote:
> > On Sun, 03 Nov, at 08:16:47PM, Dave Young wrote:
> > > 
> > > there's below one line shift problem:
> > > 
> > >                      ACPI=0xdabfe000  ACPI 2.0=0xdabfe014  SMBIOS=0xdaa9e000
> > > [    0.000000] efi:  
> > > 
> > > In fact check efi_y and the lfb_height should be compared at the begin of the
> > > loop of early_efi_write
> >  
> > Hmm... this is interesting. I can't produce this on any of my machines.
> > Where did you see this? What hardware?
> 
> It's my laptop Thinkpad T420, the screen scolls very fast, I use boot_delay=500
> to verify it with my patch for moving boot_delay param an early param.
> 
> Even without boot_delay, there's always a line at bottom with only the prefix:
> "efi:"

I will do further debug if I got time on this. It's really helpful for having efi
earlyprintk, thanks for your excellent idea of printing to efi fb!

Re: [PATCH] efi earlyprintk fix

[Matt Fleming]

On Wed, 06 Nov, at 05:23:35PM, Dave Young wrote:
> On 11/04/13 at 08:58pm, Dave Young wrote:
> > On 11/04/13 at 10:37am, Matt Fleming wrote:
> > > On Sun, 03 Nov, at 08:16:47PM, Dave Young wrote:
> > > > 
> > > > there's below one line shift problem:
> > > > 
> > > >                      ACPI=0xdabfe000  ACPI 2.0=0xdabfe014  SMBIOS=0xdaa9e000
> > > > [    0.000000] efi:  
> > > > 
> > > > In fact check efi_y and the lfb_height should be compared at the begin of the
> > > > loop of early_efi_write
> > >  
> > > Hmm... this is interesting. I can't produce this on any of my machines.
> > > Where did you see this? What hardware?
> > 
> > It's my laptop Thinkpad T420, the screen scolls very fast, I use boot_delay=500
> > to verify it with my patch for moving boot_delay param an early param.
> > 
> > Even without boot_delay, there's always a line at bottom with only the prefix:
> > "efi:"
> 
> I will do further debug if I got time on this. It's really helpful for having efi
> earlyprintk, thanks for your excellent idea of printing to efi fb!

Thanks. I suspect the code in early_efi_setup() that sets up efi_y is
somehow wrong.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efi earlyprintk fix

[Dave Young]

On 11/06/13 at 08:49am, Matt Fleming wrote:
> On Mon, 04 Nov, at 08:58:53PM, Dave Young wrote:
> > On 11/04/13 at 10:37am, Matt Fleming wrote:
> > > On Sun, 03 Nov, at 08:16:47PM, Dave Young wrote:
> > > > 
> > > > there's below one line shift problem:
> > > > 
> > > >                      ACPI=0xdabfe000  ACPI 2.0=0xdabfe014  SMBIOS=0xdaa9e000
> > > > [    0.000000] efi:  
> > > > 
> > > > In fact check efi_y and the lfb_height should be compared at the begin of the
> > > > loop of early_efi_write
> > >  
> > > Hmm... this is interesting. I can't produce this on any of my machines.
> > > Where did you see this? What hardware?
> > 
> > It's my laptop Thinkpad T420, the screen scolls very fast, I use boot_delay=500
> > to verify it with my patch for moving boot_delay param an early param.
> 
> Dave, how are you booting your laptop? Are you using grub? Does the
> earlyprintk output start at the bottom of the screen? Do you set any
> parameters for efifb?

I tried both grub and efi stub. The messages seems come from the n-1 line
of the screen. I did not set any parameters by myself.

Re: [PATCH] efi earlyprintk fix

[Dave Young]

On 11/06/13 at 09:38am, Matt Fleming wrote:
> On Wed, 06 Nov, at 05:23:35PM, Dave Young wrote:
> > On 11/04/13 at 08:58pm, Dave Young wrote:
> > > On 11/04/13 at 10:37am, Matt Fleming wrote:
> > > > On Sun, 03 Nov, at 08:16:47PM, Dave Young wrote:
> > > > > 
> > > > > there's below one line shift problem:
> > > > > 
> > > > >                      ACPI=0xdabfe000  ACPI 2.0=0xdabfe014  SMBIOS=0xdaa9e000
> > > > > [    0.000000] efi:  
> > > > > 
> > > > > In fact check efi_y and the lfb_height should be compared at the begin of the
> > > > > loop of early_efi_write
> > > >  
> > > > Hmm... this is interesting. I can't produce this on any of my machines.
> > > > Where did you see this? What hardware?
> > > 
> > > It's my laptop Thinkpad T420, the screen scolls very fast, I use boot_delay=500
> > > to verify it with my patch for moving boot_delay param an early param.
> > > 
> > > Even without boot_delay, there's always a line at bottom with only the prefix:
> > > "efi:"
> > 
> > I will do further debug if I got time on this. It's really helpful for having efi
> > earlyprintk, thanks for your excellent idea of printing to efi fb!
> 
> Thanks. I suspect the code in early_efi_setup() that sets up efi_y is
> somehow wrong.

Here is my debug message about the efifb params (all hex values):
[    0.000000] efi fb lfb width: 280
[    0.000000] efi fb lfb height: 1e0
[    0.000000] efi fb lfb base: e0000000
[    0.000000] efi fb lfb linelength: a00
[    0.000000] efi fb font height: 10
[    0.000000] efi fb font width: 8

The problem looks related to the pr_cont(), here is what I observed:
[    0.000000] efi: EFI v2.00 by Lenovo
                    ACPI=0xdabfe000  ACPI 2.0=0xdabfe014  SMBIOS=0xdaa9e000
[    0.000000] efi: 

After relooking the code, below fix is more reasonable than my original one,
But I still confused why it only happens on this machine.

--- efi.orig/arch/x86/platform/efi/early_printk.c
+++ efi/arch/x86/platform/efi/early_printk.c
@@ -95,6 +95,7 @@ early_efi_write(struct console *con, con
 	while (num) {
 		unsigned int linemax;
 		unsigned int h, count = 0;
+		bool linebreak = false;
 
 		for (s = str; *s && *s != '\n'; s++) {
 			if (count == num)
@@ -135,14 +136,16 @@ early_efi_write(struct console *con, con
 			efi_y += font->height;
 			str++;
 			num--;
+			linebreak = true;
 		}
 
 		if (efi_x >= si->lfb_width) {
 			efi_x = 0;
 			efi_y += font->height;
+			linebreak = true;
 		}
 
-		if (efi_y + font->height >= si->lfb_height) {
+		if (linebreak && (efi_y + font->height >= si->lfb_height)) {
 			u32 i;
 
 			efi_y -= font->height;

Re: [PATCH] efi earlyprintk fix

[Matt Fleming]

On Thu, 07 Nov, at 08:09:06PM, Dave Young wrote:
> Here is my debug message about the efifb params (all hex values):
> [    0.000000] efi fb lfb width: 280
> [    0.000000] efi fb lfb height: 1e0
> [    0.000000] efi fb lfb base: e0000000
> [    0.000000] efi fb lfb linelength: a00
> [    0.000000] efi fb font height: 10
> [    0.000000] efi fb font width: 8
> 
> The problem looks related to the pr_cont(), here is what I observed:
> [    0.000000] efi: EFI v2.00 by Lenovo
>                     ACPI=0xdabfe000  ACPI 2.0=0xdabfe014  SMBIOS=0xdaa9e000
> [    0.000000] efi: 
> 
> After relooking the code, below fix is more reasonable than my original one,
> But I still confused why it only happens on this machine.

Dave, could you try this patch?

---

diff --git a/arch/x86/platform/efi/early_printk.c b/arch/x86/platform/efi/early_printk.c
index 6599a0027b76..81b506d5befd 100644
--- a/arch/x86/platform/efi/early_printk.c
+++ b/arch/x86/platform/efi/early_printk.c
@@ -142,7 +142,7 @@ early_efi_write(struct console *con, const char *str, unsigned int num)
 			efi_y += font->height;
 		}
 
-		if (efi_y + font->height >= si->lfb_height) {
+		if (efi_y + font->height > si->lfb_height) {
 			u32 i;
 
 			efi_y -= font->height;
-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efi earlyprintk fix

[Dave Young]

On 11/08/13 at 10:36am, Matt Fleming wrote:
> On Thu, 07 Nov, at 08:09:06PM, Dave Young wrote:
> > Here is my debug message about the efifb params (all hex values):
> > [    0.000000] efi fb lfb width: 280
> > [    0.000000] efi fb lfb height: 1e0
> > [    0.000000] efi fb lfb base: e0000000
> > [    0.000000] efi fb lfb linelength: a00
> > [    0.000000] efi fb font height: 10
> > [    0.000000] efi fb font width: 8
> > 
> > The problem looks related to the pr_cont(), here is what I observed:
> > [    0.000000] efi: EFI v2.00 by Lenovo
> >                     ACPI=0xdabfe000  ACPI 2.0=0xdabfe014  SMBIOS=0xdaa9e000
> > [    0.000000] efi: 
> > 
> > After relooking the code, below fix is more reasonable than my original one,
> > But I still confused why it only happens on this machine.
> 
> Dave, could you try this patch?

Hi, Matt

Confirmed, your patch work ok for the pr_cont issue.

Actually I mixed two problems in my report, one is there's always one blank line
at the bottom of screen, the other is the pr_cont issue. For the second problem
change >= to > looks better. For the previous problem still need to move the
hunk below to the beginning point of early_efi_write function. It just works for me
but I'm not sure why this is not necessary for other machines. 

+		if (efi_y + font->height > si->lfb_height) {
+			u32 i;
+
+			efi_y -= font->height;
+			early_efi_scroll_up();
+
+			for (i = 0; i < font->height; i++)
+				early_efi_clear_scanline(efi_y + i);
+		}


> 
> ---
> 
> diff --git a/arch/x86/platform/efi/early_printk.c b/arch/x86/platform/efi/early_printk.c
> index 6599a0027b76..81b506d5befd 100644
> --- a/arch/x86/platform/efi/early_printk.c
> +++ b/arch/x86/platform/efi/early_printk.c
> @@ -142,7 +142,7 @@ early_efi_write(struct console *con, const char *str, unsigned int num)
>  			efi_y += font->height;
>  		}
>  
> -		if (efi_y + font->height >= si->lfb_height) {
> +		if (efi_y + font->height > si->lfb_height) {
>  			u32 i;
>  
>  			efi_y -= font->height;
> -- 
> Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efi earlyprintk fix

[Matt Fleming]

On Sat, 09 Nov, at 11:44:29AM, Dave Young wrote:
> Hi, Matt
> 
> Confirmed, your patch work ok for the pr_cont issue.
 
Excellent, thanks for testing.

> Actually I mixed two problems in my report, one is there's always one blank line
> at the bottom of screen, the other is the pr_cont issue. For the second problem
> change >= to > looks better. For the previous problem still need to move the
> hunk below to the beginning point of early_efi_write function. It just works for me
> but I'm not sure why this is not necessary for other machines. 
> 
> +		if (efi_y + font->height > si->lfb_height) {
> +			u32 i;
> +
> +			efi_y -= font->height;
> +			early_efi_scroll_up();
> +
> +			for (i = 0; i < font->height; i++)
> +				early_efi_clear_scanline(efi_y + i);
> +		}

Dave, unfortunately you're going to need to debug this further because
it doesn't make any sense that your patch is needed, unless there's a
bug in the existing code.

You may also want to rule out that your boot_delay patches are not
inserting delays after printing pr_fmt().

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efi earlyprintk fix

[Dave Young]

On 11/11/13 at 04:13pm, Matt Fleming wrote:
> On Sat, 09 Nov, at 11:44:29AM, Dave Young wrote:
> > Hi, Matt
> > 
> > Confirmed, your patch work ok for the pr_cont issue.
>  
> Excellent, thanks for testing.
> 
> > Actually I mixed two problems in my report, one is there's always one blank line
> > at the bottom of screen, the other is the pr_cont issue. For the second problem
> > change >= to > looks better. For the previous problem still need to move the
> > hunk below to the beginning point of early_efi_write function. It just works for me
> > but I'm not sure why this is not necessary for other machines. 
> > 
> > +		if (efi_y + font->height > si->lfb_height) {
> > +			u32 i;
> > +
> > +			efi_y -= font->height;
> > +			early_efi_scroll_up();
> > +
> > +			for (i = 0; i < font->height; i++)
> > +				early_efi_clear_scanline(efi_y + i);
> > +		}
> 
> Dave, unfortunately you're going to need to debug this further because
> it doesn't make any sense that your patch is needed, unless there's a
> bug in the existing code.

Ok, will dig more about this and find out why it only happens on this machine.

> 
> You may also want to rule out that your boot_delay patches are not
> inserting delays after printing pr_fmt().

In fact with or without the delay testing results are same. There's might be
something wrong in the setup as you said. I will update once I have got something.

Thanks
Dave

Re: [PATCH] efi earlyprintk fix

[Dave Young]

> It's my laptop Thinkpad T420, the screen scolls very fast, I use boot_delay=500
> to verify it with my patch for moving boot_delay param an early param.
> 
> Even without boot_delay, there's always a line at bottom with only the prefix:
> "efi:"

Update about this issue:
With latest mainline tree + efi runtime patchset, above problem does not happen
any more (with and without boot_delay).

Thanks
Dave

Re: [PATCH] efi earlyprintk fix

[Matt Fleming]

On Wed, 18 Dec, at 05:52:56PM, Dave Young wrote:
> > It's my laptop Thinkpad T420, the screen scolls very fast, I use boot_delay=500
> > to verify it with my patch for moving boot_delay param an early param.
> > 
> > Even without boot_delay, there's always a line at bottom with only the prefix:
> > "efi:"
> 
> Update about this issue:
> With latest mainline tree + efi runtime patchset, above problem does not happen
> any more (with and without boot_delay).

Thanks for the update.

-- 
Matt Fleming, Intel Open Source Technology Center