Re: [PATCH] ioc3-eth.c: add missing pci_enable_device()

Re: [PATCH] ioc3-eth.c: add missing pci_enable_device()

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> ChangeSet 1.1843.1.74, 2004/08/24 11:21:53-07:00, bjorn.helgaas@hp.com
> 
> 	[PATCH] ioc3-eth.c: add missing pci_enable_device()
> 	
> 	Add pci_enable_device()/pci_disable_device().  In the past, drivers often
> 	worked without this, but it is now required in order to route PCI interrupts
> 	correctly.
> 	
> 	Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 	Signed-off-by: Andrew Morton <akpm@osdl.org>
> 	Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> 
> 
> 
>  ioc3-eth.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> 
> diff -Nru a/drivers/net/ioc3-eth.c b/drivers/net/ioc3-eth.c
> --- a/drivers/net/ioc3-eth.c	2004-08-24 15:25:26 -07:00
> +++ b/drivers/net/ioc3-eth.c	2004-08-24 15:25:26 -07:00
> @@ -1172,9 +1172,14 @@
>  	u32 vendor, model, rev;
>  	int err;
>  
> +	if (pci_enable_device(pdev))
> +		return -ENODEV;
> +
>  	dev = alloc_etherdev(sizeof(struct ioc3_private));
> -	if (!dev)
> -		return -ENOMEM;
> +	if (!dev) {
> +		err = -ENOMEM;
> +		goto out_disable;
> +	}
>  
>  	err = pci_request_regions(pdev, "ioc3");
>  	if (err)
> @@ -1269,6 +1274,8 @@
>  	pci_release_regions(pdev);
>  out_free:
>  	free_netdev(dev);
> +out_disable:
> +	pci_disable_device(pdev);
>  	return err;
>  }
>  
> @@ -1282,6 +1289,7 @@
>  	iounmap(ioc3);
>  	pci_release_regions(pdev);
>  	free_netdev(dev);
> +	pci_disable_device(pdev);


I don't see a "signed-off-by" line from Ralf.  I noticed you never 
bothered to send this patch to me.  Did you send it to Ralf either?

ioc3 is _very_ strange device and not fully compliant to the PCI spec.

I would appreciate more review and testing before these patches go in, 
particularly against net drivers.  pci_enable_device() is NOT just a 
simple cleanup:

* each driver may (though unlikely) manage its PCI_COMMAND bits and/or 
resources in a special way.  IDE driver is an example of where 
pci_enable_device() _cannot_ be used.

* like ioc3, the hardware may be weird

* you must consider the case of two drivers for the same hardware VERY 
carefully.  Consider:

1) (DRV A) modprobe
2) (DRV A) pci_enable_device()
3) (DRV A) starts operation

4) (DRV B) modprobe
5) (DRV B) pci_enable_device()
6) (DRV B) pci_request_regions() or request_region() fails (since driver 
A owns the resources)
7) (DRV B) pci_disable_device()

8) (DRV A) fails miserably, because you just disabled IO/MEM bits from 
an _active_ driver.  BOOM.


Not all cleanups are created equal.  Proceed with caution.

	Jeff

Re: [PATCH] ioc3-eth.c: add missing pci_enable_device()

[Ralf Baechle]

On Tue, Aug 24, 2004 at 08:40:38PM -0400, Jeff Garzik wrote:

> I don't see a "signed-off-by" line from Ralf.  I noticed you never 
> bothered to send this patch to me.  Did you send it to Ralf either?
> 
> ioc3 is _very_ strange device and not fully compliant to the PCI spec.
> 
> I would appreciate more review and testing before these patches go in, 
> particularly against net drivers.  pci_enable_device() is NOT just a 
> simple cleanup:
> 
> * each driver may (though unlikely) manage its PCI_COMMAND bits and/or 
> resources in a special way.  IDE driver is an example of where 
> pci_enable_device() _cannot_ be used.
> 
> * like ioc3, the hardware may be weird
> 
> * you must consider the case of two drivers for the same hardware VERY 
> carefully.  Consider:
> 
> 1) (DRV A) modprobe
> 2) (DRV A) pci_enable_device()
> 3) (DRV A) starts operation
> 
> 4) (DRV B) modprobe
> 5) (DRV B) pci_enable_device()
> 6) (DRV B) pci_request_regions() or request_region() fails (since driver 
> A owns the resources)
> 7) (DRV B) pci_disable_device()
> 
> 8) (DRV A) fails miserably, because you just disabled IO/MEM bits from 
> an _active_ driver.  BOOM.

... and that's similar to the hole into which IOC3 is falling.  IOC3 is a
multi function device - but not in sense of the PCI spec.  IOC3 provides a
standard ethernet interface with the usual gadgets like PHY interfacing.
It also contains a PS/2 keyboard / mouse interface (thanks to which my
headless Origin 200 has each 4 keyboard and mouse connectors!), a
486-style backside bus on which a standard PC SuperIO chip providing
the 16552s and a RTC chip are hooked up.  As things are right now each of
these functions is provided by a different driver and there is no
central coordination that ensures pci_disable_device() will only be called
by the time the last driver has finished it's business.  As consequence
until we can cleanly handle this is not calling pci_disable_device().

Since the original posting I also found that the original argument about
interrupt routing is bogus; in violation of the PCI spec IOC3 supports
multiple interrupt pins.  On IP27 (Origin, Onyx 2) they're all just wired
together re-establishing PCI compliance.  That's not the case on Octane
which needs special handling outside of what the normal PCI code provides.

  Ralf

Re: [PATCH] ioc3-eth.c: add missing pci_enable_device()

[Bjorn Helgaas]

On Tuesday 24 August 2004 6:40 pm, Jeff Garzik wrote:
> Linux Kernel Mailing List wrote:
> > ChangeSet 1.1843.1.74, 2004/08/24 11:21:53-07:00, bjorn.helgaas@hp.com
> > 
> > 	[PATCH] ioc3-eth.c: add missing pci_enable_device()
> > 	
> > 	Add pci_enable_device()/pci_disable_device().  In the past, drivers often
> > 	worked without this, but it is now required in order to route PCI interrupts
> > 	correctly.
> > 	
> > 	Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > 	Signed-off-by: Andrew Morton <akpm@osdl.org>
> > 	Signed-off-by: Linus Torvalds <torvalds@osdl.org>

> I don't see a "signed-off-by" line from Ralf.  I noticed you never 
> bothered to send this patch to me.  Did you send it to Ralf either?

As a matter of fact, I did send it to Ralf, based on this:

	IOC3 DRIVER
	P:      Ralf Baechle
	M:      ralf@linux-mips.org
	L:      linux-mips@linux-mips.org
	S:      Maintained

Once I found the specific IOC3 entry, I neglected to search farther
and find the more generic "NETWORK DEVICE DRIVERS" entry, so I didn't
send it to you, Jeff; sorry about that.

> ioc3 is _very_ strange device and not fully compliant to the PCI spec.

OK, I don't know anything about ioc3, other than the fact that it
appeared to use pci_dev->irq without doing pci_enable_device().
All ACPI-based PCI interrupt routing is now done in pci_enable_device()
(in -mm, not yet in mainline), so if ioc3 were used in an ACPI-based
system, it would likely be broken.

I'm fine with reverting the change.  Here's a patch to do that:


Revert addition of pci_enable_device().

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

===== drivers/net/ioc3-eth.c 1.27 vs edited =====
--- 1.27/drivers/net/ioc3-eth.c	2004-08-24 03:08:34 -06:00
+++ edited/drivers/net/ioc3-eth.c	2004-08-25 08:56:24 -06:00
@@ -1172,14 +1172,9 @@
 	u32 vendor, model, rev;
 	int err;
 
-	if (pci_enable_device(pdev))
-		return -ENODEV;
-
 	dev = alloc_etherdev(sizeof(struct ioc3_private));
-	if (!dev) {
-		err = -ENOMEM;
-		goto out_disable;
-	}
+	if (!dev)
+		return -ENOMEM;
 
 	err = pci_request_regions(pdev, "ioc3");
 	if (err)
@@ -1274,8 +1269,6 @@
 	pci_release_regions(pdev);
 out_free:
 	free_netdev(dev);
-out_disable:
-	pci_disable_device(pdev);
 	return err;
 }
 
@@ -1289,7 +1282,6 @@
 	iounmap(ioc3);
 	pci_release_regions(pdev);
 	free_netdev(dev);
-	pci_disable_device(pdev);
 }
 
 static struct pci_device_id ioc3_pci_tbl[] = {

Re: [PATCH] ioc3-eth.c: add missing pci_enable_device()

[Christoph Hellwig]

On Wed, Aug 25, 2004 at 09:03:27AM -0600, Bjorn Helgaas wrote:
> OK, I don't know anything about ioc3, other than the fact that it
> appeared to use pci_dev->irq without doing pci_enable_device().
> All ACPI-based PCI interrupt routing is now done in pci_enable_device()
> (in -mm, not yet in mainline), so if ioc3 were used in an ACPI-based
> system, it would likely be broken.

The ioc3 is only used on mips-based systems (and some very early IA64-based
prototypes from SGI), and neither of them supports ACPI.