2.6.23-rc8-mm1, -rc7-mm1 kill audio on HP nx6325

2.6.23-rc8-mm1, -rc7-mm1 kill audio on HP nx6325

[Rafael J. Wysocki]

Hi,

This patch from Andi:

x86_64-mm-cpa-einval.patch

makes the hda_intel audio driver stop working on my HP nx6325.

The following line appears in dmesg (from 2.6.23-rc7-mm1:

ALSA /home/rafael/src/mm/linux-2.6.23-rc7-mm1/sound/pci/hda/hda_intel.c:1755: hd
a-intel: ioremap error

and the driver doesn't work afterwards.

Still, I'm not sure if the patch above is wrong or rather it exposes a problem
in the driver.

Greetings,
Rafael

Re: 2.6.23-rc8-mm1, -rc7-mm1 kill audio on HP nx6325

[Thomas Gleixner]

On Tue, 2007-09-25 at 14:08 +0200, Rafael J. Wysocki wrote:
> Hi,
> 
> This patch from Andi:
> 
> x86_64-mm-cpa-einval.patch
> 
> makes the hda_intel audio driver stop working on my HP nx6325.
> 
> The following line appears in dmesg (from 2.6.23-rc7-mm1:
> 
> ALSA /home/rafael/src/mm/linux-2.6.23-rc7-mm1/sound/pci/hda/hda_intel.c:1755: hd
> a-intel: ioremap error
> 
> and the driver doesn't work afterwards.
> 
> Still, I'm not sure if the patch above is wrong or rather it exposes a problem
> in the driver.

The patch is correct. Instead of returning "Success" in the case of a
failure of lookup_address, it now returns -EINVAL, which in turn makes
the ioremap fail.

OTOH, the driver ioremap call looks straight forward. Can you apply the
patch below and provide the resulting debug output please ?

Thanks,

	tglx

Index: linux-2.6.23-rc8-mm/arch/x86_64/mm/pageattr.c
===================================================================
--- linux-2.6.23-rc8-mm.orig/arch/x86_64/mm/pageattr.c	2007-09-25 14:05:41.000000000 +0200
+++ linux-2.6.23-rc8-mm/arch/x86_64/mm/pageattr.c	2007-09-25 14:09:35.000000000 +0200
@@ -156,8 +156,10 @@ __change_page_attr(unsigned long address
 	pgprot_t ref_prot2;
 
 	kpte = lookup_address(address);
-	if (!kpte)
+	if (!kpte) {
+		printk("lookup failed for %lu\n", address);
 		return -EINVAL;
+	}
 
 	kpte_page = virt_to_page(((unsigned long)kpte) & PAGE_MASK);
 	BUG_ON(PageCompound(kpte_page));
Index: linux-2.6.23-rc8-mm/sound/pci/hda/hda_intel.c
===================================================================
--- linux-2.6.23-rc8-mm.orig/sound/pci/hda/hda_intel.c	2007-09-25 14:05:43.000000000 +0200
+++ linux-2.6.23-rc8-mm/sound/pci/hda/hda_intel.c	2007-09-25 14:09:28.000000000 +0200
@@ -1752,7 +1752,8 @@ static int __devinit azx_create(struct s
 	chip->addr = pci_resource_start(pci, 0);
 	chip->remap_addr = ioremap_nocache(chip->addr, pci_resource_len(pci,0));
 	if (chip->remap_addr == NULL) {
-		snd_printk(KERN_ERR SFX "ioremap error\n");
+		snd_printk(KERN_ERR SFX "ioremap error: %lu %lu\n",
+			   chip->addr, pci_resource_len(pci, 0));
 		err = -ENXIO;
 		goto errout;
 	}

Re: 2.6.23-rc8-mm1, -rc7-mm1 kill audio on HP nx6325

[Rafael J. Wysocki]

On Tuesday, 25 September 2007 14:18, Thomas Gleixner wrote:
> On Tue, 2007-09-25 at 14:08 +0200, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > This patch from Andi:
> > 
> > x86_64-mm-cpa-einval.patch
> > 
> > makes the hda_intel audio driver stop working on my HP nx6325.
> > 
> > The following line appears in dmesg (from 2.6.23-rc7-mm1:
> > 
> > ALSA /home/rafael/src/mm/linux-2.6.23-rc7-mm1/sound/pci/hda/hda_intel.c:1755: hd
> > a-intel: ioremap error
> > 
> > and the driver doesn't work afterwards.
> > 
> > Still, I'm not sure if the patch above is wrong or rather it exposes a problem
> > in the driver.
> 
> The patch is correct. Instead of returning "Success" in the case of a
> failure of lookup_address, it now returns -EINVAL, which in turn makes
> the ioremap fail.
> 
> OTOH, the driver ioremap call looks straight forward. Can you apply the
> patch below and provide the resulting debug output please ?

lookup failed for 18446604438082158592
[--snipped some USB messages--]
ALSA /home/rafael/src/mm/linux-2.6.23-rc8-mm1/sound/pci/hda/hda_intel.c:1756: hda-intel: ioremap error: 2349334528 16384

Greetings,
Rafael

Re: 2.6.23-rc8-mm1, -rc7-mm1 kill audio on HP nx6325

[Thomas Gleixner]

On Tue, 2007-09-25 at 15:20 +0200, Rafael J. Wysocki wrote:
> > The patch is correct. Instead of returning "Success" in the case of a
> > failure of lookup_address, it now returns -EINVAL, which in turn makes
> > the ioremap fail.
> > 
> > OTOH, the driver ioremap call looks straight forward. Can you apply the
> > patch below and provide the resulting debug output please ?
> 
> lookup failed for 18446604438082158592
> [--snipped some USB messages--]
> ALSA /home/rafael/src/mm/linux-2.6.23-rc8-mm1/sound/pci/hda/hda_intel.c:1756: hda-intel: ioremap error: 2349334528 16384

Stupid me, hex formatting would have been easier to read :)

Lookup failed for 0xFFFF 8100 8C08 0000
ioremap:          0x0000 0000 8C08 0000 length 16384

It seems, that this patch only reveals some other wreckage. The code is
called as part of ioremap, where it adjusts the caching attributes of
the mapping, which was setup right before change_page_attr_address() is
called.

	tglx

Re: 2.6.23-rc8-mm1, -rc7-mm1 kill audio on HP nx6325

[Rafael J. Wysocki]

On Tuesday, 25 September 2007 15:44, Thomas Gleixner wrote:
> On Tue, 2007-09-25 at 15:20 +0200, Rafael J. Wysocki wrote:
> > > The patch is correct. Instead of returning "Success" in the case of a
> > > failure of lookup_address, it now returns -EINVAL, which in turn makes
> > > the ioremap fail.
> > > 
> > > OTOH, the driver ioremap call looks straight forward. Can you apply the
> > > patch below and provide the resulting debug output please ?
> > 
> > lookup failed for 18446604438082158592
> > [--snipped some USB messages--]
> > ALSA /home/rafael/src/mm/linux-2.6.23-rc8-mm1/sound/pci/hda/hda_intel.c:1756: hda-intel: ioremap error: 2349334528 16384
> 
> Stupid me, hex formatting would have been easier to read :)
> 
> Lookup failed for 0xFFFF 8100 8C08 0000
> ioremap:          0x0000 0000 8C08 0000 length 16384
> 
> It seems, that this patch only reveals some other wreckage. The code is
> called as part of ioremap, where it adjusts the caching attributes of
> the mapping, which was setup right before change_page_attr_address() is
> called.

Hm, it looks like the first address is a kernel one and the second one is
physical, so they apparently match, which means that the lookup shouldn't fail,
if I understand this correctly.

Greetings,
Rafael

Re: 2.6.23-rc8-mm1, -rc7-mm1 kill audio on HP nx6325

[Thomas Gleixner]

On Tue, 2007-09-25 at 16:29 +0200, Rafael J. Wysocki wrote: 
> > > lookup failed for 18446604438082158592
> > > [--snipped some USB messages--]
> > > ALSA /home/rafael/src/mm/linux-2.6.23-rc8-mm1/sound/pci/hda/hda_intel.c:1756: hda-intel: ioremap error: 2349334528 16384
> > 
> > Stupid me, hex formatting would have been easier to read :)
> > 
> > Lookup failed for 0xFFFF 8100 8C08 0000
> > ioremap:          0x0000 0000 8C08 0000 length 16384
> > 
> > It seems, that this patch only reveals some other wreckage. The code is
> > called as part of ioremap, where it adjusts the caching attributes of
> > the mapping, which was setup right before change_page_attr_address() is
> > called.
> 
> Hm, it looks like the first address is a kernel one and the second one is
> physical, so they apparently match, which means that the lookup shouldn't fail,
> if I understand this correctly.

Yes, the lookup address is virtual and it should be the one, which was
mapped right before the call to change_page_attr_address(). I'm looking
into that right now.

	tglx

Re: 2.6.23-rc8-mm1, -rc7-mm1 kill audio on HP nx6325

[Andrew Morton]

On Tue, 25 Sep 2007 13:53:51 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> Hi,
> 
> This patch from Andi:
> 
> x86_64-mm-cpa-einval.patch
> 
> makes the hda_intel audio driver stop working on my HP nx6325.
> 
> The following line appears in dmesg (from 2.6.23-rc7-mm1:
> 
> ALSA /home/rafael/src/mm/linux-2.6.23-rc7-mm1/sound/pci/hda/hda_intel.c:1755: hd
> a-intel: ioremap error
> 
> and the driver doesn't work afterwards.
> 
> Still, I'm not sure if the patch above is wrong or rather it exposes a problem
> in the driver.
> 

Mysterious.

ioremap_nocache() does __ioremap(..., _PAGE_PCD);, then __ioremap() does
ioremap_page_range(..., _PAGE_PCD | other_stuff) That's one.

__ioremap() then does ioremap_change_attr(..., _PAGE_PCD);.  That's two.

So I _think_ we're setting _PAGE_PCD twice on those pte's?  Unclear.  The
implementation is rather different from i386, too.

I dunno why __change_page_attr() failed though.  Perhaps this, in
change_page_attr_addr():

		if (!kernel_map || pte_present(pfn_pte(0, prot))) {

should be &&

Andi's taking a bit of time off, so we may need to park this until he
resurfaces.