* [WATCHDOG] v2.6.26 hpwdt.c fixes
@ 2008-06-18 19:49 Wim Van Sebroeck
2008-06-18 20:09 ` Linus Torvalds
0 siblings, 1 reply; 11+ messages in thread
From: Wim Van Sebroeck @ 2008-06-18 19:49 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Andrew Morton, LKML, Thomas Mingarelli
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 <wim@iguana.be>
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 <Thomas.Mingarelli@hp.com>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Author: Thomas Mingarelli <Thomas.Mingarelli@hp.com>
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 <Thomas.Mingarelli@hp.com>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Author: Thomas Mingarelli <Thomas.Mingarelli@hp.com>
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 <Thomas.Mingarelli@hp.com>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
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;
}
/*
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [WATCHDOG] v2.6.26 hpwdt.c fixes 2008-06-18 19:49 [WATCHDOG] v2.6.26 hpwdt.c fixes Wim Van Sebroeck @ 2008-06-18 20:09 ` Linus Torvalds 2008-06-18 20:30 ` Linus Torvalds ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Linus Torvalds @ 2008-06-18 20:09 UTC (permalink / raw) To: Wim Van Sebroeck; +Cc: Andrew Morton, LKML, Thomas Mingarelli 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [WATCHDOG] v2.6.26 hpwdt.c fixes 2008-06-18 20:09 ` Linus Torvalds @ 2008-06-18 20:30 ` Linus Torvalds 2008-06-19 14:58 ` Mingarelli, Thomas 2008-06-19 19:38 ` Mingarelli, Thomas 2008-06-18 21:58 ` S.Çağlar Onur 2008-06-20 19:05 ` Dave Jones 2 siblings, 2 replies; 11+ messages in thread From: Linus Torvalds @ 2008-06-18 20:30 UTC (permalink / raw) To: Wim Van Sebroeck; +Cc: Andrew Morton, LKML, Thomas Mingarelli 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"); } /* ^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [WATCHDOG] v2.6.26 hpwdt.c fixes 2008-06-18 20:30 ` Linus Torvalds @ 2008-06-19 14:58 ` Mingarelli, Thomas 2008-06-19 19:38 ` Mingarelli, Thomas 1 sibling, 0 replies; 11+ messages in thread From: Mingarelli, Thomas @ 2008-06-19 14:58 UTC (permalink / raw) To: Linus Torvalds, Wim Van Sebroeck; +Cc: Andrew Morton, LKML 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:torvalds@linux-foundation.org] 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"); } /* ^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [WATCHDOG] v2.6.26 hpwdt.c fixes 2008-06-18 20:30 ` Linus Torvalds 2008-06-19 14:58 ` Mingarelli, Thomas @ 2008-06-19 19:38 ` Mingarelli, Thomas 1 sibling, 0 replies; 11+ messages in thread From: Mingarelli, Thomas @ 2008-06-19 19:38 UTC (permalink / raw) To: Linus Torvalds, Wim Van Sebroeck; +Cc: Andrew Morton, LKML 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:torvalds@linux-foundation.org] 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"); } /* ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [WATCHDOG] v2.6.26 hpwdt.c fixes 2008-06-18 20:09 ` Linus Torvalds 2008-06-18 20:30 ` Linus Torvalds @ 2008-06-18 21:58 ` S.Çağlar Onur 2008-06-18 22:03 ` Mingarelli, Thomas 2008-06-20 19:05 ` Dave Jones 2 siblings, 1 reply; 11+ messages in thread From: S.Çağlar Onur @ 2008-06-18 21:58 UTC (permalink / raw) To: Linus Torvalds; +Cc: Wim Van Sebroeck, Andrew Morton, LKML, Thomas Mingarelli 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 <caglar@pardus.org.tr> Thanks! [1] http://marc.info/?l=linux-kernel&m=121266952602497&w=2 -- S.Çağlar Onur <caglar@pardus.org.tr> http://cekirdek.pardus.org.tr/~caglar/ Linux is like living in a teepee. No Windows, no Gates and an Apache in house! ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [WATCHDOG] v2.6.26 hpwdt.c fixes 2008-06-18 21:58 ` S.Çağlar Onur @ 2008-06-18 22:03 ` Mingarelli, Thomas 2008-06-18 22:08 ` S.Çağlar Onur 0 siblings, 1 reply; 11+ messages in thread From: Mingarelli, Thomas @ 2008-06-18 22:03 UTC (permalink / raw) To: caglar@pardus.org.tr, Linus Torvalds Cc: Wim Van Sebroeck, Andrew Morton, LKML Is this with 64 bit Linux? Tom -----Original Message----- From: S.Çağlar Onur [mailto:caglar@pardus.org.tr] 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 <caglar@pardus.org.tr> Thanks! [1] http://marc.info/?l=linux-kernel&m=121266952602497&w=2 -- S.Çağlar Onur <caglar@pardus.org.tr> http://cekirdek.pardus.org.tr/~caglar/ Linux is like living in a teepee. No Windows, no Gates and an Apache in house! ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [WATCHDOG] v2.6.26 hpwdt.c fixes 2008-06-18 22:03 ` Mingarelli, Thomas @ 2008-06-18 22:08 ` S.Çağlar Onur 0 siblings, 0 replies; 11+ messages in thread From: S.Çağlar Onur @ 2008-06-18 22:08 UTC (permalink / raw) To: Mingarelli, Thomas; +Cc: Linus Torvalds, Wim Van Sebroeck, Andrew Morton, LKML 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 <caglar@pardus.org.tr> http://cekirdek.pardus.org.tr/~caglar/ Linux is like living in a teepee. No Windows, no Gates and an Apache in house! ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [WATCHDOG] v2.6.26 hpwdt.c fixes 2008-06-18 20:09 ` Linus Torvalds 2008-06-18 20:30 ` Linus Torvalds 2008-06-18 21:58 ` S.Çağlar Onur @ 2008-06-20 19:05 ` Dave Jones 2008-06-20 19:30 ` Linus Torvalds 2 siblings, 1 reply; 11+ messages in thread From: Dave Jones @ 2008-06-20 19:05 UTC (permalink / raw) To: Linus Torvalds; +Cc: Wim Van Sebroeck, Andrew Morton, LKML, Thomas Mingarelli 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [WATCHDOG] v2.6.26 hpwdt.c fixes 2008-06-20 19:05 ` Dave Jones @ 2008-06-20 19:30 ` Linus Torvalds 2008-06-20 20:40 ` Mingarelli, Thomas 0 siblings, 1 reply; 11+ messages in thread From: Linus Torvalds @ 2008-06-20 19:30 UTC (permalink / raw) To: Dave Jones; +Cc: Wim Van Sebroeck, Andrew Morton, LKML, Thomas Mingarelli 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [WATCHDOG] v2.6.26 hpwdt.c fixes 2008-06-20 19:30 ` Linus Torvalds @ 2008-06-20 20:40 ` Mingarelli, Thomas 0 siblings, 0 replies; 11+ messages in thread From: Mingarelli, Thomas @ 2008-06-20 20:40 UTC (permalink / raw) To: Linus Torvalds, Dave Jones; +Cc: Wim Van Sebroeck, Andrew Morton, LKML That is correct. Linus' patch has fixed the issue on both 32 and 64 bit ProLiant platforms. Tom -----Original Message----- From: Linus Torvalds [mailto:torvalds@linux-foundation.org] 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2008-06-20 20:49 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-06-18 19:49 [WATCHDOG] v2.6.26 hpwdt.c fixes Wim Van Sebroeck 2008-06-18 20:09 ` Linus Torvalds 2008-06-18 20:30 ` Linus Torvalds 2008-06-19 14:58 ` Mingarelli, Thomas 2008-06-19 19:38 ` Mingarelli, Thomas 2008-06-18 21:58 ` S.Çağlar Onur 2008-06-18 22:03 ` Mingarelli, Thomas 2008-06-18 22:08 ` S.Çağlar Onur 2008-06-20 19:05 ` Dave Jones 2008-06-20 19:30 ` Linus Torvalds 2008-06-20 20:40 ` Mingarelli, Thomas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.