public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Ralf Baechle <ralf@linux-mips.org>
To: Jeff Garzik <jgarzik@pobox.com>
Cc: bjorn.helgaas@hp.com, Andrew Morton <akpm@osdl.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ioc3-eth.c: add missing pci_enable_device()
Date: Wed, 25 Aug 2004 07:49:40 +0200	[thread overview]
Message-ID: <20040825054940.GA23827@linux-mips.org> (raw)
In-Reply-To: <412BE006.8040606@pobox.com>

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

  reply	other threads:[~2004-08-25  5:50 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <200408242225.i7OMPGLQ029847@hera.kernel.org>
2004-08-25  0:40 ` [PATCH] ioc3-eth.c: add missing pci_enable_device() Jeff Garzik
2004-08-25  5:49   ` Ralf Baechle [this message]
2004-08-25 15:03   ` Bjorn Helgaas
2004-08-25 15:06     ` Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20040825054940.GA23827@linux-mips.org \
    --to=ralf@linux-mips.org \
    --cc=akpm@osdl.org \
    --cc=bjorn.helgaas@hp.com \
    --cc=jgarzik@pobox.com \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox