Hi Linus,
Please pull from 'master' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/wim/linux-2.6-watchdog.git
or if master.kernel.org hasn't synced up yet:
master.kernel.org:/pub/scm/linux/kernel/git/wim/linux-2.6-watchdog.git
This will update the following files:
drivers/watchdog/Makefile | 1 +
drivers/watchdog/hpwdt.c | 35 +++++++++++++++++++----------------
2 files changed, 20 insertions(+), 16 deletions(-)
with these Changes:
Author: Wim Van Sebroeck <[email protected]>
Date: Wed Jun 18 16:22:48 2008 +0000
Revert "[WATCHDOG] hpwdt: Fix NMI handling."
The old setup works better.
Signed-off-by: Thomas Mingarelli <[email protected]>
Signed-off-by: Wim Van Sebroeck <[email protected]>
Author: Thomas Mingarelli <[email protected]>
Date: Thu Jun 12 20:20:32 2008 +0000
[WATCHDOG] hpwdt: Add CFLAGS to get driver working
To get this driver working we need the CFLAGS_hpwdt.o += -O in the Makefile.
Signed-off-by: Thomas Mingarelli <[email protected]>
Signed-off-by: Wim Van Sebroeck <[email protected]>
Author: Thomas Mingarelli <[email protected]>
Date: Thu Jun 12 20:20:32 2008 +0000
Revert "[WATCHDOG] make watchdog/hpwdt.c:asminline_call() static"
The driver needs the asmlinkage tag and the CFLAGS line in the Makefile.
Without it the driver doesn't work.
Signed-off-by: Thomas Mingarelli <[email protected]>
Signed-off-by: Wim Van Sebroeck <[email protected]>
The Changes can also be looked at on:
http://www.kernel.org/git/?p=linux/kernel/git/wim/linux-2.6-watchdog.git;a=summary
For completeness, I added the overal diff below.
Greetings,
Wim.
================================================================================
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 25b352b..8662a6b 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_WAFER_WDT) += wafer5823wdt.o
obj-$(CONFIG_I6300ESB_WDT) += i6300esb.o
obj-$(CONFIG_ITCO_WDT) += iTCO_wdt.o iTCO_vendor_support.o
obj-$(CONFIG_IT8712F_WDT) += it8712f_wdt.o
+CFLAGS_hpwdt.o += -O
obj-$(CONFIG_HP_WATCHDOG) += hpwdt.o
obj-$(CONFIG_SC1200_WDT) += sc1200wdt.o
obj-$(CONFIG_SCx200_WDT) += scx200_wdt.o
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 6a63535..2686f3e 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -145,8 +145,8 @@ MODULE_DEVICE_TABLE(pci, hpwdt_devices);
#define HPWDT_ARCH 32
-static void asminline_call(struct cmn_registers *pi86Regs,
- unsigned long *pRomEntry)
+asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
+ unsigned long *pRomEntry)
{
asm("pushl %ebp \n\t"
"movl %esp, %ebp \n\t"
@@ -333,8 +333,8 @@ static int __devinit detect_cru_service(void)
#define HPWDT_ARCH 64
-static void asminline_call(struct cmn_registers *pi86Regs,
- unsigned long *pRomEntry)
+asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
+ unsigned long *pRomEntry)
{
asm("pushq %rbp \n\t"
"movq %rsp, %rbp \n\t"
@@ -418,20 +418,23 @@ static int hpwdt_pretimeout(struct notifier_block *nb, unsigned long ulReason,
static unsigned long rom_pl;
static int die_nmi_called;
- if (ulReason == DIE_NMI || ulReason == DIE_NMI_IPI) {
- spin_lock_irqsave(&rom_lock, rom_pl);
- if (!die_nmi_called)
- asminline_call(&cmn_regs, cru_rom_addr);
- die_nmi_called = 1;
- spin_unlock_irqrestore(&rom_lock, rom_pl);
- if (cmn_regs.u1.ral != 0) {
- panic("An NMI occurred, please see the Integrated "
- "Management Log for details.\n");
- }
+ if (ulReason != DIE_NMI && ulReason != DIE_NMI_IPI)
+ return NOTIFY_OK;
+
+ spin_lock_irqsave(&rom_lock, rom_pl);
+ if (!die_nmi_called)
+ asminline_call(&cmn_regs, cru_rom_addr);
+ die_nmi_called = 1;
+ spin_unlock_irqrestore(&rom_lock, rom_pl);
+ if (cmn_regs.u1.ral == 0) {
+ printk(KERN_WARNING "hpwdt: An NMI occurred, "
+ "but unable to determine source.\n");
+ } else {
+ panic("An NMI occurred, please see the Integrated "
+ "Management Log for details.\n");
}
- die_nmi_called = 0;
- return NOTIFY_DONE;
+ return NOTIFY_STOP;
}
/*
On Wed, 18 Jun 2008, Wim Van Sebroeck wrote:
>
> Revert "[WATCHDOG] hpwdt: Fix NMI handling."
>
> To get this driver working we need the CFLAGS_hpwdt.o += -O in the Makefile.
>
> The driver needs the asmlinkage tag and the CFLAGS line in the Makefile.
> Without it the driver doesn't work.
The driver is unbelievable shit, and should just be removed, I think.
The reason for all the games with CFLAGS and asmlinkage and utter crud
seems to be that the driver is just BROKEN. It will work purely by luck,
and depend entirely on the compiler not doing anything else AT ALL in that
asm function.
Could somebody please just fix the piece-of-sh*t thing?
Here's a totally untested patch that may or may not work. At least it
removes the assembler code that assumes that it controls the whole
function from anything that gcc will then create a function prologue and
epilogue for. Quite frankly, this is *not* the right thing to do either:
the proper thing to do is to just move the low-level call into the "asm"
statement, and leave all the function prologue/epilogue entirely to the
compiler.
But this way it isn't actively _broken_ by design, at least.
No promises. I don't have the hardware. But somebody should *really* do
this correctly.
Linus
---
drivers/watchdog/hpwdt.c | 155 ++++++++++++++++++++++++----------------------
1 files changed, 80 insertions(+), 75 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 2686f3e..eaa3f2a 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -140,49 +140,53 @@ static struct pci_device_id hpwdt_devices[] = {
};
MODULE_DEVICE_TABLE(pci, hpwdt_devices);
+extern asmlinkage void asminline_call(struct cmn_registers *pi86Regs, unsigned long *pRomEntry);
+
#ifndef CONFIG_X86_64
/* --32 Bit Bios------------------------------------------------------------ */
#define HPWDT_ARCH 32
-asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
- unsigned long *pRomEntry)
-{
- asm("pushl %ebp \n\t"
- "movl %esp, %ebp \n\t"
- "pusha \n\t"
- "pushf \n\t"
- "push %es \n\t"
- "push %ds \n\t"
- "pop %es \n\t"
- "movl 8(%ebp),%eax \n\t"
- "movl 4(%eax),%ebx \n\t"
- "movl 8(%eax),%ecx \n\t"
- "movl 12(%eax),%edx \n\t"
- "movl 16(%eax),%esi \n\t"
- "movl 20(%eax),%edi \n\t"
- "movl (%eax),%eax \n\t"
- "push %cs \n\t"
- "call *12(%ebp) \n\t"
- "pushf \n\t"
- "pushl %eax \n\t"
- "movl 8(%ebp),%eax \n\t"
- "movl %ebx,4(%eax) \n\t"
- "movl %ecx,8(%eax) \n\t"
- "movl %edx,12(%eax) \n\t"
- "movl %esi,16(%eax) \n\t"
- "movl %edi,20(%eax) \n\t"
- "movw %ds,24(%eax) \n\t"
- "movw %es,26(%eax) \n\t"
- "popl %ebx \n\t"
- "movl %ebx,(%eax) \n\t"
- "popl %ebx \n\t"
- "movl %ebx,28(%eax) \n\t"
- "pop %es \n\t"
- "popf \n\t"
- "popa \n\t"
- "leave \n\t" "ret");
-}
+asm(".text \n\t"
+ ".align 4 \n"
+ "asminline_call: \n\t"
+ "pushl %ebp \n\t"
+ "movl %esp, %ebp \n\t"
+ "pusha \n\t"
+ "pushf \n\t"
+ "push %es \n\t"
+ "push %ds \n\t"
+ "pop %es \n\t"
+ "movl 8(%ebp),%eax \n\t"
+ "movl 4(%eax),%ebx \n\t"
+ "movl 8(%eax),%ecx \n\t"
+ "movl 12(%eax),%edx \n\t"
+ "movl 16(%eax),%esi \n\t"
+ "movl 20(%eax),%edi \n\t"
+ "movl (%eax),%eax \n\t"
+ "push %cs \n\t"
+ "call *12(%ebp) \n\t"
+ "pushf \n\t"
+ "pushl %eax \n\t"
+ "movl 8(%ebp),%eax \n\t"
+ "movl %ebx,4(%eax) \n\t"
+ "movl %ecx,8(%eax) \n\t"
+ "movl %edx,12(%eax) \n\t"
+ "movl %esi,16(%eax) \n\t"
+ "movl %edi,20(%eax) \n\t"
+ "movw %ds,24(%eax) \n\t"
+ "movw %es,26(%eax) \n\t"
+ "popl %ebx \n\t"
+ "movl %ebx,(%eax) \n\t"
+ "popl %ebx \n\t"
+ "movl %ebx,28(%eax) \n\t"
+ "pop %es \n\t"
+ "popf \n\t"
+ "popa \n\t"
+ "leave \n\t"
+ "ret \n\t"
+ ".previous");
+
/*
* cru_detect
@@ -333,43 +337,44 @@ static int __devinit detect_cru_service(void)
#define HPWDT_ARCH 64
-asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
- unsigned long *pRomEntry)
-{
- asm("pushq %rbp \n\t"
- "movq %rsp, %rbp \n\t"
- "pushq %rax \n\t"
- "pushq %rbx \n\t"
- "pushq %rdx \n\t"
- "pushq %r12 \n\t"
- "pushq %r9 \n\t"
- "movq %rsi, %r12 \n\t"
- "movq %rdi, %r9 \n\t"
- "movl 4(%r9),%ebx \n\t"
- "movl 8(%r9),%ecx \n\t"
- "movl 12(%r9),%edx \n\t"
- "movl 16(%r9),%esi \n\t"
- "movl 20(%r9),%edi \n\t"
- "movl (%r9),%eax \n\t"
- "call *%r12 \n\t"
- "pushfq \n\t"
- "popq %r12 \n\t"
- "popfq \n\t"
- "movl %eax, (%r9) \n\t"
- "movl %ebx, 4(%r9) \n\t"
- "movl %ecx, 8(%r9) \n\t"
- "movl %edx, 12(%r9) \n\t"
- "movl %esi, 16(%r9) \n\t"
- "movl %edi, 20(%r9) \n\t"
- "movq %r12, %rax \n\t"
- "movl %eax, 28(%r9) \n\t"
- "popq %r9 \n\t"
- "popq %r12 \n\t"
- "popq %rdx \n\t"
- "popq %rbx \n\t"
- "popq %rax \n\t"
- "leave \n\t" "ret");
-}
+asm(".text \n\t"
+ ".align 4 \n"
+ "asminline_call: \n\t"
+ "pushq %rbp \n\t"
+ "movq %rsp, %rbp \n\t"
+ "pushq %rax \n\t"
+ "pushq %rbx \n\t"
+ "pushq %rdx \n\t"
+ "pushq %r12 \n\t"
+ "pushq %r9 \n\t"
+ "movq %rsi, %r12 \n\t"
+ "movq %rdi, %r9 \n\t"
+ "movl 4(%r9),%ebx \n\t"
+ "movl 8(%r9),%ecx \n\t"
+ "movl 12(%r9),%edx \n\t"
+ "movl 16(%r9),%esi \n\t"
+ "movl 20(%r9),%edi \n\t"
+ "movl (%r9),%eax \n\t"
+ "call *%r12 \n\t"
+ "pushfq \n\t"
+ "popq %r12 \n\t"
+ "popfq \n\t"
+ "movl %eax, (%r9) \n\t"
+ "movl %ebx, 4(%r9) \n\t"
+ "movl %ecx, 8(%r9) \n\t"
+ "movl %edx, 12(%r9) \n\t"
+ "movl %esi, 16(%r9) \n\t"
+ "movl %edi, 20(%r9) \n\t"
+ "movq %r12, %rax \n\t"
+ "movl %eax, 28(%r9) \n\t"
+ "popq %r9 \n\t"
+ "popq %r12 \n\t"
+ "popq %rdx \n\t"
+ "popq %rbx \n\t"
+ "popq %rax \n\t"
+ "leave \n\t"
+ "ret \n\t"
+ ".previous");
/*
* dmi_find_cru
On Wed, 18 Jun 2008, Linus Torvalds wrote:
>
> Quite frankly, this is *not* the right thing to do either: the proper
> thing to do is to just move the low-level call into the "asm" statement,
> and leave all the function prologue/epilogue entirely to the compiler.
Something like the following.
AGAIN! This is totally untested. It's meant as an *example* of how to use
inline asm properly, not meant to necessarily work or do the right thing.
It may be buggy as hell, for all I know. And there are probably better
ways to do this.
(This one does just the 64-bit version, because it's a bit easier: the
32-bit version needs to probably do some of the loading and storing of
registers manually in the inline asm just to avoid running out of them,
since the register pressure is worse).
Linus
---
drivers/watchdog/hpwdt.c | 50 +++++++++++++++------------------------------
1 files changed, 17 insertions(+), 33 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 2686f3e..028c957 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -336,39 +336,23 @@ static int __devinit detect_cru_service(void)
asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
unsigned long *pRomEntry)
{
- asm("pushq %rbp \n\t"
- "movq %rsp, %rbp \n\t"
- "pushq %rax \n\t"
- "pushq %rbx \n\t"
- "pushq %rdx \n\t"
- "pushq %r12 \n\t"
- "pushq %r9 \n\t"
- "movq %rsi, %r12 \n\t"
- "movq %rdi, %r9 \n\t"
- "movl 4(%r9),%ebx \n\t"
- "movl 8(%r9),%ecx \n\t"
- "movl 12(%r9),%edx \n\t"
- "movl 16(%r9),%esi \n\t"
- "movl 20(%r9),%edi \n\t"
- "movl (%r9),%eax \n\t"
- "call *%r12 \n\t"
- "pushfq \n\t"
- "popq %r12 \n\t"
- "popfq \n\t"
- "movl %eax, (%r9) \n\t"
- "movl %ebx, 4(%r9) \n\t"
- "movl %ecx, 8(%r9) \n\t"
- "movl %edx, 12(%r9) \n\t"
- "movl %esi, 16(%r9) \n\t"
- "movl %edi, 20(%r9) \n\t"
- "movq %r12, %rax \n\t"
- "movl %eax, 28(%r9) \n\t"
- "popq %r9 \n\t"
- "popq %r12 \n\t"
- "popq %rdx \n\t"
- "popq %rbx \n\t"
- "popq %rax \n\t"
- "leave \n\t" "ret");
+ asm("pushq %0 ; popfq ; call *%6; pushfq; popfq %0"
+ :"=r" (pi86Regs->reflags),
+ "=a" (pi86Regs->u1.reax),
+ "=b" (pi86Regs->u2.rebx),
+ "=c" (pi86Regs->u3.recx),
+ "=d" (pi86Regs->u4.redx),
+ "=S" (pi86Regs->resi),
+ "=D" (pi86Regs->redi)
+ :"r" (pRomEntry),
+ "0" (pi86Regs->reflags),
+ "1" (pi86Regs->u1.reax),
+ "2" (pi86Regs->u2.rebx),
+ "3" (pi86Regs->u3.recx),
+ "4" (pi86Regs->u4.redx),
+ "5" (pi86Regs->resi),
+ "6" (pi86Regs->redi)
+ :"cc", "memory");
}
/*
Hi Linus;
18 Haz 2008 Çar tarihinde, Linus Torvalds şunları yazmıştı:
>
> On Wed, 18 Jun 2008, Wim Van Sebroeck wrote:
> >
> > Revert "[WATCHDOG] hpwdt: Fix NMI handling."
> >
> > To get this driver working we need the CFLAGS_hpwdt.o += -O in the Makefile.
> >
> > The driver needs the asmlinkage tag and the CFLAGS line in the Makefile.
> > Without it the driver doesn't work.
>
> The driver is unbelievable shit, and should just be removed, I think.
>
> The reason for all the games with CFLAGS and asmlinkage and utter crud
> seems to be that the driver is just BROKEN. It will work purely by luck,
> and depend entirely on the compiler not doing anything else AT ALL in that
> asm function.
>
> Could somebody please just fix the piece-of-sh*t thing?
>
> Here's a totally untested patch that may or may not work. At least it
> removes the assembler code that assumes that it controls the whole
> function from anything that gcc will then create a function prologue and
> epilogue for. Quite frankly, this is *not* the right thing to do either:
> the proper thing to do is to just move the low-level call into the "asm"
> statement, and leave all the function prologue/epilogue entirely to the
> compiler.
>
> But this way it isn't actively _broken_ by design, at least.
>
> No promises. I don't have the hardware. But somebody should *really* do
> this correctly.
I have both the hardware and the compiler related (gcc-4.3.1) bug :) (see [1])
Your modifications works like a charm with 32bit 2.6.25.7, kernel no longer opps and watchdog lives happily with hardware.
[...]
hpwdt: New timer passed in is 30 seconds.
hpwdt: CRU Base Address: 0xfff6b800
hpwdt: CRU Offset Address: 0x0
hpwdt: CRU Length: 0x8000
hpwdt: CRU Mapped Address: 0xf885bfa8
hp Watchdog Timer Driver: 1.00, timer margin: 30 seconds( nowayout=0).
[...]
> Linus
>
> ---
> drivers/watchdog/hpwdt.c | 155 ++++++++++++++++++++++++----------------------
> 1 files changed, 80 insertions(+), 75 deletions(-)
If needed
Tested-by: S.Çağlar Onur <[email protected]>
Thanks!
[1] http://marc.info/?l=linux-kernel&m=121266952602497&w=2
--
S.Çağlar Onur <[email protected]>
http://cekirdek.pardus.org.tr/~caglar/
Linux is like living in a teepee. No Windows, no Gates and an Apache in house!
Is this with 64 bit Linux?
Tom
-----Original Message-----
From: S.?a?lar Onur [mailto:[email protected]]
Sent: Wednesday, June 18, 2008 4:58 PM
To: Linus Torvalds
Cc: Wim Van Sebroeck; Andrew Morton; LKML; Mingarelli, Thomas
Subject: Re: [WATCHDOG] v2.6.26 hpwdt.c fixes
Hi Linus;
18 Haz 2008 ?ar tarihinde, Linus Torvalds ?unlar? yazm??t?:
>
> On Wed, 18 Jun 2008, Wim Van Sebroeck wrote:
> >
> > Revert "[WATCHDOG] hpwdt: Fix NMI handling."
> >
> > To get this driver working we need the CFLAGS_hpwdt.o += -O in the Makefile.
> >
> > The driver needs the asmlinkage tag and the CFLAGS line in the Makefile.
> > Without it the driver doesn't work.
>
> The driver is unbelievable shit, and should just be removed, I think.
>
> The reason for all the games with CFLAGS and asmlinkage and utter crud
> seems to be that the driver is just BROKEN. It will work purely by luck,
> and depend entirely on the compiler not doing anything else AT ALL in that
> asm function.
>
> Could somebody please just fix the piece-of-sh*t thing?
>
> Here's a totally untested patch that may or may not work. At least it
> removes the assembler code that assumes that it controls the whole
> function from anything that gcc will then create a function prologue and
> epilogue for. Quite frankly, this is *not* the right thing to do either:
> the proper thing to do is to just move the low-level call into the "asm"
> statement, and leave all the function prologue/epilogue entirely to the
> compiler.
>
> But this way it isn't actively _broken_ by design, at least.
>
> No promises. I don't have the hardware. But somebody should *really* do
> this correctly.
I have both the hardware and the compiler related (gcc-4.3.1) bug :) (see [1])
Your modifications works like a charm with 32bit 2.6.25.7, kernel no longer opps and watchdog lives happily with hardware.
[...]
hpwdt: New timer passed in is 30 seconds.
hpwdt: CRU Base Address: 0xfff6b800
hpwdt: CRU Offset Address: 0x0
hpwdt: CRU Length: 0x8000
hpwdt: CRU Mapped Address: 0xf885bfa8
hp Watchdog Timer Driver: 1.00, timer margin: 30 seconds( nowayout=0).
[...]
> Linus
>
> ---
> drivers/watchdog/hpwdt.c | 155 ++++++++++++++++++++++++----------------------
> 1 files changed, 80 insertions(+), 75 deletions(-)
If needed
Tested-by: S.?a?lar Onur <[email protected]>
Thanks!
[1] http://marc.info/?l=linux-kernel&m=121266952602497&w=2
--
S.?a?lar Onur <[email protected]>
http://cekirdek.pardus.org.tr/~caglar/
Linux is like living in a teepee. No Windows, no Gates and an Apache in house!
Hi;
19 Haz 2008 Per tarihinde, Mingarelli, Thomas ?unlar? yazm??t?:
> Is this with 64 bit Linux?
Nope, as i wrote, its 32 bit...
[...]
> I have both the hardware and the compiler related (gcc-4.3.1) bug :) (see [1])
>
> Your modifications works like a charm with 32bit 2.6.25.7, kernel no longer opps and watchdog lives happily with hardware.
^^^^^^^^^^^^^^
Cheers
--
S.?a?lar Onur <[email protected]>
http://cekirdek.pardus.org.tr/~caglar/
Linux is like living in a teepee. No Windows, no Gates and an Apache in house!
I agree that that portion of the driver can be improved upon immensely. It was a pain getting something to work. Your 32 bit changes have been tested by two separate parties and proven to work without the -O flag.
I am in the process of testing the 64 bit changes and will post the results asap.
Thanks,
Tom
-----Original Message-----
From: Linus Torvalds [mailto:[email protected]]
Sent: Wednesday, June 18, 2008 3:30 PM
To: Wim Van Sebroeck
Cc: Andrew Morton; LKML; Mingarelli, Thomas
Subject: Re: [WATCHDOG] v2.6.26 hpwdt.c fixes
On Wed, 18 Jun 2008, Linus Torvalds wrote:
>
> Quite frankly, this is *not* the right thing to do either: the proper
> thing to do is to just move the low-level call into the "asm" statement,
> and leave all the function prologue/epilogue entirely to the compiler.
Something like the following.
AGAIN! This is totally untested. It's meant as an *example* of how to use
inline asm properly, not meant to necessarily work or do the right thing.
It may be buggy as hell, for all I know. And there are probably better
ways to do this.
(This one does just the 64-bit version, because it's a bit easier: the
32-bit version needs to probably do some of the loading and storing of
registers manually in the inline asm just to avoid running out of them,
since the register pressure is worse).
Linus
---
drivers/watchdog/hpwdt.c | 50 +++++++++++++++------------------------------
1 files changed, 17 insertions(+), 33 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 2686f3e..028c957 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -336,39 +336,23 @@ static int __devinit detect_cru_service(void)
asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
unsigned long *pRomEntry)
{
- asm("pushq %rbp \n\t"
- "movq %rsp, %rbp \n\t"
- "pushq %rax \n\t"
- "pushq %rbx \n\t"
- "pushq %rdx \n\t"
- "pushq %r12 \n\t"
- "pushq %r9 \n\t"
- "movq %rsi, %r12 \n\t"
- "movq %rdi, %r9 \n\t"
- "movl 4(%r9),%ebx \n\t"
- "movl 8(%r9),%ecx \n\t"
- "movl 12(%r9),%edx \n\t"
- "movl 16(%r9),%esi \n\t"
- "movl 20(%r9),%edi \n\t"
- "movl (%r9),%eax \n\t"
- "call *%r12 \n\t"
- "pushfq \n\t"
- "popq %r12 \n\t"
- "popfq \n\t"
- "movl %eax, (%r9) \n\t"
- "movl %ebx, 4(%r9) \n\t"
- "movl %ecx, 8(%r9) \n\t"
- "movl %edx, 12(%r9) \n\t"
- "movl %esi, 16(%r9) \n\t"
- "movl %edi, 20(%r9) \n\t"
- "movq %r12, %rax \n\t"
- "movl %eax, 28(%r9) \n\t"
- "popq %r9 \n\t"
- "popq %r12 \n\t"
- "popq %rdx \n\t"
- "popq %rbx \n\t"
- "popq %rax \n\t"
- "leave \n\t" "ret");
+ asm("pushq %0 ; popfq ; call *%6; pushfq; popfq %0"
+ :"=r" (pi86Regs->reflags),
+ "=a" (pi86Regs->u1.reax),
+ "=b" (pi86Regs->u2.rebx),
+ "=c" (pi86Regs->u3.recx),
+ "=d" (pi86Regs->u4.redx),
+ "=S" (pi86Regs->resi),
+ "=D" (pi86Regs->redi)
+ :"r" (pRomEntry),
+ "0" (pi86Regs->reflags),
+ "1" (pi86Regs->u1.reax),
+ "2" (pi86Regs->u2.rebx),
+ "3" (pi86Regs->u3.recx),
+ "4" (pi86Regs->u4.redx),
+ "5" (pi86Regs->resi),
+ "6" (pi86Regs->redi)
+ :"cc", "memory");
}
/*
Mr. Torvalds, your changes to my inline assembly code worked fine without the -O flag for both 32 and 64 bit on ProLiant hardware. Hopefully the following will be acceptable.
Thanks for the input.
Tom
--- /home/linux-2.6.26-rc6/drivers/watchdog/hpwdt.c 2008-06-12 16:22:24.000000000 -0500
+++ hpwdt.c 2008-06-19 13:42:07.000000000 -0500
@@ -123,6 +123,9 @@
static unsigned long __iomem *hpwdt_timer_reg;
static unsigned long __iomem *hpwdt_timer_con;
+extern asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
+ unsigned long *pRomEntry);
+
static DEFINE_SPINLOCK(rom_lock);
static void *cru_rom_addr;
@@ -145,44 +148,45 @@
#define HPWDT_ARCH 32
-static void asminline_call(struct cmn_registers *pi86Regs,
- unsigned long *pRomEntry)
-{
- asm("pushl %ebp \n\t"
- "movl %esp, %ebp \n\t"
- "pusha \n\t"
- "pushf \n\t"
- "push %es \n\t"
- "push %ds \n\t"
- "pop %es \n\t"
- "movl 8(%ebp),%eax \n\t"
- "movl 4(%eax),%ebx \n\t"
- "movl 8(%eax),%ecx \n\t"
- "movl 12(%eax),%edx \n\t"
- "movl 16(%eax),%esi \n\t"
- "movl 20(%eax),%edi \n\t"
- "movl (%eax),%eax \n\t"
- "push %cs \n\t"
- "call *12(%ebp) \n\t"
- "pushf \n\t"
- "pushl %eax \n\t"
- "movl 8(%ebp),%eax \n\t"
- "movl %ebx,4(%eax) \n\t"
- "movl %ecx,8(%eax) \n\t"
- "movl %edx,12(%eax) \n\t"
- "movl %esi,16(%eax) \n\t"
- "movl %edi,20(%eax) \n\t"
- "movw %ds,24(%eax) \n\t"
- "movw %es,26(%eax) \n\t"
- "popl %ebx \n\t"
- "movl %ebx,(%eax) \n\t"
- "popl %ebx \n\t"
- "movl %ebx,28(%eax) \n\t"
- "pop %es \n\t"
- "popf \n\t"
- "popa \n\t"
- "leave \n\t" "ret");
-}
+asm(".text \n\t"
+ ".align 4 \n"
+ "asminline_call: \n\t"
+ "pushl %ebp \n\t"
+ "movl %esp, %ebp \n\t"
+ "pusha \n\t"
+ "pushf \n\t"
+ "push %es \n\t"
+ "push %ds \n\t"
+ "pop %es \n\t"
+ "movl 8(%ebp),%eax \n\t"
+ "movl 4(%eax),%ebx \n\t"
+ "movl 8(%eax),%ecx \n\t"
+ "movl 12(%eax),%edx \n\t"
+ "movl 16(%eax),%esi \n\t"
+ "movl 20(%eax),%edi \n\t"
+ "movl (%eax),%eax \n\t"
+ "push %cs \n\t"
+ "call *12(%ebp) \n\t"
+ "pushf \n\t"
+ "pushl %eax \n\t"
+ "movl 8(%ebp),%eax \n\t"
+ "movl %ebx,4(%eax) \n\t"
+ "movl %ecx,8(%eax) \n\t"
+ "movl %edx,12(%eax) \n\t"
+ "movl %esi,16(%eax) \n\t"
+ "movl %edi,20(%eax) \n\t"
+ "movw %ds,24(%eax) \n\t"
+ "movw %es,26(%eax) \n\t"
+ "popl %ebx \n\t"
+ "movl %ebx,(%eax) \n\t"
+ "popl %ebx \n\t"
+ "movl %ebx,28(%eax) \n\t"
+ "pop %es \n\t"
+ "popf \n\t"
+ "popa \n\t"
+ "leave \n\t"
+ "ret \n\t"
+ ".previous");
/*
* cru_detect
@@ -333,43 +337,44 @@
#define HPWDT_ARCH 64
-static void asminline_call(struct cmn_registers *pi86Regs,
- unsigned long *pRomEntry)
-{
- asm("pushq %rbp \n\t"
- "movq %rsp, %rbp \n\t"
- "pushq %rax \n\t"
- "pushq %rbx \n\t"
- "pushq %rdx \n\t"
- "pushq %r12 \n\t"
- "pushq %r9 \n\t"
- "movq %rsi, %r12 \n\t"
- "movq %rdi, %r9 \n\t"
- "movl 4(%r9),%ebx \n\t"
- "movl 8(%r9),%ecx \n\t"
- "movl 12(%r9),%edx \n\t"
- "movl 16(%r9),%esi \n\t"
- "movl 20(%r9),%edi \n\t"
- "movl (%r9),%eax \n\t"
- "call *%r12 \n\t"
- "pushfq \n\t"
- "popq %r12 \n\t"
- "popfq \n\t"
- "movl %eax, (%r9) \n\t"
- "movl %ebx, 4(%r9) \n\t"
- "movl %ecx, 8(%r9) \n\t"
- "movl %edx, 12(%r9) \n\t"
- "movl %esi, 16(%r9) \n\t"
- "movl %edi, 20(%r9) \n\t"
- "movq %r12, %rax \n\t"
- "movl %eax, 28(%r9) \n\t"
- "popq %r9 \n\t"
- "popq %r12 \n\t"
- "popq %rdx \n\t"
- "popq %rbx \n\t"
- "popq %rax \n\t"
- "leave \n\t" "ret");
-}
+asm(".text \n\t"
+ ".align 4 \n"
+ "asminline_call: \n\t"
+ "pushq %rbp \n\t"
+ "movq %rsp, %rbp \n\t"
+ "pushq %rax \n\t"
+ "pushq %rbx \n\t"
+ "pushq %rdx \n\t"
+ "pushq %r12 \n\t"
+ "pushq %r9 \n\t"
+ "movq %rsi, %r12 \n\t"
+ "movq %rdi, %r9 \n\t"
+ "movl 4(%r9),%ebx \n\t"
+ "movl 8(%r9),%ecx \n\t"
+ "movl 12(%r9),%edx \n\t"
+ "movl 16(%r9),%esi \n\t"
+ "movl 20(%r9),%edi \n\t"
+ "movl (%r9),%eax \n\t"
+ "call *%r12 \n\t"
+ "pushfq \n\t"
+ "popq %r12 \n\t"
+ "popfq \n\t"
+ "movl %eax, (%r9) \n\t"
+ "movl %ebx, 4(%r9) \n\t"
+ "movl %ecx, 8(%r9) \n\t"
+ "movl %edx, 12(%r9) \n\t"
+ "movl %esi, 16(%r9) \n\t"
+ "movl %edi, 20(%r9) \n\t"
+ "movq %r12, %rax \n\t"
+ "movl %eax, 28(%r9) \n\t"
+ "popq %r9 \n\t"
+ "popq %r12 \n\t"
+ "popq %rdx \n\t"
+ "popq %rbx \n\t"
+ "popq %rax \n\t"
+ "leave \n\t"
+ "ret \n\t"
+ ".previous");
/*
* dmi_find_cru
-----Original Message-----
From: Linus Torvalds [mailto:[email protected]]
Sent: Wednesday, June 18, 2008 3:30 PM
To: Wim Van Sebroeck
Cc: Andrew Morton; LKML; Mingarelli, Thomas
Subject: Re: [WATCHDOG] v2.6.26 hpwdt.c fixes
On Wed, 18 Jun 2008, Linus Torvalds wrote:
>
> Quite frankly, this is *not* the right thing to do either: the proper
> thing to do is to just move the low-level call into the "asm" statement,
> and leave all the function prologue/epilogue entirely to the compiler.
Something like the following.
AGAIN! This is totally untested. It's meant as an *example* of how to use
inline asm properly, not meant to necessarily work or do the right thing.
It may be buggy as hell, for all I know. And there are probably better
ways to do this.
(This one does just the 64-bit version, because it's a bit easier: the
32-bit version needs to probably do some of the loading and storing of
registers manually in the inline asm just to avoid running out of them,
since the register pressure is worse).
Linus
---
drivers/watchdog/hpwdt.c | 50 +++++++++++++++------------------------------
1 files changed, 17 insertions(+), 33 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 2686f3e..028c957 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -336,39 +336,23 @@ static int __devinit detect_cru_service(void)
asmlinkage void asminline_call(struct cmn_registers *pi86Regs,
unsigned long *pRomEntry)
{
- asm("pushq %rbp \n\t"
- "movq %rsp, %rbp \n\t"
- "pushq %rax \n\t"
- "pushq %rbx \n\t"
- "pushq %rdx \n\t"
- "pushq %r12 \n\t"
- "pushq %r9 \n\t"
- "movq %rsi, %r12 \n\t"
- "movq %rdi, %r9 \n\t"
- "movl 4(%r9),%ebx \n\t"
- "movl 8(%r9),%ecx \n\t"
- "movl 12(%r9),%edx \n\t"
- "movl 16(%r9),%esi \n\t"
- "movl 20(%r9),%edi \n\t"
- "movl (%r9),%eax \n\t"
- "call *%r12 \n\t"
- "pushfq \n\t"
- "popq %r12 \n\t"
- "popfq \n\t"
- "movl %eax, (%r9) \n\t"
- "movl %ebx, 4(%r9) \n\t"
- "movl %ecx, 8(%r9) \n\t"
- "movl %edx, 12(%r9) \n\t"
- "movl %esi, 16(%r9) \n\t"
- "movl %edi, 20(%r9) \n\t"
- "movq %r12, %rax \n\t"
- "movl %eax, 28(%r9) \n\t"
- "popq %r9 \n\t"
- "popq %r12 \n\t"
- "popq %rdx \n\t"
- "popq %rbx \n\t"
- "popq %rax \n\t"
- "leave \n\t" "ret");
+ asm("pushq %0 ; popfq ; call *%6; pushfq; popfq %0"
+ :"=r" (pi86Regs->reflags),
+ "=a" (pi86Regs->u1.reax),
+ "=b" (pi86Regs->u2.rebx),
+ "=c" (pi86Regs->u3.recx),
+ "=d" (pi86Regs->u4.redx),
+ "=S" (pi86Regs->resi),
+ "=D" (pi86Regs->redi)
+ :"r" (pRomEntry),
+ "0" (pi86Regs->reflags),
+ "1" (pi86Regs->u1.reax),
+ "2" (pi86Regs->u2.rebx),
+ "3" (pi86Regs->u3.recx),
+ "4" (pi86Regs->u4.redx),
+ "5" (pi86Regs->resi),
+ "6" (pi86Regs->redi)
+ :"cc", "memory");
}
/*
On Wed, Jun 18, 2008 at 01:09:43PM -0700, Linus Torvalds wrote:
>
>
> On Wed, 18 Jun 2008, Wim Van Sebroeck wrote:
> >
> > Revert "[WATCHDOG] hpwdt: Fix NMI handling."
> >
> > To get this driver working we need the CFLAGS_hpwdt.o += -O in the Makefile.
> >
> > The driver needs the asmlinkage tag and the CFLAGS line in the Makefile.
> > Without it the driver doesn't work.
>
> The driver is unbelievable shit, and should just be removed, I think.
FWIW, we coincidentally got the below oops reported yesterday.
This was against 2.6.25.4, but this doesn't seem related to the NMI path,
so probably unfixed ?
Dave
BUG: unable to handle kernel paging request at 5bf393fd
IP: [<f6559dc8>]
*pdpt = 00000000369a4001 *pde = 0000000000000000
Oops: 0000 [#1] SMP
Modules linked in: hpwdt(+) ipmi_si(+) sg iTCO_wdt ipmi_msghandler i5000_edac
button iTCO_vendor_support bnx2 sr_mod edac_core joydev pcspkr cdrom ata_piix
libata cciss sd_mod scsi_mod ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd [last
unloaded: scsi_wait_scan]
Pid: 1141, comm: modprobe Not tainted (2.6.25.4-10.fc8PAE #1)
EIP: 0060:[<f6559dc8>] EFLAGS: 00210286 CPU: 1
EIP is at 0xf6559dc8
EAX: 5f32335f EBX: 000f0000 ECX: 00cd0100 EDX: 00000000
ESI: d0ffffff EDI: c39bd09b EBP: f6559da8 ESP: f6559d78
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process modprobe (pid: 1141, ti=f6559000 task=f6570e70 task.ti=f6559000)
Stack: f89694a9 f7920060 f655007b 00200286 f7927000 ffffffed f6559da8 f6559da8
c00ffee0 00000000 000f1fff c00f0000 f6559dc8 c00f0000 c00ffee0 f6559dc8
c00f0000 f896a5d0 f896a5a0 f7927000 f6559ddc c050659d f7927054 00000000
Call Trace:
[<f89694a9>] ? hpwdt_init_one+0x18b/0x3a3 [hpwdt]
[<c050659d>] ? pci_device_probe+0x39/0x5b
[<c056a66b>] ? driver_probe_device+0xc0/0x137
[<c056a806>] ? __driver_attach+0x73/0xa9
[<c0569baf>] ? bus_for_each_dev+0x37/0x5c
[<c056a4f0>] ? driver_attach+0x14/0x16
[<c056a793>] ? __driver_attach+0x0/0xa9
[<c056a2fd>] ? bus_add_driver+0x90/0x1b7
[<c056a9fc>] ? driver_register+0x47/0xa2
[<c0506740>] ? __pci_register_driver+0x35/0x61
[<f88c8017>] ? hpwdt_init+0x17/0x19 [hpwdt]
[<c044649d>] ? sys_init_module+0x1610/0x177a
[<c062e63c>] ? do_page_fault+0x528/0x909
[<c0437238>] ? param_get_int+0x0/0x15
[<c0484bd3>] ? do_sync_read+0x0/0xe9
[<c0485896>] ? sys_read+0x3b/0x60
[<c0404b7a>] ? syscall_call+0x7/0xb
=======================
Code: 00 00 00 ff 1f 0f 00 00 00 0f c0 c8 9d 55 f6 00 00 0f c0 e0 fe 0f c0 c8 9d
55 f6 00 00 0f c0 d0 a5 96 f8 a0 a5 96 f8 00 70 92 f7 <dc> 9d 55 f6 9d 65 50 c0
54 70 92 f7 00 <6>udev: renamed network interface eth0 to eth10
00 00 00 d0 a5 96 f8 f4
EIP: [<f6559dc8>] 0xf6559dc8 SS:ESP 0068:f6559d78
---[ end trace f60151d50a977a2e ]---
--
http://www.codemonkey.org.uk
On Fri, 20 Jun 2008, Dave Jones wrote:
>
> FWIW, we coincidentally got the below oops reported yesterday.
> This was against 2.6.25.4, but this doesn't seem related to the NMI path,
> so probably unfixed ?
This could be fixed by my patch - you're running in 32-bit, so it's using
the "asmcall" thing in detection.
I committed my first (hackier, but simpler) patch that was tested by some
people on both 32-bit and apparently 64-bit too. It *may* fix this.
Linus
That is correct. Linus' patch has fixed the issue on both 32 and 64 bit ProLiant platforms.
Tom
-----Original Message-----
From: Linus Torvalds [mailto:[email protected]]
Sent: Friday, June 20, 2008 2:30 PM
To: Dave Jones
Cc: Wim Van Sebroeck; Andrew Morton; LKML; Mingarelli, Thomas
Subject: Re: [WATCHDOG] v2.6.26 hpwdt.c fixes
On Fri, 20 Jun 2008, Dave Jones wrote:
>
> FWIW, we coincidentally got the below oops reported yesterday.
> This was against 2.6.25.4, but this doesn't seem related to the NMI path,
> so probably unfixed ?
This could be fixed by my patch - you're running in 32-bit, so it's using
the "asmcall" thing in detection.
I committed my first (hackier, but simpler) patch that was tested by some
people on both 32-bit and apparently 64-bit too. It *may* fix this.
Linus