PCI Express Enhanced Config Patch - 2.6.0-test11
A message that is not in the archive (sent off-list or not yet received): [email protected]
Review
Quoted 11 lines · Sundarapandian
(quoted diff) +6
-0+u32 pcie_last_accessed_device; + +unsigned long pci_exp_set_dev_base (int bus, int dev, int fn) +{ + u32 dev_base = + mmcfg_base_address | (bus << 20) | ((PCI_DEVFN (dev,fn))<<12);
(quoted diff) +4
-0+ if (dev_base != pcie_last_accessed_device){ + pcie_last_accessed_device = dev_base; + set_fixmap (FIX_PCIE_MCFG, dev_base); + }
Can you please put the details on how the fixmap is managed into an asm-i386/* file. x86-64 shares the i386 PCI subsystem and I would like to use a different implement a different method there (just mapping everything statically)
Quoted 13 lines
(quoted diff) +3
-0+} + +static int pci_express_conf_read(int seg, int bus, int devfn, int reg,int len, u32 *value)
(quoted diff) +8
-0+{ + unsigned long flags; + unsigned long base_address; + char * virt_addr; + int dev = PCI_SLOT (devfn); + int fn = PCI_FUNC (devfn); + + if (!value || ((u32)bus > 255) || ((u32)dev > 31) || ((u32)fn > 7) ||((u32)reg > 4095))
Review
Quoted 5 lines
(quoted diff) +5
-0+ return -EINVAL; + + /* Shoot misalligned transaction now */ + if (reg & (len-1)) + return -EINVAL;
It would be better to printk here because nobody will check the return value of this function
Same for _write.
Review
Quoted 8 lines
(quoted diff) +8
-0+ writew(value,(unsigned long)virt_addr+reg); + break; + case 4: + writel(value,(unsigned long)virt_addr+reg); + break; + } + /* Dummy read to flush PCI write */ + readl (virt_addr);
I thought the consensus was that reading the same address is dangerous because some registers react when they are only read.
Also you cannot readl on a w or b registers, if you do that you would need to use the same size.
Also are you sure that the old port based config read/write functions actually flushed their writes? I don't think they did. It probably would be best to just drop that read.
Otherwise it should read some save register in the same mapping.
Review
Quoted 1 line
(quoted diff) +1
-0+ if( is_pci_exp_platform() != 0){
Shouldn't there a pci_probe option for this here?
It would be probably better to add some way to turn this off at runtime, since it's still experimental and most drivers will probably work with the old methods for now.
Review
Quoted 9 lines
(quoted diff) +6
-1if (capable(CAP_SYS_ADMIN)) - size = PCI_CFG_SPACE_SIZE; +#ifdef CONFIG_PCI_EXP_ENHANCED + if (pci_find_capability(dev,PCI_CAP_ID_EXP)) + size = PCI_CFG_SPACE_EXP_SIZE; + else +#endif /*CONFIG_PCI_EXP_ENHANCED */ + size = PCI_CFG_SPACE_SIZE;
This would be somewhat cleaner in an standard function that returns the cfg space size for a device, especially since this code is often duplicated in your patch.
Review
Quoted 7 lines
(quoted diff) +7
-0+#ifdef CONFIG_PCI_EXP_ENHANCED +struct acpi_table_mcfg { + struct acpi_table_header header; + u8 reserved[8]; + u64 base_address; +} __attribute__ ((packed)); +#endif
Declarations don't need to be #ifdefed.
-Andi