More late merge bug fixes for 2.6.23

PATCH In review Andi Kleen 4 participants 10 messages Aug 15, 2007 – Aug 20, 2007

lore

Share & tools

lore

Andi Kleen

author

2007-08-15 00:40 UTC #0 lore raw

cover More late merge bug fixes for 2.6.23

More late bug fixe for 2.6.23.

-Andi

Andi Kleen

author

2007-08-15 00:41 UTC #1 lore raw

PATCH 1/4 x86_64: Fail dma_alloc_coherent on dma less devices

This should fix an oops with PCMCIA PATA devices

http://bugzilla.kernel.org/show_bug.cgi?id=8424

This is not a full fix for the problem, but probably still the right thing to do.

Signed-off-by Andi Kleen <[email protected]>

---
 arch/x86_64/kernel/pci-dma.c |    4 ++++
 1 file changed, 4 insertions(+)

arch/x86_64/kernel/pci-dma.c +4 -0

Index: linux/arch/x86_64/kernel/pci-dma.c
===================================================================
@@ -82,6 +82,10 @@ dma_alloc_coherent(struct device *dev, s
 	if (dma_mask == 0)
 		dma_mask = DMA_32BIT_MASK;
 
+	/* Device not DMA able */
+	if (dev->dma_mask == NULL)
+		return NULL;
+
 	/* Don't invoke OOM killer */
 	gfp |= __GFP_NORETRY;

Linus Torvalds

2007-08-18 17:23 UTC #2 lore raw

Re: [PATCH] [1/4] x86_64: Fail dma_alloc_coherent on dma less devices

Hmm. I think this is wrong.

Why? Because the regular 32-bit x86 code does this all completely differently, and doesn't use dma_mask at all. Instead, it _only_ uses dev->coherent_dma_mask (which, considering the name of the function, would seem to make sense).

Considering that the oops comes from this:

        /* Kludge to make it bug-to-bug compatible with i386. i386
           uses the normal dma_mask for alloc_coherent. */
        dma_mask &= *dev->dma_mask;

and that that code is *old*, and comes from when this file was called arch/x86_64/kernel/pci-gart.c, and the comment doesn't seem to even be correct any more, I really think the proper fix is likely to just *remove* that kludge that causes the oops entirely.

Anyway, I'll apply the patch, because clearly it's not going to make things *worse* (it does avoid the oops that we get now), but I don't think it's really even "probably still the right thing to do". I really think we should just remove the line that causes the oops instead, but that might change behaviour for non-oopsing cases, so I'm not ready to do that at this point.

Hmm? Who feels in charge of the DMA mapping stuff? Muli? James? Anybody?

Linus

Quoted 30 lines · Andi Kleen

This should fix an oops with PCMCIA PATA devices
http://bugzilla.kernel.org/show_bug.cgi?id=8424
This is not a full fix for the problem, but probably still the right thing to do.
Signed-off-by Andi Kleen <[email protected]>
---
 arch/x86_64/kernel/pci-dma.c |    4 ++++
 1 file changed, 4 insertions(+)
Index: linux/arch/x86_64/kernel/pci-dma.c
===================================================================
arch/x86_64/kernel/pci-dma.c +4 -0
@@ -82,6 +82,10 @@ dma_alloc_coherent(struct device *dev, s
 	if (dma_mask == 0)
 		dma_mask = DMA_32BIT_MASK;
 
+	/* Device not DMA able */
+	if (dev->dma_mask == NULL)
+		return NULL;
+
 	/* Don't invoke OOM killer */
 	gfp |= __GFP_NORETRY;
 

Andi Kleen

author

2007-08-18 19:04 UTC #3 lore raw

Re: [PATCH] [1/4] x86_64: Fail dma_alloc_coherent on dma less devices

Quoted 7 lines · On Saturday 18 August 2007 19:22:18 Linus Torvalds wrote:

Hmm. I think this is wrong.
Why? Because the regular 32-bit x86 code does this all completely differently, and doesn't use dma_mask at all. Instead, it _only_ uses dev->coherent_dma_mask (which, considering the name of the function, would seem to make sense).

Yes, see my discussion with Alan. Likely this needs to be handled in the caller to really fix the problem (pata_pcmcia oopsing when it passes a DMA incapable pcmcia device to dma_alloc_coherent)

Another possible fix proposed by James was to give the pcmcia devices a dma_mask of 0 which would also work.

Quoted 9 lines

Considering that the oops comes from this:
        /* Kludge to make it bug-to-bug compatible with i386. i386
           uses the normal dma_mask for alloc_coherent. */
        dma_mask &= *dev->dma_mask;
and that that code is *old*, and comes from when this file was called arch/x86_64/kernel/pci-gart.c, and the comment doesn't seem to even be

It might be outdated or it might now. The kludge was needed for Alsa because old i386 ignored the consistent mask and they didn't always set it correctly, but that should be obsolete now? I'm not quite sure because sound devices
are not always well tested on large memory systems which are the only ones who show this problem. Takashi, do you know if all alsa drivers set consistent mask correctly now?

But I didn't want to touch that that late -- can do it for .24.

Still failing is probably correct in this case so I included the patch.

-Andi

Takashi Iwai

2007-08-20 09:47 UTC #4 lore raw

Re: [PATCH] [1/4] x86_64: Fail dma_alloc_coherent on dma less devices

At Sat, 18 Aug 2007 21:03:25 +0200,

Quoted 11 lines · Andi Kleen

Quoted 10 lines
Considering that the oops comes from this:
        /* Kludge to make it bug-to-bug compatible with i386. i386
           uses the normal dma_mask for alloc_coherent. */
        dma_mask &= *dev->dma_mask;
and that that code is *old*, and comes from when this file was called arch/x86_64/kernel/pci-gart.c, and the comment doesn't seem to even be

Quoted 6 lines

It might be outdated or it might now. The kludge was needed for Alsa because old i386 ignored the consistent mask and they didn't always set it correctly, but that should be obsolete now? I'm not quite sure because sound devices
are not always well tested on large memory systems which are the only ones who show this problem. Takashi, do you know if all alsa drivers set consistent mask correctly now?

Yes, ALSA driver set both masks from the very beginning :)

There are some OSS drivers still calling only pci_set_dma_mask(). Through a quick look, maybe only trident.c needs a fix because it uses a mask < 32bit for ALI devices. Others use 32bit DMA mask.

Takashi

Andi Kleen

author

2007-08-15 00:42 UTC #5 lore raw

PATCH 2/4 x86_64: Fix to keep watchdog disabled by default for i386/x86_64

In reply to Andi Kleen

From: Daniel Gollub <[email protected]>
Fixed wrong expression which enabled watchdogs even if nmi_watchdog kernel parameter wasn't set. This regression got slightly introduced with commit b7471c6da94d30d3deadc55986cc38d1ff57f9ca.

Introduced NMI_DISABLED (-1) which allows to switch the value of NMI_DEFAULT without breaking the APIC NMI watchdog code (again).

Fixes:
https://bugzilla.novell.com/show_bug.cgi?id=298084
http://bugzilla.kernel.org/show_bug.cgi?id=7839
And likely some more nmi_watchdog=0 related issues.

Signed-off-by Daniel Gollub <[email protected]>

Signed-off-by Andi Kleen <[email protected]>

---

arch/i386/kernel/apic.c +1 -1

Index: linux/arch/i386/kernel/apic.c
===================================================================
@@ -1085,7 +1085,7 @@ static int __init detect_init_APIC (void
 	if (l & MSR_IA32_APICBASE_ENABLE)
 		mp_lapic_addr = l & MSR_IA32_APICBASE_BASE;
 
-	if (nmi_watchdog != NMI_NONE)
+	if (nmi_watchdog != NMI_NONE && nmi_watchdog != NMI_DISABLED)
 		nmi_watchdog = NMI_LOCAL_APIC;
 
 	printk(KERN_INFO "Found and enabled local APIC!\n");

arch/i386/kernel/nmi.c +2 -2

Index: linux/arch/i386/kernel/nmi.c
===================================================================
@@ -77,7 +77,7 @@ static int __init check_nmi_watchdog(voi
 	unsigned int *prev_nmi_count;
 	int cpu;
 
-	if ((nmi_watchdog == NMI_NONE) || (nmi_watchdog == NMI_DEFAULT))
+	if ((nmi_watchdog == NMI_NONE) || (nmi_watchdog == NMI_DISABLED))
 		return 0;
 
 	if (!atomic_read(&nmi_active))

@@ -424,7 +424,7 @@ int proc_nmi_enabled(struct ctl_table *t
 	if (!!old_state == !!nmi_watchdog_enabled)
 		return 0;
 
-	if (atomic_read(&nmi_active) < 0) {
+	if (atomic_read(&nmi_active) < 0 || nmi_watchdog == NMI_DISABLED) {
 		printk( KERN_WARNING "NMI watchdog is permanently disabled\n");
 		return -EIO;
 	}

arch/x86_64/kernel/nmi.c +2 -2

Index: linux/arch/x86_64/kernel/nmi.c
===================================================================
@@ -85,7 +85,7 @@ int __init check_nmi_watchdog (void)
 	int *counts;
 	int cpu;
 
-	if ((nmi_watchdog == NMI_NONE) || (nmi_watchdog == NMI_DEFAULT))
+	if ((nmi_watchdog == NMI_NONE) || (nmi_watchdog == NMI_DISABLED)) 
 		return 0;
 
 	if (!atomic_read(&nmi_active))

@@ -442,7 +442,7 @@ int proc_nmi_enabled(struct ctl_table *t
 	if (!!old_state == !!nmi_watchdog_enabled)
 		return 0;
 
-	if (atomic_read(&nmi_active) < 0) {
+	if (atomic_read(&nmi_active) < 0 || nmi_watchdog == NMI_DISABLED) {
 		printk( KERN_WARNING "NMI watchdog is permanently disabled\n");
 		return -EIO;
 	}

include/asm-i386/nmi.h +2 -1

Index: linux/include/asm-i386/nmi.h
===================================================================
@@ -33,11 +33,12 @@ extern int nmi_watchdog_tick (struct pt_
 
 extern atomic_t nmi_active;
 extern unsigned int nmi_watchdog;
-#define NMI_DEFAULT     -1
+#define NMI_DISABLED    -1
 #define NMI_NONE	0
 #define NMI_IO_APIC	1
 #define NMI_LOCAL_APIC	2
 #define NMI_INVALID	3
+#define NMI_DEFAULT	NMI_DISABLED
 
 struct ctl_table;
 struct file;

include/asm-x86_64/nmi.h +2 -1

Index: linux/include/asm-x86_64/nmi.h
===================================================================
@@ -64,11 +64,12 @@ extern int setup_nmi_watchdog(char *);
 
 extern atomic_t nmi_active;
 extern unsigned int nmi_watchdog;
-#define NMI_DEFAULT	-1
+#define NMI_DISABLED    -1
 #define NMI_NONE	0
 #define NMI_IO_APIC	1
 #define NMI_LOCAL_APIC	2
 #define NMI_INVALID	3
+#define NMI_DEFAULT	NMI_DISABLED
 
 struct ctl_table;
 struct file;

Andi Kleen

author

2007-08-15 00:42 UTC #6 lore raw

PATCH 3/4 x86_64: Change PMDS invocation to single macro

In reply to Andi Kleen

Very old binutils (2.12.90...) seem to have trouble with newlines in assembler macro invocation. They put them into the resulting argument expansion. In this case this lead to a parse error because a .rept expression ended up spread over multiple lines. Change the PMDS() invocation to a single line.

Signed-off-by Andi Kleen <[email protected]>

arch/x86_64/kernel/head.S +1 -2

Index: linux/arch/x86_64/kernel/head.S
===================================================================
@@ -345,8 +345,7 @@ NEXT_PAGE(level2_kernel_pgt)
 	/* 40MB kernel mapping. The kernel code cannot be bigger than that.
 	   When you change this change KERNEL_TEXT_SIZE in page.h too. */
 	/* (2^48-(2*1024*1024*1024)-((2^39)*511)-((2^30)*510)) = 0 */
-	PMDS(0x0000000000000000, __PAGE_KERNEL_LARGE_EXEC|_PAGE_GLOBAL,
-		KERNEL_TEXT_SIZE/PMD_SIZE)
+	PMDS(0x0000000000000000, __PAGE_KERNEL_LARGE_EXEC|_PAGE_GLOBAL, KERNEL_TEXT_SIZE/PMD_SIZE)
 	/* Module mapping starts here */
 	.fill	(PTRS_PER_PMD - (KERNEL_TEXT_SIZE/PMD_SIZE)),8,0

Clemens Koller

2007-08-15 12:29 UTC #7 lore raw

Re: [PATCH] [3/4] x86_64: Change PMDS invocation to single macro

Quoted 12 lines · Andi Kleen

Very old binutils (2.12.90...) seem to have trouble with newlines
in assembler macro invocation. They put them into the resulting
argument expansion. In this case this lead to a parse error because
a .rept expression ended up spread over multiple lines. Change the PMDS() invocation to a single line.
Signed-off-by Andi Kleen <[email protected]>
Index: linux/arch/x86_64/kernel/head.S
===================================================================

arch/x86_64/kernel/head.S NEXT_PAGE(level2_kernel_pgt) · line 348

Quoted 6 lines

arch/x86_64/kernel/head.S +1 -2

@@ -345,8 +345,7 @@ NEXT_PAGE(level2_kernel_pgt)
 	/* 40MB kernel mapping. The kernel code cannot be bigger than that.
 	   When you change this change KERNEL_TEXT_SIZE in page.h too. */
 	/* (2^48-(2*1024*1024*1024)-((2^39)*511)-((2^30)*510)) = 0 */
-	PMDS(0x0000000000000000, __PAGE_KERNEL_LARGE_EXEC|_PAGE_GLOBAL,
-		KERNEL_TEXT_SIZE/PMD_SIZE)
+	PMDS(0x0000000000000000, __PAGE_KERNEL_LARGE_EXEC|_PAGE_GLOBAL, KERNEL_TEXT_SIZE/PMD_SIZE)

Can you please add a comment, that this line must stay in a single line for the above reason? I would expect that the next who does some code clean up will break it again.

arch/x86_64/kernel/head.S NEXT_PAGE(level2_kernel_pgt) · line 350

Quoted 2 lines

arch/x86_64/kernel/head.S +0 -0

 	/* Module mapping starts here */
 	.fill	(PTRS_PER_PMD - (KERNEL_TEXT_SIZE/PMD_SIZE)),8,0

Regards,

Signature · 11 lines

Clemens Koller
_______________________________
R&D Imaging Devices
Anagramm GmbH
Rupert-Mayer-Str. 45/1
81379 Muenchen
Germany

http://www.anagramm-technology.com
Phone: +49-89-741518-50
Fax: +49-89-741518-19

Andi Kleen

author

2007-08-15 13:04 UTC #8 lore raw

Re: [PATCH] [3/4] x86_64: Change PMDS invocation to single macro

Quoted 3 lines

Can you please add a comment, that this line must stay in a single line for the above reason? I would expect that the next who does some code clean up will break it again.

Then we would need to add it to all macros essentially. That wouldn't be economical.

-Andi

Andi Kleen

author

2007-08-15 00:43 UTC #9 lore raw

PATCH 4/4 x86_64: Check for .cfi_rel_offset in CFI probe

In reply to Andi Kleen

Very old 64bit binutils have .cfi_startproc/endproc, but no .cfi_rel_offset. Check for .cfi_rel_offset too.

Cc [email protected]

arch/x86_64/Makefile +2 -2

Index: linux/arch/x86_64/Makefile
===================================================================
@@ -57,8 +57,8 @@ cflags-y += $(call cc-option,-mno-sse -m
 cflags-y += -maccumulate-outgoing-args
 
 # do binutils support CFI?
-cflags-y += $(call as-instr,.cfi_startproc\n.cfi_endproc,-DCONFIG_AS_CFI=1,)
-AFLAGS += $(call as-instr,.cfi_startproc\n.cfi_endproc,-DCONFIG_AS_CFI=1,)
+cflags-y += $(call as-instr,.cfi_startproc\n.cfi_rel_offset rsp${comma}0\n.cfi_endproc,-DCONFIG_AS_CFI=1,)
+AFLAGS += $(call as-instr,.cfi_startproc\n.cfi_rel_offset rsp${comma}0\n.cfi_endproc,-DCONFIG_AS_CFI=1,)
 
 # is .cfi_signal_frame supported too?
 cflags-y += $(call as-instr,.cfi_startproc\n.cfi_signal_frame\n.cfi_endproc,-DCONFIG_AS_CFI_SIGNAL_FRAME=1,)

arch/i386/Makefile +2 -2

Index: linux/arch/i386/Makefile
===================================================================
@@ -51,8 +51,8 @@ cflags-y += -maccumulate-outgoing-args
 CFLAGS				+= $(shell if [ $(call cc-version) -lt 0400 ] ; then echo $(call cc-option,-fno-unit-at-a-time); fi ;)
 
 # do binutils support CFI?
-cflags-y += $(call as-instr,.cfi_startproc\n.cfi_endproc,-DCONFIG_AS_CFI=1,)
-AFLAGS += $(call as-instr,.cfi_startproc\n.cfi_endproc,-DCONFIG_AS_CFI=1,)
+cflags-y += $(call as-instr,.cfi_startproc\n.cfi_rel_offset esp${comma}0\n.cfi_endproc,-DCONFIG_AS_CFI=1,)
+AFLAGS += $(call as-instr,.cfi_startproc\n.cfi_rel_offset esp${comma}0\n.cfi_endproc,-DCONFIG_AS_CFI=1,)
 
 # is .cfi_signal_frame supported too?
 cflags-y += $(call as-instr,.cfi_startproc\n.cfi_signal_frame\n.cfi_endproc,-DCONFIG_AS_CFI_SIGNAL_FRAME=1,)