* Re: [PATCH] ioc3-eth.c: add missing pci_enable_device()
2004-08-25 0:40 ` [PATCH] ioc3-eth.c: add missing pci_enable_device() Jeff Garzik
@ 2004-08-25 5:49 ` Ralf Baechle
2004-08-25 15:03 ` Bjorn Helgaas
1 sibling, 0 replies; 4+ messages in thread
From: Ralf Baechle @ 2004-08-25 5:49 UTC (permalink / raw)
To: Jeff Garzik; +Cc: bjorn.helgaas, Andrew Morton, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ioc3-eth.c: add missing pci_enable_device()
2004-08-25 0:40 ` [PATCH] ioc3-eth.c: add missing pci_enable_device() Jeff Garzik
2004-08-25 5:49 ` Ralf Baechle
@ 2004-08-25 15:03 ` Bjorn Helgaas
2004-08-25 15:06 ` Christoph Hellwig
1 sibling, 1 reply; 4+ messages in thread
From: Bjorn Helgaas @ 2004-08-25 15:03 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Ralf Baechle, Andrew Morton, Linux Kernel Mailing List
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[] = {
^ permalink raw reply [flat|nested] 4+ messages in thread