[PATCH v3] x86: Do not write into VGA memory space if there is no VGA device in the system

[PATCH v3] x86: Do not write into VGA memory space if there is no VGA device in the system

[Guenter Roeck]

Ensure that early_printk() does not write into VGA memory space
if there is not VGA device in the system.

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
v3:
Changes are now limited to early_printk.c. Functionality is retained, meaning
there will still be output on the VGA console before setup_early_printk()
is called, but only if a VGA device is known to exist in the system.

 arch/x86/kernel/early_printk.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index b9c830c..783cb25 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -170,10 +170,13 @@ asmlinkage void early_printk(const char *fmt, ...)
 	int n;
 	va_list ap;
 
-	va_start(ap, fmt);
-	n = vscnprintf(buf, sizeof(buf), fmt, ap);
-	early_console->write(early_console, buf, n);
-	va_end(ap);
+	if (early_console && (early_console_initialized ||
+			      boot_params.screen_info.orig_video_isVGA == 1)) {
+		va_start(ap, fmt);
+		n = vscnprintf(buf, sizeof(buf), fmt, ap);
+		early_console->write(early_console, buf, n);
+		va_end(ap);
+	}
 }
 
 static inline void early_console_register(struct console *con, int keep_early)
@@ -233,6 +236,12 @@ static int __init setup_early_printk(char *buf)
 #endif
 		buf++;
 	}
+
+	/* If there is no VGA device, don't try to use it as early console. */
+	if (early_console == &early_vga_console &&
+	    boot_params.screen_info.orig_video_isVGA != 1)
+		early_console = NULL;
+
 	return 0;
 }
 
-- 
1.7.0.87.g0901d

Re: [PATCH v3] x86: Do not write into VGA memory space if there is no VGA device in the system

[Pekka Enberg]

Guenter Roeck kirjoitti:
> Ensure that early_printk() does not write into VGA memory space
> if there is not VGA device in the system.
> 
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
> v3:
> Changes are now limited to early_printk.c. Functionality is retained, meaning
> there will still be output on the VGA console before setup_early_printk()
> is called, but only if a VGA device is known to exist in the system.
> 
>  arch/x86/kernel/early_printk.c |   17 +++++++++++++----
>  1 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> index b9c830c..783cb25 100644
> --- a/arch/x86/kernel/early_printk.c
> +++ b/arch/x86/kernel/early_printk.c
> @@ -170,10 +170,13 @@ asmlinkage void early_printk(const char *fmt, ...)
>  	int n;
>  	va_list ap;
>  
> -	va_start(ap, fmt);
> -	n = vscnprintf(buf, sizeof(buf), fmt, ap);
> -	early_console->write(early_console, buf, n);
> -	va_end(ap);
> +	if (early_console && (early_console_initialized ||
> +			      boot_params.screen_info.orig_video_isVGA == 1)) {
> +		va_start(ap, fmt);
> +		n = vscnprintf(buf, sizeof(buf), fmt, ap);
> +		early_console->write(early_console, buf, n);
> +		va_end(ap);
> +	}
>  }
>  
>  static inline void early_console_register(struct console *con, int keep_early)
> @@ -233,6 +236,12 @@ static int __init setup_early_printk(char *buf)
>  #endif
>  		buf++;
>  	}
> +
> +	/* If there is no VGA device, don't try to use it as early console. */
> +	if (early_console == &early_vga_console &&
> +	    boot_params.screen_info.orig_video_isVGA != 1)
> +		early_console = NULL;

Wouldn't it be better to switch to, say, serial console here to avoid 
the NULL check in early_printk()?

> +
>  	return 0;
>  }
> 

Re: [PATCH v3] x86: Do not write into VGA memory space if there is no VGA device in the system

[Guenter Roeck]

On Mon, Apr 26, 2010 at 05:36:05AM -0400, Pekka Enberg wrote:
> Guenter Roeck kirjoitti:
> > Ensure that early_printk() does not write into VGA memory space
> > if there is not VGA device in the system.
> > 
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> > v3:
> > Changes are now limited to early_printk.c. Functionality is retained, meaning
> > there will still be output on the VGA console before setup_early_printk()
> > is called, but only if a VGA device is known to exist in the system.
> > 
> >  arch/x86/kernel/early_printk.c |   17 +++++++++++++----
> >  1 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> > index b9c830c..783cb25 100644
> > --- a/arch/x86/kernel/early_printk.c
> > +++ b/arch/x86/kernel/early_printk.c
> > @@ -170,10 +170,13 @@ asmlinkage void early_printk(const char *fmt, ...)
> >  	int n;
> >  	va_list ap;
> >  
> > -	va_start(ap, fmt);
> > -	n = vscnprintf(buf, sizeof(buf), fmt, ap);
> > -	early_console->write(early_console, buf, n);
> > -	va_end(ap);
> > +	if (early_console && (early_console_initialized ||
> > +			      boot_params.screen_info.orig_video_isVGA == 1)) {
> > +		va_start(ap, fmt);
> > +		n = vscnprintf(buf, sizeof(buf), fmt, ap);
> > +		early_console->write(early_console, buf, n);
> > +		va_end(ap);
> > +	}
> >  }
> >  
> >  static inline void early_console_register(struct console *con, int keep_early)
> > @@ -233,6 +236,12 @@ static int __init setup_early_printk(char *buf)
> >  #endif
> >  		buf++;
> >  	}
> > +
> > +	/* If there is no VGA device, don't try to use it as early console. */
> > +	if (early_console == &early_vga_console &&
> > +	    boot_params.screen_info.orig_video_isVGA != 1)
> > +		early_console = NULL;
> 
> Wouldn't it be better to switch to, say, serial console here to avoid 
> the NULL check in early_printk()?
> 
This would be another possibility.

However, since the serial console was not explicitly selected, it would possibly
not be initialized. My understanding is that Peter had a problem with that.

One can argue that it would still be better to select the serial console in that case,
even though it might not be initialized. Actually, most likely it is (if it exists),
since the system must presumably have some means to communicate with the world.

I am pretty much open to either option, if that results in the patch being accepted. 
Just let me know which way to go.

Guenter

Re: [PATCH v3] x86: Do not write into VGA memory space if there is no VGA device in the system

[Pekka Enberg]

On Mon, Apr 26, 2010 at 5:05 PM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
> On Mon, Apr 26, 2010 at 05:36:05AM -0400, Pekka Enberg wrote:
>> Guenter Roeck kirjoitti:
>> > Ensure that early_printk() does not write into VGA memory space
>> > if there is not VGA device in the system.
>> >
>> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
>> > ---
>> > v3:
>> > Changes are now limited to early_printk.c. Functionality is retained, meaning
>> > there will still be output on the VGA console before setup_early_printk()
>> > is called, but only if a VGA device is known to exist in the system.
>> >
>> >  arch/x86/kernel/early_printk.c |   17 +++++++++++++----
>> >  1 files changed, 13 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
>> > index b9c830c..783cb25 100644
>> > --- a/arch/x86/kernel/early_printk.c
>> > +++ b/arch/x86/kernel/early_printk.c
>> > @@ -170,10 +170,13 @@ asmlinkage void early_printk(const char *fmt, ...)
>> >     int n;
>> >     va_list ap;
>> >
>> > -   va_start(ap, fmt);
>> > -   n = vscnprintf(buf, sizeof(buf), fmt, ap);
>> > -   early_console->write(early_console, buf, n);
>> > -   va_end(ap);
>> > +   if (early_console && (early_console_initialized ||
>> > +                         boot_params.screen_info.orig_video_isVGA == 1)) {
>> > +           va_start(ap, fmt);
>> > +           n = vscnprintf(buf, sizeof(buf), fmt, ap);
>> > +           early_console->write(early_console, buf, n);
>> > +           va_end(ap);
>> > +   }
>> >  }
>> >
>> >  static inline void early_console_register(struct console *con, int keep_early)
>> > @@ -233,6 +236,12 @@ static int __init setup_early_printk(char *buf)
>> >  #endif
>> >             buf++;
>> >     }
>> > +
>> > +   /* If there is no VGA device, don't try to use it as early console. */
>> > +   if (early_console == &early_vga_console &&
>> > +       boot_params.screen_info.orig_video_isVGA != 1)
>> > +           early_console = NULL;
>>
>> Wouldn't it be better to switch to, say, serial console here to avoid
>> the NULL check in early_printk()?
>>
> This would be another possibility.
>
> However, since the serial console was not explicitly selected, it would possibly
> not be initialized. My understanding is that Peter had a problem with that.
>
> One can argue that it would still be better to select the serial console in that case,
> even though it might not be initialized. Actually, most likely it is (if it exists),
> since the system must presumably have some means to communicate with the world.
>
> I am pretty much open to either option, if that results in the patch being accepted.
> Just let me know which way to go.

Peter?

Re: [PATCH v3] x86: Do not write into VGA memory space if there is no VGA device in the system

[H. Peter Anvin]

On 04/26/2010 07:35 AM, Pekka Enberg wrote:
>>>
>>> Wouldn't it be better to switch to, say, serial console here to avoid
>>> the NULL check in early_printk()?
>>>
>> This would be another possibility.
>>
>> However, since the serial console was not explicitly selected, it would possibly
>> not be initialized. My understanding is that Peter had a problem with that.
>>
>> One can argue that it would still be better to select the serial console in that case,
>> even though it might not be initialized. Actually, most likely it is (if it exists),
>> since the system must presumably have some means to communicate with the world.
>>
>> I am pretty much open to either option, if that results in the patch being accepted.
>> Just let me know which way to go.
> 
> Peter?

Yes, I really don't want to issue bytes to a serial port that isn't
known to be initialized; it could take a *very* long time even if the
port is actually present, and if the port *isn't* present it could hang
the kernel.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH v3] x86: Do not write into VGA memory space if there is no VGA device in the system

[Bastien ROUCARIES]

On Tue, Apr 20, 2010 at 6:16 PM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
> Ensure that early_printk() does not write into VGA memory space
> if there is not VGA device in the system.
>
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
> v3:
> Changes are now limited to early_printk.c. Functionality is retained, meaning
> there will still be output on the VGA console before setup_early_printk()
> is called, but only if a VGA device is known to exist in the system.
>
>  arch/x86/kernel/early_printk.c |   17 +++++++++++++----
>  1 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> index b9c830c..783cb25 100644
> --- a/arch/x86/kernel/early_printk.c
> +++ b/arch/x86/kernel/early_printk.c
> @@ -170,10 +170,13 @@ asmlinkage void early_printk(const char *fmt, ...)
>        int n;
>        va_list ap;
>
> -       va_start(ap, fmt);
> -       n = vscnprintf(buf, sizeof(buf), fmt, ap);
> -       early_console->write(early_console, buf, n);
> -       va_end(ap);
> +       if (early_console && (early_console_initialized ||
> +                             boot_params.screen_info.orig_video_isVGA == 1)) {
> +               va_start(ap, fmt);
> +               n = vscnprintf(buf, sizeof(buf), fmt, ap);
> +               early_console->write(early_console, buf, n);
> +               va_end(ap);
> +       }
>  }

Instead to this ugly if why not creating a null early_console (write
will do nothing) ?

>  static inline void early_console_register(struct console *con, int keep_early)
> @@ -233,6 +236,12 @@ static int __init setup_early_printk(char *buf)
>  #endif
>                buf++;
>        }
> +
> +       /* If there is no VGA device, don't try to use it as early console. */
> +       if (early_console == &early_vga_console &&
> +           boot_params.screen_info.orig_video_isVGA != 1)
> +               early_console = NULL;
> +
>        return 0;
>  }

And assign null console as the last console as a fallback ?

Regards

Bastien

Re: [PATCH v3] x86: Do not write into VGA memory space if there is no VGA device in the system

[Guenter Roeck]

On Mon, Apr 26, 2010 at 11:43:44AM -0400, Bastien ROUCARIES wrote:
> On Tue, Apr 20, 2010 at 6:16 PM, Guenter Roeck
> <guenter.roeck@ericsson.com> wrote:
> > Ensure that early_printk() does not write into VGA memory space
> > if there is not VGA device in the system.
> >
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> > v3:
> > Changes are now limited to early_printk.c. Functionality is retained, meaning
> > there will still be output on the VGA console before setup_early_printk()
> > is called, but only if a VGA device is known to exist in the system.
> >
> >  arch/x86/kernel/early_printk.c |   17 +++++++++++++----
> >  1 files changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> > index b9c830c..783cb25 100644
> > --- a/arch/x86/kernel/early_printk.c
> > +++ b/arch/x86/kernel/early_printk.c
> > @@ -170,10 +170,13 @@ asmlinkage void early_printk(const char *fmt, ...)
> >        int n;
> >        va_list ap;
> >
> > -       va_start(ap, fmt);
> > -       n = vscnprintf(buf, sizeof(buf), fmt, ap);
> > -       early_console->write(early_console, buf, n);
> > -       va_end(ap);
> > +       if (early_console && (early_console_initialized ||
> > +                             boot_params.screen_info.orig_video_isVGA == 1)) {
> > +               va_start(ap, fmt);
> > +               n = vscnprintf(buf, sizeof(buf), fmt, ap);
> > +               early_console->write(early_console, buf, n);
> > +               va_end(ap);
> > +       }
> >  }
> 
> Instead to this ugly if why not creating a null early_console (write
> will do nothing) ?
> 
> >  static inline void early_console_register(struct console *con, int keep_early)
> > @@ -233,6 +236,12 @@ static int __init setup_early_printk(char *buf)
> >  #endif
> >                buf++;
> >        }
> > +
> > +       /* If there is no VGA device, don't try to use it as early console. */
> > +       if (early_console == &early_vga_console &&
> > +           boot_params.screen_info.orig_video_isVGA != 1)
> > +               early_console = NULL;
> > +
> >        return 0;
> >  }
> 
> And assign null console as the last console as a fallback ?
> 
Sounds good to me. Peter, would that get your approval ?

Guenter