Date: Thu, 27 Dec 2007 06:09:13 -0800
From: Arjan van de Ven <arjan@infradead.org>
To: Jeff Garzik <jeff@garzik.org>
Cc: linux-kernel@vger.kernel.org,
       Linus Torvalds <torvalds@linux-foundation.org>, gregkh@suse.de,
       inux-pci@atrey.karlin.mff.cuni.cz,
       Benjamin Herrenschmidt <benh@kernel.crashing.org>,
       Martin Mares <mj@ucw.cz>, Matthew Wilcox <matthew@wil.cx>
Subject: Re: [Patch v2] Make PCI extended config space (MMCONFIG) a driver
 opt-in
Message-ID: <20071227060913.56173f74@laptopd505.fenrus.org>
In-Reply-To: <47739203.3060809@garzik.org>
References: <20071225032605.29147200@laptopd505.fenrus.org>
	<47739203.3060809@garzik.org>
Organization: Intel
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3094
Lines: 82

On Thu, 27 Dec 2007 06:52:35 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> Arjan van de Ven wrote:
> > This patch also adds a sysfs property for each device into which
> > root can write a '1' to enable extended configuration space. The
> > kernel will print a notice into dmesg when this happens (including
> > the name of the app) so that if the system crashes as a result of
> > this action, the user can know what action/tool caused it.
> 
> 
> Comments:
> 
> 1) [minor] With a bit in struct pci_dev, 

I have this

> there is no need for
> separate raw_pci_ops.  That will simplify your patch, with no
> functionality change.

but sadly your second statement is not correct. Part of the complication is that all PCI config ops
operate on busses not devices; at first I thought "just add a bit and be done with it", but sadly it's
not quite the case. Due to the per-bus nature of the ops, you end up having 2 type of bus operations, 
and that's just boilerplate (prototypes, exports and stuff) but it makes up most of the lines of the patch

In addition, a separate raw_pci_ops (for x86 only!) is needed anyway since it's quite likely that 
we'll have various options of each case (extended or not) and we want to pick the best one for each case,
at which point you really do need the 2 variables.

> 
> "golden" arches (no pun intended) may implement raw_pci_ops that 
> _always_ work with extended config space, and simply ignore that bit,
> if that is how their underlying non-mmconfig-nor-type1 hardware is
> implemented.

that is what I implemented already in the patch that you commented on ;-)

> 
> 
> 2) [non-minor] hmmmm.
> 
> 	[jgarzik@core ~]$ lspci -n | wc -l
> 	23
> 
> So I would have to perform 23 sysfs twiddles, before I could obtain a 
> full and unabridged 'lspci -vvvxxx'?

not you as human, but "lspci" ought to yes.

> 
> For the userspace interface, the most-often-used knob for diagnostic 
> purposes will be the easiest one.  And that's

the easiest one is an option to lspci. Nothing more nothing less.

Making a global knob in kernel space is a lot more tricky, and in addition
really there's enough cases where userspace wants the one device anyway
Doing the "for each device I'm about to dump" in lspci is pretty much as hard as doing
the global one (if not simpler)

> 
> 3) [minor] architectures must be able to override 
> pci_enable_ext_config().  see "golden arches".

see the patch. All pci_enable_ext_config() does is set a flag.
The architecture decides what to do with that flag. Golden
architectures can just totally ignore the flag and always expose
the full space. 
(In fact, the patch assumes all-but-x86 to be golden here; which is fair)


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/