Re: [WATCHDOG] v2.6.26 hpwdt.c fixes

From: Linus Torvalds
Date: Wed Jun 18 2008 - 16:10:41 EST



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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/