Regression in serial console on ia64 after 2.6.22

Regression in serial console on ia64 after 2.6.22

[Horms]

Hi,

18a8bd949d6adb311ea816125ff65050df1f3f6e appears to have caused
a regression in the serial console for ia64. I have observed the
problem on Intel Tiger2 and HP RX2620 when the console or early
cons option is specified.

For reference

Tiger2: console=uart,io,0x2f8,115200n8
RX2620: console=uart,mmio,0xff5e0000,115200n8

uart may also be uart8250, it makes no difference with regards to
this problem. console may be earlycons, that also makes no
difference as 18a8bd949d6adb311ea816125ff65050df1f3f6e handles
both options the almost the same way.

On the RX2620 a work around is to omit the console and earlycons
parameters all together, as the code in efi_setup_pcdp_console() will
autodetect the console and set it up correctly.


I believe that the problem is as follows:

After 18a8bd949d6adb311ea816125ff65050df1f3f6e, both the console
and earlycons options are handled by early_param, using the callback
setup_early_serial8250_console().

On ia64, for the hardware that I am observing this problem on
(HP RX2620 and Intel Tiger2), I believe that I am seeing the
following calls occur.

->arch/ia64/kernel/setup.c:setup_arch()
 ->parse_early_param()
  ->setup_early_serial8250_console()    N.B callback
   ->early_serial8250_setup()
    ->init_port()
     ->serial_out()
      ->outb()
       ->__outb()
        ->platform_outb()
         ->ia64_mv.outb()

However, ia64_mv may not be set until after machvec_init() is called
by setup_arch(), which occurs after the call to parse_early_param().

The reason for this ordering is that machvec_init() may be
influenced by the machvec command line argument, which
is also handled by early_param. A chicken an egg problem.


A fairly simple work-around would be to call machvec_init earlier
and have it parse the command like itself.

Another solution would be to split up early_serial8250_setup()
or setup_early_serial8250_console() somehow, such that init_port()
is not called as part of the early_param callback, but rather later
explicitly (probably by each architecture's setup_arch()). This is
somewhat similar to the way that machvec is handled.

I'm sure there are are other good, possibly better solutions.
Hence this email.


On a related note, I think that the code in efi_setup_pcdp_console()
needs to be updated to reflect that console= and earlycons= are
basically the same thing now.

I have not investigated the effects of
18a8bd949d6adb311ea816125ff65050df1f3f6e on
sn_serial_console_early_setup()

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Re: Regression in serial console on ia64 after 2.6.22

[Yinghai Lu]

Horms wrote:
> Hi,
> 
> 18a8bd949d6adb311ea816125ff65050df1f3f6e appears to have caused
> a regression in the serial console for ia64. I have observed the
> problem on Intel Tiger2 and HP RX2620 when the console or early
> cons option is specified.
> 
> For reference
> 
> Tiger2: console=uart,io,0x2f8,115200n8
> RX2620: console=uart,mmio,0xff5e0000,115200n8
> 
> uart may also be uart8250, it makes no difference with regards to
> this problem. console may be earlycons, that also makes no
> difference as 18a8bd949d6adb311ea816125ff65050df1f3f6e handles
> both options the almost the same way.
> 
> On the RX2620 a work around is to omit the console and earlycons
> parameters all together, as the code in efi_setup_pcdp_console() will
> autodetect the console and set it up correctly.
> 
> 
> I believe that the problem is as follows:
> 
> After 18a8bd949d6adb311ea816125ff65050df1f3f6e, both the console
> and earlycons options are handled by early_param, using the callback
> setup_early_serial8250_console().
> 
> On ia64, for the hardware that I am observing this problem on
> (HP RX2620 and Intel Tiger2), I believe that I am seeing the
> following calls occur.
> 
> ->arch/ia64/kernel/setup.c:setup_arch()
>  ->parse_early_param()
>   ->setup_early_serial8250_console()    N.B callback
>    ->early_serial8250_setup()
>     ->init_port()
>      ->serial_out()
>       ->outb()
>        ->__outb()
>         ->platform_outb()
>          ->ia64_mv.outb()
> 
> However, ia64_mv may not be set until after machvec_init() is called
> by setup_arch(), which occurs after the call to parse_early_param().
> 
> The reason for this ordering is that machvec_init() may be
> influenced by the machvec command line argument, which
> is also handled by early_param. A chicken an egg problem.
> 
> 
> A fairly simple work-around would be to call machvec_init earlier
> and have it parse the command like itself.
> 
> Another solution would be to split up early_serial8250_setup()
> or setup_early_serial8250_console() somehow, such that init_port()
> is not called as part of the early_param callback, but rather later
> explicitly (probably by each architecture's setup_arch()). This is
> somewhat similar to the way that machvec is handled.
> 
> I'm sure there are are other good, possibly better solutions.
> Hence this email.

how about modify boot_command_line and put machvec at the first entry. and use setup_machvec to call machvev_init.

YH

Re: Regression in serial console on ia64 after 2.6.22

[Yinghai Lu]

IA64

Subject         : Regression in serial console on ia64 after 2.6.22
References      : http://marc.info/?l=linux-ia64&m\x118483645914066&w=2
Last known good : ?
Submitter       : Horms <horms@verge.net.au>
Caused-By       : Yinghai Lu <Yinghai.Lu@Sun.COM>
                  commit 18a8bd949d6adb311ea816125ff65050df1f3f6e
Handled-By      : ?
Status          : unknown


please test this patch.

YH


[PATCH] ia64: move machvec_init before parse_early_param

So ia64_mv is initialized before early console

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>

diff --git a/arch/ia64/kernel/machvec.c b/arch/ia64/kernel/machvec.c
index 13df337..a94feaa 100644
--- a/arch/ia64/kernel/machvec.c
+++ b/arch/ia64/kernel/machvec.c
@@ -14,12 +14,6 @@ struct ia64_machine_vector ia64_mv;
 EXPORT_SYMBOL(ia64_mv);
 
 static __initdata const char *mvec_name;
-static __init int setup_mvec(char *s)
-{
-	mvec_name = s;
-	return 0;
-}
-early_param("machvec", setup_mvec);
 
 static struct ia64_machine_vector * __init
 lookup_machvec (const char *name)
@@ -42,6 +36,10 @@ machvec_init (const char *name)
 
 	if (!name)
 		name = mvec_name ? mvec_name : acpi_get_sysname();
+
+	if (!mvec_name)
+		mvec_name = name;
+
 	mv = lookup_machvec(name);
 	if (!mv)
 		panic("generic kernel failed to find machine vector for"
diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
index cf06fe7..b06d7b7 100644
--- a/arch/ia64/kernel/setup.c
+++ b/arch/ia64/kernel/setup.c
@@ -481,6 +481,9 @@ int __init reserve_elfcorehdr(unsigned long *start, unsigned long *end)
 void __init
 setup_arch (char **cmdline_p)
 {
+#ifdef CONFIG_IA64_GENERIC
+	char *mvstr;
+#endif
 	unw_init();
 
 	ia64_patch_vtop((u64) __start___vtop_patchlist, (u64) __end___vtop_patchlist);
@@ -491,12 +494,15 @@ setup_arch (char **cmdline_p)
 	efi_init();
 	io_port_init();
 
-	parse_early_param();
-
 #ifdef CONFIG_IA64_GENERIC
-	machvec_init(NULL);
+	mvstr = strstr(*cmd_line_p, "machvec=")
+	if (mvstr)
+		mvstr = strchr(mvstr, '=') + 1;
+	machvec_init(mvstr);
 #endif
 
+	parse_early_param();
+
 	if (early_console_setup(*cmdline_p) = 0)
 		mark_bsp_online();
 

Re: Regression in serial console on ia64 after 2.6.22

[Horms]

On Thu, Jul 19, 2007 at 07:08:30PM -0700, Yinghai Lu wrote:
> 
> how about modify boot_command_line and put machvec at the first entry.
> and use setup_machvec to call machvev_init.

This could work, but I think that it would most likely be cleaner
to just have machvec do its own parsing.

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Re: Regression in serial console on ia64 after 2.6.22

[Horms]

On Tue, Jul 24, 2007 at 04:57:32PM -0700, Yinghai Lu wrote:
> 
> IA64
> 
> Subject         : Regression in serial console on ia64 after 2.6.22
> References      : http://marc.info/?l=linux-ia64&m\x118483645914066&w=2
> Last known good : ?
> Submitter       : Horms <horms@verge.net.au>
> Caused-By       : Yinghai Lu <Yinghai.Lu@Sun.COM>
>                   commit 18a8bd949d6adb311ea816125ff65050df1f3f6e
> Handled-By      : ?
> Status          : unknown
> 
> 
> please test this patch.
> 
> YH

I just posted a similar patch (sorry, I didn't see your post until
afterwards). I guess that we both have the same ideas :)

> 
> 
> [PATCH] ia64: move machvec_init before parse_early_param
> 
> So ia64_mv is initialized before early console
> 
> Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
> 
> diff --git a/arch/ia64/kernel/machvec.c b/arch/ia64/kernel/machvec.c
> index 13df337..a94feaa 100644
> --- a/arch/ia64/kernel/machvec.c
> +++ b/arch/ia64/kernel/machvec.c
> @@ -14,12 +14,6 @@ struct ia64_machine_vector ia64_mv;
>  EXPORT_SYMBOL(ia64_mv);
>  
>  static __initdata const char *mvec_name;
> -static __init int setup_mvec(char *s)
> -{
> -	mvec_name = s;
> -	return 0;
> -}
> -early_param("machvec", setup_mvec);
>  
>  static struct ia64_machine_vector * __init
>  lookup_machvec (const char *name)
> @@ -42,6 +36,10 @@ machvec_init (const char *name)
>  
>  	if (!name)
>  		name = mvec_name ? mvec_name : acpi_get_sysname();

I think that mvec_name can be disposed of all together as
its only used by the line above and setup_mvec(), which in
turn is only used by early_param. I got rid of mvec_name in
my incantation of this patch, and it seemed to work quite well.

> +
> +	if (!mvec_name)
> +		mvec_name = name;
> +
>  	mv = lookup_machvec(name);
>  	if (!mv)
>  		panic("generic kernel failed to find machine vector for"
> diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
> index cf06fe7..b06d7b7 100644
> --- a/arch/ia64/kernel/setup.c
> +++ b/arch/ia64/kernel/setup.c
> @@ -481,6 +481,9 @@ int __init reserve_elfcorehdr(unsigned long *start, unsigned long *end)
>  void __init
>  setup_arch (char **cmdline_p)
>  {
> +#ifdef CONFIG_IA64_GENERIC
> +	char *mvstr;
> +#endif
>  	unw_init();
>  
>  	ia64_patch_vtop((u64) __start___vtop_patchlist, (u64) __end___vtop_patchlist);
> @@ -491,12 +494,15 @@ setup_arch (char **cmdline_p)
>  	efi_init();
>  	io_port_init();
>  
> -	parse_early_param();
> -
>  #ifdef CONFIG_IA64_GENERIC
> -	machvec_init(NULL);
> +	mvstr = strstr(*cmd_line_p, "machvec=")
> +	if (mvstr)
> +		mvstr = strchr(mvstr, '=') + 1;
> +	machvec_init(mvstr);
>  #endif

Do you need to copy and truncate mvstr on ' ' in the case
that machvec= isn't the last argument on the command line?
I did this in my patch, though I didn't test to see if
it was uneccessary.


>  
> +	parse_early_param();
> +
>  	if (early_console_setup(*cmdline_p) = 0)
>  		mark_bsp_online();
>  
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Re: Regression in serial console on ia64 after 2.6.22

[Yinghai Lu]

Horms wrote:
> On Tue, Jul 24, 2007 at 04:57:32PM -0700, Yinghai Lu wrote:
>> IA64
>>
>> Subject         : Regression in serial console on ia64 after 2.6.22
>> References      : http://marc.info/?l=linux-ia64&m\x118483645914066&w=2
>> Last known good : ?
>> Submitter       : Horms <horms@verge.net.au>
>> Caused-By       : Yinghai Lu <Yinghai.Lu@Sun.COM>
>>                   commit 18a8bd949d6adb311ea816125ff65050df1f3f6e
>> Handled-By      : ?
>> Status          : unknown
>>
>>
>> please test this patch.
>>
>> YH
> 
> I just posted a similar patch (sorry, I didn't see your post until
> afterwards). I guess that we both have the same ideas :)
> 
>>
>> [PATCH] ia64: move machvec_init before parse_early_param
>>
>> So ia64_mv is initialized before early console
>>
>> Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
>>
>> diff --git a/arch/ia64/kernel/machvec.c b/arch/ia64/kernel/machvec.c
>> index 13df337..a94feaa 100644
>> --- a/arch/ia64/kernel/machvec.c
>> +++ b/arch/ia64/kernel/machvec.c
>> @@ -14,12 +14,6 @@ struct ia64_machine_vector ia64_mv;
>>  EXPORT_SYMBOL(ia64_mv);
>>  
>>  static __initdata const char *mvec_name;
>> -static __init int setup_mvec(char *s)
>> -{
>> -	mvec_name = s;
>> -	return 0;
>> -}
>> -early_param("machvec", setup_mvec);
>>  
>>  static struct ia64_machine_vector * __init
>>  lookup_machvec (const char *name)
>> @@ -42,6 +36,10 @@ machvec_init (const char *name)
>>  
>>  	if (!name)
>>  		name = mvec_name ? mvec_name : acpi_get_sysname();
> 
> I think that mvec_name can be disposed of all together as
> its only used by the line above and setup_mvec(), which in
> turn is only used by early_param. I got rid of mvec_name in
> my incantation of this patch, and it seemed to work quite well.

with cmd_line or NULL, mvec_nanme could get assigned.
next machvec_init(NULL) could reuse that

> 
>> +
>> +	if (!mvec_name)
>> +		mvec_name = name;
>> +
>>  	mv = lookup_machvec(name);
>>  	if (!mv)
>>  		panic("generic kernel failed to find machine vector for"
>> diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
>> index cf06fe7..b06d7b7 100644
>> --- a/arch/ia64/kernel/setup.c
>> +++ b/arch/ia64/kernel/setup.c
>> @@ -481,6 +481,9 @@ int __init reserve_elfcorehdr(unsigned long *start, unsigned long *end)
>>  void __init
>>  setup_arch (char **cmdline_p)
>>  {
>> +#ifdef CONFIG_IA64_GENERIC
>> +	char *mvstr;
>> +#endif
>>  	unw_init();
>>  
>>  	ia64_patch_vtop((u64) __start___vtop_patchlist, (u64) __end___vtop_patchlist);
>> @@ -491,12 +494,15 @@ setup_arch (char **cmdline_p)
>>  	efi_init();
>>  	io_port_init();
>>  
>> -	parse_early_param();
>> -
>>  #ifdef CONFIG_IA64_GENERIC
>> -	machvec_init(NULL);
>> +	mvstr = strstr(*cmd_line_p, "machvec=")
>> +	if (mvstr)
>> +		mvstr = strchr(mvstr, '=') + 1;
>> +	machvec_init(mvstr);
>>  #endif
> 
> Do you need to copy and truncate mvstr on ' ' in the case
> that machvec= isn't the last argument on the command line?
> I did this in my patch, though I didn't test to see if
> it was uneccessary.

or may need to update lookup_machvec...
from strcmp to strstr...

just want to the code to be less changes as possible.

YH

Re: Regression in serial console on ia64 after 2.6.22

[Andrew Morton]

On Wed, 25 Jul 2007 08:39:01 -0700 Yinghai Lu <Yinghai.Lu@Sun.COM> wrote:

> >> Subject         : Regression in serial console on ia64 after 2.6.22
> >> References      : http://marc.info/?l=linux-ia64&m\x118483645914066&w=2
> >> Last known good : ?
> >> Submitter       : Horms <horms@verge.net.au>
> >> Caused-By       : Yinghai Lu <Yinghai.Lu@Sun.COM>
> >>                   commit 18a8bd949d6adb311ea816125ff65050df1f3f6e

afaik we still haven't fixed this and I don't think we've seen
the final agreed-upon patch?

Thanks.

Re: Regression in serial console on ia64 after 2.6.22

[Yinghai Lu]

Andrew Morton wrote:
> On Wed, 25 Jul 2007 08:39:01 -0700 Yinghai Lu <Yinghai.Lu@Sun.COM> wrote:
> 
>>>> Subject         : Regression in serial console on ia64 after 2.6.22
>>>> References      : http://marc.info/?l=linux-ia64&m\x118483645914066&w=2
>>>> Last known good : ?
>>>> Submitter       : Horms <horms@verge.net.au>
>>>> Caused-By       : Yinghai Lu <Yinghai.Lu@Sun.COM>
>>>>                   commit 18a8bd949d6adb311ea816125ff65050df1f3f6e
> 
> afaik we still haven't fixed this and I don't think we've seen
> the final agreed-upon patch?
> 
> Thanks.

Horms' patch already got into mainline via IA64 maintainer...

Thanks

Yinghai Lu