Skip to contentSkip to conversation

ioc3-eth.c: add missing pci_enable_device()

PATCH Jeff Garzik 4 participants 4 messages Aug 25, 2004 1 message not in the archive Network drivers

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jeff Garzik

author
#1 lore raw
Quoted 48 lines · Linux Kernel Mailing List

ChangeSet 1.1843.1.74, 2004/08/24 11:21:53-07:00, [email protected]

[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 <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>
 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

drivers/net/ioc3-eth.c +9 -2
@@ -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;
 }

drivers/net/ioc3-eth.c line 1292

Quoted 4 lines
drivers/net/ioc3-eth.c +1 -0
@@ -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

#2 lore raw
Quoted 22 lines · Jeff Garzik

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

Quoted 8 lines

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

#3 lore raw
In reply to Jeff Garzik
Quoted 9 lines · Jeff Garzik
Quoted 8 lines · Linux Kernel Mailing List

ChangeSet 1.1843.1.74, 2004/08/24 11:21:53-07:00, [email protected]

[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.

Quoted 6 lines
Quoted 3 lines
Signed-off-by Bjorn Helgaas <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>

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: [email protected]
L: [email protected]
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.

Quoted 1 line

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 <[email protected]>

===== drivers/net/ioc3-eth.c 1.27 vs edited =====

drivers/net/ioc3-eth.c +2 -10
@@ -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[] = {
#4 lore raw
Quoted 5 lines · Bjorn Helgaas

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.