Re: pci: Arch hook to determine config space size

[Brian King <brking@us.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2425 bytes --]

Arnd Bergmann wrote:
> On Maandag 31 Januar 2005 22:35, Brian King wrote:
> 
>>Matthew Wilcox wrote:
>>
>>>Basically, ppc64's config ops are broken and need to check the offset
>>>being read.  Here's i386:
>>>
>>>static int pci_conf1_write (int seg, int bus, int devfn, int reg, int len, u32 v
>>>alue)
>>>{
>>>        unsigned long flags;
>>>
>>>        if ((bus > 255) || (devfn > 255) || (reg > 255)) 
>>>                return -EINVAL;
>>
>>Here is a pure ppc64 implementation that does this.
> 
> 
> Actually, it doesn't:
> 
> 
>>+static int config_access_valid(struct device_node *dn, int where)
>>+{
>>+       struct device_node *hose_dn = dn->phb->arch_data;
>>+
>>+       if (where < 256 || hose_dn->pci_ext_config_space)
>>+               return 1;
> 
> 
> This needs a check for (where < 4096) in case of PCIe or PCI-X.

Done.

>>@@ -62,6 +72,8 @@ static int rtas_read_config(struct devic
>>                return PCIBIOS_DEVICE_NOT_FOUND;
>>        if (where & (size - 1))
>>                return PCIBIOS_BAD_REGISTER_NUMBER;
>>+       if (!config_access_valid(dn, where))
>>+               return PCIBIOS_BAD_REGISTER_NUMBER;
>> 
>>        addr = (dn->busno << 16) | (dn->devfn << 8) | where;
> 
> 
> addr is still wrong, see my previous mail.

Fixed.


>>@@ -285,6 +309,7 @@ static int __devinit setup_phb(struct de
>>        phb->arch_data = dev;
>>        phb->ops = &rtas_pci_ops;
>>        phb->buid = get_phb_buid(dev);
>>+       get_phb_config_space_type(dev);
>> 
>>        return 0;
>> }
> 
> 
> Isn't the config space size a property of the PCI device instead of the
> host bridge? For a PCI device behind a PCIe host bridge, this could
> still lead to an incorrect config space accesses.

It is a property of both. Accessing config space beyond the first 256 
bytes will only work if both the PCI device and the host bridge support 
it. The problem I ran into was generic pci code issuing a config read to 
offset 256 after checking that the device supports it when the host 
bridge did not support it.

> PS: I got a permanent fatal error from <linux-pci@vger.kernel.org>, does
> that list actually exist?

Sorry about that... Should be fixed on this thread now. I checked the 
archives and saw a thread related to adding another L: line to the 
MAINTAINERS file for the linux-pci list. Greg - was some flavor of that 
patch going in?

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

[-- Attachment #2: ppc64_pcix_mode2_cfg.patch --]
[-- Type: text/plain, Size: 3670 bytes --]



When working with a PCI-X Mode 2 adapter on a PCI-X Mode 1 PPC64
system, the current code used to determine the config space size
of a device results in a PCI Master abort and an EEH error, resulting
in the device being taken offline. This patch checks OF to see if
the PCI bridge supports PCI-X Mode 2 and fails config accesses beyond
256 bytes if it does not.

Signed-off-by: Brian King <brking@us.ibm.com>
---

 linux-2.6.11-rc2-bk9-bjking1/arch/ppc64/kernel/pSeries_pci.c |   33 ++++++++++-
 linux-2.6.11-rc2-bk9-bjking1/include/asm-ppc64/prom.h        |    1 
 2 files changed, 32 insertions(+), 2 deletions(-)

diff -puN arch/ppc64/kernel/pSeries_pci.c~ppc64_pcix_mode2_cfg arch/ppc64/kernel/pSeries_pci.c
--- linux-2.6.11-rc2-bk9/arch/ppc64/kernel/pSeries_pci.c~ppc64_pcix_mode2_cfg	2005-01-31 14:32:01.000000000 -0600
+++ linux-2.6.11-rc2-bk9-bjking1/arch/ppc64/kernel/pSeries_pci.c	2005-01-31 16:17:08.000000000 -0600
@@ -52,6 +52,18 @@ static int s7a_workaround;
 
 extern struct mpic *pSeries_mpic;
 
+static int config_access_valid(struct device_node *dn, int where)
+{
+	struct device_node *hose_dn = dn->phb->arch_data;
+
+	if (where < 256)
+		return 1;
+	if (where < 4096 && hose_dn->pci_ext_config_space)
+		return 1;
+
+	return 0;
+}
+
 static int rtas_read_config(struct device_node *dn, int where, int size, u32 *val)
 {
 	int returnval = -1;
@@ -62,8 +74,11 @@ static int rtas_read_config(struct devic
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	if (where & (size - 1))
 		return PCIBIOS_BAD_REGISTER_NUMBER;
+	if (!config_access_valid(dn, where))
+		return PCIBIOS_BAD_REGISTER_NUMBER;
 
-	addr = (dn->busno << 16) | (dn->devfn << 8) | where;
+	addr = ((where & 0xf00) << 20) | (dn->busno << 16) |
+		(dn->devfn << 8) | (where & 0xff);
 	buid = dn->phb->buid;
 	if (buid) {
 		ret = rtas_call(ibm_read_pci_config, 4, 2, &returnval,
@@ -110,8 +125,11 @@ static int rtas_write_config(struct devi
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	if (where & (size - 1))
 		return PCIBIOS_BAD_REGISTER_NUMBER;
+	if (!config_access_valid(dn, where))
+		return PCIBIOS_BAD_REGISTER_NUMBER;
 
-	addr = (dn->busno << 16) | (dn->devfn << 8) | where;
+	addr = ((where & 0xf00) << 20) | (dn->busno << 16) |
+		(dn->devfn << 8) | (where & 0xff);
 	buid = dn->phb->buid;
 	if (buid) {
 		ret = rtas_call(ibm_write_pci_config, 5, 1, NULL, addr, buid >> 32, buid & 0xffffffff, size, (ulong) val);
@@ -270,6 +288,16 @@ static int phb_set_bus_ranges(struct dev
 	return 0;
 }
 
+static void __devinit get_phb_config_space_type(struct device_node *dn)
+{
+	int *type = (int *)get_property(dn, "ibm,pci-config-space-type", NULL);
+
+	if (type && *type == 1)
+		dn->pci_ext_config_space = 1;
+	else
+		dn->pci_ext_config_space = 0;
+}
+
 static int __devinit setup_phb(struct device_node *dev,
 			       struct pci_controller *phb,
 			       unsigned int addr_size_words)
@@ -285,6 +313,7 @@ static int __devinit setup_phb(struct de
 	phb->arch_data = dev;
 	phb->ops = &rtas_pci_ops;
 	phb->buid = get_phb_buid(dev);
+	get_phb_config_space_type(dev);
 
 	return 0;
 }
diff -puN include/asm-ppc64/prom.h~ppc64_pcix_mode2_cfg include/asm-ppc64/prom.h
--- linux-2.6.11-rc2-bk9/include/asm-ppc64/prom.h~ppc64_pcix_mode2_cfg	2005-01-31 14:32:01.000000000 -0600
+++ linux-2.6.11-rc2-bk9-bjking1/include/asm-ppc64/prom.h	2005-01-31 14:32:01.000000000 -0600
@@ -137,6 +137,7 @@ struct device_node {
 	int	devfn;			/* for pci devices */
 	int	eeh_mode;		/* See eeh.h for possible EEH_MODEs */
 	int	eeh_config_addr;
+	int	pci_ext_config_space;	/* for phb's or bridges */
 	struct  pci_controller *phb;	/* for pci devices */
 	struct	iommu_table *iommu_table;	/* for phb's or bridges */
 
_