PCI Cleanup
PATCH 1/1 PCI Cleanup
Linus, Alan, et al.
Here's a patch I'd like to see included in the 2.5 mainline. Some of the code
in arch/i386/pci/direct.c is a bit ugly and definitely redundant. This patch cleans that up. I've tested in on PCI configuration 1 systems, but I haven't had a chance to test this on any systems that use PCI configuration 2. If anyone can do that, that would be awesome.
[mcd@arrakis src]$ diffstat pci_fix-2531.patch
direct.c | 158
+++++++++++++++++++-------------------------------------------- 1 files changed, 48 insertions(+), 110 deletions(-)
The gist of the patch is that it move the indirection one layer down and saves
about 60+ lines of code. Rather than having pci_conf{1|2}_{read|write}_config_{byte|word|dword}, this patch removes the 1/2 distinction by pushing that down a layer, and calling a generic pointer instead. That pointer is set at init time by the PCI init code. This pulls out 6 functions (pci_conf2_*) that were exact duplicates of the others (pci_conf1_*), but differed by 1 character each (s/conf1/conf2/).
Linus, please apply...
Cheers!
-Matt
arch/i386/pci/direct.c +48 -110
@@ -69,76 +69,8 @@return 0 ; } -#undef PCI_CONF1_ADDRESS -static int pci_conf1_read_config_byte (struct pci_dev *dev,int where,u8 *value) -{ -int result; -u32 data; - -if (!value) -return -EINVAL ; - - result =pci_conf1_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,1 , &data); - - *value = (u8 )data; - -return result; -} - -static int pci_conf1_read_config_word (struct pci_dev *dev,int where,u16 *value) -{ -int result; -u32 data; - -if (!value) -return -EINVAL ; - - result =pci_conf1_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,2 , &data); - - *value = (u16 )data; - -return result; -} - -static int pci_conf1_read_config_dword (struct pci_dev *dev,int where,u32 *value) -{ -if (!value) -return -EINVAL ; - -return pci_conf1_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,4 , value); -} - -static int pci_conf1_write_config_byte (struct pci_dev *dev,int where,u8 value) -{ -return pci_conf1_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,1 , value); -} - -static int pci_conf1_write_config_word (struct pci_dev *dev,int where,u16 value) -{ -return pci_conf1_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,2 , value); -} - -static int pci_conf1_write_config_dword (struct pci_dev *dev,int where,u32 value) -{ -return pci_conf1_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,4 , value); -} - -static struct pci_ops pci_direct_conf1 = { - pci_conf1_read_config_byte, - pci_conf1_read_config_word, - pci_conf1_read_config_dword, - pci_conf1_write_config_byte, - pci_conf1_write_config_word, - pci_conf1_write_config_dword -};/*
@@ -217,57 +149,70 @@#undef PCI_CONF2_ADDRESS -static int pci_conf2_read_config_byte (struct pci_dev *dev,int where,u8 *value) + + +static int pci_config_read_byte (struct pci_dev *dev,int where,u8 *value) {int result;u32 data; - result =pci_conf2_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), + +if (!value) +return -EINVAL ; + + result =pci_config_read (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,1 , &data); *value = (u8 )data;return result; } -static int pci_conf2_read_config_word (struct pci_dev *dev,int where,u16 *value) +static int pci_config_read_word (struct pci_dev *dev,int where,u16 *value) {int result;u32 data; - result =pci_conf2_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), + +if (!value) +return -EINVAL ; + + result =pci_config_read (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,2 , &data); *value = (u16 )data;return result; } -static int pci_conf2_read_config_dword (struct pci_dev *dev,int where,u32 *value) +static int pci_config_read_dword (struct pci_dev *dev,int where,u32 *value) { -return pci_conf2_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), +if (!value) +return -EINVAL ; + +return pci_config_read (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,4 , value); } -static int pci_conf2_write_config_byte (struct pci_dev *dev,int where,u8 value) +static int pci_config_write_byte (struct pci_dev *dev,int where,u8 value) { -return pci_conf2_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), +return pci_config_write (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,1 , value); } -static int pci_conf2_write_config_word (struct pci_dev *dev,int where,u16 value) +static int pci_config_write_word (struct pci_dev *dev,int where,u16 value) { -return pci_conf2_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), +return pci_config_write (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,2 , value); } -static int pci_conf2_write_config_dword (struct pci_dev *dev,int where,u32 value) +static int pci_config_write_dword (struct pci_dev *dev,int where,u32 value) { -return pci_conf2_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), +return pci_config_write (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,4 , value); } -static struct pci_ops pci_direct_conf2 = { - pci_conf2_read_config_byte, - pci_conf2_read_config_word, - pci_conf2_read_config_dword, - pci_conf2_write_config_byte, - pci_conf2_write_config_word, - pci_conf2_write_config_dword +static struct pci_ops pci_direct = { + pci_config_read_byte, + pci_config_read_word, + pci_config_read_dword, + pci_config_write_byte, + pci_config_write_word, + pci_config_write_dword };
@@ -301,7 +246,7 @@return 0 ; } -static struct pci_ops *__devinit pci_check_direct (void ) +static int __init pci_direct_init (void ) {unsigned int tmp;unsigned long flags;
@@ -312,17 +257,21 @@* Check if configuration type 1 works. */ if (pci_probe &PCI_PROBE_CONF1 ) { + pci_config_read = pci_conf1_read; + pci_config_write = pci_conf1_write;outb (0x01 ,0xCFB ); tmp =inl (0xCF8 );outl (0x80000000 ,0xCF8 );if (inl (0xCF8 ) ==0x80000000 && -pci_sanity_check (&pci_direct_conf1)) { +pci_sanity_check (&pci_direct)) {outl (tmp,0xCF8 );local_irq_restore (flags);printk (KERN_INFO "PCI: Using configuration type 1\n" );if (!request_region (0xCF8 ,8 ,"PCI conf1" )) -return NULL ; -return &pci_direct_conf1; + pci_root_ops =NULL ; +else + pci_root_ops = &pci_direct; +return 0 ; }outl (tmp,0xCF8 ); }
@@ -331,36 +280,25 @@* Check if configuration type 2 works. */ if (pci_probe &PCI_PROBE_CONF2 ) { + pci_config_read = pci_conf2_read; + pci_config_write = pci_conf2_write;outb (0x00 ,0xCFB );outb (0x00 ,0xCF8 );outb (0x00 ,0xCFA );if (inb (0xCF8 ) ==0x00 &&inb (0xCFA ) ==0x00 && -pci_sanity_check (&pci_direct_conf2)) { +pci_sanity_check (&pci_direct)) {local_irq_restore (flags);printk (KERN_INFO "PCI: Using configuration type 2\n" );if (!request_region (0xCF8 ,4 ,"PCI conf2" )) -return NULL ; -return &pci_direct_conf2; + pci_root_ops =NULL ; +else + pci_root_ops = &pci_direct; +return 0 ; } }local_irq_restore (flags); -return NULL ; -} - -static int __init pci_direct_init (void ) -{ -if ((pci_probe & (PCI_PROBE_CONF1 |PCI_PROBE_CONF2 )) - && (pci_root_ops =pci_check_direct ())) { -if (pci_root_ops == &pci_direct_conf1) { - pci_config_read = pci_conf1_read; - pci_config_write = pci_conf1_write; - } -else { - pci_config_read = pci_conf2_read; - pci_config_write = pci_conf2_write; - } - } + pci_root_ops =NULL ;return 0 ; }
arch/i386/pci/direct.c line 101
Quoted 6 lines · Matthew Dobson
arch/i386/pci/direct.c +0
-6-if (!value) -return -EINVAL ; - - result =pci_conf1_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,2 , &data); -
This stil has the same problems as it did last time you posted it. The pointless NULL check and the increased complexity over duplicating about 60 lines of code and having pci_conf1 ops cleanly as we do in 2.4.
The !value check is extremely bad because it turns a critical debuggable software error into a silent unnoticed mistake.
Fixing the code instead of just resending it might improve the changes of it being merged no end.
Alan
Quoted 13 lines · Matthew Dobson
arch/i386/pci/direct.c line 101
Quoted 7 lines
arch/i386/pci/direct.c +0
-6-if (!value) -return -EINVAL ; - - result =pci_conf1_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,2 , &data); -This stil has the same problems as it did last time you posted it. The pointless NULL check and the increased complexity over duplicating about 60 lines of code and having
pci_conf1ops cleanly as we do in 2.4.
Quoted 5 lines
The !value check is extremely bad because it turns a critical debuggable software error into a silent unnoticed mistake.
Fixing the code instead of just resending it might improve the changes of it being merged no end.
Alan, please *look* at the patch. The NULL check is already there, he's REMOVING about 60 lines of duplicated code, reducing the complexity, and shifting the indirection up one level to get rid of redundancy.
If you want to delete the NULL check as well, that's fine, but totally a side issue. Ironically, the very snippet of code you quoted is all prefaced with "-", no?
M.
Quoted 8 lines · Martin J. Bligh
Alan, please *look* at the patch. The NULL check is already there, he's REMOVING about 60 lines of duplicated code, reducing the complexity, and shifting the indirection up one level to get rid of redundancy.
If you want to delete the NULL check as well, that's fine, but totally a side issue. Ironically, the very snippet of code you quoted is all prefaced with "-", no?
I pointed out before the null check was flawed. And all I see is the same identical patch churned out again. Regardless of whether that paticular stupid error was in the old code, not fixing it in the new code when its pointed out is a bit of a mess.
I'm not sure its a simplification either. More function pointers don't always make for neater - but thats a side issue. If the NULL check goes I'm not too worried about the other stuff.
Quoted 8 lines
I pointed out before the null check was flawed. And all I see is the same identical patch churned out again. Regardless of whether that paticular stupid error was in the old code, not fixing it in the new code when its pointed out is a bit of a mess.
I'm not sure its a simplification either. More function pointers don't always make for neater - but thats a side issue. If the NULL check goes I'm not too worried about the other stuff.
OK, that's easily disposed of, and it'll remove even more lines of redundant code.
Thanks,
M.
Quoted 4 lines · Alan Cox
I pointed out before the null check was flawed. And all I see is the same identical patch churned out again. Regardless of whether that paticular stupid error was in the old code, not fixing it in the new code when its pointed out is a bit of a mess.
The NULL check has been removed. The current version is attatched. It now pulls out 75 lines of code. I must have missed your complaints about the null check in previous emails... The check was done in the old pci_conf1 funcs, and
not in the pci_conf2 ones.. I thought this was strange as well, but I just
left them in...
Quoted 3 lines
I'm not sure its a simplification either. More function pointers don't always make for neater - but thats a side issue. If the NULL check goes I'm not too worried about the other stuff.
I think that it is definitely a simplification, although I am a bit biased ;) It makes it easier for other configuration types to hook into the system as well (I'm partial to NUMA-Q as well ;). All they have to do is hijack the pci_config_(read|write) function pointers.
Also, has anyone had a chance to look at it with pci_conf2? I have no particular reason to believe it doesn't work, as the code paths are almost identical, I'd just be nice to have a confirmation.
Cheers!
-Matt
arch/i386/pci/direct.c +36 -111
@@ -69,76 +69,7 @@return 0 ; } -#undef PCI_CONF1_ADDRESS - -static int pci_conf1_read_config_byte (struct pci_dev *dev,int where,u8 *value) -{ -int result; -u32 data; - -if (!value) -return -EINVAL ; - - result =pci_conf1_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,1 , &data); - - *value = (u8 )data; - -return result; -} - -static int pci_conf1_read_config_word (struct pci_dev *dev,int where,u16 *value) -{ -int result; -u32 data; - -if (!value) -return -EINVAL ; - - result =pci_conf1_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,2 , &data); - - *value = (u16 )data; - -return result; -} - -static int pci_conf1_read_config_dword (struct pci_dev *dev,int where,u32 *value) -{ -if (!value) -return -EINVAL ; - -return pci_conf1_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,4 , value); -} - -static int pci_conf1_write_config_byte (struct pci_dev *dev,int where,u8 value) -{ -return pci_conf1_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,1 , value); -} - -static int pci_conf1_write_config_word (struct pci_dev *dev,int where,u16 value) -{ -return pci_conf1_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,2 , value); -} - -static int pci_conf1_write_config_dword (struct pci_dev *dev,int where,u32 value) -{ -return pci_conf1_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,4 , value); -} - -static struct pci_ops pci_direct_conf1 = { - pci_conf1_read_config_byte, - pci_conf1_read_config_word, - pci_conf1_read_config_dword, - pci_conf1_write_config_byte, - pci_conf1_write_config_word, - pci_conf1_write_config_dword -};/*
@@ -217,57 +149,58 @@#undef PCI_CONF2_ADDRESS -static int pci_conf2_read_config_byte (struct pci_dev *dev,int where,u8 *value) + +static int pci_config_read_byte (struct pci_dev *dev,int where,u8 *value) {int result;u32 data; - result =pci_conf2_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), + result =pci_config_read (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,1 , &data); *value = (u8 )data;return result; } -static int pci_conf2_read_config_word (struct pci_dev *dev,int where,u16 *value) +static int pci_config_read_word (struct pci_dev *dev,int where,u16 *value) {int result;u32 data; - result =pci_conf2_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), + result =pci_config_read (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,2 , &data); *value = (u16 )data;return result; } -static int pci_conf2_read_config_dword (struct pci_dev *dev,int where,u32 *value) +static int pci_config_read_dword (struct pci_dev *dev,int where,u32 *value) { -return pci_conf2_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), +return pci_config_read (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,4 , value); } -static int pci_conf2_write_config_byte (struct pci_dev *dev,int where,u8 value) +static int pci_config_write_byte (struct pci_dev *dev,int where,u8 value) { -return pci_conf2_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), +return pci_config_write (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,1 , value); } -static int pci_conf2_write_config_word (struct pci_dev *dev,int where,u16 value) +static int pci_config_write_word (struct pci_dev *dev,int where,u16 value) { -return pci_conf2_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), +return pci_config_write (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,2 , value); } -static int pci_conf2_write_config_dword (struct pci_dev *dev,int where,u32 value) +static int pci_config_write_dword (struct pci_dev *dev,int where,u32 value) { -return pci_conf2_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), +return pci_config_write (0 , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn), where,4 , value); } -static struct pci_ops pci_direct_conf2 = { - pci_conf2_read_config_byte, - pci_conf2_read_config_word, - pci_conf2_read_config_dword, - pci_conf2_write_config_byte, - pci_conf2_write_config_word, - pci_conf2_write_config_dword +static struct pci_ops pci_direct = { + pci_config_read_byte, + pci_config_read_word, + pci_config_read_dword, + pci_config_write_byte, + pci_config_write_word, + pci_config_write_dword };
@@ -301,7 +235,7 @@return 0 ; } -static struct pci_ops *__devinit pci_check_direct (void ) +static int __init pci_direct_init (void ) {unsigned int tmp;unsigned long flags;
@@ -312,17 +246,21 @@* Check if configuration type 1 works. */ if (pci_probe &PCI_PROBE_CONF1 ) { + pci_config_read = pci_conf1_read; + pci_config_write = pci_conf1_write;outb (0x01 ,0xCFB ); tmp =inl (0xCF8 );outl (0x80000000 ,0xCF8 );if (inl (0xCF8 ) ==0x80000000 && -pci_sanity_check (&pci_direct_conf1)) { +pci_sanity_check (&pci_direct)) {outl (tmp,0xCF8 );local_irq_restore (flags);printk (KERN_INFO "PCI: Using configuration type 1\n" );if (!request_region (0xCF8 ,8 ,"PCI conf1" )) -return NULL ; -return &pci_direct_conf1; + pci_root_ops =NULL ; +else + pci_root_ops = &pci_direct; +return 0 ; }outl (tmp,0xCF8 ); }
@@ -331,36 +269,25 @@* Check if configuration type 2 works. */ if (pci_probe &PCI_PROBE_CONF2 ) { + pci_config_read = pci_conf2_read; + pci_config_write = pci_conf2_write;outb (0x00 ,0xCFB );outb (0x00 ,0xCF8 );outb (0x00 ,0xCFA );if (inb (0xCF8 ) ==0x00 &&inb (0xCFA ) ==0x00 && -pci_sanity_check (&pci_direct_conf2)) { +pci_sanity_check (&pci_direct)) {local_irq_restore (flags);printk (KERN_INFO "PCI: Using configuration type 2\n" );if (!request_region (0xCF8 ,4 ,"PCI conf2" )) -return NULL ; -return &pci_direct_conf2; + pci_root_ops =NULL ; +else + pci_root_ops = &pci_direct; +return 0 ; } }local_irq_restore (flags); -return NULL ; -} - -static int __init pci_direct_init (void ) -{ -if ((pci_probe & (PCI_PROBE_CONF1 |PCI_PROBE_CONF2 )) - && (pci_root_ops =pci_check_direct ())) { -if (pci_root_ops == &pci_direct_conf1) { - pci_config_read = pci_conf1_read; - pci_config_write = pci_conf1_write; - } -else { - pci_config_read = pci_conf2_read; - pci_config_write = pci_conf2_write; - } - } + pci_root_ops =NULL ;return 0 ; }
Quoted 5 lines · Matthew Dobson
I think that it is definitely a simplification, although I am a bit biased ;) It makes it easier for other configuration types to hook into the system as well (I'm partial to NUMA-Q as well ;). All they have to do is hijack the
pci_config_(read|write)function pointers.
Hmm..
Quite frankly, to me it looks like the real issue is that you don't want to have the byte/word/dword stuff replicated three times.
And your cleanup avoids the replication, but I think it has other badness in it: in particular it depends on those two global pointers. Which makes it really hard to have (for example) per-segment functions, or hard for drivers to hook into it (there's one IDE driver in particular that wants to do conf2 accesses, and nothing else. So it duplicates its own conf2 functions right now, because it has no way to hook into the generic ones).
If that is the case, wouldn't the _real_ cleanup be to just change the format of pci_config_read() entirely, and just make it get the size.
Another way of saying this:
Right now the config interface is all based around having unique functions for all sizes. So callers do "pci_read_config_word()" and that name-based size calling convention is propagated all the way down.
Your patch multiplexes the sizes at the lowest level.
And I'd suggest multiplexing them at a higher level. Instead of six different pcibios_read_config_byte etc functions, move the multiplexing up, make make them just two functions at _that_ level (and make siz #defines to make it compatible with existing users).
Ehh?
Linus
Quoted 2 lines
Quite frankly, to me it looks like the real issue is that you don't want to have the byte/word/dword stuff replicated three times.
Exactly - we can do that, but it just seems messy.
Quoted 6 lines
And your cleanup avoids the replication, but I think it has other badness in it: in particular it depends on those two global pointers. Which makes it really hard to have (for example) per-segment functions, or hard for drivers to hook into it (there's one IDE driver in particular that wants to do conf2 accesses, and nothing else. So it duplicates its own conf2 functions right now, because it has no way to hook into the generic ones).
OK, that IDE thing smacks of unmitigated evil to me, but if things are relying on it, we shouldn't change it.
Quoted 4 lines
And I'd suggest multiplexing them at a higher level. Instead of six different
pcibios_read_config_byteetc functions, move the multiplexing up, make make them just two functions at _that_ level (and make siz #defines to make it compatible with existing users).
Yup, that sounds like ultimately the correct thing to do. Will try to get that way done instead. Should clean things up nicely ....
M.
Quoted 5 lines · Martin J. Bligh
Quoted 2 lines
to do conf2 accesses, and nothing else. So it duplicates its own conf2 functions right now, because it has no way to hook into the generic ones).
OK, that IDE thing smacks of unmitigated evil to me, but if things are relying on it, we shouldn't change it.
It wants to force its own conf1/conf2 over the BIOS even if BIOS is preferred because some BIOSes dont honour the size requested and the hardware has bugs.
That to me says there may well be cleaner approaches.
Alan
Quoted 9 lines · Alan Cox
Quoted 3 lines
OK, that IDE thing smacks of unmitigated evil to me, but if things are relying on it, we shouldn't change it.
It wants to force its own conf1/conf2 over the BIOS even if BIOS is preferred because some BIOSes dont honour the size requested and the hardware has bugs.
That to me says there may well be cleaner approaches.
The thing I liked about the separate structures for function pointers for conf1/conf2 is that I could at least _see_ that the IDE driver might some day be changed to just do
.. conf2_struct->pci_config_read_byte(..) ..
even if (judging by past performance) this would never happen ;)
This is why I'd like to continue with the notion of having a well-defined structure that contains all the pointers (and one default case). Now, shrinking those structures down to 2 entries instead of 6 sounds like a fine idea to me, but short-circuiting them internally sounds bad because it loses the ability to use the pci config space functions independently of each other.
Linus
Linus,
I think I'm getting what you're aiming for here. See if this patch comes close
to what you're looking for.
The basic idea is we get rid of the byte/word/dword functions and just put a
size arg in the generic calls. I put back the different struct pci_ops for conf1 and conf2, so that drivers and other code could use either config option.
I also fixed the macros in access.h so that it correctly calls the new functions.
Please *don't* apply this as it will break a bunch of stuff (just about all PCI code ;)
Cheers!
-Matt
Quoted 17 lines · Linus Torvalds
Quoted 7 lines · Alan Cox
Quoted 1 line
OK, that IDE thing smacks of unmitigated evil to me, but if things are relying on it, we shouldn't change it.
It wants to force its own conf1/conf2 over the BIOS even if BIOS is preferred because some BIOSes dont honour the size requested and the hardware has bugs.
That to me says there may well be cleaner approaches.
The thing I liked about the separate structures for function pointers for conf1/conf2 is that I could at least _see_ that the IDE driver might some day be changed to just do
.. conf2_struct->pci_config_read_byte(..) ..
Quoted 7 lines
even if (judging by past performance) this would never happen ;)
This is why I'd like to continue with the notion of having a well-defined structure that contains all the pointers (and one default case). Now, shrinking those structures down to 2 entries instead of 6 sounds like a fine idea to me, but short-circuiting them internally sounds bad because it loses the ability to use the pci config space functions independently of each other.
Linus
arch/i386/pci/direct.c +28 -123
@@ -69,75 +69,23 @@return 0 ; } -#undef PCI_CONF1_ADDRESS -static int pci_conf1_read_config_byte (struct pci_dev *dev,int where,u8 *value) +static int pci_conf1_read_size_indep (struct pci_dev *dev,int where,int size,u32 *value) { -int result; -u32 data; - -if (!value) -return -EINVAL ; - - result =pci_conf1_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,1 , &data); - - *value = (u8 )data; - -return result; -} - -static int pci_conf1_read_config_word (struct pci_dev *dev,int where,u16 *value) -{ -int result; -u32 data; - -if (!value) -return -EINVAL ; - - result =pci_conf1_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,2 , &data); - - *value = (u16 )data; - -return result; -} - -static int pci_conf1_read_config_dword (struct pci_dev *dev,int where,u32 *value) -{ -if (!value) -return -EINVAL ; -return pci_conf1_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,4 , value); +PCI_FUNC (dev->devfn), where, size, value); } -static int pci_conf1_write_config_byte (struct pci_dev *dev,int where,u8 value) +static int pci_conf1_write_size_indep (struct pci_dev *dev,int where,int size,u32 value) {return pci_conf1_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,1 , value); -} - -static int pci_conf1_write_config_word (struct pci_dev *dev,int where,u16 value) -{ -return pci_conf1_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,2 , value); -} - -static int pci_conf1_write_config_dword (struct pci_dev *dev,int where,u32 value) -{ -return pci_conf1_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,4 , value); +PCI_FUNC (dev->devfn), where, size, value); }static struct pci_ops pci_direct_conf1 = { - pci_conf1_read_config_byte, - pci_conf1_read_config_word, - pci_conf1_read_config_dword, - pci_conf1_write_config_byte, - pci_conf1_write_config_word, - pci_conf1_write_config_dword + pci_conf1_read_size_indep, + pci_conf1_write_size_indep };
@@ -217,57 +165,21 @@#undef PCI_CONF2_ADDRESS -static int pci_conf2_read_config_byte (struct pci_dev *dev,int where,u8 *value) -{ -int result; -u32 data; - result =pci_conf2_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,1 , &data); - *value = (u8 )data; -return result; -} - -static int pci_conf2_read_config_word (struct pci_dev *dev,int where,u16 *value) -{ -int result; -u32 data; - result =pci_conf2_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,2 , &data); - *value = (u16 )data; -return result; -} - -static int pci_conf2_read_config_dword (struct pci_dev *dev,int where,u32 *value) +static int pci_conf2_read_size_indep (struct pci_dev *dev,int where,int size,u32 *value) {return pci_conf2_read (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,4 , value); -} - -static int pci_conf2_write_config_byte (struct pci_dev *dev,int where,u8 value) -{ -return pci_conf2_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,1 , value); +PCI_FUNC (dev->devfn), where, size, value); } -static int pci_conf2_write_config_word (struct pci_dev *dev,int where,u16 value) +static int pci_conf2_write_size_indep (struct pci_dev *dev,int where,int size,u32 value) {return pci_conf2_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,2 , value); -} - -static int pci_conf2_write_config_dword (struct pci_dev *dev,int where,u32 value) -{ -return pci_conf2_write (0 , dev->bus->number,PCI_SLOT (dev->devfn), -PCI_FUNC (dev->devfn), where,4 , value); +PCI_FUNC (dev->devfn), where, size, value); }static struct pci_ops pci_direct_conf2 = { - pci_conf2_read_config_byte, - pci_conf2_read_config_word, - pci_conf2_read_config_dword, - pci_conf2_write_config_byte, - pci_conf2_write_config_word, - pci_conf2_write_config_dword + pci_conf2_read_size_indep, + pci_conf2_write_size_indep };
@@ -292,16 +204,16 @@ bus.number =0 ; dev.bus = &bus;for (dev.devfn=0 ; dev.devfn <0x100 ; dev.devfn++) -if ((!o->read_word (&dev,PCI_CLASS_DEVICE , &x) && +if ((!o->read (&dev,PCI_CLASS_DEVICE ,2 , &x) && (x ==PCI_CLASS_BRIDGE_HOST || x ==PCI_CLASS_DISPLAY_VGA )) || - (!o->read_word (&dev,PCI_VENDOR_ID , &x) && + (!o->read (&dev,PCI_VENDOR_ID ,2 , &x) && (x ==PCI_VENDOR_ID_INTEL || x ==PCI_VENDOR_ID_COMPAQ )))return 1 ;DBG ("PCI: Sanity check failed\n" );return 0 ; } -static struct pci_ops *__devinit pci_check_direct (void ) +static int __init pci_direct_init (void ) {unsigned int tmp;unsigned long flags;
@@ -312,6 +224,8 @@* Check if configuration type 1 works. */ if (pci_probe &PCI_PROBE_CONF1 ) { + pci_config_read = pci_conf1_read; + pci_config_write = pci_conf1_write;outb (0x01 ,0xCFB ); tmp =inl (0xCF8 );outl (0x80000000 ,0xCF8 );
@@ -321,8 +235,10 @@local_irq_restore (flags);printk (KERN_INFO "PCI: Using configuration type 1\n" );if (!request_region (0xCF8 ,8 ,"PCI conf1" )) -return NULL ; -return &pci_direct_conf1; + pci_root_ops =NULL ; +else + pci_root_ops = &pci_direct_conf1; +return 0 ; }outl (tmp,0xCF8 ); }
@@ -331,6 +247,8 @@* Check if configuration type 2 works. */ if (pci_probe &PCI_PROBE_CONF2 ) { + pci_config_read = pci_conf2_read; + pci_config_write = pci_conf2_write;outb (0x00 ,0xCFB );outb (0x00 ,0xCF8 );outb (0x00 ,0xCFA );
@@ -339,28 +257,15 @@local_irq_restore (flags);printk (KERN_INFO "PCI: Using configuration type 2\n" );if (!request_region (0xCF8 ,4 ,"PCI conf2" )) -return NULL ; -return &pci_direct_conf2; + pci_root_ops =NULL ; +else + pci_root_ops = &pci_direct_conf2; +return 0 ; } }local_irq_restore (flags); -return NULL ; -} - -static int __init pci_direct_init (void ) -{ -if ((pci_probe & (PCI_PROBE_CONF1 |PCI_PROBE_CONF2 )) - && (pci_root_ops =pci_check_direct ())) { -if (pci_root_ops == &pci_direct_conf1) { - pci_config_read = pci_conf1_read; - pci_config_write = pci_conf1_write; - } -else { - pci_config_read = pci_conf2_read; - pci_config_write = pci_conf2_write; - } - } + pci_root_ops =NULL ;return 0 ; }
drivers/pci/access.c +23 -9
@@ -19,24 +19,38 @@#define PCI_word_BAD (pos & 1)#define PCI_dword_BAD (pos & 3) -#define PCI_OP (rw,size,type) \ -int pci_##rw##_config_##size (struct pci_dev *dev, int pos, type value) \ +#define PCI_OP_READ (size,type,len) \ +int pci_read_config_##size (struct pci_dev *dev, int pos, type *value) \ { \ int res; \ unsigned long flags; \ + u32 data; \ if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \ spin_lock_irqsave(&pci_lock, flags); \ - res = dev->bus->ops->rw##_##size(dev, pos, value); \ + res = dev->bus->ops->read(dev, pos, len, &data); \ + *value = (type)data; \ spin_unlock_irqrestore(&pci_lock, flags); \ return res; \ } -PCI_OP(read, byte, u8 *) -PCI_OP(read, word, u16 *) -PCI_OP(read, dword, u32 *) -PCI_OP(write, byte, u8) -PCI_OP(write, word, u16) -PCI_OP(write, dword, u32) +#define PCI_OP_WRITE (size,type,len) \ +int pci_write_config_##size (struct pci_dev *dev, int pos, type value) \ +{ \ + int res; \ + unsigned long flags; \ + if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \ + spin_lock_irqsave(&pci_lock, flags); \ + res = dev->bus->ops->write(dev, pos, len, value); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return res; \ +} + +PCI_OP_READ(byte, u8, 1) +PCI_OP_READ(word, u16, 2) +PCI_OP_READ(dword, u32, 4) +PCI_OP_WRITE(byte, u8, 1) +PCI_OP_WRITE(word, u16, 2) +PCI_OP_WRITE(dword, u32, 4)EXPORT_SYMBOL (pci_read_config_byte);EXPORT_SYMBOL (pci_read_config_word);
include/linux/pci.h +2 -6
@@ -456,12 +456,8 @@/* Low-level architecture-dependent routines */ struct pci_ops { -int (*read_byte)(struct pci_dev *,int where,u8 *val); -int (*read_word)(struct pci_dev *,int where,u16 *val); -int (*read_dword)(struct pci_dev *,int where,u32 *val); -int (*write_byte)(struct pci_dev *,int where,u8 val); -int (*write_word)(struct pci_dev *,int where,u16 val); -int (*write_dword)(struct pci_dev *,int where,u32 val); +int (*read)(struct pci_dev *,int where,int size,u32 *val); +int (*write)(struct pci_dev *,int where,int size,u32 val); };struct pbus_set_ranges_data
Quoted 2 lines · Matthew Dobson
I think I'm getting what you're aiming for here. See if this patch comes close
to what you're looking for.
This seems to be more what I was thinking of, yes (although please drop the "size_independent" part of the names, that just looks too horrible ;)
Linus
Quoted 6 lines · Linus Torvalds
Quoted 2 lines · Matthew Dobson
I think I'm getting what you're aiming for here. See if this patch comes close
to what you're looking for.This seems to be more what I was thinking of, yes (although please drop the "
size_independent" part of the names, that just looks too horrible ;)
Fair enough.. Couldn't think of a fun creative name anyway... Just took the road more traveled and renamed the old pci_conf1_read __pci_conf1_read and the old pci_conf1_read_size_indep pci_conf1_read. Much prettier. And it even seems to work on i386. I definitely wouldn't try booting this on any other arch's though! ;)
-Matt
arch/i386/pci/direct.c +3 -9
@@ -13,7 +13,7 @@#define PCI_CONF1_ADDRESS (bus, dev, fn, reg) \ (0x80000000 | (bus << 16) | (dev << 11) | (fn << 8) | (reg & ~3)) -static int pci_conf1_read (int seg,int bus,int dev,int fn,int reg,int len,u32 *value) +static int __pci_conf1_read (int seg,int bus,int dev,int fn,int reg,int len,u32 *value) {unsigned long flags;
@@ -41,7 +41,7 @@return 0 ; } -static int pci_conf1_write (int seg,int bus,int dev,int fn,int reg,int len,u32 value) +static int __pci_conf1_write (int seg,int bus,int dev,int fn,int reg,int len,u32 value) {unsigned long flags;
@@ -69,75 +69,23 @@return 0 ; } -#undef PCI_CONF1_ADDRESS -static int pci_conf1_read_config_byte (struct pci_dev *dev,int where,u8 *value) +static int pci_conf1_read (struct pci_dev *dev,int where,int size,u32 *value) { -int result; -u32 data; - -if (!value) -
return -EINVAL; - - result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn), - PCI_FUNC(dev->devfn), where, 1, &data);
-
- *value = (u8)data;
-
- return result;
+ return __pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn), +
PCI_FUNC(dev->devfn), where, size, value);
}
-static int pci_conf1_read_config_word(struct pci_dev *dev, int where, u16 *value) +static int pci_conf1_write(struct pci_dev *dev, int where, int size, u32 value)
{
- int result;
- u32 data;
-
- if (!value)
-
return -EINVAL; - - result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn), - PCI_FUNC(dev->devfn), where, 2, &data);
-
- *value = (u16)data;
-
- return result;
-}
-
-static int pci_conf1_read_config_dword(struct pci_dev *dev, int where, u32 *value) -{
- if (!value)
-
return -EINVAL; - - return pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn), - PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_conf1_write_config_byte(struct pci_dev *dev, int where, u8 value) -{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn), -
PCI_FUNC(dev->devfn), where, 1, value);
-}
-
-static int pci_conf1_write_config_word(struct pci_dev *dev, int where, u16 value) -{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 2, value);
-}
-
-static int pci_conf1_write_config_dword(struct pci_dev *dev, int where, u32 value) -{
- return pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 4, value); + return __pci_conf1_write(0, dev->bus->number, PCI_SLOT(dev->devfn), + PCI_FUNC(dev->devfn), where, size, value);
}
static struct pci_ops pci_direct_conf1 = {
- pci_conf1_read_config_byte,
- pci_conf1_read_config_word,
- pci_conf1_read_config_dword,
- pci_conf1_write_config_byte,
- pci_conf1_write_config_word,
- pci_conf1_write_config_dword
+ read: pci_conf1_read,
+ write: pci_conf1_write
};
@@ -147,7 +95,7 @@
#define PCI_CONF2_ADDRESS(dev, reg) (u16)(0xC000 | (dev << 8) | reg)
-static int pci_conf2_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value) +static int __pci_conf2_read (int seg, int bus, int dev, int fn, int reg, int len, u32 *value)
{
unsigned long flags;
@@ -181,7 +129,7 @@
return 0;
}
-static int pci_conf2_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value) +static int __pci_conf2_write (int seg, int bus, int dev, int fn, int reg, int len, u32 value)
{
unsigned long flags;
@@ -217,57 +165,21 @@
#undef PCI_CONF2_ADDRESS
-static int pci_conf2_read_config_byte(struct pci_dev *dev, int where, u8 *value) -{
- int result;
- u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn), -
PCI_FUNC(dev->devfn), where, 1, &data);
- *value = (u8)data;
- return result;
-}
-
-static int pci_conf2_read_config_word(struct pci_dev *dev, int where, u16 *value) -{
- int result;
- u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn), -
PCI_FUNC(dev->devfn), where, 2, &data);
- *value = (u16)data;
- return result;
-}
-
-static int pci_conf2_read_config_dword(struct pci_dev *dev, int where, u32 *value) -{
- return pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 4, value);
-}
-
-static int pci_conf2_write_config_byte(struct pci_dev *dev, int where, u8 value) +static int pci_conf2_read(struct pci_dev *dev, int where, int size, u32 *value)
{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn), -
PCI_FUNC(dev->devfn), where, 1, value); + return __pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn), + PCI_FUNC(dev->devfn), where, size, value);
}
-static int pci_conf2_write_config_word(struct pci_dev *dev, int where, u16 value) +static int pci_conf2_write(struct pci_dev *dev, int where, int size, u32 value)
{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 2, value);
-}
-
-static int pci_conf2_write_config_dword(struct pci_dev *dev, int where, u32 value) -{
- return pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn),
-
PCI_FUNC(dev->devfn), where, 4, value); + return __pci_conf2_write(0, dev->bus->number, PCI_SLOT(dev->devfn), + PCI_FUNC(dev->devfn), where, size, value);
}
static struct pci_ops pci_direct_conf2 = {
- pci_conf2_read_config_byte,
- pci_conf2_read_config_word,
- pci_conf2_read_config_dword,
- pci_conf2_write_config_byte,
- pci_conf2_write_config_word,
- pci_conf2_write_config_dword
+ read: pci_conf2_read,
+ write: pci_conf2_write
};
@@ -292,16 +204,16 @@
bus.number = 0;
dev.bus = &bus;
for(dev.devfn=0; dev.devfn < 0x100; dev.devfn++)
-
if ((!o->read_word(&dev, PCI_CLASS_DEVICE, &x) && + if ((!o->read(&dev, PCI_CLASS_DEVICE, 2, &x) && (x == PCI_CLASS_BRIDGE_HOST || x == PCI_CLASS_DISPLAY_VGA)) || - (!o->read_word(&dev, PCI_VENDOR_ID, &x) && + (!o->read(&dev, PCI_VENDOR_ID, 2, &x) &&
(x == PCI_VENDOR_ID_INTEL || x == PCI_VENDOR_ID_COMPAQ)))
return 1;
DBG("PCI: Sanity check failed\n");
return 0;
}
-static struct pci_ops * __devinit pci_check_direct(void) +static int __init pci_direct_init(void)
{
unsigned int tmp;
unsigned long flags;
@@ -321,8 +233,10 @@
local_irq_restore(flags); printk(KERN_INFO "PCI: Using configuration type 1\n"); if (!request_region(0xCF8, 8, "PCI conf1")) - return NULL; - return &pci_direct_conf1; + pci_root_ops = NULL; + else + pci_root_ops = &pci_direct_conf1; + return 0;
}
outl (tmp, 0xCF8);
}
@@ -339,28 +253,15 @@
local_irq_restore(flags); printk(KERN_INFO "PCI: Using configuration type 2\n"); if (!request_region(0xCF8, 4, "PCI conf2")) - return NULL; - return &pci_direct_conf2; + pci_root_ops = NULL; + else + pci_root_ops = &pci_direct_conf2; + return 0;
}
}
local_irq_restore(flags);
- return NULL;
-}
-
-static int __init pci_direct_init(void)
-{
- if ((pci_probe & (PCI_PROBE_CONF1 | PCI_PROBE_CONF2)) -
&& (pci_root_ops = pci_check_direct())) {
-
if (pci_root_ops == &pci_direct_conf1) {
-
pci_config_read = pci_conf1_read;
-
pci_config_write = pci_conf1_write;
-
}
-
else {
-
pci_config_read = pci_conf2_read;
-
pci_config_write = pci_conf2_write;
-
}- }
+ pci_root_ops = NULL;
return 0;
}
drivers/pci/access.c +2 -2
@@ -19,24 +19,38 @@#define PCI_word_BAD (pos & 1)#define PCI_dword_BAD (pos & 3) -#define PCI_OP (rw,size,type) \ -int pci_##rw##_config_##size (struct pci_dev *dev, int pos, type value) \ +#define PCI_OP_READ (size,type,len) \ +int pci_read_config_##size (struct pci_dev *dev, int pos, type *value) \ {
\ int res; \ unsigned long flags; \ + u32 data; \
if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
spin_lock_irqsave(&pci_lock, flags); \
- res = dev->bus->ops->rw##_##size(dev, pos, value); \
+ res = dev->bus->ops->read(dev, pos, len, &data); \
+ *value = (type)data; \
spin_unlock_irqrestore(&pci_lock, flags); \
return res; \
}
-PCI_OP(read, byte, u8 *)
-PCI_OP(read, word, u16 *)
-PCI_OP(read, dword, u32 *)
-PCI_OP(write, byte, u8)
-PCI_OP(write, word, u16)
-PCI_OP(write, dword, u32)
+#define PCI_OP_WRITE(size,type,len) \
+int pci_write_config_##size (struct pci_dev *dev, int pos, type value) \
+{
\
+ int res;
\+ unsigned long flags; \
+ if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
+ spin_lock_irqsave(&pci_lock, flags); \
+ res = dev->bus->ops->write(dev, pos, len, value); \
+ spin_unlock_irqrestore(&pci_lock, flags); \
+ return res; \
+}
+
+PCI_OP_READ(byte, u8, 1)
+PCI_OP_READ(word, u16, 2)
+PCI_OP_READ(dword, u32, 4)
+PCI_OP_WRITE(byte, u8, 1)
+PCI_OP_WRITE(word, u16, 2)
+PCI_OP_WRITE(dword, u32, 4)
EXPORT_SYMBOL(pci_read_config_byte);
EXPORT_SYMBOL(pci_read_config_word);
include/linux/pci.h +2 -6
@@ -456,12 +456,8 @@/* Low-level architecture-dependent routines */ struct pci_ops { -int (*read_byte)(struct pci_dev *,int where,u8 *val); -int (*read_word)(struct pci_dev *,int where,u16 *val); -int (*read_dword)(struct pci_dev *,int where,u32 *val); -int (*write_byte)(struct pci_dev *,int where,u8 val); -int (*write_word)(struct pci_dev *,int where,u16 val); -int (*write_dword)(struct pci_dev *,int where,u32 val); +int (*read)(struct pci_dev *,int where,int size,u32 *val); +int (*write)(struct pci_dev *,int where,int size,u32 val); };struct pbus_set_ranges_data
OK... Here's the latest version. Sorry about that last posting... Stupid line wrapping broke the patch! :( This patch also removes the pci_config_(read|write) function pointers. People shouldn't be using these (I don't think) and should be using the pci_ops structure linked through the pci_dev structure. These end up calling the same functions that the pci_config_(read|write) pointers refer to anyway. The only places I can see that these are being used in the kernel are in drivers/acpi/osl.c... Anyone care to comment on the use there or if it can be changed? I've cc'd the authors of the file...
A full version that fixes all arch's should be coming soon.
-Matt
Quoted 11 lines · Linus Torvalds
Quoted 2 lines · Matthew Dobson
I think I'm getting what you're aiming for here. See if this patch comes close
to what you're looking for.This seems to be more what I was thinking of, yes (although please drop the "
size_independent" part of the names, that just looks too horrible ;)Linus
arch/i386/pci/common.c +0 -3
@@ -25,9 +25,6 @@struct pci_bus *pci_root_bus =NULL ;struct pci_ops *pci_root_ops =NULL ; -int (*pci_config_read)(int seg,int bus,int dev,int fn,int reg,int len,u32 *value) =NULL ; -int (*pci_config_write)(int seg,int bus,int dev,int fn,int reg,int len,u32 value) =NULL ; -/* * legacy, numa, and acpi all want to call pcibios_scan_root * from their initcalls. This flag prevents that.
arch/i386/pci/direct.c +33 -132
252 lines not on this page · raw message
arch/i386/pci/pcbios.c +10 -63
107 lines not on this page · raw message
drivers/pci/access.c +23 -9
@@ -19,24 +19,38 @@#define PCI_word_BAD (pos & 1)#define PCI_dword_BAD (pos & 3) -#define PCI_OP (rw,size,type) \ -int pci_##rw##_config_##size (struct pci_dev *dev, int pos, type value) \ +#define PCI_OP_READ (size,type,len) \ +int pci_read_config_##size (struct pci_dev *dev, int pos, type *value) \ { \ int res; \ unsigned long flags; \ + u32 data; \ if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \ spin_lock_irqsave(&pci_lock, flags); \ - res = dev->bus->ops->rw##_##size(dev, pos, value); \ + res = dev->bus->ops->read(dev, pos, len, &data); \ + *value = (type)data; \ spin_unlock_irqrestore(&pci_lock, flags); \ return res; \ } -PCI_OP(read, byte, u8 *) -PCI_OP(read, word, u16 *) -PCI_OP(read, dword, u32 *) -PCI_OP(write, byte, u8) -PCI_OP(write, word, u16) -PCI_OP(write, dword, u32) +#define PCI_OP_WRITE (size,type,len) \ +int pci_write_config_##size (struct pci_dev *dev, int pos, type value) \ +{ \ + int res; \ + unsigned long flags; \ + if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \ + spin_lock_irqsave(&pci_lock, flags); \ + res = dev->bus->ops->write(dev, pos, len, value); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return res; \ +} + +PCI_OP_READ(byte, u8, 1) +PCI_OP_READ(word, u16, 2) +PCI_OP_READ(dword, u32, 4) +PCI_OP_WRITE(byte, u8, 1) +PCI_OP_WRITE(word, u16, 2) +PCI_OP_WRITE(dword, u32, 4)EXPORT_SYMBOL (pci_read_config_byte);EXPORT_SYMBOL (pci_read_config_word);
include/asm-i386/pci.h +0 -2
@@ -22,8 +22,6 @@void pcibios_config_init (void );struct pci_bus *pcibios_scan_root (int bus); -extern int (*pci_config_read)(int seg,int bus,int dev,int fn,int reg,int len,u32 *value); -extern int (*pci_config_write)(int seg,int bus,int dev,int fn,int reg,int len,u32 value);void pcibios_set_master (struct pci_dev *dev);void pcibios_penalize_isa_irq (int irq);
include/linux/pci.h +2 -6
@@ -456,12 +456,8 @@/* Low-level architecture-dependent routines */ struct pci_ops { -int (*read_byte)(struct pci_dev *,int where,u8 *val); -int (*read_word)(struct pci_dev *,int where,u16 *val); -int (*read_dword)(struct pci_dev *,int where,u32 *val); -int (*write_byte)(struct pci_dev *,int where,u8 val); -int (*write_word)(struct pci_dev *,int where,u16 val); -int (*write_dword)(struct pci_dev *,int where,u32 val); +int (*read)(struct pci_dev *,int where,int size,u32 *val); +int (*write)(struct pci_dev *,int where,int size,u32 val); };struct pbus_set_ranges_data
Hi!
Quoted 5 lines
It wants to force its own conf1/conf2 over the BIOS even if BIOS is preferred because some BIOSes dont honour the size requested and the hardware has bugs.
That to me says there may well be cleaner approaches.
I haven't looked at the current source yet, but the PCI access code used to work this way: if both BIOS and direct access were enabled (which was the default) and both worked, BIOS was used only for getting information about interrupt routing and last bus number and direct access for everything else. Hence the problematic BIOS calls were used only if there were no other possibility or the user explicitly told us to.
Have a nice fortnight
Signature · 3 lines
Martin `MJ' Mares <[email protected]> http://atrey.karlin.mff.cuni.cz/~mj/ Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth No fortune, no luck, no sig!
Quoted 9 lines
This stil has the same problems as it did last time you posted it. The pointless NULL check and the increased complexity over duplicating about 60 lines of code and having
pci_conf1ops cleanly as we do in 2.4.The !value check is extremely bad because it turns a critical debuggable software error into a silent unnoticed mistake.
Fixing the code instead of just resending it might improve the changes of it being merged no end.
BTW, it wasn't just resent, it had all the NUMA-Q stuff stripped out of it, and was just the core code cleanup. He's replacing pci_conf1_read_config_byte and pci_conf2_read_config_byte with pci_conf_read_config_byte. The indirection level has now switched from pci_root_ops to pci_config_(read|write). These were set up before, but never seemed to be actually used.
I'm perfectly prepared to accept that the patch doesn't work, or is bad, but not for the reasons you gave. To make the thing easier to read, what he's effectively doing is this
-static int pci_conf1_read_config_byte(struct pci_dev *dev, int where, u8 *value) -{
- int result;
- u32 data;
-
- if (!value)
- return -EINVAL;
-
- result = pci_conf1_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, &data);
-
- *value = (u8)data;
-
- return result;
-}
-
-static int pci_conf2_read_config_byte(struct pci_dev *dev, int where, u8 *value)
- {
- int result;
- u32 data;
- result = pci_conf2_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
- PCI_FUNC(dev->devfn), where, 1, &data);
- *value = (u8)data;
- return result;
- }
-static int pci_conf2_read_config_byte(struct pci_dev *dev, int where, u8 *value) +
+static int pci_config_read_byte(struct pci_dev *dev, int where, u8 *value)
+ {
+ int result;
+ u32 data;
+
+ if (!value)
+ return -EINVAL;
+
+ result = pci_config_read(0, dev->bus->number, PCI_SLOT(dev->devfn),
+ PCI_FUNC(dev->devfn), where, 1, &data);
+ *value = (u8)data;
+ return result;
+}
Quoted 3 lines · Martin J. Bligh
BTW, it wasn't just resent, it had all the NUMA-Q stuff stripped out of it, and was just the core code cleanup. He's replacing
pci_conf1_read_config_byteandpci_conf2_read_config_byte
Apologies that part of the changing between the two diffs I didn't notice. I've just stripped the value check out of 2.4 except for the BIOS mode ones (they tend to produce -weird- tracebacks) where it BUG()'s on that.