public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Jeff Garzik <jeff@garzik.org>
To: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Loic Prylli <loic@myri.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Arjan van de Ven <arjan@infradead.org>,
	linux-kernel@vger.kernel.org, gregkh@suse.de,
	linux-pci@atrey.karlin.mff.cuni.cz,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>
Subject: Re: [patch] Make MMCONFIG space (extended PCI config space) a	driver opt-in issue
Date: Mon, 24 Dec 2007 02:22:10 -0500	[thread overview]
Message-ID: <476F5E22.3060007@garzik.org> (raw)
In-Reply-To: <20071223110337.GA25441@jurassic.park.msu.ru>

Ivan Kokshaysky wrote:
> On Sun, Dec 23, 2007 at 12:44:30AM -0500, Jeff Garzik wrote:
>> Failures are more predictable and more consistent with an all-or-none 
>> scenario.  The all-or-none solutions are the least complex on the software 
>> side, and far more widely tested than any mixed config access scheme.
> 
> Nope. The vast majority of mmconfig problems that happen at boot time
> actually have nothing to do with "broken" or "working" mmconfig.
> Typically these are
> - mmconf area overlapped during BAR sizing;
> - BIOS (or kernel) placed some MMIO in the middle of mmconfig area,
>   which looks like some random device "doesn't like mmconfig".
> 
> This is a result of all-or-none approach, as mmconfig is fundamentally
> unsafe until after PCI init is done.

The phrase "all or none" specifically describes the current practice in 
arch/x86/pci/mmconfig_{32,64}.c whereby a PCI bus always has one, and 
only one, access method.

So the problems you describe are unrelated to "all or none" as I was 
describing it.


> The mixed access that Loic proposes allows to avoid these boot problems
> just for free.

False.  If you have overlapping areas, then clearly it is 
board-dependent (undefined) what happens -- you might trigger an 
mmconfig access by writel() to your PCI device's MMIO BAR space.

Consider, too, what the current code in arch/x86/pci/mmconfig_{32,64}.c 
does:  it uses the range BIOS told to use for mmconfig, no more no less.

Clearly many early mmconfig BIOSes have buggy resource allocation... 
Thus if mmconfig is not known working on a system, turn it off 100% for 
all buses.  End of story.


> Systems that have both non-mmconf PCI(X) and mmconf PCI-E
> domains could be handled almost for free as well.

The existing code in arch/x86/pci/mmconfig_{32,64}.c today handles 
mixing of PCI-E and PCI-X just fine.  As noted above, its done on a 
per-bus and per-segment basis today.


> And probably most important thing is that the x86 pci_conf implementation
> would be so much simpler and cleaner that I honestly don't understand
> why are we still discussing it ;-)

The proposed API adds code, so I don't see how it simplifies things.

The current approach is quite clean anyway:

1) "raw_pci_ops" points to a single set of PCI config accessors.
2) For mmconfig, if the BIOS did not tell us to use mmconfig with a 
given PCI bus, fall back to type1 accesses.


> At the same time, making drivers to request extended config space
> still makes sense. I think of pci_request_ext_conf(dev, drv_name) which
> doesn't set any per-device flag, but
> - returns success or failure depending on mmconf availability;
> - can be logged by kernel to make it easier to debug if something
>   goes wrong.

I agree with the function as noted in response to Linus's "Sound ok with 
this plan?" email.  But note -- users and developers also need to access 
extended config space, even if driver did not request it.  Even if there 
is no driver at all.

Otherwise the value of "lspci -vvvxxx" debugging reports from users is 
diminished.

	Jeff



  parent reply	other threads:[~2007-12-24  7:23 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-12-22 12:31 [patch] Make MMCONFIG space (extended PCI config space) a driver opt-in issue Arjan van de Ven
2007-12-22 12:35 ` [patch] opt the sky2 driver into using extended config space Arjan van de Ven
2007-12-22 20:28   ` Stephen Hemminger
2007-12-22 14:20 ` [patch] Make MMCONFIG space (extended PCI config space) a driver opt-in issue Jeff Garzik
2007-12-22 14:47   ` Arjan van de Ven
2007-12-22 15:54     ` Matthew Wilcox
2007-12-22 16:02       ` Arjan van de Ven
2007-12-22 21:10         ` Matthew Wilcox
2007-12-23 23:33           ` Benjamin Herrenschmidt
2007-12-22 18:06     ` Linus Torvalds
2007-12-22 19:30       ` Martin Mares
2007-12-22 20:15         ` Arjan van de Ven
2007-12-22 20:36           ` Martin Mares
2007-12-23  1:29           ` Jeff Garzik
2007-12-23  1:26       ` Jeff Garzik
2007-12-23  3:46         ` Linus Torvalds
2007-12-23  4:11           ` Jeff Garzik
2007-12-23  4:35             ` Linus Torvalds
2007-12-23  4:52               ` Jeff Garzik
2007-12-23  5:00                 ` Linus Torvalds
2007-12-23  5:09                   ` Jeff Garzik
2007-12-23 21:09                   ` Benjamin Herrenschmidt
2007-12-23 21:15                     ` Martin Mares
2007-12-23 22:32                       ` Ivan Kokshaysky
2007-12-23 23:06                       ` Benjamin Herrenschmidt
2007-12-23 23:19                         ` Benjamin Herrenschmidt
2007-12-23  5:27                 ` Loic Prylli
2007-12-23  5:44                   ` Jeff Garzik
2007-12-23  8:33                     ` Loic Prylli
2007-12-23 11:03                     ` Ivan Kokshaysky
2007-12-23 17:32                       ` Linus Torvalds
2007-12-24  7:31                         ` Jeff Garzik
2007-12-24 18:51                           ` Linus Torvalds
2007-12-24 21:22                             ` Matthew Wilcox
2007-12-27 11:46                             ` Jeff Garzik
2007-12-27 14:09                               ` Arjan van de Ven
2007-12-24  7:22                       ` Jeff Garzik [this message]
2007-12-24 15:47                         ` Ivan Kokshaysky
2007-12-23 21:13                     ` Benjamin Herrenschmidt
2007-12-23 10:33                 ` Arjan van de Ven
2007-12-24  7:04                   ` Jeff Garzik
2007-12-24 11:49                     ` Arjan van de Ven
2007-12-24 11:54                       ` Jeff Garzik
2007-12-24 12:00                         ` Arjan van de Ven
2007-12-25  9:22                           ` Martin Mares
2007-12-25  9:40                             ` Martin Mares
2007-12-23  4:13           ` Jeff Garzik
2007-12-23  4:18             ` Jeff Garzik
2007-12-23  4:44               ` Linus Torvalds
2007-12-23  5:04                 ` Jeff Garzik
2007-12-23  5:18                   ` Linus Torvalds
2007-12-23  5:22                     ` Jeff Garzik
2007-12-23  4:40             ` Linus Torvalds
2007-12-23 10:18               ` Martin Mares
2007-12-23  5:09             ` Loic Prylli
2007-12-23  5:57             ` H. Peter Anvin
2007-12-23  1:34     ` Jeff Garzik
2007-12-22 20:43 ` Benjamin Herrenschmidt
     [not found] <fa.pFsY/52FEkQYqrDPnPMxmcUOsRY@ifi.uio.no>
2007-12-22 16:22 ` Robert Hancock
2007-12-22 19:25   ` Greg KH
2007-12-23  0:56     ` Jeff Garzik

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=476F5E22.3060007@garzik.org \
    --to=jeff@garzik.org \
    --cc=arjan@infradead.org \
    --cc=benh@kernel.crashing.org \
    --cc=gregkh@suse.de \
    --cc=ink@jurassic.park.msu.ru \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@atrey.karlin.mff.cuni.cz \
    --cc=loic@myri.com \
    --cc=torvalds@linux-foundation.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