Re: [Patch v2] Make PCI extended config space (MMCONFIG) a driver opt-in

Loic Prylli
2008-01-13T18:25:04Z

On 1/12/2008 12:45 PM, Arjan van de Ven wrote:
> On Sat, 12 Jan 2008 17:40:30 +0300
> Ivan Kokshaysky <ink@jurassic.park.msu.ru> wrote:
>
>>
>> + if (reg < 256)
>> + return pci_conf1_read(seg,bus,devfn,reg,len,value);
>> +
>>
>
>
> btw this is my main objection to your patch; it intertwines the conf1 and mmconfig code even more.
> When (and I'm saying "when" not "if") systems arrive that only have MMCONFIG for some of the devices,
> we'll have to detangle this again, and I'm really not looking forward to that.
>

conf1 has been a hardcoded dependencies of mmconfig for years. Ivan's
patch does not make it worse (in fact it considerably simplifies that
code, making it easier to untangle later).

IMHO, either your patch or Ivan's can be a good base, but:

1) For your remark above to be given any consideration, your patch
should be modified to remove the hardcoded conf1 from the *current*
mmconfig code, otherwise we end up with 3 set of ops (mmconfig + conf1+
a possible third set of operations) intertwined in a confusing manner.
And removing that dependency is not a straightforward operation unless
you also do 2):

2) the pci_enable_ext_config() function and dev->ext_cfg_space field,
sysfs interface should be removed from the patch. There has never been
a problem reporting crashes or any undefined behaviour while trying to
access ext-conf-space, all the problems where *using mmconfig to access
legacy-conf-space*. The "if (dev->cfg_space_ext > 0)" checks can instead
be replaced by "if (reg >= 256)".
Otherwise when using per-device explicit enabling, just *checking*
whether ext-conf-space is available by calling pci_enable_ext_config(),
will make some of the old problems of *loosing legacy conf-space* come
back: you would have introduced a new user-space and kernel API while
only solving half the problems, not a good deal.

if you do 1) and 2), then you really support the good properties you
claimed:
- You can use mmconfig for ext-space and something else for legacy-space.
- You can use mmconfig for everything (for instance if conf1 is not
implemented).

Of course it is as straightforward to modify Ivan's patch to also have
the same properties.

Loic

Loic