Re: 2.6.8.1-mm2 (nvidia breakage)

Re: 2.6.8.1-mm2 (nvidia breakage)

[Bjorn Helgaas]

Hi Michael,

> kernel 2.6.8.1-mm2 booting here with acpi_os_name=Linux and pci=routeirq.
> Without pci=routeirq hangs at startx (nvidia).

I assume this problem is with the Nvidia binary-only driver?  My guess
is that the driver doesn't call pci_enable_device() before using
pci_dev->irq.  I don't have source for that driver, so I can't verify
this.

If the driver is from Nvidia, please contact their technical support
and request that they fix the driver.  I can help explain the issue if
necessary.

I also noticed this in your dmesg:

	floppy: controller ACPI FDC0 at I/O 0x3f7-0x3f7 irq 6 dma channel 2
	Using anticipatory io scheduler
	Floppy drive(s): fd0 is 1.44M
	floppy0: no floppy controllers found

I hope that you have the floppy controller disabled in your BIOS,
or at least have no floppy drives connected.  Can you confirm that?
I expected that ACPI would not report the controller at all in such
a case, but it looks like yours reports the controller with a zero-
length I/O port region.  I probably should check for that.

If you do have a floppy drive, it probably doesn't work anymore
unless you specify "floppy=no_acpi".  If this is the case, we'll
have to figure out a quirk or something to make it work.

Thanks for the testing report!

Re: 2.6.8.1-mm2 (nvidia breakage)

[Alan Cox]

On Iau, 2004-08-19 at 17:51, Bjorn Helgaas wrote:
> I assume this problem is with the Nvidia binary-only driver?  My guess
> is that the driver doesn't call pci_enable_device() before using
> pci_dev->irq.  I don't have source for that driver, so I can't verify
> this.

An obvious test would be for someone with an Nvidia to write a little
module that does nothing but pci_enable_device it. Load and unload that
then see what happens

Re: 2.6.8.1-mm2 (nvidia breakage)

[Terence Ripperda]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]


the original bug report was during module load time, when we're
probing our devices via the pci_driver's probe callback. this is well
before we hook up interrupts or do anything in our closed source code.

I'm attaching a trimmed down version of our driver that pretty much
only does this probe (complete source is included). I don't know if
this will reproduce the original bug or not.

Let me know if I can help anymore.

Thanks,
Terence

On Thu, Aug 19, 2004 at 06:53:56PM +0100, alan@lxorguk.ukuu.org.uk wrote:
> On Iau, 2004-08-19 at 17:51, Bjorn Helgaas wrote:
> > I assume this problem is with the Nvidia binary-only driver?  My guess
> > is that the driver doesn't call pci_enable_device() before using
> > pci_dev->irq.  I don't have source for that driver, so I can't verify
> > this.
> 
> An obvious test would be for someone with an Nvidia to write a little
> module that does nothing but pci_enable_device it. Load and unload that
> then see what happens
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[-- Attachment #2: nv_mini.tar.gz --]
[-- Type: application/octet-stream, Size: 7676 bytes --]

Re: 2.6.8.1-mm2 (nvidia breakage)

[Bjorn Helgaas]

On Thursday 19 August 2004 3:51 pm, Terence Ripperda wrote:
> the original bug report was during module load time, when we're
> probing our devices via the pci_driver's probe callback. this is well
> before we hook up interrupts or do anything in our closed source code.
> 
> I'm attaching a trimmed down version of our driver that pretty much
> only does this probe (complete source is included). I don't know if
> this will reproduce the original bug or not.

Thanks, this is enough to show the problem:

	nv_kern_probe(struct pci_dev *dev, ...)
	{
		...
		nv->interrupt_line = dev->irq;
		...
		if (pci_enable_device(dev) != 0)

The driver is looking at dev->irq before calling pci_enable_device().
But dev->irq is not necessarily initialized before pci_enable_device().

I'm not a PCI expert, but I'm not sure you should be looking at
all the other dev->resource[] stuff before pci_enable_device()
either.  Most of the "modern" drivers in the tree seem to do
pci_enable_device() very early in the probe() function, i.e., 
see tg3.c.

Re: 2.6.8.1-mm2 (nvidia breakage)

[Alan Cox]

On Iau, 2004-08-19 at 23:03, Bjorn Helgaas wrote:
> The driver is looking at dev->irq before calling pci_enable_device().
> But dev->irq is not necessarily initialized before pci_enable_device().
> 
> I'm not a PCI expert, but I'm not sure you should be looking at
> all the other dev->resource[] stuff before pci_enable_device()
> either. 

Indeed. pci_enable_device may assign the actual BARs as well as the
interrupt lines to the device. I suspect its harmless here because the
base video device is BIOS configured.

Re: 2.6.8.1-mm2 (nvidia breakage)

[Terence Ripperda]

Thanks Bjorn,

I'll move that call to pci_enable_device to earlier in the probe call.

but in Kevin's original email, he's hitting an oops within the
pci_enable_device call. is that likely due to pci_enable_device being
called late?

Thanks,
Terence

On Thu, Aug 19, 2004 at 04:03:55PM -0600, bjorn.helgaas@hp.com wrote:
> On Thursday 19 August 2004 3:51 pm, Terence Ripperda wrote:
> > the original bug report was during module load time, when we're
> > probing our devices via the pci_driver's probe callback. this is well
> > before we hook up interrupts or do anything in our closed source code.
> > 
> > I'm attaching a trimmed down version of our driver that pretty much
> > only does this probe (complete source is included). I don't know if
> > this will reproduce the original bug or not.
> 
> Thanks, this is enough to show the problem:
> 
> 	nv_kern_probe(struct pci_dev *dev, ...)
> 	{
> 		...
> 		nv->interrupt_line = dev->irq;
> 		...
> 		if (pci_enable_device(dev) != 0)
> 
> The driver is looking at dev->irq before calling pci_enable_device().
> But dev->irq is not necessarily initialized before pci_enable_device().
> 
> I'm not a PCI expert, but I'm not sure you should be looking at
> all the other dev->resource[] stuff before pci_enable_device()
> either.  Most of the "modern" drivers in the tree seem to do
> pci_enable_device() very early in the probe() function, i.e., 
> see tg3.c.

Re: 2.6.8.1-mm2 (nvidia breakage)

[Bjorn Helgaas]

On Thursday 19 August 2004 4:58 pm, Terence Ripperda wrote:
> I'll move that call to pci_enable_device to earlier in the probe call.

Thanks.

> but in Kevin's original email, he's hitting an oops within the
> pci_enable_device call. is that likely due to pci_enable_device being
> called late?

I don't seem to have an email from Kevin.  Can you forward it to
me?  The thread I know about started with mail from Michael Geithe
that Andrew Morton forwarded to me:

> kernel 2.6.8.1-mm2 booting here with acpi_os_name=Linux and pci=routeirq.
> Without pci=routeirq hangs at startx (nvidia).

I didn't see any oops there.

Re: 2.6.8.1-mm2 (nvidia breakage)

[Michael Geithe]

On Friday 20 August 2004 17:53, you wrote:

> I didn't see any oops there.

Hi Bjorn,

i searched my logs and found that.

Aug 20 18:03:53 stargate kernel: irq 16: nobody cared!
Aug 20 18:03:53 stargate kernel:  [<c0106520>] __report_bad_irq+0x24/0x7b
Aug 20 18:03:53 stargate kernel:  [<c01065f9>] note_interrupt+0x64/0x88
Aug 20 18:03:53 stargate kernel:  [<c010694d>] do_IRQ+0x1af/0x1fb
Aug 20 18:03:53 stargate kernel:  [<c0104890>] common_interrupt+0x18/0x20
Aug 20 18:03:53 stargate kernel: handlers:
Aug 20 18:03:53 stargate kernel: [<f9a11145>] (usb_hcd_irq+0x0/0x5d [usbcore])
Aug 20 18:03:53 stargate kernel: [<f9a11145>] (usb_hcd_irq+0x0/0x5d [usbcore])
Aug 20 18:03:53 stargate kernel: Disabling IRQ #16


-- 
Michael Geithe

-

Re: 2.6.8.1-mm2 (nvidia breakage)

[Terence Ripperda]

sure, I meant to include this link in the earlier email and forgot:

http://www.uwsg.indiana.edu/hypermail/linux/kernel/0408.1/0127.html

On Fri, Aug 20, 2004 at 09:53:11AM -0600, bjorn.helgaas@hp.com wrote:
> I don't seem to have an email from Kevin.  Can you forward it to
> me?  The thread I know about started with mail from Michael Geithe
> that Andrew Morton forwarded to me:

Re: 2.6.8.1-mm2 (nvidia breakage)

[Bjorn Helgaas]

On Thursday 19 August 2004 4:58 pm, Terence Ripperda wrote:
> but in Kevin's original email[1], he's hitting an oops within the
> pci_enable_device call. is that likely due to pci_enable_device being
> called late?

[1] http://www.uwsg.indiana.edu/hypermail/linux/kernel/0408.1/0127.html

Kevin, I suspect the oops you're seeing is due to the problem mentioned
here (add_pin_to_irq() is __init but shouldn't be):

    http://groups.google.com/groups?dq=&start=75&hl=en&lr=&ie=UTF-8&group=fa.linux.kernel&c2coff=1&selm=fa.j8igrci.1fgg680%40ifi.uio.no

Can you try Randy's patch without using "pci=routeirq" and see whether
it resolves the oops?  Of course, the nvidia driver still won't work
because it's looking at pci_dev->irq before calling pci_enable_device(),
but that's a separate issue.

Re: 2.6.8.1-mm2 (nvidia breakage)

[Terence Ripperda]

[-- Attachment #1: Type: text/plain, Size: 407 bytes --]

On Mon, Aug 23, 2004 at 09:30:18AM -0600, bjorn.helgaas@hp.com wrote:
> Of course, the nvidia driver still won't work
> because it's looking at pci_dev->irq before calling pci_enable_device(),
> but that's a separate issue.

as Alan pointed out, the video device is bios configured, so may not
be hit by this. nonetheless, we've applied a patch along these lines
to our internal codebase.

Thanks,
Terence


[-- Attachment #2: nv_enable_pci.patch --]
[-- Type: text/plain, Size: 3640 bytes --]

--- nv/nv.c	2004-08-23 13:58:15.000000000 -0500
+++ nv.new/nv.c	2004-08-23 13:58:35.000000000 -0500
@@ -1225,6 +1225,7 @@
         {
             nv_state_t *nv = NV_STATE_PTR(&nv_linux_devices[i]);
             release_mem_region(nv->bar.regs.address, nv->bar.regs.size);
+            pci_disable_device(nv_linux_devices[i].dev);
         }
     }
 
@@ -3516,6 +3517,28 @@
         return -1;
     }
 
+    // enable io, mem, and bus-mastering in pci config space
+    if (pci_enable_device(dev) != 0)
+    {
+        nv_printf(NV_DBG_ERRORS,
+            "NVRM: pci_enable_device failed, aborting\n");
+        return -1;
+    }
+
+    // request ownership of our bars
+    // keeps other drivers from banging our registers.
+    // only do this for registers, as vesafb requests our framebuffer and will
+    // keep us from working properly
+    if (!request_mem_region(dev->resource[0].start,
+                            dev->resource[0].end - dev->resource[0].start + 1,
+                            "nvidia"))
+    {
+        nv_printf(NV_DBG_ERRORS,
+            "NVRM: pci_request_regions failed, aborting\n");
+        goto err_disable_dev;
+    }
+    pci_set_master(dev);
+
     /* initialize bus-dependent config state */
     nvl = &nv_linux_devices[num_nv_devices];
     nv  = NV_STATE_PTR(nvl);
@@ -3545,7 +3568,7 @@
         nv_printf(NV_DBG_ERRORS, "NVRM: Please check your BIOS settings.         \n");
         nv_printf(NV_DBG_ERRORS, "NVRM: [Plug & Play OS   ] should be set to NO  \n");
         nv_printf(NV_DBG_ERRORS, "NVRM: [Assign IRQ to VGA] should be set to YES \n");
-        return -1;
+        goto err_zero_dev;
     }
 
     /* sanity check the IO apertures */
@@ -3569,39 +3592,9 @@
                 nv->bar.fb.address, nv->bar.fb.size);
         }
 
-        /* Clear out the data */
-        os_mem_set(nvl, 0, sizeof(nv_linux_state_t));
-
-        return -1;
-    }
-
-    // request ownership of our bars
-    // keeps other drivers from banging our registers.
-    // only do this for registers, as vesafb requests our framebuffer and will
-    // keep us from working properly
-    if (!request_mem_region(nv->bar.regs.address, nv->bar.regs.size, "nvidia"))
-    {
-        nv_printf(NV_DBG_ERRORS,
-            "NVRM: pci_request_regions failed, aborting\n");
-
-        /* Clear out the data */
-        os_mem_set(nvl, 0, sizeof(nv_linux_state_t));
-
-        return -1;
+        goto err_zero_dev;
     }
 
-    // enable io, mem, and bus-mastering in pci config space
-    if (pci_enable_device(dev) != 0)
-    {
-        nv_printf(NV_DBG_ERRORS,
-            "NVRM: pci_enable_device failed, aborting\n");
-
-        pci_release_regions(dev);
-        os_mem_set(nvl, 0, sizeof(nv_linux_state_t));
-
-        return -1;
-    }
-    pci_set_master(nvl->dev);
 
 #if defined(NV_BUILD_NV_PAT_SUPPORT)
     if (nvos_find_pci_express_capability(nvl->dev))
@@ -3618,13 +3611,7 @@
     if (nv->bar.regs.map == NULL)
     {
         nv_printf(NV_DBG_ERRORS, "NVRM: failed to map registers!!\n");
-
-        pci_release_regions(dev);
-
-        /* Clear out the data */
-        os_mem_set(nvl, 0, sizeof(nv_linux_state_t));
-
-        return -1;
+        goto err_zero_dev;
     }
     nv->flags |= NV_FLAG_MAP_REGS_EARLY;
 #endif
@@ -3641,6 +3628,15 @@
     num_nv_devices++;
 
     return 0;
+
+err_zero_dev:
+    os_mem_set(nvl, 0, sizeof(nv_linux_state_t));
+    release_mem_region(dev->resource[0].start,
+                       dev->resource[0].end - dev->resource[0].start + 1);
+
+err_disable_dev:
+    pci_disable_device(dev);
+    return -1;
 }
 
 int NV_API_CALL nv_no_incoherent_mappings

Re: 2.6.8.1-mm2 (nvidia breakage)

[Bjorn Helgaas]

On Monday 23 August 2004 1:01 pm, Terence Ripperda wrote:
> On Mon, Aug 23, 2004 at 09:30:18AM -0600, bjorn.helgaas@hp.com wrote:
> > Of course, the nvidia driver still won't work
> > because it's looking at pci_dev->irq before calling pci_enable_device(),
> > but that's a separate issue.
> 
> as Alan pointed out, the video device is bios configured, so may not
> be hit by this. nonetheless, we've applied a patch along these lines
> to our internal codebase.

To be pedantically clear about this, looking at pci_dev->irq before
calling pci_enable_device() is *guaranteed* to fail, regardless of
what the BIOS does.  So nvidia users will have to use "pci=routeirq"
until there's a new version of the nvidia driver.

Alan was specifically referring to the BAR configuration that may
be done by the BIOS.

I'm assuming your patch makes the driver call pci_enable_device()
before using either irq or BAR information.  That's the best way
to future-proof the driver.

Re: 2.6.8.1-mm2 (nvidia breakage)

[Terence Ripperda]

On Tue, Aug 24, 2004 at 09:26:42AM -0600, bjorn.helgaas@hp.com wrote:
> To be pedantically clear about this, looking at pci_dev->irq before
> calling pci_enable_device() is *guaranteed* to fail, regardless of
> what the BIOS does.  So nvidia users will have to use "pci=routeirq"
> until there's a new version of the nvidia driver.

ah, ok. thanks for clarifying.

> I'm assuming your patch makes the driver call pci_enable_device()
> before using either irq or BAR information.  That's the best way
> to future-proof the driver.

yes, that's exactly what we've done.

Thanks,
Terence

Re: 2.6.8.1-mm2 (nvidia breakage)

[Roland Dreier]

    Bjorn> To be pedantically clear about this, looking at
    Bjorn> pci_dev->irq before calling pci_enable_device() is
    Bjorn> *guaranteed* to fail, regardless of what the BIOS does.  So
    Bjorn> nvidia users will have to use "pci=routeirq" until there's
    Bjorn> a new version of the nvidia driver.

Terence, correct me if I'm wrong, but the change to add
pci_enable_device() goes in the part of the nvidia driver that has
source available.  So users can apply this patch themselves even
without another Nvidia release.

 - Roland

Re: 2.6.8.1-mm2 (nvidia breakage)

[Terence Ripperda]

On Tue, Aug 24, 2004 at 10:36:14AM -0700, roland@topspin.com wrote:
> Terence, correct me if I'm wrong, but the change to add
> pci_enable_device() goes in the part of the nvidia driver that has
> source available.  So users can apply this patch themselves even
> without another Nvidia release.

correct.

Thanks,
Terence

Re: 2.6.8.1-mm2 (nvidia breakage) [u]

[Martin Schlemmer [c]]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

On Wed, 2004-08-25 at 00:03, Terence Ripperda wrote:
> On Tue, Aug 24, 2004 at 10:36:14AM -0700, roland@topspin.com wrote:
> > Terence, correct me if I'm wrong, but the change to add
> > pci_enable_device() goes in the part of the nvidia driver that has
> > source available.  So users can apply this patch themselves even
> > without another Nvidia release.
> 
> correct.
> 

Should just pass it along to minion.de ...


-- 
Martin Schlemmer

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]