Regression in serial console on ia64 after 2.6.22

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.n
et/~horms/
  W: http://www.valinux.co.jp
/en/

-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

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
-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

IA64

Subject         : Regression in serial console on ia64 after
2.6.22
References      : http://marc.info/?l=linux-ia64&m=118483645914066
&w=2
Last known good : ?
Submitter       : Horms <hormsverge.net.au>
Caused-By       : Yinghai Lu <Yinghai.LuSun.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.lusun.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();
 


-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

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=118483645914066
&w=2
>> Last known good : ?
>> Submitter       : Horms <hormsverge.net.au>
>> Caused-By       : Yinghai Lu <Yinghai.LuSun.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.lusun.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
-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

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

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

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

Thanks.
-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

Andrew Morton wrote:
> On Wed, 25 Jul 2007 08:39:01 -0700 Yinghai Lu
<Yinghai.LuSun.COM> wrote:
> 
>>>> Subject         : Regression in serial
console on ia64 after 2.6.22
>>>> References      : http://marc.info/?l=linux-ia64&m=118483645914066
&w=2
>>>> Last known good : ?
>>>> Submitter       : Horms <hormsverge.net.au>
>>>> Caused-By       : Yinghai Lu
<Yinghai.LuSun.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
-
To unsubscribe from this list: send the line
"unsubscribe linux-ia64" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html