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

[Loic Prylli <loic@myri.com>]

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