mach_reboot_fixups()

mach_reboot_fixups()

[Jan Beulich]

Ingo,

was it intentional to remove the call to mach_reboot_fixups() during
the merge of reboot_{32,64}.c? If so, it seems odd that
reboot_fixups_32.c was left in the tree (and there was even stuff
added to it). But it would rather seem that those machines dealt with
in that file would not reboot properly anymore (for one of my boxes
I added an entry in that table to make it boot properly, which is why
I noticed the code not being called anymore)...

Thanks, Jan

Re: mach_reboot_fixups()

[Ingo Molnar]

* Jan Beulich <jbeulich@novell.com> wrote:

> Ingo,
> 
> was it intentional to remove the call to mach_reboot_fixups() during 
> the merge of reboot_{32,64}.c? If so, it seems odd that 
> reboot_fixups_32.c was left in the tree (and there was even stuff 
> added to it). But it would rather seem that those machines dealt with 
> in that file would not reboot properly anymore (for one of my boxes I 
> added an entry in that table to make it boot properly, which is why I 
> noticed the code not being called anymore)...

good catch Jan! The patch below should fix it.

Can you see any other material difference due to the unification? 
reboot_mode is now written to 0x472 unconditionally, but we can consider 
that a bugfix. Otherwise the mode and ordering of reboot sequences seems 
to be equivalent to me.

	Ingo

------------------->
Subject: x86: re-add reboot fixups
From: Ingo Molnar <mingo@elte.hu>
Date: Thu Mar 06 18:29:43 CET 2008

Jan Beulich noticed that the reboot fixups went missing during
reboot.c unification.

(commit 4d022e35fd7e07c522c7863fee6f07e53cf3fc14)

Geode and a few other rare boards with special reboot quirks are
affected.

Reported-by: Jan Beulich <jbeulich@novell.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/reboot.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux-x86.q/arch/x86/kernel/reboot.c
===================================================================
--- linux-x86.q.orig/arch/x86/kernel/reboot.c
+++ linux-x86.q/arch/x86/kernel/reboot.c
@@ -335,6 +335,10 @@ static inline void kb_wait(void)
 	}
 }
 
+void __attribute__((weak)) mach_reboot_fixups(void)
+{
+}
+
 void native_machine_emergency_restart(void)
 {
 	int i;
@@ -342,6 +346,8 @@ void native_machine_emergency_restart(vo
 	/* Tell the BIOS if we want cold or warm reboot */
 	*((unsigned short *)__va(0x472)) = reboot_mode;
 
+	mach_reboot_fixups(); /* for board specific fixups */
+
 	for (;;) {
 		/* Could also try the reset bit in the Hammer NB */
 		switch (reboot_type) {

Re: mach_reboot_fixups()

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> Can you see any other material difference due to the unification? 
> reboot_mode is now written to 0x472 unconditionally, but we can 
> consider that a bugfix. Otherwise the mode and ordering of reboot 
> sequences seems to be equivalent to me.

btw., i removed mach_reboot.h as well in x86.git - it's now unused.

	Ingo

Re: mach_reboot_fixups()

[Jan Beulich]

>>> Ingo Molnar <mingo@elte.hu> 06.03.08 18:43 >>>
>
>* Jan Beulich <jbeulich@novell.com> wrote:
>
>> Ingo,
>> 
>> was it intentional to remove the call to mach_reboot_fixups() during 
>> the merge of reboot_{32,64}.c? If so, it seems odd that 
>> reboot_fixups_32.c was left in the tree (and there was even stuff 
>> added to it). But it would rather seem that those machines dealt with 
>> in that file would not reboot properly anymore (for one of my boxes I 
>> added an entry in that table to make it boot properly, which is why I 
>> noticed the code not being called anymore)...
>
>good catch Jan! The patch below should fix it.
>
>Can you see any other material difference due to the unification? 
>reboot_mode is now written to 0x472 unconditionally, but we can consider 
>that a bugfix. Otherwise the mode and ordering of reboot sequences seems 
>to be equivalent to me.

Not exactly - when rebooting through EFI or BIOS, the old code didn't
go through mach_reboot_fixups(), and I think that is the correct
behavior (albeit, when the EFI path fell back to the triple fault
mechanism, it should have honored the fixup logic, and I think it is
more correct that the new code tries the keyboard method first).
Perhaps the most reasonable way to go is to honor all reboot=
settings without using the override code first:

--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -329,6 +329,10 @@ static inline void kb_wait(void)
 	}
 }
 
+void __attribute__((weak)) mach_reboot_fixups(void)
+{
+}
+
 static void native_machine_emergency_restart(void)
 {
 	int i;
@@ -337,9 +341,11 @@ static void native_machine_emergency_res
 	*((unsigned short *)__va(0x472)) = reboot_mode;
 
 	for (;;) {
+
 		/* Could also try the reset bit in the Hammer NB */
 		switch (reboot_type) {
 		case BOOT_KBD:
+			mach_reboot_fixups(); /* for board specific fixups */
 			for (i = 0; i < 10; i++) {
 				kb_wait();
 				udelay(50);

(with the exception that reboot=keyboard will still have the effect
of honoring the fixups, but I think this is better than further
complicating the logic).

In case you want to take this,
Signed-off-by: Jan Beulich <jbeulich@novell.com>

Jan

Re: mach_reboot_fixups()

[Ingo Molnar]

* Jan Beulich <jbeulich@novell.com> wrote:

> Not exactly - when rebooting through EFI or BIOS, the old code didn't 
> go through mach_reboot_fixups(), and I think that is the correct 
> behavior (albeit, when the EFI path fell back to the triple fault 
> mechanism, it should have honored the fixup logic, and I think it is 
> more correct that the new code tries the keyboard method first). 
> Perhaps the most reasonable way to go is to honor all reboot= settings 
> without using the override code first:

>  		case BOOT_KBD:
> +			mach_reboot_fixups(); /* for board specific fixups */

ok, agreed.

> (with the exception that reboot=keyboard will still have the effect of 
> honoring the fixups, but I think this is better than further 
> complicating the logic).

the fixups are really "emergency exceptions" - for (a very low number 
of) broken boards that just cannot reboot via the default BOOT_KBD 
method. In that sense it's a small detail how widely we apply the 
exceptions - those boards probably wont be rebooted via reboot=bios 
[people only use reboot options when they have trouble rebooting].

> In case you want to take this,
> Signed-off-by: Jan Beulich <jbeulich@novell.com>

i modified the patch and added your signoff - thanks,

	Ingo