LS1043A : "synchronous abort" at boot due to PCI config read

LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Dear developers,

I currently have two functional workarounds for this issue but would like to know which one you would recommend, if any :-)
I'm using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous external abort" when booting because of a PCI config read 
during PCI scan.

I'm using a custom hardware (based on LS1043ARDB) having a PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI slot 
for legacy devices. This bridge only supports PCI-Compatible config accesses (offset 0x00-0xFF).
On this PCI slot I connect a PCI module made of a PCI-to-PCIe bridge plus PCIe devices behind.
The problem occurs when the kernel probes the PCIe devices : as they are PCIe devices, the kernel does a PCI config read access at 
offset 0x100 to check if "PCIe extended capability registers" are accessible (see drivers/pci/probe.c, function 
pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI bridge that is in the path reports an error to the CPU for this 
access, and it seems there's no way to disable that on this bridge.

The first workaround I found was to patch drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set to 0x9400 instead of 
0x9401 (for PCIE_ABSERR register) to disable error reporting. This only impacts an NXP part of the Linux kernel code, but I'm not 
sure this is a good idea (however it seems to be like that on Intel platforms where even MEM accesses to a no-device address return 
FF without any error).

I've also tried another workaround that works : patch drivers/pci/probe.c to use bus_flags to remember if a bus is behind a bridge 
without extended address capability, to avoid PCi config read accesses at offset 0x100 in
pci_cfg_space_size() / pci_cfg_space_size_ext(). But this patch impacts the generic PCI probe method of Linux.

Any Idea to properly handle that issue ?

Best regards

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Ard Biesheuvel]

(add Bjorn and linux-pci)

On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
> Dear developers,
>
> I currently have two functional workarounds for this issue but would like to know which one you would recommend, if any :-)
> I'm using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous external abort" when booting because of a PCI config read
> during PCI scan.
>
> I'm using a custom hardware (based on LS1043ARDB) having a PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI slot
> for legacy devices. This bridge only supports PCI-Compatible config accesses (offset 0x00-0xFF).
> On this PCI slot I connect a PCI module made of a PCI-to-PCIe bridge plus PCIe devices behind.
> The problem occurs when the kernel probes the PCIe devices : as they are PCIe devices, the kernel does a PCI config read access at
> offset 0x100 to check if "PCIe extended capability registers" are accessible (see drivers/pci/probe.c, function
> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI bridge that is in the path reports an error to the CPU for this
> access, and it seems there's no way to disable that on this bridge.
>
> The first workaround I found was to patch drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set to 0x9400 instead of
> 0x9401 (for PCIE_ABSERR register) to disable error reporting. This only impacts an NXP part of the Linux kernel code, but I'm not
> sure this is a good idea (however it seems to be like that on Intel platforms where even MEM accesses to a no-device address return
> FF without any error).
>
> I've also tried another workaround that works : patch drivers/pci/probe.c to use bus_flags to remember if a bus is behind a bridge
> without extended address capability, to avoid PCi config read accesses at offset 0x100 in
> pci_cfg_space_size() / pci_cfg_space_size_ext(). But this patch impacts the generic PCI probe method of Linux.
>
> Any Idea to properly handle that issue ?
>

This seems like a rather unusual configuration, but I guess that if
the first bridge/switch advertises its inability to support extended
config space accesses, we should not be performing them on any of its
subordinate buses. How does the PEX8112 advertise this limitation?

That said, I wonder if it is reasonable in the first place to expect
that a PCIe device works as expected passing through a legacy PCI
layer like that.

LS1043A : "synchronous abort" at boot due to PCI config read

[Ard Biesheuvel]

(add Bjorn and linux-pci)

On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
> Dear developers,
>
> I currently have two functional workarounds for this issue but would like to know which one you would recommend, if any :-)
> I'm using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous external abort" when booting because of a PCI config read
> during PCI scan.
>
> I'm using a custom hardware (based on LS1043ARDB) having a PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI slot
> for legacy devices. This bridge only supports PCI-Compatible config accesses (offset 0x00-0xFF).
> On this PCI slot I connect a PCI module made of a PCI-to-PCIe bridge plus PCIe devices behind.
> The problem occurs when the kernel probes the PCIe devices : as they are PCIe devices, the kernel does a PCI config read access at
> offset 0x100 to check if "PCIe extended capability registers" are accessible (see drivers/pci/probe.c, function
> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI bridge that is in the path reports an error to the CPU for this
> access, and it seems there's no way to disable that on this bridge.
>
> The first workaround I found was to patch drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set to 0x9400 instead of
> 0x9401 (for PCIE_ABSERR register) to disable error reporting. This only impacts an NXP part of the Linux kernel code, but I'm not
> sure this is a good idea (however it seems to be like that on Intel platforms where even MEM accesses to a no-device address return
> FF without any error).
>
> I've also tried another workaround that works : patch drivers/pci/probe.c to use bus_flags to remember if a bus is behind a bridge
> without extended address capability, to avoid PCi config read accesses at offset 0x100 in
> pci_cfg_space_size() / pci_cfg_space_size_ext(). But this patch impacts the generic PCI probe method of Linux.
>
> Any Idea to properly handle that issue ?
>

This seems like a rather unusual configuration, but I guess that if
the first bridge/switch advertises its inability to support extended
config space accesses, we should not be performing them on any of its
subordinate buses. How does the PEX8112 advertise this limitation?

That said, I wonder if it is reasonable in the first place to expect
that a PCIe device works as expected passing through a legacy PCI
layer like that.

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

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

Le 27/04/2018 10:43, Ard Biesheuvel a écrit :
> (add Bjorn and linux-pci)
>
> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
>> Dear developers,
>>
>> I currently have two functional workarounds for this issue but would like to know which one you would recommend, if any :-)
>> I'm using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous external abort" when booting because of a PCI config read
>> during PCI scan.
>>
>> I'm using a custom hardware (based on LS1043ARDB) having a PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI slot
>> for legacy devices. This bridge only supports PCI-Compatible config accesses (offset 0x00-0xFF).
>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe bridge plus PCIe devices behind.
>> The problem occurs when the kernel probes the PCIe devices : as they are PCIe devices, the kernel does a PCI config read access at
>> offset 0x100 to check if "PCIe extended capability registers" are accessible (see drivers/pci/probe.c, function
>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI bridge that is in the path reports an error to the CPU for this
>> access, and it seems there's no way to disable that on this bridge.
>>
>> The first workaround I found was to patch drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set to 0x9400 instead of
>> 0x9401 (for PCIE_ABSERR register) to disable error reporting. This only impacts an NXP part of the Linux kernel code, but I'm not
>> sure this is a good idea (however it seems to be like that on Intel platforms where even MEM accesses to a no-device address return
>> FF without any error).
>>
>> I've also tried another workaround that works : patch drivers/pci/probe.c to use bus_flags to remember if a bus is behind a bridge
>> without extended address capability, to avoid PCi config read accesses at offset 0x100 in
>> pci_cfg_space_size() / pci_cfg_space_size_ext(). But this patch impacts the generic PCI probe method of Linux.
>>
>> Any Idea to properly handle that issue ?
>>
> This seems like a rather unusual configuration, but I guess that if
> the first bridge/switch advertises its inability to support extended
> config space accesses, we should not be performing them on any of its
> subordinate buses. How does the PEX8112 advertise this limitation?
>
> That said, I wonder if it is reasonable in the first place to expect
> that a PCIe device works as expected passing through a legacy PCI
> layer like that.
>
> .
The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but has no PCI_CAP_ID_PCIX capability.
As I understand the lack of PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no support for PCI config offset >=0x100).
Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this limitation would be advertised by the lack of PCI_X_STATUS_266MHZ 
and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at pci_cfg_space_size())

I'm currently using the attached patch (for kernel 4.1.35-rt41 from NXP Yocto BSP). It uses bus_flags to remember if a bus is behind 
a bridge without extended address capability to avoid PCi config accesses at offset >= 0x100. Thanks to this patch I now have a 
functional system with functional PCI/PCIe devices.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cfgspace.patch --]
[-- Type: text/x-patch; name=cfgspace.patch, Size: 1943 bytes --]

--- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
+++ include/linux/pci.h	2018-03-26 16:51:27.660000000 +0000
@@ -193,6 +193,7 @@
 enum pci_bus_flags {
 	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
 	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
+	PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
 };
 
 /* These values come from the PCI Express Spec */
--- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
+++ drivers/pci/probe.c	2018-03-26 16:54:30.830000000 +0000
@@ -827,6 +827,28 @@
 			child->primary = primary;
 			pci_bus_insert_busn_res(child, secondary, subordinate);
 			child->bridge_ctl = bctl;
+
+			{
+				int pos;
+				u32 status;
+				bool pci_compat_cfg_space = false;
+
+				if (!pci_is_pcie(dev) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE)) {
+					/* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse mode, we have to check for PCI-X capabilities */
+					pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
+					if (pos) {
+						pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
+						if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
+							pci_compat_cfg_space = true;
+					} else {
+						pci_compat_cfg_space = true;
+					}
+					if (pci_compat_cfg_space) {
+						dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compatible config space\n", dev->vendor, dev->device);
+						child->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
+					}
+				}
+			}
 		}
 
 		cmax = pci_scan_child_bus(child);
@@ -1098,6 +1120,11 @@
 			goto fail;
 	}
 
+	if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
+		dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due to parent bus(es)\n", dev->vendor, dev->device);
+		return PCI_CFG_SPACE_SIZE;
+	}
+
 	return pci_cfg_space_size_ext(dev);
 
  fail:

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Le 27/04/2018 10:43, Ard Biesheuvel a ?crit :
> (add Bjorn and linux-pci)
>
> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
>> Dear developers,
>>
>> I currently have two functional workarounds for this issue but would like to know which one you would recommend, if any :-)
>> I'm using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous external abort" when booting because of a PCI config read
>> during PCI scan.
>>
>> I'm using a custom hardware (based on LS1043ARDB) having a PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI slot
>> for legacy devices. This bridge only supports PCI-Compatible config accesses (offset 0x00-0xFF).
>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe bridge plus PCIe devices behind.
>> The problem occurs when the kernel probes the PCIe devices : as they are PCIe devices, the kernel does a PCI config read access at
>> offset 0x100 to check if "PCIe extended capability registers" are accessible (see drivers/pci/probe.c, function
>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI bridge that is in the path reports an error to the CPU for this
>> access, and it seems there's no way to disable that on this bridge.
>>
>> The first workaround I found was to patch drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set to 0x9400 instead of
>> 0x9401 (for PCIE_ABSERR register) to disable error reporting. This only impacts an NXP part of the Linux kernel code, but I'm not
>> sure this is a good idea (however it seems to be like that on Intel platforms where even MEM accesses to a no-device address return
>> FF without any error).
>>
>> I've also tried another workaround that works : patch drivers/pci/probe.c to use bus_flags to remember if a bus is behind a bridge
>> without extended address capability, to avoid PCi config read accesses at offset 0x100 in
>> pci_cfg_space_size() / pci_cfg_space_size_ext(). But this patch impacts the generic PCI probe method of Linux.
>>
>> Any Idea to properly handle that issue ?
>>
> This seems like a rather unusual configuration, but I guess that if
> the first bridge/switch advertises its inability to support extended
> config space accesses, we should not be performing them on any of its
> subordinate buses. How does the PEX8112 advertise this limitation?
>
> That said, I wonder if it is reasonable in the first place to expect
> that a PCIe device works as expected passing through a legacy PCI
> layer like that.
>
> .
The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but has no PCI_CAP_ID_PCIX capability.
As I understand the lack of PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no support for PCI config offset >=0x100).
Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this limitation would be advertised by the lack of PCI_X_STATUS_266MHZ 
and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at pci_cfg_space_size())

I'm currently using the attached patch (for kernel 4.1.35-rt41 from NXP Yocto BSP). It uses bus_flags to remember if a bus is behind 
a bridge without extended address capability to avoid PCi config accesses at offset >= 0x100. Thanks to this patch I now have a 
functional system with functional PCI/PCIe devices.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cfgspace.patch
Type: text/x-patch
Size: 1943 bytes
Desc: cfgspace.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180427/ac2bb836/attachment.bin>

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Bjorn Helgaas]

On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
> Le 27/04/2018 10:43, Ard Biesheuvel a écrit :
> > (add Bjorn and linux-pci)
> >
> > On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
> >> Dear developers,
> >>
> >> I currently have two functional workarounds for this issue but
> >> would like to know which one you would recommend, if any :-) I'm
> >> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
> >> external abort" when booting because of a PCI config read during
> >> PCI scan.
> >>
> >> I'm using a custom hardware (based on LS1043ARDB) having a
> >> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
> >> slot for legacy devices. This bridge only supports PCI-Compatible
> >> config accesses (offset 0x00-0xFF).

I would guess the PEX8112 itself has 4K of config space, but it only
forwards 256 bytes of config space to the conventional PCI secondary
bus.

> >> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
> >> bridge plus PCIe devices behind.
> >>
> >> The problem occurs when the kernel probes the PCIe devices : as
> >> they are PCIe devices, the kernel does a PCI config read access
> >> at offset 0x100 to check if "PCIe extended capability registers"
> >> are accessible (see drivers/pci/probe.c, function
> >> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
> >> bridge that is in the path reports an error to the CPU for this
> >> access, and it seems there's no way to disable that on this
> >> bridge.
> >>
> >> The first workaround I found was to patch
> >> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
> >> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
> >> error reporting. This only impacts an NXP part of the Linux
> >> kernel code, but I'm not sure this is a good idea (however it
> >> seems to be like that on Intel platforms where even MEM accesses
> >> to a no-device address return FF without any error).
> >>
> >> I've also tried another workaround that works : patch
> >> drivers/pci/probe.c to use bus_flags to remember if a bus is
> >> behind a bridge without extended address capability, to avoid PCi
> >> config read accesses at offset 0x100 in pci_cfg_space_size() /
> >> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
> >> probe method of Linux.
> >>
> >> Any Idea to properly handle that issue ?
> >>
> > This seems like a rather unusual configuration, but I guess that
> > if the first bridge/switch advertises its inability to support
> > extended config space accesses, we should not be performing them
> > on any of its subordinate buses. How does the PEX8112 advertise
> > this limitation?
> >
> > That said, I wonder if it is reasonable in the first place to
> > expect that a PCIe device works as expected passing through a
> > legacy PCI layer like that.
> >
> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
> support for PCI config offset >=0x100).

Sounds right to me.

> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
> pci_cfg_space_size())

Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266MHZ
should be enough, but it shouldn't hurt to check for either
PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.

> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
> bridge without extended address capability to avoid PCi config
> accesses at offset >= 0x100. Thanks to this patch I now have a
> functional system with functional PCI/PCIe devices.

The patch seems like it's looking at the right things, but I don't
want to build it into pci_scan_bridge_extend() because that function
is much too complicated already.

I'd rather build it into pci_cfg_space_size() or
pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
that case, I think all 4K would be accessible on the PCI-X side.

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac91b6fd0bcd..d8b091f0bcd1 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
  * pci_cfg_space_size - Get the configuration space size of the PCI device
  * @dev: PCI device
  *
- * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express devices
+ * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Express devices
  * have 4096 bytes.  Even if the device is capable, that doesn't mean we can
  * access it.  Maybe we don't have a way to generate extended config space
  * accesses, or the device is behind a reverse Express bridge.  So we try
@@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
  */
 static int pci_cfg_space_size_ext(struct pci_dev *dev)
 {
+	struct pci_dev *bridge = pci_upstream_bridge(dev);
 	u32 status;
 	int pos = PCI_CFG_SPACE_SIZE;
 
+	if (bridge && pci_is_pcie(bridge) &&
+	    pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
+		return PCI_CFG_SPACE_SIZE;
+
 	if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
 		return PCI_CFG_SPACE_SIZE;
 	if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))

> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
> +++ include/linux/pci.h	2018-03-26 16:51:27.660000000 +0000
> @@ -193,6 +193,7 @@
>  enum pci_bus_flags {
>  	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>  	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
> +	PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>  };
>  
>  /* These values come from the PCI Express Spec */
> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
> +++ drivers/pci/probe.c	2018-03-26 16:54:30.830000000 +0000
> @@ -827,6 +827,28 @@
>  			child->primary = primary;
>  			pci_bus_insert_busn_res(child, secondary, subordinate);
>  			child->bridge_ctl = bctl;
> +
> +			{
> +				int pos;
> +				u32 status;
> +				bool pci_compat_cfg_space = false;
> +
> +				if (!pci_is_pcie(dev) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE)) {
> +					/* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse mode, we have to check for PCI-X capabilities */
> +					pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> +					if (pos) {
> +						pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
> +						if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
> +							pci_compat_cfg_space = true;
> +					} else {
> +						pci_compat_cfg_space = true;
> +					}
> +					if (pci_compat_cfg_space) {
> +						dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compatible config space\n", dev->vendor, dev->device);
> +						child->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
> +					}
> +				}
> +			}
>  		}
>  
>  		cmax = pci_scan_child_bus(child);
> @@ -1098,6 +1120,11 @@
>  			goto fail;
>  	}
>  
> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
> +		dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due to parent bus(es)\n", dev->vendor, dev->device);
> +		return PCI_CFG_SPACE_SIZE;
> +	}
> +
>  	return pci_cfg_space_size_ext(dev);
>  
>   fail:

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

LS1043A : "synchronous abort" at boot due to PCI config read

[Bjorn Helgaas]

On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
> Le 27/04/2018 10:43, Ard Biesheuvel a ?crit :
> > (add Bjorn and linux-pci)
> >
> > On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
> >> Dear developers,
> >>
> >> I currently have two functional workarounds for this issue but
> >> would like to know which one you would recommend, if any :-) I'm
> >> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
> >> external abort" when booting because of a PCI config read during
> >> PCI scan.
> >>
> >> I'm using a custom hardware (based on LS1043ARDB) having a
> >> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
> >> slot for legacy devices. This bridge only supports PCI-Compatible
> >> config accesses (offset 0x00-0xFF).

I would guess the PEX8112 itself has 4K of config space, but it only
forwards 256 bytes of config space to the conventional PCI secondary
bus.

> >> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
> >> bridge plus PCIe devices behind.
> >>
> >> The problem occurs when the kernel probes the PCIe devices : as
> >> they are PCIe devices, the kernel does a PCI config read access
> >> at offset 0x100 to check if "PCIe extended capability registers"
> >> are accessible (see drivers/pci/probe.c, function
> >> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
> >> bridge that is in the path reports an error to the CPU for this
> >> access, and it seems there's no way to disable that on this
> >> bridge.
> >>
> >> The first workaround I found was to patch
> >> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
> >> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
> >> error reporting. This only impacts an NXP part of the Linux
> >> kernel code, but I'm not sure this is a good idea (however it
> >> seems to be like that on Intel platforms where even MEM accesses
> >> to a no-device address return FF without any error).
> >>
> >> I've also tried another workaround that works : patch
> >> drivers/pci/probe.c to use bus_flags to remember if a bus is
> >> behind a bridge without extended address capability, to avoid PCi
> >> config read accesses at offset 0x100 in pci_cfg_space_size() /
> >> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
> >> probe method of Linux.
> >>
> >> Any Idea to properly handle that issue ?
> >>
> > This seems like a rather unusual configuration, but I guess that
> > if the first bridge/switch advertises its inability to support
> > extended config space accesses, we should not be performing them
> > on any of its subordinate buses. How does the PEX8112 advertise
> > this limitation?
> >
> > That said, I wonder if it is reasonable in the first place to
> > expect that a PCIe device works as expected passing through a
> > legacy PCI layer like that.
> >
> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
> support for PCI config offset >=0x100).

Sounds right to me.

> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
> pci_cfg_space_size())

Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266MHZ
should be enough, but it shouldn't hurt to check for either
PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.

> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
> bridge without extended address capability to avoid PCi config
> accesses at offset >= 0x100. Thanks to this patch I now have a
> functional system with functional PCI/PCIe devices.

The patch seems like it's looking at the right things, but I don't
want to build it into pci_scan_bridge_extend() because that function
is much too complicated already.

I'd rather build it into pci_cfg_space_size() or
pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
that case, I think all 4K would be accessible on the PCI-X side.

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac91b6fd0bcd..d8b091f0bcd1 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
  * pci_cfg_space_size - Get the configuration space size of the PCI device
  * @dev: PCI device
  *
- * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express devices
+ * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Express devices
  * have 4096 bytes.  Even if the device is capable, that doesn't mean we can
  * access it.  Maybe we don't have a way to generate extended config space
  * accesses, or the device is behind a reverse Express bridge.  So we try
@@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
  */
 static int pci_cfg_space_size_ext(struct pci_dev *dev)
 {
+	struct pci_dev *bridge = pci_upstream_bridge(dev);
 	u32 status;
 	int pos = PCI_CFG_SPACE_SIZE;
 
+	if (bridge && pci_is_pcie(bridge) &&
+	    pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
+		return PCI_CFG_SPACE_SIZE;
+
 	if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
 		return PCI_CFG_SPACE_SIZE;
 	if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))

> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
> +++ include/linux/pci.h	2018-03-26 16:51:27.660000000 +0000
> @@ -193,6 +193,7 @@
>  enum pci_bus_flags {
>  	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>  	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
> +	PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>  };
>  
>  /* These values come from the PCI Express Spec */
> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
> +++ drivers/pci/probe.c	2018-03-26 16:54:30.830000000 +0000
> @@ -827,6 +827,28 @@
>  			child->primary = primary;
>  			pci_bus_insert_busn_res(child, secondary, subordinate);
>  			child->bridge_ctl = bctl;
> +
> +			{
> +				int pos;
> +				u32 status;
> +				bool pci_compat_cfg_space = false;
> +
> +				if (!pci_is_pcie(dev) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE)) {
> +					/* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse mode, we have to check for PCI-X capabilities */
> +					pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> +					if (pos) {
> +						pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
> +						if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
> +							pci_compat_cfg_space = true;
> +					} else {
> +						pci_compat_cfg_space = true;
> +					}
> +					if (pci_compat_cfg_space) {
> +						dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compatible config space\n", dev->vendor, dev->device);
> +						child->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
> +					}
> +				}
> +			}
>  		}
>  
>  		cmax = pci_scan_child_bus(child);
> @@ -1098,6 +1120,11 @@
>  			goto fail;
>  	}
>  
> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
> +		dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due to parent bus(es)\n", dev->vendor, dev->device);
> +		return PCI_CFG_SPACE_SIZE;
> +	}
> +
>  	return pci_cfg_space_size_ext(dev);
>  
>   fail:

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Le 27/04/2018 18:56, Bjorn Helgaas a =E9crit :
> On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
>> Le 27/04/2018 10:43, Ard Biesheuvel a =E9crit :
>>> (add Bjorn and linux-pci)
>>>
>>> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrot=
e:
>>>> Dear developers,
>>>>
>>>> I currently have two functional workarounds for this issue but
>>>> would like to know which one you would recommend, if any :-) I'm
>>>> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
>>>> external abort" when booting because of a PCI config read during
>>>> PCI scan.
>>>>
>>>> I'm using a custom hardware (based on LS1043ARDB) having a
>>>> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
>>>> slot for legacy devices. This bridge only supports PCI-Compatible
>>>> config accesses (offset 0x00-0xFF).
> I would guess the PEX8112 itself has 4K of config space, but it only
> forwards 256 bytes of config space to the conventional PCI secondary
> bus.
>
>>>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
>>>> bridge plus PCIe devices behind.
>>>>
>>>> The problem occurs when the kernel probes the PCIe devices : as
>>>> they are PCIe devices, the kernel does a PCI config read access
>>>> at offset 0x100 to check if "PCIe extended capability registers"
>>>> are accessible (see drivers/pci/probe.c, function
>>>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
>>>> bridge that is in the path reports an error to the CPU for this
>>>> access, and it seems there's no way to disable that on this
>>>> bridge.
>>>>
>>>> The first workaround I found was to patch
>>>> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
>>>> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
>>>> error reporting. This only impacts an NXP part of the Linux
>>>> kernel code, but I'm not sure this is a good idea (however it
>>>> seems to be like that on Intel platforms where even MEM accesses
>>>> to a no-device address return FF without any error).
>>>>
>>>> I've also tried another workaround that works : patch
>>>> drivers/pci/probe.c to use bus_flags to remember if a bus is
>>>> behind a bridge without extended address capability, to avoid PCi
>>>> config read accesses at offset 0x100 in pci_cfg_space_size() /
>>>> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
>>>> probe method of Linux.
>>>>
>>>> Any Idea to properly handle that issue ?
>>>>
>>> This seems like a rather unusual configuration, but I guess that
>>> if the first bridge/switch advertises its inability to support
>>> extended config space accesses, we should not be performing them
>>> on any of its subordinate buses. How does the PEX8112 advertise
>>> this limitation?
>>>
>>> That said, I wonder if it is reasonable in the first place to
>>> expect that a PCIe device works as expected passing through a
>>> legacy PCI layer like that.
>>>
>> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
>> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
>> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
>> support for PCI config offset >=3D0x100).
> Sounds right to me.
>
>> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
>> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
>> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
>> pci_cfg_space_size())
> Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266MHZ
> should be enough, but it shouldn't hurt to check for either
> PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.
>
>> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
>> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
>> bridge without extended address capability to avoid PCi config
>> accesses at offset >=3D 0x100. Thanks to this patch I now have a
>> functional system with functional PCI/PCIe devices.
> The patch seems like it's looking at the right things, but I don't
> want to build it into pci_scan_bridge_extend() because that function
> is much too complicated already.
>
> I'd rather build it into pci_cfg_space_size() or
> pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
> This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
> that case, I think all 4K would be accessible on the PCI-X side.
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac91b6fd0bcd..d8b091f0bcd1 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *=
dev)
>    * pci_cfg_space_size - Get the configuration space size of the PCI dev=
ice
>    * @dev: PCI device
>    *
> - * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express devic=
es
> + * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Express =
devices
>    * have 4096 bytes.  Even if the device is capable, that doesn't mean w=
e can
>    * access it.  Maybe we don't have a way to generate extended config sp=
ace
>    * accesses, or the device is behind a reverse Express bridge.  So we t=
ry
> @@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev =
*dev)
>    */
>   static int pci_cfg_space_size_ext(struct pci_dev *dev)
>   {
> +	struct pci_dev *bridge =3D pci_upstream_bridge(dev);
>   	u32 status;
>   	int pos =3D PCI_CFG_SPACE_SIZE;
>   =

> +	if (bridge && pci_is_pcie(bridge) &&
> +	    pci_pcie_type(bridge) =3D=3D PCI_EXP_TYPE_PCI_BRIDGE)
> +		return PCI_CFG_SPACE_SIZE;
> +
>   	if (pci_read_config_dword(dev, pos, &status) !=3D PCIBIOS_SUCCESSFUL)
>   		return PCI_CFG_SPACE_SIZE;
>   	if (status =3D=3D 0xffffffff || pci_ext_cfg_is_aliased(dev))
>
>> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
>> +++ include/linux/pci.h	2018-03-26 16:51:27.660000000 +0000
>> @@ -193,6 +193,7 @@
>>   enum pci_bus_flags {
>>   	PCI_BUS_FLAGS_NO_MSI   =3D (__force pci_bus_flags_t) 1,
>>   	PCI_BUS_FLAGS_NO_MMRBC =3D (__force pci_bus_flags_t) 2,
>> +	PCI_BUS_FLAGS_COMPAT_CFG_SPACE =3D (__force pci_bus_flags_t) 4,
>>   };
>>   =

>>   /* These values come from the PCI Express Spec */
>> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
>> +++ drivers/pci/probe.c	2018-03-26 16:54:30.830000000 +0000
>> @@ -827,6 +827,28 @@
>>   			child->primary =3D primary;
>>   			pci_bus_insert_busn_res(child, secondary, subordinate);
>>   			child->bridge_ctl =3D bctl;
>> +
>> +			{
>> +				int pos;
>> +				u32 status;
>> +				bool pci_compat_cfg_space =3D false;
>> +
>> +				if (!pci_is_pcie(dev) || (pci_pcie_type(dev) =3D=3D PCI_EXP_TYPE_PC=
IE_BRIDGE) || (pci_pcie_type(dev) =3D=3D PCI_EXP_TYPE_PCI_BRIDGE)) {
>> +					/* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse m=
ode, we have to check for PCI-X capabilities */
>> +					pos =3D pci_find_capability(dev, PCI_CAP_ID_PCIX);
>> +					if (pos) {
>> +						pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
>> +						if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
>> +							pci_compat_cfg_space =3D true;
>> +					} else {
>> +						pci_compat_cfg_space =3D true;
>> +					}
>> +					if (pci_compat_cfg_space) {
>> +						dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compati=
ble config space\n", dev->vendor, dev->device);
>> +						child->bus_flags |=3D PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>> +					}
>> +				}
>> +			}
>>   		}
>>   =

>>   		cmax =3D pci_scan_child_bus(child);
>> @@ -1098,6 +1120,11 @@
>>   			goto fail;
>>   	}
>>   =

>> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
>> +		dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due=
 to parent bus(es)\n", dev->vendor, dev->device);
>> +		return PCI_CFG_SPACE_SIZE;
>> +	}
>> +
>>   	return pci_cfg_space_size_ext(dev);
>>   =

>>    fail:
Bjorn,
If I'm right about your proposed patch to pci_cfg_space_size_ext(), *bridge=
 is pointing to the upper device of device *dev being =

checked. I understand the purpose, but I think this fails for my config tha=
t is :

LS1043 PCIe root -> PEX8112 PCIe-to-PCI bridge -> PMC slot connector -> PCI=
-to-PCIe bridge -> PCIe switch (4 ports) -> 4 PCIe =

devices (one on each port)

because :
- when pci_cfg_space_size_ext() is run on the 4 PCIe devices, *bridge is th=
e PCIe switch which is not matching =

PCI_EXP_TYPE_PCI_BRIDGE. In this case *bridge should also be checked for th=
e parent bus of the PCIe switch, and so on.
- when pci_cfg_space_size_ext() is run for the PCI-to-PCIe bridge, *bridge =
is the PEX8112 that is also not matching =

PCI_EXP_TYPE_PCI_BRIDGE but PCI_EXP_TYPE_PCIE_BRIDGE. This leads to a confi=
g access at offset 0x100 to the PCI-to-PCIe bridge, so a =

crash (because of the PEX8112)

I think setting a bit in bus_flags when creating a child bus is very effici=
ent because once set it is automatically inherited by all =

child buses and then the only thing that pci_cfg_space_size() has to do for=
 each device is to check for this bit. Also this =

PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag is actually a bus property that is comp=
liant with the purpose of bus_flags.

I agree that pci_scan_bridge_extend() is already too complicated, so would =
you be okay to only add one line to it :
   pci_bus_set_compat_cfg_space(child);
and put all the code I suggested in this new function pci_bus_set_compat_cf=
g_space() ? (also supporting PCI-X Mode 2 devices)
Improvement : this function can return immediately if the child bus has alr=
eady inherited the flag from its parent.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Le 27/04/2018 18:56, Bjorn Helgaas a ?crit :
> On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
>> Le 27/04/2018 10:43, Ard Biesheuvel a ?crit :
>>> (add Bjorn and linux-pci)
>>>
>>> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
>>>> Dear developers,
>>>>
>>>> I currently have two functional workarounds for this issue but
>>>> would like to know which one you would recommend, if any :-) I'm
>>>> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
>>>> external abort" when booting because of a PCI config read during
>>>> PCI scan.
>>>>
>>>> I'm using a custom hardware (based on LS1043ARDB) having a
>>>> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
>>>> slot for legacy devices. This bridge only supports PCI-Compatible
>>>> config accesses (offset 0x00-0xFF).
> I would guess the PEX8112 itself has 4K of config space, but it only
> forwards 256 bytes of config space to the conventional PCI secondary
> bus.
>
>>>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
>>>> bridge plus PCIe devices behind.
>>>>
>>>> The problem occurs when the kernel probes the PCIe devices : as
>>>> they are PCIe devices, the kernel does a PCI config read access
>>>> at offset 0x100 to check if "PCIe extended capability registers"
>>>> are accessible (see drivers/pci/probe.c, function
>>>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
>>>> bridge that is in the path reports an error to the CPU for this
>>>> access, and it seems there's no way to disable that on this
>>>> bridge.
>>>>
>>>> The first workaround I found was to patch
>>>> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
>>>> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
>>>> error reporting. This only impacts an NXP part of the Linux
>>>> kernel code, but I'm not sure this is a good idea (however it
>>>> seems to be like that on Intel platforms where even MEM accesses
>>>> to a no-device address return FF without any error).
>>>>
>>>> I've also tried another workaround that works : patch
>>>> drivers/pci/probe.c to use bus_flags to remember if a bus is
>>>> behind a bridge without extended address capability, to avoid PCi
>>>> config read accesses at offset 0x100 in pci_cfg_space_size() /
>>>> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
>>>> probe method of Linux.
>>>>
>>>> Any Idea to properly handle that issue ?
>>>>
>>> This seems like a rather unusual configuration, but I guess that
>>> if the first bridge/switch advertises its inability to support
>>> extended config space accesses, we should not be performing them
>>> on any of its subordinate buses. How does the PEX8112 advertise
>>> this limitation?
>>>
>>> That said, I wonder if it is reasonable in the first place to
>>> expect that a PCIe device works as expected passing through a
>>> legacy PCI layer like that.
>>>
>> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
>> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
>> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
>> support for PCI config offset >=0x100).
> Sounds right to me.
>
>> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
>> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
>> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
>> pci_cfg_space_size())
> Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266MHZ
> should be enough, but it shouldn't hurt to check for either
> PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.
>
>> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
>> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
>> bridge without extended address capability to avoid PCi config
>> accesses at offset >= 0x100. Thanks to this patch I now have a
>> functional system with functional PCI/PCIe devices.
> The patch seems like it's looking at the right things, but I don't
> want to build it into pci_scan_bridge_extend() because that function
> is much too complicated already.
>
> I'd rather build it into pci_cfg_space_size() or
> pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
> This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
> that case, I think all 4K would be accessible on the PCI-X side.
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac91b6fd0bcd..d8b091f0bcd1 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>    * pci_cfg_space_size - Get the configuration space size of the PCI device
>    * @dev: PCI device
>    *
> - * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express devices
> + * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Express devices
>    * have 4096 bytes.  Even if the device is capable, that doesn't mean we can
>    * access it.  Maybe we don't have a way to generate extended config space
>    * accesses, or the device is behind a reverse Express bridge.  So we try
> @@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>    */
>   static int pci_cfg_space_size_ext(struct pci_dev *dev)
>   {
> +	struct pci_dev *bridge = pci_upstream_bridge(dev);
>   	u32 status;
>   	int pos = PCI_CFG_SPACE_SIZE;
>   
> +	if (bridge && pci_is_pcie(bridge) &&
> +	    pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
> +		return PCI_CFG_SPACE_SIZE;
> +
>   	if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
>   		return PCI_CFG_SPACE_SIZE;
>   	if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))
>
>> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
>> +++ include/linux/pci.h	2018-03-26 16:51:27.660000000 +0000
>> @@ -193,6 +193,7 @@
>>   enum pci_bus_flags {
>>   	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>>   	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
>> +	PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>>   };
>>   
>>   /* These values come from the PCI Express Spec */
>> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
>> +++ drivers/pci/probe.c	2018-03-26 16:54:30.830000000 +0000
>> @@ -827,6 +827,28 @@
>>   			child->primary = primary;
>>   			pci_bus_insert_busn_res(child, secondary, subordinate);
>>   			child->bridge_ctl = bctl;
>> +
>> +			{
>> +				int pos;
>> +				u32 status;
>> +				bool pci_compat_cfg_space = false;
>> +
>> +				if (!pci_is_pcie(dev) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE)) {
>> +					/* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse mode, we have to check for PCI-X capabilities */
>> +					pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
>> +					if (pos) {
>> +						pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
>> +						if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
>> +							pci_compat_cfg_space = true;
>> +					} else {
>> +						pci_compat_cfg_space = true;
>> +					}
>> +					if (pci_compat_cfg_space) {
>> +						dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compatible config space\n", dev->vendor, dev->device);
>> +						child->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>> +					}
>> +				}
>> +			}
>>   		}
>>   
>>   		cmax = pci_scan_child_bus(child);
>> @@ -1098,6 +1120,11 @@
>>   			goto fail;
>>   	}
>>   
>> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
>> +		dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due to parent bus(es)\n", dev->vendor, dev->device);
>> +		return PCI_CFG_SPACE_SIZE;
>> +	}
>> +
>>   	return pci_cfg_space_size_ext(dev);
>>   
>>    fail:
Bjorn,
If I'm right about your proposed patch to pci_cfg_space_size_ext(), *bridge is pointing to the upper device of device *dev being 
checked. I understand the purpose, but I think this fails for my config that is :

LS1043 PCIe root -> PEX8112 PCIe-to-PCI bridge -> PMC slot connector -> PCI-to-PCIe bridge -> PCIe switch (4 ports) -> 4 PCIe 
devices (one on each port)

because :
- when pci_cfg_space_size_ext() is run on the 4 PCIe devices, *bridge is the PCIe switch which is not matching 
PCI_EXP_TYPE_PCI_BRIDGE. In this case *bridge should also be checked for the parent bus of the PCIe switch, and so on.
- when pci_cfg_space_size_ext() is run for the PCI-to-PCIe bridge, *bridge is the PEX8112 that is also not matching 
PCI_EXP_TYPE_PCI_BRIDGE but PCI_EXP_TYPE_PCIE_BRIDGE. This leads to a config access at offset 0x100 to the PCI-to-PCIe bridge, so a 
crash (because of the PEX8112)

I think setting a bit in bus_flags when creating a child bus is very efficient because once set it is automatically inherited by all 
child buses and then the only thing that pci_cfg_space_size() has to do for each device is to check for this bit. Also this 
PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag is actually a bus property that is compliant with the purpose of bus_flags.

I agree that pci_scan_bridge_extend() is already too complicated, so would you be okay to only add one line to it :
   pci_bus_set_compat_cfg_space(child);
and put all the code I suggested in this new function pci_bus_set_compat_cfg_space() ? (also supporting PCI-X Mode 2 devices)
Improvement : this function can return immediately if the child bus has already inherited the flag from its parent.

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

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

Le 30/04/2018 10:46, Gilles BULOZ a écrit :
> Le 27/04/2018 18:56, Bjorn Helgaas a écrit :
>> On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
>>> Le 27/04/2018 10:43, Ard Biesheuvel a écrit :
>>>> (add Bjorn and linux-pci)
>>>>
>>>> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
>>>>> Dear developers,
>>>>>
>>>>> I currently have two functional workarounds for this issue but
>>>>> would like to know which one you would recommend, if any :-) I'm
>>>>> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
>>>>> external abort" when booting because of a PCI config read during
>>>>> PCI scan.
>>>>>
>>>>> I'm using a custom hardware (based on LS1043ARDB) having a
>>>>> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
>>>>> slot for legacy devices. This bridge only supports PCI-Compatible
>>>>> config accesses (offset 0x00-0xFF).
>> I would guess the PEX8112 itself has 4K of config space, but it only
>> forwards 256 bytes of config space to the conventional PCI secondary
>> bus.
>>
>>>>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
>>>>> bridge plus PCIe devices behind.
>>>>>
>>>>> The problem occurs when the kernel probes the PCIe devices : as
>>>>> they are PCIe devices, the kernel does a PCI config read access
>>>>> at offset 0x100 to check if "PCIe extended capability registers"
>>>>> are accessible (see drivers/pci/probe.c, function
>>>>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
>>>>> bridge that is in the path reports an error to the CPU for this
>>>>> access, and it seems there's no way to disable that on this
>>>>> bridge.
>>>>>
>>>>> The first workaround I found was to patch
>>>>> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
>>>>> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
>>>>> error reporting. This only impacts an NXP part of the Linux
>>>>> kernel code, but I'm not sure this is a good idea (however it
>>>>> seems to be like that on Intel platforms where even MEM accesses
>>>>> to a no-device address return FF without any error).
>>>>>
>>>>> I've also tried another workaround that works : patch
>>>>> drivers/pci/probe.c to use bus_flags to remember if a bus is
>>>>> behind a bridge without extended address capability, to avoid PCi
>>>>> config read accesses at offset 0x100 in pci_cfg_space_size() /
>>>>> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
>>>>> probe method of Linux.
>>>>>
>>>>> Any Idea to properly handle that issue ?
>>>>>
>>>> This seems like a rather unusual configuration, but I guess that
>>>> if the first bridge/switch advertises its inability to support
>>>> extended config space accesses, we should not be performing them
>>>> on any of its subordinate buses. How does the PEX8112 advertise
>>>> this limitation?
>>>>
>>>> That said, I wonder if it is reasonable in the first place to
>>>> expect that a PCIe device works as expected passing through a
>>>> legacy PCI layer like that.
>>>>
>>> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
>>> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
>>> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
>>> support for PCI config offset >=0x100).
>> Sounds right to me.
>>
>>> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
>>> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
>>> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
>>> pci_cfg_space_size())
>> Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266MHZ
>> should be enough, but it shouldn't hurt to check for either
>> PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.
>>
>>> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
>>> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
>>> bridge without extended address capability to avoid PCi config
>>> accesses at offset >= 0x100. Thanks to this patch I now have a
>>> functional system with functional PCI/PCIe devices.
>> The patch seems like it's looking at the right things, but I don't
>> want to build it into pci_scan_bridge_extend() because that function
>> is much too complicated already.
>>
>> I'd rather build it into pci_cfg_space_size() or
>> pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
>> This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
>> that case, I think all 4K would be accessible on the PCI-X side.
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index ac91b6fd0bcd..d8b091f0bcd1 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>>    * pci_cfg_space_size - Get the configuration space size of the PCI device
>>    * @dev: PCI device
>>    *
>> - * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express devices
>> + * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Express devices
>>    * have 4096 bytes.  Even if the device is capable, that doesn't mean we can
>>    * access it.  Maybe we don't have a way to generate extended config space
>>    * accesses, or the device is behind a reverse Express bridge.  So we try
>> @@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>>    */
>>   static int pci_cfg_space_size_ext(struct pci_dev *dev)
>>   {
>> +    struct pci_dev *bridge = pci_upstream_bridge(dev);
>>       u32 status;
>>       int pos = PCI_CFG_SPACE_SIZE;
>>   +    if (bridge && pci_is_pcie(bridge) &&
>> +        pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
>> +        return PCI_CFG_SPACE_SIZE;
>> +
>>       if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
>>           return PCI_CFG_SPACE_SIZE;
>>       if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))
>>
>>> --- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000
>>> +++ include/linux/pci.h    2018-03-26 16:51:27.660000000 +0000
>>> @@ -193,6 +193,7 @@
>>>   enum pci_bus_flags {
>>>       PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>>>       PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
>>> +    PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>>>   };
>>>     /* These values come from the PCI Express Spec */
>>> --- drivers/pci/probe.c.orig    2018-01-22 09:29:52.000000000 +0000
>>> +++ drivers/pci/probe.c    2018-03-26 16:54:30.830000000 +0000
>>> @@ -827,6 +827,28 @@
>>>               child->primary = primary;
>>>               pci_bus_insert_busn_res(child, secondary, subordinate);
>>>               child->bridge_ctl = bctl;
>>> +
>>> +            {
>>> +                int pos;
>>> +                u32 status;
>>> +                bool pci_compat_cfg_space = false;
>>> +
>>> +                if (!pci_is_pcie(dev) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) == 
>>> PCI_EXP_TYPE_PCI_BRIDGE)) {
>>> +                    /* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse mode, we have to check for PCI-X 
>>> capabilities */
>>> +                    pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
>>> +                    if (pos) {
>>> +                        pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
>>> +                        if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
>>> +                            pci_compat_cfg_space = true;
>>> +                    } else {
>>> +                        pci_compat_cfg_space = true;
>>> +                    }
>>> +                    if (pci_compat_cfg_space) {
>>> +                        dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compatible config space\n", dev->vendor, 
>>> dev->device);
>>> +                        child->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>>> +                    }
>>> +                }
>>> +            }
>>>           }
>>>             cmax = pci_scan_child_bus(child);
>>> @@ -1098,6 +1120,11 @@
>>>               goto fail;
>>>       }
>>>   +    if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
>>> +        dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due to parent bus(es)\n", dev->vendor, dev->device);
>>> +        return PCI_CFG_SPACE_SIZE;
>>> +    }
>>> +
>>>       return pci_cfg_space_size_ext(dev);
>>>      fail:
> Bjorn,
> If I'm right about your proposed patch to pci_cfg_space_size_ext(), *bridge is pointing to the upper device of device *dev being 
> checked. I understand the purpose, but I think this fails for my config that is :
>
> LS1043 PCIe root -> PEX8112 PCIe-to-PCI bridge -> PMC slot connector -> PCI-to-PCIe bridge -> PCIe switch (4 ports) -> 4 PCIe 
> devices (one on each port)
>
> because :
> - when pci_cfg_space_size_ext() is run on the 4 PCIe devices, *bridge is the PCIe switch which is not matching 
> PCI_EXP_TYPE_PCI_BRIDGE. In this case *bridge should also be checked for the parent bus of the PCIe switch, and so on.
> - when pci_cfg_space_size_ext() is run for the PCI-to-PCIe bridge, *bridge is the PEX8112 that is also not matching 
> PCI_EXP_TYPE_PCI_BRIDGE but PCI_EXP_TYPE_PCIE_BRIDGE. This leads to a config access at offset 0x100 to the PCI-to-PCIe bridge, so 
> a crash (because of the PEX8112)
>
> I think setting a bit in bus_flags when creating a child bus is very efficient because once set it is automatically inherited by 
> all child buses and then the only thing that pci_cfg_space_size() has to do for each device is to check for this bit. Also this 
> PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag is actually a bus property that is compliant with the purpose of bus_flags.
>
> I agree that pci_scan_bridge_extend() is already too complicated, so would you be okay to only add one line to it :
>   pci_bus_set_compat_cfg_space(child);
> and put all the code I suggested in this new function pci_bus_set_compat_cfg_space() ? (also supporting PCI-X Mode 2 devices)
> Improvement : this function can return immediately if the child bus has already inherited the flag from its parent.
>
I mean something like the attached patch I tested this morning...
Sorry, this is for old kernel 4.1.35 but just to clarify what I propose (also applies to 4.16.6 by changing value of 
PCI_BUS_FLAGS_COMPAT_CFG_SPACE in pci.h to 8).

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cfgspace2_4.1.35.patch --]
[-- Type: text/x-patch; name=cfgspace2_4.1.35.patch, Size: 2201 bytes --]

--- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
+++ include/linux/pci.h	2018-04-30 09:50:57.660000000 +0000
@@ -193,6 +193,7 @@
 enum pci_bus_flags {
 	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
 	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
+	PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
 };
 
 /* These values come from the PCI Express Spec */
--- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
+++ drivers/pci/probe.c	2018-04-30 13:29:50.600000000 +0000
@@ -754,6 +754,35 @@
 					 PCI_EXP_RTCTL_CRSSVE);
 }
 
+static void pci_bus_check_compat_cfg_space(struct pci_bus *bus)
+{
+	struct pci_dev *dev = bus->self;
+	bool pci_compat_cfg_space = false;
+	int pos;
+	u32 status;
+
+	if (bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
+		return;
+
+	if (!pci_is_pcie(dev) || /* PCI/PCI bridge */
+	    (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */
+	    (pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PCIe/PCI bridge in reverse mode */
+		pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
+		if (pos) {
+			pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
+			if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
+				pci_compat_cfg_space = true;
+		} else {
+			pci_compat_cfg_space = true;
+		}
+		if (pci_compat_cfg_space) {
+			dev_info(&dev->dev, "bus %02x limited to PCI-Compatible config space\n",
+				 bus->number);
+			bus->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
+		}
+	}
+}
+
 /*
  * If it's a bridge, configure it and scan the bus behind it.
  * For CardBus bridges, we don't scan behind as the devices will
@@ -827,6 +856,7 @@
 			child->primary = primary;
 			pci_bus_insert_busn_res(child, secondary, subordinate);
 			child->bridge_ctl = bctl;
+			pci_bus_check_compat_cfg_space(child);
 		}
 
 		cmax = pci_scan_child_bus(child);
@@ -1084,6 +1114,9 @@
 	u32 status;
 	u16 class;
 
+	if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
+		return PCI_CFG_SPACE_SIZE;
+
 	class = dev->class >> 8;
 	if (class == PCI_CLASS_BRIDGE_HOST)
 		return pci_cfg_space_size_ext(dev);

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Le 30/04/2018 10:46, Gilles BULOZ a ?crit :
> Le 27/04/2018 18:56, Bjorn Helgaas a ?crit :
>> On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
>>> Le 27/04/2018 10:43, Ard Biesheuvel a ?crit :
>>>> (add Bjorn and linux-pci)
>>>>
>>>> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
>>>>> Dear developers,
>>>>>
>>>>> I currently have two functional workarounds for this issue but
>>>>> would like to know which one you would recommend, if any :-) I'm
>>>>> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
>>>>> external abort" when booting because of a PCI config read during
>>>>> PCI scan.
>>>>>
>>>>> I'm using a custom hardware (based on LS1043ARDB) having a
>>>>> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
>>>>> slot for legacy devices. This bridge only supports PCI-Compatible
>>>>> config accesses (offset 0x00-0xFF).
>> I would guess the PEX8112 itself has 4K of config space, but it only
>> forwards 256 bytes of config space to the conventional PCI secondary
>> bus.
>>
>>>>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
>>>>> bridge plus PCIe devices behind.
>>>>>
>>>>> The problem occurs when the kernel probes the PCIe devices : as
>>>>> they are PCIe devices, the kernel does a PCI config read access
>>>>> at offset 0x100 to check if "PCIe extended capability registers"
>>>>> are accessible (see drivers/pci/probe.c, function
>>>>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
>>>>> bridge that is in the path reports an error to the CPU for this
>>>>> access, and it seems there's no way to disable that on this
>>>>> bridge.
>>>>>
>>>>> The first workaround I found was to patch
>>>>> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
>>>>> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
>>>>> error reporting. This only impacts an NXP part of the Linux
>>>>> kernel code, but I'm not sure this is a good idea (however it
>>>>> seems to be like that on Intel platforms where even MEM accesses
>>>>> to a no-device address return FF without any error).
>>>>>
>>>>> I've also tried another workaround that works : patch
>>>>> drivers/pci/probe.c to use bus_flags to remember if a bus is
>>>>> behind a bridge without extended address capability, to avoid PCi
>>>>> config read accesses at offset 0x100 in pci_cfg_space_size() /
>>>>> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
>>>>> probe method of Linux.
>>>>>
>>>>> Any Idea to properly handle that issue ?
>>>>>
>>>> This seems like a rather unusual configuration, but I guess that
>>>> if the first bridge/switch advertises its inability to support
>>>> extended config space accesses, we should not be performing them
>>>> on any of its subordinate buses. How does the PEX8112 advertise
>>>> this limitation?
>>>>
>>>> That said, I wonder if it is reasonable in the first place to
>>>> expect that a PCIe device works as expected passing through a
>>>> legacy PCI layer like that.
>>>>
>>> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
>>> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
>>> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
>>> support for PCI config offset >=0x100).
>> Sounds right to me.
>>
>>> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
>>> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
>>> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
>>> pci_cfg_space_size())
>> Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266MHZ
>> should be enough, but it shouldn't hurt to check for either
>> PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.
>>
>>> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
>>> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
>>> bridge without extended address capability to avoid PCi config
>>> accesses at offset >= 0x100. Thanks to this patch I now have a
>>> functional system with functional PCI/PCIe devices.
>> The patch seems like it's looking at the right things, but I don't
>> want to build it into pci_scan_bridge_extend() because that function
>> is much too complicated already.
>>
>> I'd rather build it into pci_cfg_space_size() or
>> pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
>> This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
>> that case, I think all 4K would be accessible on the PCI-X side.
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index ac91b6fd0bcd..d8b091f0bcd1 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>>    * pci_cfg_space_size - Get the configuration space size of the PCI device
>>    * @dev: PCI device
>>    *
>> - * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express devices
>> + * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Express devices
>>    * have 4096 bytes.  Even if the device is capable, that doesn't mean we can
>>    * access it.  Maybe we don't have a way to generate extended config space
>>    * accesses, or the device is behind a reverse Express bridge.  So we try
>> @@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>>    */
>>   static int pci_cfg_space_size_ext(struct pci_dev *dev)
>>   {
>> +    struct pci_dev *bridge = pci_upstream_bridge(dev);
>>       u32 status;
>>       int pos = PCI_CFG_SPACE_SIZE;
>>   +    if (bridge && pci_is_pcie(bridge) &&
>> +        pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
>> +        return PCI_CFG_SPACE_SIZE;
>> +
>>       if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
>>           return PCI_CFG_SPACE_SIZE;
>>       if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))
>>
>>> --- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000
>>> +++ include/linux/pci.h    2018-03-26 16:51:27.660000000 +0000
>>> @@ -193,6 +193,7 @@
>>>   enum pci_bus_flags {
>>>       PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>>>       PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
>>> +    PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>>>   };
>>>     /* These values come from the PCI Express Spec */
>>> --- drivers/pci/probe.c.orig    2018-01-22 09:29:52.000000000 +0000
>>> +++ drivers/pci/probe.c    2018-03-26 16:54:30.830000000 +0000
>>> @@ -827,6 +827,28 @@
>>>               child->primary = primary;
>>>               pci_bus_insert_busn_res(child, secondary, subordinate);
>>>               child->bridge_ctl = bctl;
>>> +
>>> +            {
>>> +                int pos;
>>> +                u32 status;
>>> +                bool pci_compat_cfg_space = false;
>>> +
>>> +                if (!pci_is_pcie(dev) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) == 
>>> PCI_EXP_TYPE_PCI_BRIDGE)) {
>>> +                    /* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse mode, we have to check for PCI-X 
>>> capabilities */
>>> +                    pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
>>> +                    if (pos) {
>>> +                        pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
>>> +                        if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
>>> +                            pci_compat_cfg_space = true;
>>> +                    } else {
>>> +                        pci_compat_cfg_space = true;
>>> +                    }
>>> +                    if (pci_compat_cfg_space) {
>>> +                        dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compatible config space\n", dev->vendor, 
>>> dev->device);
>>> +                        child->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>>> +                    }
>>> +                }
>>> +            }
>>>           }
>>>             cmax = pci_scan_child_bus(child);
>>> @@ -1098,6 +1120,11 @@
>>>               goto fail;
>>>       }
>>>   +    if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
>>> +        dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due to parent bus(es)\n", dev->vendor, dev->device);
>>> +        return PCI_CFG_SPACE_SIZE;
>>> +    }
>>> +
>>>       return pci_cfg_space_size_ext(dev);
>>>      fail:
> Bjorn,
> If I'm right about your proposed patch to pci_cfg_space_size_ext(), *bridge is pointing to the upper device of device *dev being 
> checked. I understand the purpose, but I think this fails for my config that is :
>
> LS1043 PCIe root -> PEX8112 PCIe-to-PCI bridge -> PMC slot connector -> PCI-to-PCIe bridge -> PCIe switch (4 ports) -> 4 PCIe 
> devices (one on each port)
>
> because :
> - when pci_cfg_space_size_ext() is run on the 4 PCIe devices, *bridge is the PCIe switch which is not matching 
> PCI_EXP_TYPE_PCI_BRIDGE. In this case *bridge should also be checked for the parent bus of the PCIe switch, and so on.
> - when pci_cfg_space_size_ext() is run for the PCI-to-PCIe bridge, *bridge is the PEX8112 that is also not matching 
> PCI_EXP_TYPE_PCI_BRIDGE but PCI_EXP_TYPE_PCIE_BRIDGE. This leads to a config access at offset 0x100 to the PCI-to-PCIe bridge, so 
> a crash (because of the PEX8112)
>
> I think setting a bit in bus_flags when creating a child bus is very efficient because once set it is automatically inherited by 
> all child buses and then the only thing that pci_cfg_space_size() has to do for each device is to check for this bit. Also this 
> PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag is actually a bus property that is compliant with the purpose of bus_flags.
>
> I agree that pci_scan_bridge_extend() is already too complicated, so would you be okay to only add one line to it :
>   pci_bus_set_compat_cfg_space(child);
> and put all the code I suggested in this new function pci_bus_set_compat_cfg_space() ? (also supporting PCI-X Mode 2 devices)
> Improvement : this function can return immediately if the child bus has already inherited the flag from its parent.
>
I mean something like the attached patch I tested this morning...
Sorry, this is for old kernel 4.1.35 but just to clarify what I propose (also applies to 4.16.6 by changing value of 
PCI_BUS_FLAGS_COMPAT_CFG_SPACE in pci.h to 8).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cfgspace2_4.1.35.patch
Type: text/x-patch
Size: 2201 bytes
Desc: cfgspace2_4.1.35.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180430/de70abf8/attachment-0001.bin>

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Bjorn Helgaas]

On Mon, Apr 30, 2018 at 01:36:53PM +0000, Gilles Buloz wrote:
> Le 30/04/2018 10:46, Gilles BULOZ a =E9crit :
> > Le 27/04/2018 18:56, Bjorn Helgaas a =E9crit :
> >> On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
> >>> Le 27/04/2018 10:43, Ard Biesheuvel a =E9crit :
> >>>> (add Bjorn and linux-pci)
> >>>>
> >>>> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> w=
rote:
> >>>>> Dear developers,
> >>>>>
> >>>>> I currently have two functional workarounds for this issue but
> >>>>> would like to know which one you would recommend, if any :-) I'm
> >>>>> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
> >>>>> external abort" when booting because of a PCI config read during
> >>>>> PCI scan.
> >>>>>
> >>>>> I'm using a custom hardware (based on LS1043ARDB) having a
> >>>>> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
> >>>>> slot for legacy devices. This bridge only supports PCI-Compatible
> >>>>> config accesses (offset 0x00-0xFF).
> >> I would guess the PEX8112 itself has 4K of config space, but it only
> >> forwards 256 bytes of config space to the conventional PCI secondary
> >> bus.
> >>
> >>>>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
> >>>>> bridge plus PCIe devices behind.
> >>>>>
> >>>>> The problem occurs when the kernel probes the PCIe devices : as
> >>>>> they are PCIe devices, the kernel does a PCI config read access
> >>>>> at offset 0x100 to check if "PCIe extended capability registers"
> >>>>> are accessible (see drivers/pci/probe.c, function
> >>>>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
> >>>>> bridge that is in the path reports an error to the CPU for this
> >>>>> access, and it seems there's no way to disable that on this
> >>>>> bridge.
> >>>>>
> >>>>> The first workaround I found was to patch
> >>>>> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
> >>>>> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
> >>>>> error reporting. This only impacts an NXP part of the Linux
> >>>>> kernel code, but I'm not sure this is a good idea (however it
> >>>>> seems to be like that on Intel platforms where even MEM accesses
> >>>>> to a no-device address return FF without any error).
> >>>>>
> >>>>> I've also tried another workaround that works : patch
> >>>>> drivers/pci/probe.c to use bus_flags to remember if a bus is
> >>>>> behind a bridge without extended address capability, to avoid PCi
> >>>>> config read accesses at offset 0x100 in pci_cfg_space_size() /
> >>>>> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
> >>>>> probe method of Linux.
> >>>>>
> >>>>> Any Idea to properly handle that issue ?
> >>>>>
> >>>> This seems like a rather unusual configuration, but I guess that
> >>>> if the first bridge/switch advertises its inability to support
> >>>> extended config space accesses, we should not be performing them
> >>>> on any of its subordinate buses. How does the PEX8112 advertise
> >>>> this limitation?
> >>>>
> >>>> That said, I wonder if it is reasonable in the first place to
> >>>> expect that a PCIe device works as expected passing through a
> >>>> legacy PCI layer like that.
> >>>>
> >>> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
> >>> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
> >>> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
> >>> support for PCI config offset >=3D0x100).
> >> Sounds right to me.
> >>
> >>> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
> >>> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
> >>> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
> >>> pci_cfg_space_size())
> >> Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266M=
HZ
> >> should be enough, but it shouldn't hurt to check for either
> >> PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.
> >>
> >>> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
> >>> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
> >>> bridge without extended address capability to avoid PCi config
> >>> accesses at offset >=3D 0x100. Thanks to this patch I now have a
> >>> functional system with functional PCI/PCIe devices.
> >> The patch seems like it's looking at the right things, but I don't
> >> want to build it into pci_scan_bridge_extend() because that function
> >> is much too complicated already.
> >>
> >> I'd rather build it into pci_cfg_space_size() or
> >> pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
> >> This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
> >> that case, I think all 4K would be accessible on the PCI-X side.
> >>
> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> >> index ac91b6fd0bcd..d8b091f0bcd1 100644
> >> --- a/drivers/pci/probe.c
> >> +++ b/drivers/pci/probe.c
> >> @@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_de=
v *dev)
> >>    * pci_cfg_space_size - Get the configuration space size of the PCI =
device
> >>    * @dev: PCI device
> >>    *
> >> - * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express de=
vices
> >> + * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Expre=
ss devices
> >>    * have 4096 bytes.  Even if the device is capable, that doesn't mea=
n we can
> >>    * access it.  Maybe we don't have a way to generate extended config=
 space
> >>    * accesses, or the device is behind a reverse Express bridge.  So w=
e try
> >> @@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_d=
ev *dev)
> >>    */
> >>   static int pci_cfg_space_size_ext(struct pci_dev *dev)
> >>   {
> >> +    struct pci_dev *bridge =3D pci_upstream_bridge(dev);
> >>       u32 status;
> >>       int pos =3D PCI_CFG_SPACE_SIZE;
> >>   +    if (bridge && pci_is_pcie(bridge) &&
> >> +        pci_pcie_type(bridge) =3D=3D PCI_EXP_TYPE_PCI_BRIDGE)
> >> +        return PCI_CFG_SPACE_SIZE;
> >> +
> >>       if (pci_read_config_dword(dev, pos, &status) !=3D PCIBIOS_SUCCES=
SFUL)
> >>           return PCI_CFG_SPACE_SIZE;
> >>       if (status =3D=3D 0xffffffff || pci_ext_cfg_is_aliased(dev))
> >>
> >>> --- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000
> >>> +++ include/linux/pci.h    2018-03-26 16:51:27.660000000 +0000
> >>> @@ -193,6 +193,7 @@
> >>>   enum pci_bus_flags {
> >>>       PCI_BUS_FLAGS_NO_MSI   =3D (__force pci_bus_flags_t) 1,
> >>>       PCI_BUS_FLAGS_NO_MMRBC =3D (__force pci_bus_flags_t) 2,
> >>> +    PCI_BUS_FLAGS_COMPAT_CFG_SPACE =3D (__force pci_bus_flags_t) 4,
> >>>   };
> >>>     /* These values come from the PCI Express Spec */
> >>> --- drivers/pci/probe.c.orig    2018-01-22 09:29:52.000000000 +0000
> >>> +++ drivers/pci/probe.c    2018-03-26 16:54:30.830000000 +0000
> >>> @@ -827,6 +827,28 @@
> >>>               child->primary =3D primary;
> >>>               pci_bus_insert_busn_res(child, secondary, subordinate);
> >>>               child->bridge_ctl =3D bctl;
> >>> +
> >>> +            {
> >>> +                int pos;
> >>> +                u32 status;
> >>> +                bool pci_compat_cfg_space =3D false;
> >>> +
> >>> +                if (!pci_is_pcie(dev) || (pci_pcie_type(dev) =3D=3D =
PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) =3D=3D =

> >>> PCI_EXP_TYPE_PCI_BRIDGE)) {
> >>> +                    /* for PCI/PCI bridges, or PCIe/PCI bridge in fo=
rward or reverse mode, we have to check for PCI-X =

> >>> capabilities */
> >>> +                    pos =3D pci_find_capability(dev, PCI_CAP_ID_PCIX=
);
> >>> +                    if (pos) {
> >>> +                        pci_read_config_dword(dev, pos + PCI_X_STATU=
S, &status);
> >>> +                        if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_=
STATUS_533MHZ)))
> >>> +                            pci_compat_cfg_space =3D true;
> >>> +                    } else {
> >>> +                        pci_compat_cfg_space =3D true;
> >>> +                    }
> >>> +                    if (pci_compat_cfg_space) {
> >>> +                        dev_info(&dev->dev, "[%04x:%04x] Child bus l=
imited to PCI-Compatible config space\n", dev->vendor, =

> >>> dev->device);
> >>> +                        child->bus_flags |=3D PCI_BUS_FLAGS_COMPAT_C=
FG_SPACE;
> >>> +                    }
> >>> +                }
> >>> +            }
> >>>           }
> >>>             cmax =3D pci_scan_child_bus(child);
> >>> @@ -1098,6 +1120,11 @@
> >>>               goto fail;
> >>>       }
> >>>   +    if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
> >>> +        dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space=
 only due to parent bus(es)\n", dev->vendor, dev->device);
> >>> +        return PCI_CFG_SPACE_SIZE;
> >>> +    }
> >>> +
> >>>       return pci_cfg_space_size_ext(dev);
> >>>      fail:
> > Bjorn,
> > If I'm right about your proposed patch to
> > pci_cfg_space_size_ext(), *bridge is pointing to the upper device
> > of device *dev being checked. I understand the purpose, but I
> > think this fails for my config that is :
> >
> > LS1043 PCIe root -> PEX8112 PCIe-to-PCI bridge -> PMC slot connector ->=
 PCI-to-PCIe bridge -> PCIe switch (4 ports) -> 4 PCIe =

> > devices (one on each port)
> >
> > because :
> > - when pci_cfg_space_size_ext() is run on the 4 PCIe devices,
> > *bridge is the PCIe switch which is not matching
> > PCI_EXP_TYPE_PCI_BRIDGE. In this case *bridge should also be
> > checked for the parent bus of the PCIe switch, and so on.
> > - when pci_cfg_space_size_ext() is run for the PCI-to-PCIe bridge,
> > *bridge is the PEX8112 that is also not matching
> > PCI_EXP_TYPE_PCI_BRIDGE but PCI_EXP_TYPE_PCIE_BRIDGE. This leads
> > to a config access at offset 0x100 to the PCI-to-PCIe bridge, so a
> > crash (because of the PEX8112)
> >
> > I think setting a bit in bus_flags when creating a child bus is
> > very efficient because once set it is automatically inherited by
> > all child buses and then the only thing that pci_cfg_space_size()
> > has to do for each device is to check for this bit. Also this
> > PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag is actually a bus property
> > that is compliant with the purpose of bus_flags.

Yeah, it needs to be inherited somehow, and I don't like the idea of
traversing up the tree, so I prefer your idea.  Although I don't
actually see the inheritance in the patch below -- I thought there
would be something like this:

  dev =3D bus->self;
  parent_bus =3D dev->bus;
  if (parent_bus && parent_bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
    bus->bus_flags |=3D PCI_BUS_FLAGS_COMPAT_CFG_SPACE;

pci_scan_bridge_extend() calls pci_add_new_bus() from two places.  You
added a call to pci_bus_check_compat_cfg_space() at one of them, and
it's not obvious why we wouldn't need it at the other place, too.

Can you set this up in pci_alloc_child_bus()?  If you can put it
there, it would be clear that every time we allocate a secondary bus,
we figure out whether extended config space is accessible on that bus.

That doesn't cover the root bus case, where we currently assume the
host bridge can generate config accesses to all config space supported
by devices on the root bus.  But we don't have a problem there, so I
guess we don't need to worry about it now.

If you can put it in pci_alloc_child_bus(), could you make your new
function return a boolean, e.g., pci_bus_ext_cfg_accessible(), or
similar, and then use the result to set the
PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag?  Names like "*_check_*()" don't
tell the reader much about what's happening.

> > I agree that pci_scan_bridge_extend() is already too complicated,
> > so would you be okay to only add one line to it :
> >   pci_bus_set_compat_cfg_space(child);
> > and put all the code I suggested in this new function
> > pci_bus_set_compat_cfg_space() ? (also supporting PCI-X Mode 2
> > devices)
> >
> > Improvement : this function can return immediately if the child
> > bus has already inherited the flag from its parent.

> I mean something like the attached patch I tested this morning...
> Sorry, this is for old kernel 4.1.35 but just to clarify what I
> propose (also applies to 4.16.6 by changing value of
> PCI_BUS_FLAGS_COMPAT_CFG_SPACE in pci.h to 8).

> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
> +++ include/linux/pci.h	2018-04-30 09:50:57.660000000 +0000
> @@ -193,6 +193,7 @@
>  enum pci_bus_flags {
>  	PCI_BUS_FLAGS_NO_MSI   =3D (__force pci_bus_flags_t) 1,
>  	PCI_BUS_FLAGS_NO_MMRBC =3D (__force pci_bus_flags_t) 2,
> +	PCI_BUS_FLAGS_COMPAT_CFG_SPACE =3D (__force pci_bus_flags_t) 4,
>  };
>  =

>  /* These values come from the PCI Express Spec */
> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
> +++ drivers/pci/probe.c	2018-04-30 13:29:50.600000000 +0000
> @@ -754,6 +754,35 @@
>  					 PCI_EXP_RTCTL_CRSSVE);
>  }
>  =

> +static void pci_bus_check_compat_cfg_space(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev =3D bus->self;
> +	bool pci_compat_cfg_space =3D false;
> +	int pos;
> +	u32 status;
> +
> +	if (bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
> +		return;
> +
> +	if (!pci_is_pcie(dev) || /* PCI/PCI bridge */
> +	    (pci_pcie_type(dev) =3D=3D PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI=
 bridge in forward mode */
> +	    (pci_pcie_type(dev) =3D=3D PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PCIe/PCI=
 bridge in reverse mode */
> +		pos =3D pci_find_capability(dev, PCI_CAP_ID_PCIX);
> +		if (pos) {
> +			pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
> +			if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
> +				pci_compat_cfg_space =3D true;
> +		} else {
> +			pci_compat_cfg_space =3D true;
> +		}
> +		if (pci_compat_cfg_space) {
> +			dev_info(&dev->dev, "bus %02x limited to PCI-Compatible config space\=
n",
> +				 bus->number);
> +			bus->bus_flags |=3D PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
> +		}
> +	}
> +}
> +
>  /*
>   * If it's a bridge, configure it and scan the bus behind it.
>   * For CardBus bridges, we don't scan behind as the devices will
> @@ -827,6 +856,7 @@
>  			child->primary =3D primary;
>  			pci_bus_insert_busn_res(child, secondary, subordinate);
>  			child->bridge_ctl =3D bctl;
> +			pci_bus_check_compat_cfg_space(child);
>  		}
>  =

>  		cmax =3D pci_scan_child_bus(child);
> @@ -1084,6 +1114,9 @@
>  	u32 status;
>  	u16 class;
>  =

> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
> +		return PCI_CFG_SPACE_SIZE;
> +
>  	class =3D dev->class >> 8;
>  	if (class =3D=3D PCI_CLASS_BRIDGE_HOST)
>  		return pci_cfg_space_size_ext(dev);

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

LS1043A : "synchronous abort" at boot due to PCI config read

[Bjorn Helgaas]

On Mon, Apr 30, 2018 at 01:36:53PM +0000, Gilles Buloz wrote:
> Le 30/04/2018 10:46, Gilles BULOZ a ?crit :
> > Le 27/04/2018 18:56, Bjorn Helgaas a ?crit :
> >> On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
> >>> Le 27/04/2018 10:43, Ard Biesheuvel a ?crit :
> >>>> (add Bjorn and linux-pci)
> >>>>
> >>>> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
> >>>>> Dear developers,
> >>>>>
> >>>>> I currently have two functional workarounds for this issue but
> >>>>> would like to know which one you would recommend, if any :-) I'm
> >>>>> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
> >>>>> external abort" when booting because of a PCI config read during
> >>>>> PCI scan.
> >>>>>
> >>>>> I'm using a custom hardware (based on LS1043ARDB) having a
> >>>>> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
> >>>>> slot for legacy devices. This bridge only supports PCI-Compatible
> >>>>> config accesses (offset 0x00-0xFF).
> >> I would guess the PEX8112 itself has 4K of config space, but it only
> >> forwards 256 bytes of config space to the conventional PCI secondary
> >> bus.
> >>
> >>>>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
> >>>>> bridge plus PCIe devices behind.
> >>>>>
> >>>>> The problem occurs when the kernel probes the PCIe devices : as
> >>>>> they are PCIe devices, the kernel does a PCI config read access
> >>>>> at offset 0x100 to check if "PCIe extended capability registers"
> >>>>> are accessible (see drivers/pci/probe.c, function
> >>>>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
> >>>>> bridge that is in the path reports an error to the CPU for this
> >>>>> access, and it seems there's no way to disable that on this
> >>>>> bridge.
> >>>>>
> >>>>> The first workaround I found was to patch
> >>>>> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
> >>>>> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
> >>>>> error reporting. This only impacts an NXP part of the Linux
> >>>>> kernel code, but I'm not sure this is a good idea (however it
> >>>>> seems to be like that on Intel platforms where even MEM accesses
> >>>>> to a no-device address return FF without any error).
> >>>>>
> >>>>> I've also tried another workaround that works : patch
> >>>>> drivers/pci/probe.c to use bus_flags to remember if a bus is
> >>>>> behind a bridge without extended address capability, to avoid PCi
> >>>>> config read accesses at offset 0x100 in pci_cfg_space_size() /
> >>>>> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
> >>>>> probe method of Linux.
> >>>>>
> >>>>> Any Idea to properly handle that issue ?
> >>>>>
> >>>> This seems like a rather unusual configuration, but I guess that
> >>>> if the first bridge/switch advertises its inability to support
> >>>> extended config space accesses, we should not be performing them
> >>>> on any of its subordinate buses. How does the PEX8112 advertise
> >>>> this limitation?
> >>>>
> >>>> That said, I wonder if it is reasonable in the first place to
> >>>> expect that a PCIe device works as expected passing through a
> >>>> legacy PCI layer like that.
> >>>>
> >>> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
> >>> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
> >>> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
> >>> support for PCI config offset >=0x100).
> >> Sounds right to me.
> >>
> >>> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
> >>> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
> >>> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
> >>> pci_cfg_space_size())
> >> Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266MHZ
> >> should be enough, but it shouldn't hurt to check for either
> >> PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.
> >>
> >>> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
> >>> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
> >>> bridge without extended address capability to avoid PCi config
> >>> accesses at offset >= 0x100. Thanks to this patch I now have a
> >>> functional system with functional PCI/PCIe devices.
> >> The patch seems like it's looking at the right things, but I don't
> >> want to build it into pci_scan_bridge_extend() because that function
> >> is much too complicated already.
> >>
> >> I'd rather build it into pci_cfg_space_size() or
> >> pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
> >> This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
> >> that case, I think all 4K would be accessible on the PCI-X side.
> >>
> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> >> index ac91b6fd0bcd..d8b091f0bcd1 100644
> >> --- a/drivers/pci/probe.c
> >> +++ b/drivers/pci/probe.c
> >> @@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
> >>    * pci_cfg_space_size - Get the configuration space size of the PCI device
> >>    * @dev: PCI device
> >>    *
> >> - * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express devices
> >> + * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Express devices
> >>    * have 4096 bytes.  Even if the device is capable, that doesn't mean we can
> >>    * access it.  Maybe we don't have a way to generate extended config space
> >>    * accesses, or the device is behind a reverse Express bridge.  So we try
> >> @@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
> >>    */
> >>   static int pci_cfg_space_size_ext(struct pci_dev *dev)
> >>   {
> >> +    struct pci_dev *bridge = pci_upstream_bridge(dev);
> >>       u32 status;
> >>       int pos = PCI_CFG_SPACE_SIZE;
> >>   +    if (bridge && pci_is_pcie(bridge) &&
> >> +        pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
> >> +        return PCI_CFG_SPACE_SIZE;
> >> +
> >>       if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
> >>           return PCI_CFG_SPACE_SIZE;
> >>       if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))
> >>
> >>> --- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000
> >>> +++ include/linux/pci.h    2018-03-26 16:51:27.660000000 +0000
> >>> @@ -193,6 +193,7 @@
> >>>   enum pci_bus_flags {
> >>>       PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
> >>>       PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
> >>> +    PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
> >>>   };
> >>>     /* These values come from the PCI Express Spec */
> >>> --- drivers/pci/probe.c.orig    2018-01-22 09:29:52.000000000 +0000
> >>> +++ drivers/pci/probe.c    2018-03-26 16:54:30.830000000 +0000
> >>> @@ -827,6 +827,28 @@
> >>>               child->primary = primary;
> >>>               pci_bus_insert_busn_res(child, secondary, subordinate);
> >>>               child->bridge_ctl = bctl;
> >>> +
> >>> +            {
> >>> +                int pos;
> >>> +                u32 status;
> >>> +                bool pci_compat_cfg_space = false;
> >>> +
> >>> +                if (!pci_is_pcie(dev) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) == 
> >>> PCI_EXP_TYPE_PCI_BRIDGE)) {
> >>> +                    /* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse mode, we have to check for PCI-X 
> >>> capabilities */
> >>> +                    pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> >>> +                    if (pos) {
> >>> +                        pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
> >>> +                        if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
> >>> +                            pci_compat_cfg_space = true;
> >>> +                    } else {
> >>> +                        pci_compat_cfg_space = true;
> >>> +                    }
> >>> +                    if (pci_compat_cfg_space) {
> >>> +                        dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compatible config space\n", dev->vendor, 
> >>> dev->device);
> >>> +                        child->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
> >>> +                    }
> >>> +                }
> >>> +            }
> >>>           }
> >>>             cmax = pci_scan_child_bus(child);
> >>> @@ -1098,6 +1120,11 @@
> >>>               goto fail;
> >>>       }
> >>>   +    if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
> >>> +        dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due to parent bus(es)\n", dev->vendor, dev->device);
> >>> +        return PCI_CFG_SPACE_SIZE;
> >>> +    }
> >>> +
> >>>       return pci_cfg_space_size_ext(dev);
> >>>      fail:
> > Bjorn,
> > If I'm right about your proposed patch to
> > pci_cfg_space_size_ext(), *bridge is pointing to the upper device
> > of device *dev being checked. I understand the purpose, but I
> > think this fails for my config that is :
> >
> > LS1043 PCIe root -> PEX8112 PCIe-to-PCI bridge -> PMC slot connector -> PCI-to-PCIe bridge -> PCIe switch (4 ports) -> 4 PCIe 
> > devices (one on each port)
> >
> > because :
> > - when pci_cfg_space_size_ext() is run on the 4 PCIe devices,
> > *bridge is the PCIe switch which is not matching
> > PCI_EXP_TYPE_PCI_BRIDGE. In this case *bridge should also be
> > checked for the parent bus of the PCIe switch, and so on.
> > - when pci_cfg_space_size_ext() is run for the PCI-to-PCIe bridge,
> > *bridge is the PEX8112 that is also not matching
> > PCI_EXP_TYPE_PCI_BRIDGE but PCI_EXP_TYPE_PCIE_BRIDGE. This leads
> > to a config access at offset 0x100 to the PCI-to-PCIe bridge, so a
> > crash (because of the PEX8112)
> >
> > I think setting a bit in bus_flags when creating a child bus is
> > very efficient because once set it is automatically inherited by
> > all child buses and then the only thing that pci_cfg_space_size()
> > has to do for each device is to check for this bit. Also this
> > PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag is actually a bus property
> > that is compliant with the purpose of bus_flags.

Yeah, it needs to be inherited somehow, and I don't like the idea of
traversing up the tree, so I prefer your idea.  Although I don't
actually see the inheritance in the patch below -- I thought there
would be something like this:

  dev = bus->self;
  parent_bus = dev->bus;
  if (parent_bus && parent_bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
    bus->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;

pci_scan_bridge_extend() calls pci_add_new_bus() from two places.  You
added a call to pci_bus_check_compat_cfg_space() at one of them, and
it's not obvious why we wouldn't need it at the other place, too.

Can you set this up in pci_alloc_child_bus()?  If you can put it
there, it would be clear that every time we allocate a secondary bus,
we figure out whether extended config space is accessible on that bus.

That doesn't cover the root bus case, where we currently assume the
host bridge can generate config accesses to all config space supported
by devices on the root bus.  But we don't have a problem there, so I
guess we don't need to worry about it now.

If you can put it in pci_alloc_child_bus(), could you make your new
function return a boolean, e.g., pci_bus_ext_cfg_accessible(), or
similar, and then use the result to set the
PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag?  Names like "*_check_*()" don't
tell the reader much about what's happening.

> > I agree that pci_scan_bridge_extend() is already too complicated,
> > so would you be okay to only add one line to it :
> >   pci_bus_set_compat_cfg_space(child);
> > and put all the code I suggested in this new function
> > pci_bus_set_compat_cfg_space() ? (also supporting PCI-X Mode 2
> > devices)
> >
> > Improvement : this function can return immediately if the child
> > bus has already inherited the flag from its parent.

> I mean something like the attached patch I tested this morning...
> Sorry, this is for old kernel 4.1.35 but just to clarify what I
> propose (also applies to 4.16.6 by changing value of
> PCI_BUS_FLAGS_COMPAT_CFG_SPACE in pci.h to 8).

> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
> +++ include/linux/pci.h	2018-04-30 09:50:57.660000000 +0000
> @@ -193,6 +193,7 @@
>  enum pci_bus_flags {
>  	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>  	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
> +	PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>  };
>  
>  /* These values come from the PCI Express Spec */
> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
> +++ drivers/pci/probe.c	2018-04-30 13:29:50.600000000 +0000
> @@ -754,6 +754,35 @@
>  					 PCI_EXP_RTCTL_CRSSVE);
>  }
>  
> +static void pci_bus_check_compat_cfg_space(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev = bus->self;
> +	bool pci_compat_cfg_space = false;
> +	int pos;
> +	u32 status;
> +
> +	if (bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
> +		return;
> +
> +	if (!pci_is_pcie(dev) || /* PCI/PCI bridge */
> +	    (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */
> +	    (pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PCIe/PCI bridge in reverse mode */
> +		pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> +		if (pos) {
> +			pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
> +			if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
> +				pci_compat_cfg_space = true;
> +		} else {
> +			pci_compat_cfg_space = true;
> +		}
> +		if (pci_compat_cfg_space) {
> +			dev_info(&dev->dev, "bus %02x limited to PCI-Compatible config space\n",
> +				 bus->number);
> +			bus->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
> +		}
> +	}
> +}
> +
>  /*
>   * If it's a bridge, configure it and scan the bus behind it.
>   * For CardBus bridges, we don't scan behind as the devices will
> @@ -827,6 +856,7 @@
>  			child->primary = primary;
>  			pci_bus_insert_busn_res(child, secondary, subordinate);
>  			child->bridge_ctl = bctl;
> +			pci_bus_check_compat_cfg_space(child);
>  		}
>  
>  		cmax = pci_scan_child_bus(child);
> @@ -1084,6 +1114,9 @@
>  	u32 status;
>  	u16 class;
>  
> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
> +		return PCI_CFG_SPACE_SIZE;
> +
>  	class = dev->class >> 8;
>  	if (class == PCI_CLASS_BRIDGE_HOST)
>  		return pci_cfg_space_size_ext(dev);

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Le 30/04/2018 19:04, Bjorn Helgaas a =E9crit :
> On Mon, Apr 30, 2018 at 01:36:53PM +0000, Gilles Buloz wrote:
>> Le 30/04/2018 10:46, Gilles BULOZ a =E9crit :
>>> Le 27/04/2018 18:56, Bjorn Helgaas a =E9crit :
>>>> On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
>>>>> Le 27/04/2018 10:43, Ard Biesheuvel a =E9crit :
>>>>>> (add Bjorn and linux-pci)
>>>>>>
>>>>>> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> w=
rote:
>>>>>>> Dear developers,
>>>>>>>
>>>>>>> I currently have two functional workarounds for this issue but
>>>>>>> would like to know which one you would recommend, if any :-) I'm
>>>>>>> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
>>>>>>> external abort" when booting because of a PCI config read during
>>>>>>> PCI scan.
>>>>>>>
>>>>>>> I'm using a custom hardware (based on LS1043ARDB) having a
>>>>>>> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
>>>>>>> slot for legacy devices. This bridge only supports PCI-Compatible
>>>>>>> config accesses (offset 0x00-0xFF).
>>>> I would guess the PEX8112 itself has 4K of config space, but it only
>>>> forwards 256 bytes of config space to the conventional PCI secondary
>>>> bus.
>>>>
>>>>>>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
>>>>>>> bridge plus PCIe devices behind.
>>>>>>>
>>>>>>> The problem occurs when the kernel probes the PCIe devices : as
>>>>>>> they are PCIe devices, the kernel does a PCI config read access
>>>>>>> at offset 0x100 to check if "PCIe extended capability registers"
>>>>>>> are accessible (see drivers/pci/probe.c, function
>>>>>>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
>>>>>>> bridge that is in the path reports an error to the CPU for this
>>>>>>> access, and it seems there's no way to disable that on this
>>>>>>> bridge.
>>>>>>>
>>>>>>> The first workaround I found was to patch
>>>>>>> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
>>>>>>> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
>>>>>>> error reporting. This only impacts an NXP part of the Linux
>>>>>>> kernel code, but I'm not sure this is a good idea (however it
>>>>>>> seems to be like that on Intel platforms where even MEM accesses
>>>>>>> to a no-device address return FF without any error).
>>>>>>>
>>>>>>> I've also tried another workaround that works : patch
>>>>>>> drivers/pci/probe.c to use bus_flags to remember if a bus is
>>>>>>> behind a bridge without extended address capability, to avoid PCi
>>>>>>> config read accesses at offset 0x100 in pci_cfg_space_size() /
>>>>>>> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
>>>>>>> probe method of Linux.
>>>>>>>
>>>>>>> Any Idea to properly handle that issue ?
>>>>>>>
>>>>>> This seems like a rather unusual configuration, but I guess that
>>>>>> if the first bridge/switch advertises its inability to support
>>>>>> extended config space accesses, we should not be performing them
>>>>>> on any of its subordinate buses. How does the PEX8112 advertise
>>>>>> this limitation?
>>>>>>
>>>>>> That said, I wonder if it is reasonable in the first place to
>>>>>> expect that a PCIe device works as expected passing through a
>>>>>> legacy PCI layer like that.
>>>>>>
>>>>> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
>>>>> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
>>>>> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
>>>>> support for PCI config offset >=3D0x100).
>>>> Sounds right to me.
>>>>
>>>>> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
>>>>> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
>>>>> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
>>>>> pci_cfg_space_size())
>>>> Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266M=
HZ
>>>> should be enough, but it shouldn't hurt to check for either
>>>> PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.
>>>>
>>>>> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
>>>>> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
>>>>> bridge without extended address capability to avoid PCi config
>>>>> accesses at offset >=3D 0x100. Thanks to this patch I now have a
>>>>> functional system with functional PCI/PCIe devices.
>>>> The patch seems like it's looking at the right things, but I don't
>>>> want to build it into pci_scan_bridge_extend() because that function
>>>> is much too complicated already.
>>>>
>>>> I'd rather build it into pci_cfg_space_size() or
>>>> pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
>>>> This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
>>>> that case, I think all 4K would be accessible on the PCI-X side.
>>>>
>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>> index ac91b6fd0bcd..d8b091f0bcd1 100644
>>>> --- a/drivers/pci/probe.c
>>>> +++ b/drivers/pci/probe.c
>>>> @@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_de=
v *dev)
>>>>     * pci_cfg_space_size - Get the configuration space size of the PCI=
 device
>>>>     * @dev: PCI device
>>>>     *
>>>> - * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express de=
vices
>>>> + * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Expre=
ss devices
>>>>     * have 4096 bytes.  Even if the device is capable, that doesn't me=
an we can
>>>>     * access it.  Maybe we don't have a way to generate extended confi=
g space
>>>>     * accesses, or the device is behind a reverse Express bridge.  So =
we try
>>>> @@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_d=
ev *dev)
>>>>     */
>>>>    static int pci_cfg_space_size_ext(struct pci_dev *dev)
>>>>    {
>>>> +    struct pci_dev *bridge =3D pci_upstream_bridge(dev);
>>>>        u32 status;
>>>>        int pos =3D PCI_CFG_SPACE_SIZE;
>>>>    +    if (bridge && pci_is_pcie(bridge) &&
>>>> +        pci_pcie_type(bridge) =3D=3D PCI_EXP_TYPE_PCI_BRIDGE)
>>>> +        return PCI_CFG_SPACE_SIZE;
>>>> +
>>>>        if (pci_read_config_dword(dev, pos, &status) !=3D PCIBIOS_SUCCE=
SSFUL)
>>>>            return PCI_CFG_SPACE_SIZE;
>>>>        if (status =3D=3D 0xffffffff || pci_ext_cfg_is_aliased(dev))
>>>>
>>>>> --- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000
>>>>> +++ include/linux/pci.h    2018-03-26 16:51:27.660000000 +0000
>>>>> @@ -193,6 +193,7 @@
>>>>>    enum pci_bus_flags {
>>>>>        PCI_BUS_FLAGS_NO_MSI   =3D (__force pci_bus_flags_t) 1,
>>>>>        PCI_BUS_FLAGS_NO_MMRBC =3D (__force pci_bus_flags_t) 2,
>>>>> +    PCI_BUS_FLAGS_COMPAT_CFG_SPACE =3D (__force pci_bus_flags_t) 4,
>>>>>    };
>>>>>      /* These values come from the PCI Express Spec */
>>>>> --- drivers/pci/probe.c.orig    2018-01-22 09:29:52.000000000 +0000
>>>>> +++ drivers/pci/probe.c    2018-03-26 16:54:30.830000000 +0000
>>>>> @@ -827,6 +827,28 @@
>>>>>                child->primary =3D primary;
>>>>>                pci_bus_insert_busn_res(child, secondary, subordinate)=
;
>>>>>                child->bridge_ctl =3D bctl;
>>>>> +
>>>>> +            {
>>>>> +                int pos;
>>>>> +                u32 status;
>>>>> +                bool pci_compat_cfg_space =3D false;
>>>>> +
>>>>> +                if (!pci_is_pcie(dev) || (pci_pcie_type(dev) =3D=3D =
PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) =3D=3D
>>>>> PCI_EXP_TYPE_PCI_BRIDGE)) {
>>>>> +                    /* for PCI/PCI bridges, or PCIe/PCI bridge in fo=
rward or reverse mode, we have to check for PCI-X
>>>>> capabilities */
>>>>> +                    pos =3D pci_find_capability(dev, PCI_CAP_ID_PCIX=
);
>>>>> +                    if (pos) {
>>>>> +                        pci_read_config_dword(dev, pos + PCI_X_STATU=
S, &status);
>>>>> +                        if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_=
STATUS_533MHZ)))
>>>>> +                            pci_compat_cfg_space =3D true;
>>>>> +                    } else {
>>>>> +                        pci_compat_cfg_space =3D true;
>>>>> +                    }
>>>>> +                    if (pci_compat_cfg_space) {
>>>>> +                        dev_info(&dev->dev, "[%04x:%04x] Child bus l=
imited to PCI-Compatible config space\n", dev->vendor,
>>>>> dev->device);
>>>>> +                        child->bus_flags |=3D PCI_BUS_FLAGS_COMPAT_C=
FG_SPACE;
>>>>> +                    }
>>>>> +                }
>>>>> +            }
>>>>>            }
>>>>>              cmax =3D pci_scan_child_bus(child);
>>>>> @@ -1098,6 +1120,11 @@
>>>>>                goto fail;
>>>>>        }
>>>>>    +    if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
>>>>> +        dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space=
 only due to parent bus(es)\n", dev->vendor, dev->device);
>>>>> +        return PCI_CFG_SPACE_SIZE;
>>>>> +    }
>>>>> +
>>>>>        return pci_cfg_space_size_ext(dev);
>>>>>       fail:
>>> Bjorn,
>>> If I'm right about your proposed patch to
>>> pci_cfg_space_size_ext(), *bridge is pointing to the upper device
>>> of device *dev being checked. I understand the purpose, but I
>>> think this fails for my config that is :
>>>
>>> LS1043 PCIe root -> PEX8112 PCIe-to-PCI bridge -> PMC slot connector ->=
 PCI-to-PCIe bridge -> PCIe switch (4 ports) -> 4 PCIe
>>> devices (one on each port)
>>>
>>> because :
>>> - when pci_cfg_space_size_ext() is run on the 4 PCIe devices,
>>> *bridge is the PCIe switch which is not matching
>>> PCI_EXP_TYPE_PCI_BRIDGE. In this case *bridge should also be
>>> checked for the parent bus of the PCIe switch, and so on.
>>> - when pci_cfg_space_size_ext() is run for the PCI-to-PCIe bridge,
>>> *bridge is the PEX8112 that is also not matching
>>> PCI_EXP_TYPE_PCI_BRIDGE but PCI_EXP_TYPE_PCIE_BRIDGE. This leads
>>> to a config access at offset 0x100 to the PCI-to-PCIe bridge, so a
>>> crash (because of the PEX8112)
>>>
>>> I think setting a bit in bus_flags when creating a child bus is
>>> very efficient because once set it is automatically inherited by
>>> all child buses and then the only thing that pci_cfg_space_size()
>>> has to do for each device is to check for this bit. Also this
>>> PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag is actually a bus property
>>> that is compliant with the purpose of bus_flags.
> Yeah, it needs to be inherited somehow, and I don't like the idea of
> traversing up the tree, so I prefer your idea.  Although I don't
> actually see the inheritance in the patch below -- I thought there
> would be something like this:
>
>    dev =3D bus->self;
>    parent_bus =3D dev->bus;
>    if (parent_bus && parent_bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPA=
CE)
>      bus->bus_flags |=3D PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>
> pci_scan_bridge_extend() calls pci_add_new_bus() from two places.  You
> added a call to pci_bus_check_compat_cfg_space() at one of them, and
> it's not obvious why we wouldn't need it at the other place, too.
>
> Can you set this up in pci_alloc_child_bus()?  If you can put it
> there, it would be clear that every time we allocate a secondary bus,
> we figure out whether extended config space is accessible on that bus.
>
> That doesn't cover the root bus case, where we currently assume the
> host bridge can generate config accesses to all config space supported
> by devices on the root bus.  But we don't have a problem there, so I
> guess we don't need to worry about it now.
>
> If you can put it in pci_alloc_child_bus(), could you make your new
> function return a boolean, e.g., pci_bus_ext_cfg_accessible(), or
> similar, and then use the result to set the
> PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag?  Names like "*_check_*()" don't
> tell the reader much about what's happening.
>
>>> I agree that pci_scan_bridge_extend() is already too complicated,
>>> so would you be okay to only add one line to it :
>>>    pci_bus_set_compat_cfg_space(child);
>>> and put all the code I suggested in this new function
>>> pci_bus_set_compat_cfg_space() ? (also supporting PCI-X Mode 2
>>> devices)
>>>
>>> Improvement : this function can return immediately if the child
>>> bus has already inherited the flag from its parent.
>> I mean something like the attached patch I tested this morning...
>> Sorry, this is for old kernel 4.1.35 but just to clarify what I
>> propose (also applies to 4.16.6 by changing value of
>> PCI_BUS_FLAGS_COMPAT_CFG_SPACE in pci.h to 8).
>> --- include/linux/pci.h.orig=092018-03-26 16:51:18.050000000 +0000
>> +++ include/linux/pci.h=092018-04-30 09:50:57.660000000 +0000
>> @@ -193,6 +193,7 @@
>>   enum pci_bus_flags {
>>   =09PCI_BUS_FLAGS_NO_MSI   =3D (__force pci_bus_flags_t) 1,
>>   =09PCI_BUS_FLAGS_NO_MMRBC =3D (__force pci_bus_flags_t) 2,
>> +=09PCI_BUS_FLAGS_COMPAT_CFG_SPACE =3D (__force pci_bus_flags_t) 4,
>>   };
>>  =20
>>   /* These values come from the PCI Express Spec */
>> --- drivers/pci/probe.c.orig=092018-01-22 09:29:52.000000000 +0000
>> +++ drivers/pci/probe.c=092018-04-30 13:29:50.600000000 +0000
>> @@ -754,6 +754,35 @@
>>   =09=09=09=09=09 PCI_EXP_RTCTL_CRSSVE);
>>   }
>>  =20
>> +static void pci_bus_check_compat_cfg_space(struct pci_bus *bus)
>> +{
>> +=09struct pci_dev *dev =3D bus->self;
>> +=09bool pci_compat_cfg_space =3D false;
>> +=09int pos;
>> +=09u32 status;
>> +
>> +=09if (bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
>> +=09=09return;
>> +
>> +=09if (!pci_is_pcie(dev) || /* PCI/PCI bridge */
>> +=09    (pci_pcie_type(dev) =3D=3D PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/=
PCI bridge in forward mode */
>> +=09    (pci_pcie_type(dev) =3D=3D PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PCIe/=
PCI bridge in reverse mode */
>> +=09=09pos =3D pci_find_capability(dev, PCI_CAP_ID_PCIX);
>> +=09=09if (pos) {
>> +=09=09=09pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
>> +=09=09=09if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
>> +=09=09=09=09pci_compat_cfg_space =3D true;
>> +=09=09} else {
>> +=09=09=09pci_compat_cfg_space =3D true;
>> +=09=09}
>> +=09=09if (pci_compat_cfg_space) {
>> +=09=09=09dev_info(&dev->dev, "bus %02x limited to PCI-Compatible config=
 space\n",
>> +=09=09=09=09 bus->number);
>> +=09=09=09bus->bus_flags |=3D PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>> +=09=09}
>> +=09}
>> +}
>> +
>>   /*
>>    * If it's a bridge, configure it and scan the bus behind it.
>>    * For CardBus bridges, we don't scan behind as the devices will
>> @@ -827,6 +856,7 @@
>>   =09=09=09child->primary =3D primary;
>>   =09=09=09pci_bus_insert_busn_res(child, secondary, subordinate);
>>   =09=09=09child->bridge_ctl =3D bctl;
>> +=09=09=09pci_bus_check_compat_cfg_space(child);
>>   =09=09}
>>  =20
>>   =09=09cmax =3D pci_scan_child_bus(child);
>> @@ -1084,6 +1114,9 @@
>>   =09u32 status;
>>   =09u16 class;
>>  =20
>> +=09if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
>> +=09=09return PCI_CFG_SPACE_SIZE;
>> +
>>   =09class =3D dev->class >> 8;
>>   =09if (class =3D=3D PCI_CLASS_BRIDGE_HOST)
>>   =09=09return pci_cfg_space_size_ext(dev);
>
The inheritence is made by this line in pci_alloc_child_bus() :
     child->bus_flags =3D parent->bus_flags;
So once we detect a limitation on a bridge impacting a child bus and that w=
e set the flag in child->bus_flags, this flag is=20
automatically present in the child->bus_flags of all its children buses.

I agree with your remarks and will create a function named pci_bus_check_co=
mpat_cfg_space() that will be called from=20
pci_alloc_child_bus().
I'll test that on Wednesday 2th and will give you my feedback.

LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Le 30/04/2018 19:04, Bjorn Helgaas a ?crit :
> On Mon, Apr 30, 2018 at 01:36:53PM +0000, Gilles Buloz wrote:
>> Le 30/04/2018 10:46, Gilles BULOZ a ?crit :
>>> Le 27/04/2018 18:56, Bjorn Helgaas a ?crit :
>>>> On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
>>>>> Le 27/04/2018 10:43, Ard Biesheuvel a ?crit :
>>>>>> (add Bjorn and linux-pci)
>>>>>>
>>>>>> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
>>>>>>> Dear developers,
>>>>>>>
>>>>>>> I currently have two functional workarounds for this issue but
>>>>>>> would like to know which one you would recommend, if any :-) I'm
>>>>>>> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
>>>>>>> external abort" when booting because of a PCI config read during
>>>>>>> PCI scan.
>>>>>>>
>>>>>>> I'm using a custom hardware (based on LS1043ARDB) having a
>>>>>>> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
>>>>>>> slot for legacy devices. This bridge only supports PCI-Compatible
>>>>>>> config accesses (offset 0x00-0xFF).
>>>> I would guess the PEX8112 itself has 4K of config space, but it only
>>>> forwards 256 bytes of config space to the conventional PCI secondary
>>>> bus.
>>>>
>>>>>>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
>>>>>>> bridge plus PCIe devices behind.
>>>>>>>
>>>>>>> The problem occurs when the kernel probes the PCIe devices : as
>>>>>>> they are PCIe devices, the kernel does a PCI config read access
>>>>>>> at offset 0x100 to check if "PCIe extended capability registers"
>>>>>>> are accessible (see drivers/pci/probe.c, function
>>>>>>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
>>>>>>> bridge that is in the path reports an error to the CPU for this
>>>>>>> access, and it seems there's no way to disable that on this
>>>>>>> bridge.
>>>>>>>
>>>>>>> The first workaround I found was to patch
>>>>>>> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
>>>>>>> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
>>>>>>> error reporting. This only impacts an NXP part of the Linux
>>>>>>> kernel code, but I'm not sure this is a good idea (however it
>>>>>>> seems to be like that on Intel platforms where even MEM accesses
>>>>>>> to a no-device address return FF without any error).
>>>>>>>
>>>>>>> I've also tried another workaround that works : patch
>>>>>>> drivers/pci/probe.c to use bus_flags to remember if a bus is
>>>>>>> behind a bridge without extended address capability, to avoid PCi
>>>>>>> config read accesses at offset 0x100 in pci_cfg_space_size() /
>>>>>>> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
>>>>>>> probe method of Linux.
>>>>>>>
>>>>>>> Any Idea to properly handle that issue ?
>>>>>>>
>>>>>> This seems like a rather unusual configuration, but I guess that
>>>>>> if the first bridge/switch advertises its inability to support
>>>>>> extended config space accesses, we should not be performing them
>>>>>> on any of its subordinate buses. How does the PEX8112 advertise
>>>>>> this limitation?
>>>>>>
>>>>>> That said, I wonder if it is reasonable in the first place to
>>>>>> expect that a PCIe device works as expected passing through a
>>>>>> legacy PCI layer like that.
>>>>>>
>>>>> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
>>>>> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
>>>>> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
>>>>> support for PCI config offset >=0x100).
>>>> Sounds right to me.
>>>>
>>>>> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
>>>>> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
>>>>> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
>>>>> pci_cfg_space_size())
>>>> Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266MHZ
>>>> should be enough, but it shouldn't hurt to check for either
>>>> PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.
>>>>
>>>>> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
>>>>> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
>>>>> bridge without extended address capability to avoid PCi config
>>>>> accesses at offset >= 0x100. Thanks to this patch I now have a
>>>>> functional system with functional PCI/PCIe devices.
>>>> The patch seems like it's looking at the right things, but I don't
>>>> want to build it into pci_scan_bridge_extend() because that function
>>>> is much too complicated already.
>>>>
>>>> I'd rather build it into pci_cfg_space_size() or
>>>> pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
>>>> This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
>>>> that case, I think all 4K would be accessible on the PCI-X side.
>>>>
>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>> index ac91b6fd0bcd..d8b091f0bcd1 100644
>>>> --- a/drivers/pci/probe.c
>>>> +++ b/drivers/pci/probe.c
>>>> @@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>>>>     * pci_cfg_space_size - Get the configuration space size of the PCI device
>>>>     * @dev: PCI device
>>>>     *
>>>> - * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express devices
>>>> + * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Express devices
>>>>     * have 4096 bytes.  Even if the device is capable, that doesn't mean we can
>>>>     * access it.  Maybe we don't have a way to generate extended config space
>>>>     * accesses, or the device is behind a reverse Express bridge.  So we try
>>>> @@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>>>>     */
>>>>    static int pci_cfg_space_size_ext(struct pci_dev *dev)
>>>>    {
>>>> +    struct pci_dev *bridge = pci_upstream_bridge(dev);
>>>>        u32 status;
>>>>        int pos = PCI_CFG_SPACE_SIZE;
>>>>    +    if (bridge && pci_is_pcie(bridge) &&
>>>> +        pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
>>>> +        return PCI_CFG_SPACE_SIZE;
>>>> +
>>>>        if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
>>>>            return PCI_CFG_SPACE_SIZE;
>>>>        if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))
>>>>
>>>>> --- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000
>>>>> +++ include/linux/pci.h    2018-03-26 16:51:27.660000000 +0000
>>>>> @@ -193,6 +193,7 @@
>>>>>    enum pci_bus_flags {
>>>>>        PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>>>>>        PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
>>>>> +    PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>>>>>    };
>>>>>      /* These values come from the PCI Express Spec */
>>>>> --- drivers/pci/probe.c.orig    2018-01-22 09:29:52.000000000 +0000
>>>>> +++ drivers/pci/probe.c    2018-03-26 16:54:30.830000000 +0000
>>>>> @@ -827,6 +827,28 @@
>>>>>                child->primary = primary;
>>>>>                pci_bus_insert_busn_res(child, secondary, subordinate);
>>>>>                child->bridge_ctl = bctl;
>>>>> +
>>>>> +            {
>>>>> +                int pos;
>>>>> +                u32 status;
>>>>> +                bool pci_compat_cfg_space = false;
>>>>> +
>>>>> +                if (!pci_is_pcie(dev) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) ==
>>>>> PCI_EXP_TYPE_PCI_BRIDGE)) {
>>>>> +                    /* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse mode, we have to check for PCI-X
>>>>> capabilities */
>>>>> +                    pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
>>>>> +                    if (pos) {
>>>>> +                        pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
>>>>> +                        if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
>>>>> +                            pci_compat_cfg_space = true;
>>>>> +                    } else {
>>>>> +                        pci_compat_cfg_space = true;
>>>>> +                    }
>>>>> +                    if (pci_compat_cfg_space) {
>>>>> +                        dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compatible config space\n", dev->vendor,
>>>>> dev->device);
>>>>> +                        child->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>>>>> +                    }
>>>>> +                }
>>>>> +            }
>>>>>            }
>>>>>              cmax = pci_scan_child_bus(child);
>>>>> @@ -1098,6 +1120,11 @@
>>>>>                goto fail;
>>>>>        }
>>>>>    +    if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
>>>>> +        dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due to parent bus(es)\n", dev->vendor, dev->device);
>>>>> +        return PCI_CFG_SPACE_SIZE;
>>>>> +    }
>>>>> +
>>>>>        return pci_cfg_space_size_ext(dev);
>>>>>       fail:
>>> Bjorn,
>>> If I'm right about your proposed patch to
>>> pci_cfg_space_size_ext(), *bridge is pointing to the upper device
>>> of device *dev being checked. I understand the purpose, but I
>>> think this fails for my config that is :
>>>
>>> LS1043 PCIe root -> PEX8112 PCIe-to-PCI bridge -> PMC slot connector -> PCI-to-PCIe bridge -> PCIe switch (4 ports) -> 4 PCIe
>>> devices (one on each port)
>>>
>>> because :
>>> - when pci_cfg_space_size_ext() is run on the 4 PCIe devices,
>>> *bridge is the PCIe switch which is not matching
>>> PCI_EXP_TYPE_PCI_BRIDGE. In this case *bridge should also be
>>> checked for the parent bus of the PCIe switch, and so on.
>>> - when pci_cfg_space_size_ext() is run for the PCI-to-PCIe bridge,
>>> *bridge is the PEX8112 that is also not matching
>>> PCI_EXP_TYPE_PCI_BRIDGE but PCI_EXP_TYPE_PCIE_BRIDGE. This leads
>>> to a config access at offset 0x100 to the PCI-to-PCIe bridge, so a
>>> crash (because of the PEX8112)
>>>
>>> I think setting a bit in bus_flags when creating a child bus is
>>> very efficient because once set it is automatically inherited by
>>> all child buses and then the only thing that pci_cfg_space_size()
>>> has to do for each device is to check for this bit. Also this
>>> PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag is actually a bus property
>>> that is compliant with the purpose of bus_flags.
> Yeah, it needs to be inherited somehow, and I don't like the idea of
> traversing up the tree, so I prefer your idea.  Although I don't
> actually see the inheritance in the patch below -- I thought there
> would be something like this:
>
>    dev = bus->self;
>    parent_bus = dev->bus;
>    if (parent_bus && parent_bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
>      bus->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>
> pci_scan_bridge_extend() calls pci_add_new_bus() from two places.  You
> added a call to pci_bus_check_compat_cfg_space() at one of them, and
> it's not obvious why we wouldn't need it at the other place, too.
>
> Can you set this up in pci_alloc_child_bus()?  If you can put it
> there, it would be clear that every time we allocate a secondary bus,
> we figure out whether extended config space is accessible on that bus.
>
> That doesn't cover the root bus case, where we currently assume the
> host bridge can generate config accesses to all config space supported
> by devices on the root bus.  But we don't have a problem there, so I
> guess we don't need to worry about it now.
>
> If you can put it in pci_alloc_child_bus(), could you make your new
> function return a boolean, e.g., pci_bus_ext_cfg_accessible(), or
> similar, and then use the result to set the
> PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag?  Names like "*_check_*()" don't
> tell the reader much about what's happening.
>
>>> I agree that pci_scan_bridge_extend() is already too complicated,
>>> so would you be okay to only add one line to it :
>>>    pci_bus_set_compat_cfg_space(child);
>>> and put all the code I suggested in this new function
>>> pci_bus_set_compat_cfg_space() ? (also supporting PCI-X Mode 2
>>> devices)
>>>
>>> Improvement : this function can return immediately if the child
>>> bus has already inherited the flag from its parent.
>> I mean something like the attached patch I tested this morning...
>> Sorry, this is for old kernel 4.1.35 but just to clarify what I
>> propose (also applies to 4.16.6 by changing value of
>> PCI_BUS_FLAGS_COMPAT_CFG_SPACE in pci.h to 8).
>> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
>> +++ include/linux/pci.h	2018-04-30 09:50:57.660000000 +0000
>> @@ -193,6 +193,7 @@
>>   enum pci_bus_flags {
>>   	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>>   	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
>> +	PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>>   };
>>   
>>   /* These values come from the PCI Express Spec */
>> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
>> +++ drivers/pci/probe.c	2018-04-30 13:29:50.600000000 +0000
>> @@ -754,6 +754,35 @@
>>   					 PCI_EXP_RTCTL_CRSSVE);
>>   }
>>   
>> +static void pci_bus_check_compat_cfg_space(struct pci_bus *bus)
>> +{
>> +	struct pci_dev *dev = bus->self;
>> +	bool pci_compat_cfg_space = false;
>> +	int pos;
>> +	u32 status;
>> +
>> +	if (bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
>> +		return;
>> +
>> +	if (!pci_is_pcie(dev) || /* PCI/PCI bridge */
>> +	    (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */
>> +	    (pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PCIe/PCI bridge in reverse mode */
>> +		pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
>> +		if (pos) {
>> +			pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
>> +			if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
>> +				pci_compat_cfg_space = true;
>> +		} else {
>> +			pci_compat_cfg_space = true;
>> +		}
>> +		if (pci_compat_cfg_space) {
>> +			dev_info(&dev->dev, "bus %02x limited to PCI-Compatible config space\n",
>> +				 bus->number);
>> +			bus->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>> +		}
>> +	}
>> +}
>> +
>>   /*
>>    * If it's a bridge, configure it and scan the bus behind it.
>>    * For CardBus bridges, we don't scan behind as the devices will
>> @@ -827,6 +856,7 @@
>>   			child->primary = primary;
>>   			pci_bus_insert_busn_res(child, secondary, subordinate);
>>   			child->bridge_ctl = bctl;
>> +			pci_bus_check_compat_cfg_space(child);
>>   		}
>>   
>>   		cmax = pci_scan_child_bus(child);
>> @@ -1084,6 +1114,9 @@
>>   	u32 status;
>>   	u16 class;
>>   
>> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
>> +		return PCI_CFG_SPACE_SIZE;
>> +
>>   	class = dev->class >> 8;
>>   	if (class == PCI_CLASS_BRIDGE_HOST)
>>   		return pci_cfg_space_size_ext(dev);
>
The inheritence is made by this line in pci_alloc_child_bus() :
     child->bus_flags = parent->bus_flags;
So once we detect a limitation on a bridge impacting a child bus and that we set the flag in child->bus_flags, this flag is 
automatically present in the child->bus_flags of all its children buses.

I agree with your remarks and will create a function named pci_bus_check_compat_cfg_space() that will be called from 
pci_alloc_child_bus().
I'll test that on Wednesday 2th and will give you my feedback.

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

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

Le 30/04/2018 19:53, Gilles BULOZ a écrit :
> Le 30/04/2018 19:04, Bjorn Helgaas a écrit :
>> On Mon, Apr 30, 2018 at 01:36:53PM +0000, Gilles Buloz wrote:
>>> Le 30/04/2018 10:46, Gilles BULOZ a écrit :
>>>> Le 27/04/2018 18:56, Bjorn Helgaas a écrit :
>>>>> On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
>>>>>> Le 27/04/2018 10:43, Ard Biesheuvel a écrit :
>>>>>>> (add Bjorn and linux-pci)
>>>>>>>
>>>>>>> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
>>>>>>>> Dear developers,
>>>>>>>>
>>>>>>>> I currently have two functional workarounds for this issue but
>>>>>>>> would like to know which one you would recommend, if any :-) I'm
>>>>>>>> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
>>>>>>>> external abort" when booting because of a PCI config read during
>>>>>>>> PCI scan.
>>>>>>>>
>>>>>>>> I'm using a custom hardware (based on LS1043ARDB) having a
>>>>>>>> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
>>>>>>>> slot for legacy devices. This bridge only supports PCI-Compatible
>>>>>>>> config accesses (offset 0x00-0xFF).
>>>>> I would guess the PEX8112 itself has 4K of config space, but it only
>>>>> forwards 256 bytes of config space to the conventional PCI secondary
>>>>> bus.
>>>>>
>>>>>>>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
>>>>>>>> bridge plus PCIe devices behind.
>>>>>>>>
>>>>>>>> The problem occurs when the kernel probes the PCIe devices : as
>>>>>>>> they are PCIe devices, the kernel does a PCI config read access
>>>>>>>> at offset 0x100 to check if "PCIe extended capability registers"
>>>>>>>> are accessible (see drivers/pci/probe.c, function
>>>>>>>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
>>>>>>>> bridge that is in the path reports an error to the CPU for this
>>>>>>>> access, and it seems there's no way to disable that on this
>>>>>>>> bridge.
>>>>>>>>
>>>>>>>> The first workaround I found was to patch
>>>>>>>> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
>>>>>>>> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
>>>>>>>> error reporting. This only impacts an NXP part of the Linux
>>>>>>>> kernel code, but I'm not sure this is a good idea (however it
>>>>>>>> seems to be like that on Intel platforms where even MEM accesses
>>>>>>>> to a no-device address return FF without any error).
>>>>>>>>
>>>>>>>> I've also tried another workaround that works : patch
>>>>>>>> drivers/pci/probe.c to use bus_flags to remember if a bus is
>>>>>>>> behind a bridge without extended address capability, to avoid PCi
>>>>>>>> config read accesses at offset 0x100 in pci_cfg_space_size() /
>>>>>>>> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
>>>>>>>> probe method of Linux.
>>>>>>>>
>>>>>>>> Any Idea to properly handle that issue ?
>>>>>>>>
>>>>>>> This seems like a rather unusual configuration, but I guess that
>>>>>>> if the first bridge/switch advertises its inability to support
>>>>>>> extended config space accesses, we should not be performing them
>>>>>>> on any of its subordinate buses. How does the PEX8112 advertise
>>>>>>> this limitation?
>>>>>>>
>>>>>>> That said, I wonder if it is reasonable in the first place to
>>>>>>> expect that a PCIe device works as expected passing through a
>>>>>>> legacy PCI layer like that.
>>>>>>>
>>>>>> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
>>>>>> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
>>>>>> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
>>>>>> support for PCI config offset >=0x100).
>>>>> Sounds right to me.
>>>>>
>>>>>> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
>>>>>> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
>>>>>> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
>>>>>> pci_cfg_space_size())
>>>>> Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266MHZ
>>>>> should be enough, but it shouldn't hurt to check for either
>>>>> PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.
>>>>>
>>>>>> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
>>>>>> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
>>>>>> bridge without extended address capability to avoid PCi config
>>>>>> accesses at offset >= 0x100. Thanks to this patch I now have a
>>>>>> functional system with functional PCI/PCIe devices.
>>>>> The patch seems like it's looking at the right things, but I don't
>>>>> want to build it into pci_scan_bridge_extend() because that function
>>>>> is much too complicated already.
>>>>>
>>>>> I'd rather build it into pci_cfg_space_size() or
>>>>> pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
>>>>> This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
>>>>> that case, I think all 4K would be accessible on the PCI-X side.
>>>>>
>>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>>> index ac91b6fd0bcd..d8b091f0bcd1 100644
>>>>> --- a/drivers/pci/probe.c
>>>>> +++ b/drivers/pci/probe.c
>>>>> @@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>>>>>     * pci_cfg_space_size - Get the configuration space size of the PCI device
>>>>>     * @dev: PCI device
>>>>>     *
>>>>> - * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express devices
>>>>> + * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Express devices
>>>>>     * have 4096 bytes.  Even if the device is capable, that doesn't mean we can
>>>>>     * access it.  Maybe we don't have a way to generate extended config space
>>>>>     * accesses, or the device is behind a reverse Express bridge.  So we try
>>>>> @@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>>>>>     */
>>>>>    static int pci_cfg_space_size_ext(struct pci_dev *dev)
>>>>>    {
>>>>> +    struct pci_dev *bridge = pci_upstream_bridge(dev);
>>>>>        u32 status;
>>>>>        int pos = PCI_CFG_SPACE_SIZE;
>>>>>    +    if (bridge && pci_is_pcie(bridge) &&
>>>>> +        pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
>>>>> +        return PCI_CFG_SPACE_SIZE;
>>>>> +
>>>>>        if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
>>>>>            return PCI_CFG_SPACE_SIZE;
>>>>>        if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))
>>>>>
>>>>>> --- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000
>>>>>> +++ include/linux/pci.h    2018-03-26 16:51:27.660000000 +0000
>>>>>> @@ -193,6 +193,7 @@
>>>>>>    enum pci_bus_flags {
>>>>>>        PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>>>>>>        PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
>>>>>> +    PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>>>>>>    };
>>>>>>      /* These values come from the PCI Express Spec */
>>>>>> --- drivers/pci/probe.c.orig    2018-01-22 09:29:52.000000000 +0000
>>>>>> +++ drivers/pci/probe.c    2018-03-26 16:54:30.830000000 +0000
>>>>>> @@ -827,6 +827,28 @@
>>>>>>                child->primary = primary;
>>>>>>                pci_bus_insert_busn_res(child, secondary, subordinate);
>>>>>>                child->bridge_ctl = bctl;
>>>>>> +
>>>>>> +            {
>>>>>> +                int pos;
>>>>>> +                u32 status;
>>>>>> +                bool pci_compat_cfg_space = false;
>>>>>> +
>>>>>> +                if (!pci_is_pcie(dev) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) ==
>>>>>> PCI_EXP_TYPE_PCI_BRIDGE)) {
>>>>>> +                    /* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse mode, we have to check for PCI-X
>>>>>> capabilities */
>>>>>> +                    pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
>>>>>> +                    if (pos) {
>>>>>> +                        pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
>>>>>> +                        if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
>>>>>> +                            pci_compat_cfg_space = true;
>>>>>> +                    } else {
>>>>>> +                        pci_compat_cfg_space = true;
>>>>>> +                    }
>>>>>> +                    if (pci_compat_cfg_space) {
>>>>>> +                        dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compatible config space\n", dev->vendor,
>>>>>> dev->device);
>>>>>> +                        child->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>>>>>> +                    }
>>>>>> +                }
>>>>>> +            }
>>>>>>            }
>>>>>>              cmax = pci_scan_child_bus(child);
>>>>>> @@ -1098,6 +1120,11 @@
>>>>>>                goto fail;
>>>>>>        }
>>>>>>    +    if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
>>>>>> +        dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due to parent bus(es)\n", dev->vendor, dev->device);
>>>>>> +        return PCI_CFG_SPACE_SIZE;
>>>>>> +    }
>>>>>> +
>>>>>>        return pci_cfg_space_size_ext(dev);
>>>>>>       fail:
>>>> Bjorn,
>>>> If I'm right about your proposed patch to
>>>> pci_cfg_space_size_ext(), *bridge is pointing to the upper device
>>>> of device *dev being checked. I understand the purpose, but I
>>>> think this fails for my config that is :
>>>>
>>>> LS1043 PCIe root -> PEX8112 PCIe-to-PCI bridge -> PMC slot connector -> PCI-to-PCIe bridge -> PCIe switch (4 ports) -> 4 PCIe
>>>> devices (one on each port)
>>>>
>>>> because :
>>>> - when pci_cfg_space_size_ext() is run on the 4 PCIe devices,
>>>> *bridge is the PCIe switch which is not matching
>>>> PCI_EXP_TYPE_PCI_BRIDGE. In this case *bridge should also be
>>>> checked for the parent bus of the PCIe switch, and so on.
>>>> - when pci_cfg_space_size_ext() is run for the PCI-to-PCIe bridge,
>>>> *bridge is the PEX8112 that is also not matching
>>>> PCI_EXP_TYPE_PCI_BRIDGE but PCI_EXP_TYPE_PCIE_BRIDGE. This leads
>>>> to a config access at offset 0x100 to the PCI-to-PCIe bridge, so a
>>>> crash (because of the PEX8112)
>>>>
>>>> I think setting a bit in bus_flags when creating a child bus is
>>>> very efficient because once set it is automatically inherited by
>>>> all child buses and then the only thing that pci_cfg_space_size()
>>>> has to do for each device is to check for this bit. Also this
>>>> PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag is actually a bus property
>>>> that is compliant with the purpose of bus_flags.
>> Yeah, it needs to be inherited somehow, and I don't like the idea of
>> traversing up the tree, so I prefer your idea.  Although I don't
>> actually see the inheritance in the patch below -- I thought there
>> would be something like this:
>>
>>    dev = bus->self;
>>    parent_bus = dev->bus;
>>    if (parent_bus && parent_bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
>>      bus->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>>
>> pci_scan_bridge_extend() calls pci_add_new_bus() from two places.  You
>> added a call to pci_bus_check_compat_cfg_space() at one of them, and
>> it's not obvious why we wouldn't need it at the other place, too.
>>
>> Can you set this up in pci_alloc_child_bus()?  If you can put it
>> there, it would be clear that every time we allocate a secondary bus,
>> we figure out whether extended config space is accessible on that bus.
>>
>> That doesn't cover the root bus case, where we currently assume the
>> host bridge can generate config accesses to all config space supported
>> by devices on the root bus.  But we don't have a problem there, so I
>> guess we don't need to worry about it now.
>>
>> If you can put it in pci_alloc_child_bus(), could you make your new
>> function return a boolean, e.g., pci_bus_ext_cfg_accessible(), or
>> similar, and then use the result to set the
>> PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag?  Names like "*_check_*()" don't
>> tell the reader much about what's happening.
>>
>>>> I agree that pci_scan_bridge_extend() is already too complicated,
>>>> so would you be okay to only add one line to it :
>>>>    pci_bus_set_compat_cfg_space(child);
>>>> and put all the code I suggested in this new function
>>>> pci_bus_set_compat_cfg_space() ? (also supporting PCI-X Mode 2
>>>> devices)
>>>>
>>>> Improvement : this function can return immediately if the child
>>>> bus has already inherited the flag from its parent.
>>> I mean something like the attached patch I tested this morning...
>>> Sorry, this is for old kernel 4.1.35 but just to clarify what I
>>> propose (also applies to 4.16.6 by changing value of
>>> PCI_BUS_FLAGS_COMPAT_CFG_SPACE in pci.h to 8).
>>> --- include/linux/pci.h.orig    2018-03-26 16:51:18.050000000 +0000
>>> +++ include/linux/pci.h    2018-04-30 09:50:57.660000000 +0000
>>> @@ -193,6 +193,7 @@
>>>   enum pci_bus_flags {
>>>       PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>>>       PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
>>> +    PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>>>   };
>>>     /* These values come from the PCI Express Spec */
>>> --- drivers/pci/probe.c.orig    2018-01-22 09:29:52.000000000 +0000
>>> +++ drivers/pci/probe.c    2018-04-30 13:29:50.600000000 +0000
>>> @@ -754,6 +754,35 @@
>>>                        PCI_EXP_RTCTL_CRSSVE);
>>>   }
>>>   +static void pci_bus_check_compat_cfg_space(struct pci_bus *bus)
>>> +{
>>> +    struct pci_dev *dev = bus->self;
>>> +    bool pci_compat_cfg_space = false;
>>> +    int pos;
>>> +    u32 status;
>>> +
>>> +    if (bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
>>> +        return;
>>> +
>>> +    if (!pci_is_pcie(dev) || /* PCI/PCI bridge */
>>> +        (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */
>>> +        (pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE)) { /* PCIe/PCI bridge in reverse mode */
>>> +        pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
>>> +        if (pos) {
>>> +            pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
>>> +            if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
>>> +                pci_compat_cfg_space = true;
>>> +        } else {
>>> +            pci_compat_cfg_space = true;
>>> +        }
>>> +        if (pci_compat_cfg_space) {
>>> +            dev_info(&dev->dev, "bus %02x limited to PCI-Compatible config space\n",
>>> +                 bus->number);
>>> +            bus->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>>> +        }
>>> +    }
>>> +}
>>> +
>>>   /*
>>>    * If it's a bridge, configure it and scan the bus behind it.
>>>    * For CardBus bridges, we don't scan behind as the devices will
>>> @@ -827,6 +856,7 @@
>>>               child->primary = primary;
>>>               pci_bus_insert_busn_res(child, secondary, subordinate);
>>>               child->bridge_ctl = bctl;
>>> +            pci_bus_check_compat_cfg_space(child);
>>>           }
>>>             cmax = pci_scan_child_bus(child);
>>> @@ -1084,6 +1114,9 @@
>>>       u32 status;
>>>       u16 class;
>>>   +    if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
>>> +        return PCI_CFG_SPACE_SIZE;
>>> +
>>>       class = dev->class >> 8;
>>>       if (class == PCI_CLASS_BRIDGE_HOST)
>>>           return pci_cfg_space_size_ext(dev);
>>
> The inheritence is made by this line in pci_alloc_child_bus() :
>     child->bus_flags = parent->bus_flags;
> So once we detect a limitation on a bridge impacting a child bus and that we set the flag in child->bus_flags, this flag is 
> automatically present in the child->bus_flags of all its children buses.
>
> I agree with your remarks and will create a function named pci_bus_check_compat_cfg_space() that will be called from 
> pci_alloc_child_bus().
> I'll test that on Wednesday 2th and will give you my feedback.
Hi Bjorn,
See attached patch (tested ok this morning)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cfgspace3_4.1.35.patch --]
[-- Type: text/x-patch; name=cfgspace3_4.1.35.patch, Size: 2246 bytes --]

--- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
+++ include/linux/pci.h	2018-04-30 18:29:14.140000000 +0000
@@ -193,6 +193,7 @@
 enum pci_bus_flags {
 	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
 	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
+	PCI_BUS_FLAGS_NO_EXTCFG = (__force pci_bus_flags_t) 4,
 };
 
 /* These values come from the PCI Express Spec */
--- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
+++ drivers/pci/probe.c	2018-05-02 13:44:35.530000000 +0000
@@ -664,6 +664,23 @@
 	}
 }
 
+static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bridge)
+{
+	int pos;
+	u32 status;
+
+	if (!pci_is_pcie(bridge) || /* PCI/PCI bridge */
+	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */
+	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PCIe/PCI bridge in reverse mode */
+		pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
+		if (pos)
+			pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
+		return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ));
+	}
+
+	return true;
+}
+
 static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 					   struct pci_dev *bridge, int busnr)
 {
@@ -725,6 +742,19 @@
 	/* Create legacy_io and legacy_mem files for this bus */
 	pci_create_legacy_files(child);
 
+	/*
+	 * if bus_flags inherited from parent bus do not already report lack of extended config
+	 * space support, check if supported by child bus by checking its parent bridge
+	 */
+	if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)) {
+		if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) {
+			child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
+			dev_info(&child->dev, "extended config space not accessible due to parent bridge\n");
+		}
+	} else {
+		dev_info(&child->dev, "extended config space not accessible due to parent bus\n");
+	}
+
 	return child;
 }
 
@@ -1084,6 +1114,9 @@
 	u32 status;
 	u16 class;
 
+	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
+		return PCI_CFG_SPACE_SIZE;
+
 	class = dev->class >> 8;
 	if (class == PCI_CLASS_BRIDGE_HOST)
 		return pci_cfg_space_size_ext(dev);

LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Le 30/04/2018 19:53, Gilles BULOZ a ?crit :
> Le 30/04/2018 19:04, Bjorn Helgaas a ?crit :
>> On Mon, Apr 30, 2018 at 01:36:53PM +0000, Gilles Buloz wrote:
>>> Le 30/04/2018 10:46, Gilles BULOZ a ?crit :
>>>> Le 27/04/2018 18:56, Bjorn Helgaas a ?crit :
>>>>> On Fri, Apr 27, 2018 at 12:29:32PM +0000, Gilles Buloz wrote:
>>>>>> Le 27/04/2018 10:43, Ard Biesheuvel a ?crit :
>>>>>>> (add Bjorn and linux-pci)
>>>>>>>
>>>>>>> On 13 April 2018 at 19:32, Gilles Buloz <Gilles.Buloz@kontron.com> wrote:
>>>>>>>> Dear developers,
>>>>>>>>
>>>>>>>> I currently have two functional workarounds for this issue but
>>>>>>>> would like to know which one you would recommend, if any :-) I'm
>>>>>>>> using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous
>>>>>>>> external abort" when booting because of a PCI config read during
>>>>>>>> PCI scan.
>>>>>>>>
>>>>>>>> I'm using a custom hardware (based on LS1043ARDB) having a
>>>>>>>> PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI
>>>>>>>> slot for legacy devices. This bridge only supports PCI-Compatible
>>>>>>>> config accesses (offset 0x00-0xFF).
>>>>> I would guess the PEX8112 itself has 4K of config space, but it only
>>>>> forwards 256 bytes of config space to the conventional PCI secondary
>>>>> bus.
>>>>>
>>>>>>>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe
>>>>>>>> bridge plus PCIe devices behind.
>>>>>>>>
>>>>>>>> The problem occurs when the kernel probes the PCIe devices : as
>>>>>>>> they are PCIe devices, the kernel does a PCI config read access
>>>>>>>> at offset 0x100 to check if "PCIe extended capability registers"
>>>>>>>> are accessible (see drivers/pci/probe.c, function
>>>>>>>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI
>>>>>>>> bridge that is in the path reports an error to the CPU for this
>>>>>>>> access, and it seems there's no way to disable that on this
>>>>>>>> bridge.
>>>>>>>>
>>>>>>>> The first workaround I found was to patch
>>>>>>>> drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set
>>>>>>>> to 0x9400 instead of 0x9401 (for PCIE_ABSERR register) to disable
>>>>>>>> error reporting. This only impacts an NXP part of the Linux
>>>>>>>> kernel code, but I'm not sure this is a good idea (however it
>>>>>>>> seems to be like that on Intel platforms where even MEM accesses
>>>>>>>> to a no-device address return FF without any error).
>>>>>>>>
>>>>>>>> I've also tried another workaround that works : patch
>>>>>>>> drivers/pci/probe.c to use bus_flags to remember if a bus is
>>>>>>>> behind a bridge without extended address capability, to avoid PCi
>>>>>>>> config read accesses at offset 0x100 in pci_cfg_space_size() /
>>>>>>>> pci_cfg_space_size_ext(). But this patch impacts the generic PCI
>>>>>>>> probe method of Linux.
>>>>>>>>
>>>>>>>> Any Idea to properly handle that issue ?
>>>>>>>>
>>>>>>> This seems like a rather unusual configuration, but I guess that
>>>>>>> if the first bridge/switch advertises its inability to support
>>>>>>> extended config space accesses, we should not be performing them
>>>>>>> on any of its subordinate buses. How does the PEX8112 advertise
>>>>>>> this limitation?
>>>>>>>
>>>>>>> That said, I wonder if it is reasonable in the first place to
>>>>>>> expect that a PCIe device works as expected passing through a
>>>>>>> legacy PCI layer like that.
>>>>>>>
>>>>>> The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but
>>>>>> has no PCI_CAP_ID_PCIX capability.  As I understand the lack of
>>>>>> PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no
>>>>>> support for PCI config offset >=0x100).
>>>>> Sounds right to me.
>>>>>
>>>>>> Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this
>>>>>> limitation would be advertised by the lack of PCI_X_STATUS_266MHZ
>>>>>> and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at
>>>>>> pci_cfg_space_size())
>>>>> Also sounds right.  Per the PCI-X spec, checking for PCI_X_STATUS_266MHZ
>>>>> should be enough, but it shouldn't hurt to check for either
>>>>> PCI_X_STATUS_266MHZ or PCI_X_STATUS_533MHZ.
>>>>>
>>>>>> I'm currently using the attached patch (for kernel 4.1.35-rt41 from
>>>>>> NXP Yocto BSP). It uses bus_flags to remember if a bus is behind a
>>>>>> bridge without extended address capability to avoid PCi config
>>>>>> accesses at offset >= 0x100. Thanks to this patch I now have a
>>>>>> functional system with functional PCI/PCIe devices.
>>>>> The patch seems like it's looking at the right things, but I don't
>>>>> want to build it into pci_scan_bridge_extend() because that function
>>>>> is much too complicated already.
>>>>>
>>>>> I'd rather build it into pci_cfg_space_size() or
>>>>> pci_cfg_space_size_ext() somehow.  Maybe something along these lines?
>>>>> This doesn't account for the case of a PCIe-to-PCI-X Mode 2 bridge; in
>>>>> that case, I think all 4K would be accessible on the PCI-X side.
>>>>>
>>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>>> index ac91b6fd0bcd..d8b091f0bcd1 100644
>>>>> --- a/drivers/pci/probe.c
>>>>> +++ b/drivers/pci/probe.c
>>>>> @@ -1367,7 +1367,7 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>>>>>     * pci_cfg_space_size - Get the configuration space size of the PCI device
>>>>>     * @dev: PCI device
>>>>>     *
>>>>> - * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express devices
>>>>> + * Regular PCI devices have 256 bytes, but PCI-X Mode 2 and PCI Express devices
>>>>>     * have 4096 bytes.  Even if the device is capable, that doesn't mean we can
>>>>>     * access it.  Maybe we don't have a way to generate extended config space
>>>>>     * accesses, or the device is behind a reverse Express bridge.  So we try
>>>>> @@ -1376,9 +1376,14 @@ static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
>>>>>     */
>>>>>    static int pci_cfg_space_size_ext(struct pci_dev *dev)
>>>>>    {
>>>>> +    struct pci_dev *bridge = pci_upstream_bridge(dev);
>>>>>        u32 status;
>>>>>        int pos = PCI_CFG_SPACE_SIZE;
>>>>>    +    if (bridge && pci_is_pcie(bridge) &&
>>>>> +        pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)
>>>>> +        return PCI_CFG_SPACE_SIZE;
>>>>> +
>>>>>        if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
>>>>>            return PCI_CFG_SPACE_SIZE;
>>>>>        if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))
>>>>>
>>>>>> --- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000
>>>>>> +++ include/linux/pci.h    2018-03-26 16:51:27.660000000 +0000
>>>>>> @@ -193,6 +193,7 @@
>>>>>>    enum pci_bus_flags {
>>>>>>        PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>>>>>>        PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
>>>>>> +    PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>>>>>>    };
>>>>>>      /* These values come from the PCI Express Spec */
>>>>>> --- drivers/pci/probe.c.orig    2018-01-22 09:29:52.000000000 +0000
>>>>>> +++ drivers/pci/probe.c    2018-03-26 16:54:30.830000000 +0000
>>>>>> @@ -827,6 +827,28 @@
>>>>>>                child->primary = primary;
>>>>>>                pci_bus_insert_busn_res(child, secondary, subordinate);
>>>>>>                child->bridge_ctl = bctl;
>>>>>> +
>>>>>> +            {
>>>>>> +                int pos;
>>>>>> +                u32 status;
>>>>>> +                bool pci_compat_cfg_space = false;
>>>>>> +
>>>>>> +                if (!pci_is_pcie(dev) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) ==
>>>>>> PCI_EXP_TYPE_PCI_BRIDGE)) {
>>>>>> +                    /* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse mode, we have to check for PCI-X
>>>>>> capabilities */
>>>>>> +                    pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
>>>>>> +                    if (pos) {
>>>>>> +                        pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
>>>>>> +                        if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
>>>>>> +                            pci_compat_cfg_space = true;
>>>>>> +                    } else {
>>>>>> +                        pci_compat_cfg_space = true;
>>>>>> +                    }
>>>>>> +                    if (pci_compat_cfg_space) {
>>>>>> +                        dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compatible config space\n", dev->vendor,
>>>>>> dev->device);
>>>>>> +                        child->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>>>>>> +                    }
>>>>>> +                }
>>>>>> +            }
>>>>>>            }
>>>>>>              cmax = pci_scan_child_bus(child);
>>>>>> @@ -1098,6 +1120,11 @@
>>>>>>                goto fail;
>>>>>>        }
>>>>>>    +    if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
>>>>>> +        dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due to parent bus(es)\n", dev->vendor, dev->device);
>>>>>> +        return PCI_CFG_SPACE_SIZE;
>>>>>> +    }
>>>>>> +
>>>>>>        return pci_cfg_space_size_ext(dev);
>>>>>>       fail:
>>>> Bjorn,
>>>> If I'm right about your proposed patch to
>>>> pci_cfg_space_size_ext(), *bridge is pointing to the upper device
>>>> of device *dev being checked. I understand the purpose, but I
>>>> think this fails for my config that is :
>>>>
>>>> LS1043 PCIe root -> PEX8112 PCIe-to-PCI bridge -> PMC slot connector -> PCI-to-PCIe bridge -> PCIe switch (4 ports) -> 4 PCIe
>>>> devices (one on each port)
>>>>
>>>> because :
>>>> - when pci_cfg_space_size_ext() is run on the 4 PCIe devices,
>>>> *bridge is the PCIe switch which is not matching
>>>> PCI_EXP_TYPE_PCI_BRIDGE. In this case *bridge should also be
>>>> checked for the parent bus of the PCIe switch, and so on.
>>>> - when pci_cfg_space_size_ext() is run for the PCI-to-PCIe bridge,
>>>> *bridge is the PEX8112 that is also not matching
>>>> PCI_EXP_TYPE_PCI_BRIDGE but PCI_EXP_TYPE_PCIE_BRIDGE. This leads
>>>> to a config access at offset 0x100 to the PCI-to-PCIe bridge, so a
>>>> crash (because of the PEX8112)
>>>>
>>>> I think setting a bit in bus_flags when creating a child bus is
>>>> very efficient because once set it is automatically inherited by
>>>> all child buses and then the only thing that pci_cfg_space_size()
>>>> has to do for each device is to check for this bit. Also this
>>>> PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag is actually a bus property
>>>> that is compliant with the purpose of bus_flags.
>> Yeah, it needs to be inherited somehow, and I don't like the idea of
>> traversing up the tree, so I prefer your idea.  Although I don't
>> actually see the inheritance in the patch below -- I thought there
>> would be something like this:
>>
>>    dev = bus->self;
>>    parent_bus = dev->bus;
>>    if (parent_bus && parent_bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
>>      bus->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>>
>> pci_scan_bridge_extend() calls pci_add_new_bus() from two places.  You
>> added a call to pci_bus_check_compat_cfg_space() at one of them, and
>> it's not obvious why we wouldn't need it at the other place, too.
>>
>> Can you set this up in pci_alloc_child_bus()?  If you can put it
>> there, it would be clear that every time we allocate a secondary bus,
>> we figure out whether extended config space is accessible on that bus.
>>
>> That doesn't cover the root bus case, where we currently assume the
>> host bridge can generate config accesses to all config space supported
>> by devices on the root bus.  But we don't have a problem there, so I
>> guess we don't need to worry about it now.
>>
>> If you can put it in pci_alloc_child_bus(), could you make your new
>> function return a boolean, e.g., pci_bus_ext_cfg_accessible(), or
>> similar, and then use the result to set the
>> PCI_BUS_FLAGS_COMPAT_CFG_SPACE flag?  Names like "*_check_*()" don't
>> tell the reader much about what's happening.
>>
>>>> I agree that pci_scan_bridge_extend() is already too complicated,
>>>> so would you be okay to only add one line to it :
>>>>    pci_bus_set_compat_cfg_space(child);
>>>> and put all the code I suggested in this new function
>>>> pci_bus_set_compat_cfg_space() ? (also supporting PCI-X Mode 2
>>>> devices)
>>>>
>>>> Improvement : this function can return immediately if the child
>>>> bus has already inherited the flag from its parent.
>>> I mean something like the attached patch I tested this morning...
>>> Sorry, this is for old kernel 4.1.35 but just to clarify what I
>>> propose (also applies to 4.16.6 by changing value of
>>> PCI_BUS_FLAGS_COMPAT_CFG_SPACE in pci.h to 8).
>>> --- include/linux/pci.h.orig    2018-03-26 16:51:18.050000000 +0000
>>> +++ include/linux/pci.h    2018-04-30 09:50:57.660000000 +0000
>>> @@ -193,6 +193,7 @@
>>>   enum pci_bus_flags {
>>>       PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>>>       PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
>>> +    PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
>>>   };
>>>     /* These values come from the PCI Express Spec */
>>> --- drivers/pci/probe.c.orig    2018-01-22 09:29:52.000000000 +0000
>>> +++ drivers/pci/probe.c    2018-04-30 13:29:50.600000000 +0000
>>> @@ -754,6 +754,35 @@
>>>                        PCI_EXP_RTCTL_CRSSVE);
>>>   }
>>>   +static void pci_bus_check_compat_cfg_space(struct pci_bus *bus)
>>> +{
>>> +    struct pci_dev *dev = bus->self;
>>> +    bool pci_compat_cfg_space = false;
>>> +    int pos;
>>> +    u32 status;
>>> +
>>> +    if (bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
>>> +        return;
>>> +
>>> +    if (!pci_is_pcie(dev) || /* PCI/PCI bridge */
>>> +        (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */
>>> +        (pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE)) { /* PCIe/PCI bridge in reverse mode */
>>> +        pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
>>> +        if (pos) {
>>> +            pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
>>> +            if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
>>> +                pci_compat_cfg_space = true;
>>> +        } else {
>>> +            pci_compat_cfg_space = true;
>>> +        }
>>> +        if (pci_compat_cfg_space) {
>>> +            dev_info(&dev->dev, "bus %02x limited to PCI-Compatible config space\n",
>>> +                 bus->number);
>>> +            bus->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
>>> +        }
>>> +    }
>>> +}
>>> +
>>>   /*
>>>    * If it's a bridge, configure it and scan the bus behind it.
>>>    * For CardBus bridges, we don't scan behind as the devices will
>>> @@ -827,6 +856,7 @@
>>>               child->primary = primary;
>>>               pci_bus_insert_busn_res(child, secondary, subordinate);
>>>               child->bridge_ctl = bctl;
>>> +            pci_bus_check_compat_cfg_space(child);
>>>           }
>>>             cmax = pci_scan_child_bus(child);
>>> @@ -1084,6 +1114,9 @@
>>>       u32 status;
>>>       u16 class;
>>>   +    if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE)
>>> +        return PCI_CFG_SPACE_SIZE;
>>> +
>>>       class = dev->class >> 8;
>>>       if (class == PCI_CLASS_BRIDGE_HOST)
>>>           return pci_cfg_space_size_ext(dev);
>>
> The inheritence is made by this line in pci_alloc_child_bus() :
>     child->bus_flags = parent->bus_flags;
> So once we detect a limitation on a bridge impacting a child bus and that we set the flag in child->bus_flags, this flag is 
> automatically present in the child->bus_flags of all its children buses.
>
> I agree with your remarks and will create a function named pci_bus_check_compat_cfg_space() that will be called from 
> pci_alloc_child_bus().
> I'll test that on Wednesday 2th and will give you my feedback.
Hi Bjorn,
See attached patch (tested ok this morning)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cfgspace3_4.1.35.patch
Type: text/x-patch
Size: 2246 bytes
Desc: cfgspace3_4.1.35.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180502/bd83d6f9/attachment.bin>

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Bjorn Helgaas]

On Wed, May 02, 2018 at 12:57:31PM +0000, Gilles Buloz wrote:
> Hi Bjorn,
> See attached patch (tested ok this morning)

This looks good.  Minor comments below.

I can fix minor things myself, but I do need a signed-off-by from you
before applying (see
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst)

Please add a changelog, too, and include the patch inline (as opposed
to as an attachment) if possible.

> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
> +++ include/linux/pci.h	2018-04-30 18:29:14.140000000 +0000
> @@ -193,6 +193,7 @@
>  enum pci_bus_flags {
>  	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>  	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
> +	PCI_BUS_FLAGS_NO_EXTCFG = (__force pci_bus_flags_t) 4,

Best if you can rebase this to v4.17-rc1.

>  };
>  
>  /* These values come from the PCI Express Spec */
> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
> +++ drivers/pci/probe.c	2018-05-02 13:44:35.530000000 +0000
> @@ -664,6 +664,23 @@
>  	}
>  }
>  
> +static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bridge)
> +{
> +	int pos;
> +	u32 status;
> +
> +	if (!pci_is_pcie(bridge) || /* PCI/PCI bridge */
> +	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */
> +	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PCIe/PCI bridge in reverse mode */
> +		pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
> +		if (pos)
> +			pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
> +		return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ));
> +	}

Please arrange this so everything fits in 80 columns.

If you can split it into several simpler "if" statements rather
than one with a complicated expression, that would also be good.

> +
> +	return true;
> +}
> +
>  static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  					   struct pci_dev *bridge, int busnr)
>  {
> @@ -725,6 +742,19 @@
>  	/* Create legacy_io and legacy_mem files for this bus */
>  	pci_create_legacy_files(child);
>  
> +	/*
> +	 * if bus_flags inherited from parent bus do not already report lack of extended config
> +	 * space support, check if supported by child bus by checking its parent bridge
> +	 */

Wrap to fit in 80 columns.

> +	if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)) {

The double negative makes this a little bit hard to read.  Maybe it
could be improved by reversing the sense of something?

> +		if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) {
> +			child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
> +			dev_info(&child->dev, "extended config space not accessible due to parent bridge\n");

In v4.17-rc1, there's a pci_info() that should work here (instead of
dev_info()).

> +		}
> +	} else {
> +		dev_info(&child->dev, "extended config space not accessible due to parent bus\n");
> +	}
> +
>  	return child;
>  }
>  
> @@ -1084,6 +1114,9 @@
>  	u32 status;
>  	u16 class;
>  
> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> +		return PCI_CFG_SPACE_SIZE;
> +
>  	class = dev->class >> 8;
>  	if (class == PCI_CLASS_BRIDGE_HOST)
>  		return pci_cfg_space_size_ext(dev);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

LS1043A : "synchronous abort" at boot due to PCI config read

[Bjorn Helgaas]

On Wed, May 02, 2018 at 12:57:31PM +0000, Gilles Buloz wrote:
> Hi Bjorn,
> See attached patch (tested ok this morning)

This looks good.  Minor comments below.

I can fix minor things myself, but I do need a signed-off-by from you
before applying (see
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst)

Please add a changelog, too, and include the patch inline (as opposed
to as an attachment) if possible.

> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
> +++ include/linux/pci.h	2018-04-30 18:29:14.140000000 +0000
> @@ -193,6 +193,7 @@
>  enum pci_bus_flags {
>  	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>  	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
> +	PCI_BUS_FLAGS_NO_EXTCFG = (__force pci_bus_flags_t) 4,

Best if you can rebase this to v4.17-rc1.

>  };
>  
>  /* These values come from the PCI Express Spec */
> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
> +++ drivers/pci/probe.c	2018-05-02 13:44:35.530000000 +0000
> @@ -664,6 +664,23 @@
>  	}
>  }
>  
> +static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bridge)
> +{
> +	int pos;
> +	u32 status;
> +
> +	if (!pci_is_pcie(bridge) || /* PCI/PCI bridge */
> +	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */
> +	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PCIe/PCI bridge in reverse mode */
> +		pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
> +		if (pos)
> +			pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
> +		return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ));
> +	}

Please arrange this so everything fits in 80 columns.

If you can split it into several simpler "if" statements rather
than one with a complicated expression, that would also be good.

> +
> +	return true;
> +}
> +
>  static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  					   struct pci_dev *bridge, int busnr)
>  {
> @@ -725,6 +742,19 @@
>  	/* Create legacy_io and legacy_mem files for this bus */
>  	pci_create_legacy_files(child);
>  
> +	/*
> +	 * if bus_flags inherited from parent bus do not already report lack of extended config
> +	 * space support, check if supported by child bus by checking its parent bridge
> +	 */

Wrap to fit in 80 columns.

> +	if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)) {

The double negative makes this a little bit hard to read.  Maybe it
could be improved by reversing the sense of something?

> +		if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) {
> +			child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
> +			dev_info(&child->dev, "extended config space not accessible due to parent bridge\n");

In v4.17-rc1, there's a pci_info() that should work here (instead of
dev_info()).

> +		}
> +	} else {
> +		dev_info(&child->dev, "extended config space not accessible due to parent bus\n");
> +	}
> +
>  	return child;
>  }
>  
> @@ -1084,6 +1114,9 @@
>  	u32 status;
>  	u16 class;
>  
> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> +		return PCI_CFG_SPACE_SIZE;
> +
>  	class = dev->class >> 8;
>  	if (class == PCI_CLASS_BRIDGE_HOST)
>  		return pci_cfg_space_size_ext(dev);

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Le 02/05/2018 15:26, Bjorn Helgaas a =E9crit :
> On Wed, May 02, 2018 at 12:57:31PM +0000, Gilles Buloz wrote:
>> Hi Bjorn,
>> See attached patch (tested ok this morning)
> This looks good.  Minor comments below.
>
> I can fix minor things myself, but I do need a signed-off-by from you
> before applying (see
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Docu=
mentation/process/submitting-patches.rst)
>
> Please add a changelog, too, and include the patch inline (as opposed
> to as an attachment) if possible.
>
>> --- include/linux/pci.h.orig=092018-03-26 16:51:18.050000000 +0000
>> +++ include/linux/pci.h=092018-04-30 18:29:14.140000000 +0000
>> @@ -193,6 +193,7 @@
>>   enum pci_bus_flags {
>>   =09PCI_BUS_FLAGS_NO_MSI   =3D (__force pci_bus_flags_t) 1,
>>   =09PCI_BUS_FLAGS_NO_MMRBC =3D (__force pci_bus_flags_t) 2,
>> +=09PCI_BUS_FLAGS_NO_EXTCFG =3D (__force pci_bus_flags_t) 4,
> Best if you can rebase this to v4.17-rc1.
>
>>   };
>>  =20
>>   /* These values come from the PCI Express Spec */
>> --- drivers/pci/probe.c.orig=092018-01-22 09:29:52.000000000 +0000
>> +++ drivers/pci/probe.c=092018-05-02 13:44:35.530000000 +0000
>> @@ -664,6 +664,23 @@
>>   =09}
>>   }
>>  =20
>> +static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bri=
dge)
>> +{
>> +=09int pos;
>> +=09u32 status;
>> +
>> +=09if (!pci_is_pcie(bridge) || /* PCI/PCI bridge */
>> +=09    (pci_pcie_type(bridge) =3D=3D PCI_EXP_TYPE_PCIE_BRIDGE) || /* PC=
Ie/PCI bridge in forward mode */
>> +=09    (pci_pcie_type(bridge) =3D=3D PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PC=
Ie/PCI bridge in reverse mode */
>> +=09=09pos =3D pci_find_capability(bridge, PCI_CAP_ID_PCIX);
>> +=09=09if (pos)
>> +=09=09=09pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
>> +=09=09return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MH=
Z));
>> +=09}
> Please arrange this so everything fits in 80 columns.
>
> If you can split it into several simpler "if" statements rather
> than one with a complicated expression, that would also be good.
>
>> +
>> +=09return true;
>> +}
>> +
>>   static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>>   =09=09=09=09=09   struct pci_dev *bridge, int busnr)
>>   {
>> @@ -725,6 +742,19 @@
>>   =09/* Create legacy_io and legacy_mem files for this bus */
>>   =09pci_create_legacy_files(child);
>>  =20
>> +=09/*
>> +=09 * if bus_flags inherited from parent bus do not already report lack=
 of extended config
>> +=09 * space support, check if supported by child bus by checking its pa=
rent bridge
>> +=09 */
> Wrap to fit in 80 columns.
>
>> +=09if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)) {
> The double negative makes this a little bit hard to read.  Maybe it
> could be improved by reversing the sense of something?
>
>> +=09=09if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) {
>> +=09=09=09child->bus_flags |=3D PCI_BUS_FLAGS_NO_EXTCFG;
>> +=09=09=09dev_info(&child->dev, "extended config space not accessible du=
e to parent bridge\n");
> In v4.17-rc1, there's a pci_info() that should work here (instead of
> dev_info()).
>
>> +=09=09}
>> +=09} else {
>> +=09=09dev_info(&child->dev, "extended config space not accessible due t=
o parent bus\n");
>> +=09}
>> +
>>   =09return child;
>>   }
>>  =20
>> @@ -1084,6 +1114,9 @@
>>   =09u32 status;
>>   =09u16 class;
>>  =20
>> +=09if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
>> +=09=09return PCI_CFG_SPACE_SIZE;
>> +
>>   =09class =3D dev->class >> 8;
>>   =09if (class =3D=3D PCI_CLASS_BRIDGE_HOST)
>>   =09=09return pci_cfg_space_size_ext(dev);
> .
>
OK I'm going to learn about signing (sorry this is my first "official" patc=
h).
I'll download kernel v4.17-rc1 and write the patch for it; however I hope I=
'll be able to test it on my platform without the=20
freescale addons I have on 4.1.35, because I don't want to send an untested=
 patch.
For "if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG))", I don't=
 understand what you mean with "double negative", as I=20
only have one "!"

Do you think it's worth keeping the two dev_info() ? The code would be smal=
ler without; however this may help to have it for debug.=20
Maybe use _dbg instead of _info ?

LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Le 02/05/2018 15:26, Bjorn Helgaas a ?crit :
> On Wed, May 02, 2018 at 12:57:31PM +0000, Gilles Buloz wrote:
>> Hi Bjorn,
>> See attached patch (tested ok this morning)
> This looks good.  Minor comments below.
>
> I can fix minor things myself, but I do need a signed-off-by from you
> before applying (see
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst)
>
> Please add a changelog, too, and include the patch inline (as opposed
> to as an attachment) if possible.
>
>> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
>> +++ include/linux/pci.h	2018-04-30 18:29:14.140000000 +0000
>> @@ -193,6 +193,7 @@
>>   enum pci_bus_flags {
>>   	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
>>   	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
>> +	PCI_BUS_FLAGS_NO_EXTCFG = (__force pci_bus_flags_t) 4,
> Best if you can rebase this to v4.17-rc1.
>
>>   };
>>   
>>   /* These values come from the PCI Express Spec */
>> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
>> +++ drivers/pci/probe.c	2018-05-02 13:44:35.530000000 +0000
>> @@ -664,6 +664,23 @@
>>   	}
>>   }
>>   
>> +static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bridge)
>> +{
>> +	int pos;
>> +	u32 status;
>> +
>> +	if (!pci_is_pcie(bridge) || /* PCI/PCI bridge */
>> +	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */
>> +	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PCIe/PCI bridge in reverse mode */
>> +		pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
>> +		if (pos)
>> +			pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
>> +		return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ));
>> +	}
> Please arrange this so everything fits in 80 columns.
>
> If you can split it into several simpler "if" statements rather
> than one with a complicated expression, that would also be good.
>
>> +
>> +	return true;
>> +}
>> +
>>   static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>>   					   struct pci_dev *bridge, int busnr)
>>   {
>> @@ -725,6 +742,19 @@
>>   	/* Create legacy_io and legacy_mem files for this bus */
>>   	pci_create_legacy_files(child);
>>   
>> +	/*
>> +	 * if bus_flags inherited from parent bus do not already report lack of extended config
>> +	 * space support, check if supported by child bus by checking its parent bridge
>> +	 */
> Wrap to fit in 80 columns.
>
>> +	if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)) {
> The double negative makes this a little bit hard to read.  Maybe it
> could be improved by reversing the sense of something?
>
>> +		if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) {
>> +			child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
>> +			dev_info(&child->dev, "extended config space not accessible due to parent bridge\n");
> In v4.17-rc1, there's a pci_info() that should work here (instead of
> dev_info()).
>
>> +		}
>> +	} else {
>> +		dev_info(&child->dev, "extended config space not accessible due to parent bus\n");
>> +	}
>> +
>>   	return child;
>>   }
>>   
>> @@ -1084,6 +1114,9 @@
>>   	u32 status;
>>   	u16 class;
>>   
>> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
>> +		return PCI_CFG_SPACE_SIZE;
>> +
>>   	class = dev->class >> 8;
>>   	if (class == PCI_CLASS_BRIDGE_HOST)
>>   		return pci_cfg_space_size_ext(dev);
> .
>
OK I'm going to learn about signing (sorry this is my first "official" patch).
I'll download kernel v4.17-rc1 and write the patch for it; however I hope I'll be able to test it on my platform without the 
freescale addons I have on 4.1.35, because I don't want to send an untested patch.
For "if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG))", I don't understand what you mean with "double negative", as I 
only have one "!"

Do you think it's worth keeping the two dev_info() ? The code would be smaller without; however this may help to have it for debug. 
Maybe use _dbg instead of _info ?

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Bjorn Helgaas]

On Wed, May 02, 2018 at 01:48:27PM +0000, Gilles Buloz wrote:
> Le 02/05/2018 15:26, Bjorn Helgaas a =E9crit :
> > On Wed, May 02, 2018 at 12:57:31PM +0000, Gilles Buloz wrote:
> >> Hi Bjorn,
> >> See attached patch (tested ok this morning)
> > This looks good.  Minor comments below.
> >
> > I can fix minor things myself, but I do need a signed-off-by from you
> > before applying (see
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Do=
cumentation/process/submitting-patches.rst)
> >
> > Please add a changelog, too, and include the patch inline (as opposed
> > to as an attachment) if possible.
> >
> >> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
> >> +++ include/linux/pci.h	2018-04-30 18:29:14.140000000 +0000
> >> @@ -193,6 +193,7 @@
> >>   enum pci_bus_flags {
> >>   	PCI_BUS_FLAGS_NO_MSI   =3D (__force pci_bus_flags_t) 1,
> >>   	PCI_BUS_FLAGS_NO_MMRBC =3D (__force pci_bus_flags_t) 2,
> >> +	PCI_BUS_FLAGS_NO_EXTCFG =3D (__force pci_bus_flags_t) 4,
> > Best if you can rebase this to v4.17-rc1.
> >
> >>   };
> >>   =

> >>   /* These values come from the PCI Express Spec */
> >> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
> >> +++ drivers/pci/probe.c	2018-05-02 13:44:35.530000000 +0000
> >> @@ -664,6 +664,23 @@
> >>   	}
> >>   }
> >>   =

> >> +static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *b=
ridge)
> >> +{
> >> +	int pos;
> >> +	u32 status;
> >> +
> >> +	if (!pci_is_pcie(bridge) || /* PCI/PCI bridge */
> >> +	    (pci_pcie_type(bridge) =3D=3D PCI_EXP_TYPE_PCIE_BRIDGE) || /* PC=
Ie/PCI bridge in forward mode */
> >> +	    (pci_pcie_type(bridge) =3D=3D PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PC=
Ie/PCI bridge in reverse mode */
> >> +		pos =3D pci_find_capability(bridge, PCI_CAP_ID_PCIX);
> >> +		if (pos)
> >> +			pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
> >> +		return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)=
);
> >> +	}
> > Please arrange this so everything fits in 80 columns.
> >
> > If you can split it into several simpler "if" statements rather
> > than one with a complicated expression, that would also be good.
> >
> >> +
> >> +	return true;
> >> +}
> >> +
> >>   static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> >>   					   struct pci_dev *bridge, int busnr)
> >>   {
> >> @@ -725,6 +742,19 @@
> >>   	/* Create legacy_io and legacy_mem files for this bus */
> >>   	pci_create_legacy_files(child);
> >>   =

> >> +	/*
> >> +	 * if bus_flags inherited from parent bus do not already report lack=
 of extended config
> >> +	 * space support, check if supported by child bus by checking its pa=
rent bridge
> >> +	 */
> > Wrap to fit in 80 columns.
> >
> >> +	if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)) {
> > The double negative makes this a little bit hard to read.  Maybe it
> > could be improved by reversing the sense of something?
> >
> >> +		if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) {
> >> +			child->bus_flags |=3D PCI_BUS_FLAGS_NO_EXTCFG;
> >> +			dev_info(&child->dev, "extended config space not accessible due to=
 parent bridge\n");
> > In v4.17-rc1, there's a pci_info() that should work here (instead of
> > dev_info()).
> >
> >> +		}
> >> +	} else {
> >> +		dev_info(&child->dev, "extended config space not accessible due to =
parent bus\n");
> >> +	}
> >> +
> >>   	return child;
> >>   }
> >>   =

> >> @@ -1084,6 +1114,9 @@
> >>   	u32 status;
> >>   	u16 class;
> >>   =

> >> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> >> +		return PCI_CFG_SPACE_SIZE;
> >> +
> >>   	class =3D dev->class >> 8;
> >>   	if (class =3D=3D PCI_CLASS_BRIDGE_HOST)
> >>   		return pci_cfg_space_size_ext(dev);
> > .
> >
> OK I'm going to learn about signing (sorry this is my first
> "official" patch).

Great, welcome!  The signoff is no big deal -- it's just plain text
(no crypto signature or anything) and it's basically just an assertion
that you wrote it and have the right to contribute it.

> I'll download kernel v4.17-rc1 and write the patch for it; however I
> hope I'll be able to test it on my platform without the freescale
> addons I have on 4.1.35, because I don't want to send an untested
> patch.

Don't worry too much about the 4.1 vs 4.17 issue.  If you tested it
on 4.1.35 that should be fine.

> For "if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG))",
> I don't understand what you mean with "double negative", as I only
> have one "!"

The "!" and the "NO" part of "NO_EXTCFG" is what I meant.  E.g., maybe
the flag could be something like "COMPAT_CFG_ONLY" so there's no
negation in the test at all.

> Do you think it's worth keeping the two dev_info() ? The code would
> be smaller without; however this may help to have it for debug.
> Maybe use _dbg instead of _info ?

Probably one pci_info() is enough as a clue that extended config space =

isn't available below this point in the hierarchy.

I personally don't like the _dbg() version because it's so complicated
to figure out when the output is enabled.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

LS1043A : "synchronous abort" at boot due to PCI config read

[Bjorn Helgaas]

On Wed, May 02, 2018 at 01:48:27PM +0000, Gilles Buloz wrote:
> Le 02/05/2018 15:26, Bjorn Helgaas a ?crit :
> > On Wed, May 02, 2018 at 12:57:31PM +0000, Gilles Buloz wrote:
> >> Hi Bjorn,
> >> See attached patch (tested ok this morning)
> > This looks good.  Minor comments below.
> >
> > I can fix minor things myself, but I do need a signed-off-by from you
> > before applying (see
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst)
> >
> > Please add a changelog, too, and include the patch inline (as opposed
> > to as an attachment) if possible.
> >
> >> --- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
> >> +++ include/linux/pci.h	2018-04-30 18:29:14.140000000 +0000
> >> @@ -193,6 +193,7 @@
> >>   enum pci_bus_flags {
> >>   	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
> >>   	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
> >> +	PCI_BUS_FLAGS_NO_EXTCFG = (__force pci_bus_flags_t) 4,
> > Best if you can rebase this to v4.17-rc1.
> >
> >>   };
> >>   
> >>   /* These values come from the PCI Express Spec */
> >> --- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
> >> +++ drivers/pci/probe.c	2018-05-02 13:44:35.530000000 +0000
> >> @@ -664,6 +664,23 @@
> >>   	}
> >>   }
> >>   
> >> +static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bridge)
> >> +{
> >> +	int pos;
> >> +	u32 status;
> >> +
> >> +	if (!pci_is_pcie(bridge) || /* PCI/PCI bridge */
> >> +	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */
> >> +	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)) {  /* PCIe/PCI bridge in reverse mode */
> >> +		pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
> >> +		if (pos)
> >> +			pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
> >> +		return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ));
> >> +	}
> > Please arrange this so everything fits in 80 columns.
> >
> > If you can split it into several simpler "if" statements rather
> > than one with a complicated expression, that would also be good.
> >
> >> +
> >> +	return true;
> >> +}
> >> +
> >>   static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> >>   					   struct pci_dev *bridge, int busnr)
> >>   {
> >> @@ -725,6 +742,19 @@
> >>   	/* Create legacy_io and legacy_mem files for this bus */
> >>   	pci_create_legacy_files(child);
> >>   
> >> +	/*
> >> +	 * if bus_flags inherited from parent bus do not already report lack of extended config
> >> +	 * space support, check if supported by child bus by checking its parent bridge
> >> +	 */
> > Wrap to fit in 80 columns.
> >
> >> +	if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)) {
> > The double negative makes this a little bit hard to read.  Maybe it
> > could be improved by reversing the sense of something?
> >
> >> +		if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) {
> >> +			child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
> >> +			dev_info(&child->dev, "extended config space not accessible due to parent bridge\n");
> > In v4.17-rc1, there's a pci_info() that should work here (instead of
> > dev_info()).
> >
> >> +		}
> >> +	} else {
> >> +		dev_info(&child->dev, "extended config space not accessible due to parent bus\n");
> >> +	}
> >> +
> >>   	return child;
> >>   }
> >>   
> >> @@ -1084,6 +1114,9 @@
> >>   	u32 status;
> >>   	u16 class;
> >>   
> >> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> >> +		return PCI_CFG_SPACE_SIZE;
> >> +
> >>   	class = dev->class >> 8;
> >>   	if (class == PCI_CLASS_BRIDGE_HOST)
> >>   		return pci_cfg_space_size_ext(dev);
> > .
> >
> OK I'm going to learn about signing (sorry this is my first
> "official" patch).

Great, welcome!  The signoff is no big deal -- it's just plain text
(no crypto signature or anything) and it's basically just an assertion
that you wrote it and have the right to contribute it.

> I'll download kernel v4.17-rc1 and write the patch for it; however I
> hope I'll be able to test it on my platform without the freescale
> addons I have on 4.1.35, because I don't want to send an untested
> patch.

Don't worry too much about the 4.1 vs 4.17 issue.  If you tested it
on 4.1.35 that should be fine.

> For "if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG))",
> I don't understand what you mean with "double negative", as I only
> have one "!"

The "!" and the "NO" part of "NO_EXTCFG" is what I meant.  E.g., maybe
the flag could be something like "COMPAT_CFG_ONLY" so there's no
negation in the test at all.

> Do you think it's worth keeping the two dev_info() ? The code would
> be smaller without; however this may help to have it for debug.
> Maybe use _dbg instead of _info ?

Probably one pci_info() is enough as a clue that extended config space 
isn't available below this point in the hierarchy.

I personally don't like the _dbg() version because it's so complicated
to figure out when the output is enabled.

Bjorn

Re: LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Subject:    [PATCH] For exception at PCI probe due to bridge reporting UR

Even if a device supports extended config access, no such access must be
done to this device If there's a bridge not supporting that in the path
to this device. Doing such access with UR reporting enabled on the root
bridge leads to an exception.

This is the case on a LS1043A CPU (NXP QorIQ Layerscape) platform with
the following bus topology :
  LS1043 PCIe root
    -> PEX8112 PCIe-to-PCI bridge (not supporting ext cfg on PCI side)
      -> PMC slot connector (for legacy PMC modules)
With a PMC module topology as follows :
  PMC connector
    -> PCI-to-PCIe bridge
      -> PCIe switch (4 ports)
        -> 4 PCIe devices (one on each port)
In this case all devices behind the PEX8112 are supporting extended config
access but this is prohibited by the PEX8112. Without this patch, an
exception (synchronous abort) occurs in pci_cfg_space_size_ext().

This patch checks the parent bridge of each allocated child bus to know if
extended config access is supported on the child bus, and sets a flag in
child->bus_flags if not supported. This  flag is inherited by all children
buses of this child bus and then is checked to avoid this unsupported
accesses to every device on these buses.

Thanks

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
Gilles BULOZ
Senior software engineer
Kontron France
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D


Signed-off-by: Gilles Buloz <gilles.buloz@kontron.com>

--- linux-4.17-rc1/include/linux/pci.h.orig=092018-04-16 01:24:20.000000000=
 +0000
+++ linux-4.17-rc1/include/linux/pci.h=092018-05-03 09:53:03.270000000 +000=
0
@@ -217,6 +217,7 @@ enum pci_bus_flags {
  =09PCI_BUS_FLAGS_NO_MSI=09=3D (__force pci_bus_flags_t) 1,
  =09PCI_BUS_FLAGS_NO_MMRBC=09=3D (__force pci_bus_flags_t) 2,
  =09PCI_BUS_FLAGS_NO_AERSID=09=3D (__force pci_bus_flags_t) 4,
+=09PCI_BUS_FLAGS_NO_EXTCFG=09=3D (__force pci_bus_flags_t) 8,
  };
 =20
  /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */
--- linux-4.17-rc1/drivers/pci/probe.c.orig=092018-05-03 09:45:21.110000000=
 +0000
+++ linux-4.17-rc1/drivers/pci/probe.c=092018-05-03 09:46:50.550000000 +000=
0
@@ -882,6 +882,24 @@ free:
  =09return err;
  }
 =20
+static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bridge=
)
+{
+=09int pos;
+=09u32 status;
+
+=09if (pci_is_pcie(bridge) &&
+=09    (pci_pcie_type(bridge) !=3D PCI_EXP_TYPE_PCIE_BRIDGE) &&
+=09    (pci_pcie_type(bridge) !=3D PCI_EXP_TYPE_PCI_BRIDGE))
+=09=09return true;
+
+=09/* PCI/PCI, or PCIe/PCI (forward), or PCI/PCIe (reverse) bridge */
+=09pos =3D pci_find_capability(bridge, PCI_CAP_ID_PCIX);
+=09if (pos)
+=09=09pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
+
+=09return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ));
+}
+
  static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
  =09=09=09=09=09   struct pci_dev *bridge, int busnr)
  {
@@ -930,6 +948,20 @@ static struct pci_bus *pci_alloc_child_b
  =09}
  =09bridge->subordinate =3D child;
 =20
+=09/*
+=09 * if bus_flags inherited from parent bus do not already report lack of
+=09 * extended config space support, check if supported by child bus by
+=09 * checking its parent bridge
+=09 */
+=09if (child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG) {
+=09=09pci_info(child, "extended config space not accessible due to parent =
bus\n");
+=09} else {
+=09=09if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) {
+=09=09=09child->bus_flags |=3D PCI_BUS_FLAGS_NO_EXTCFG;
+=09=09=09pci_info(child, "extended config space not accessible due to pare=
nt bridge\n");
+=09=09}
+=09}
+
  add_dev:
  =09pci_set_bus_msi_domain(child);
  =09ret =3D device_register(&child->dev);
@@ -1393,6 +1425,9 @@ int pci_cfg_space_size(struct pci_dev *d
  =09u32 status;
  =09u16 class;
 =20
+=09if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
+=09=09return PCI_CFG_SPACE_SIZE;
+
  =09class =3D dev->class >> 8;
  =09if (class =3D=3D PCI_CLASS_BRIDGE_HOST)
  =09=09return pci_cfg_space_size_ext(dev);

LS1043A : "synchronous abort" at boot due to PCI config read

[Gilles Buloz]

Subject:    [PATCH] For exception at PCI probe due to bridge reporting UR

Even if a device supports extended config access, no such access must be
done to this device If there's a bridge not supporting that in the path
to this device. Doing such access with UR reporting enabled on the root
bridge leads to an exception.

This is the case on a LS1043A CPU (NXP QorIQ Layerscape) platform with
the following bus topology :
  LS1043 PCIe root
    -> PEX8112 PCIe-to-PCI bridge (not supporting ext cfg on PCI side)
      -> PMC slot connector (for legacy PMC modules)
With a PMC module topology as follows :
  PMC connector
    -> PCI-to-PCIe bridge
      -> PCIe switch (4 ports)
        -> 4 PCIe devices (one on each port)
In this case all devices behind the PEX8112 are supporting extended config
access but this is prohibited by the PEX8112. Without this patch, an
exception (synchronous abort) occurs in pci_cfg_space_size_ext().

This patch checks the parent bridge of each allocated child bus to know if
extended config access is supported on the child bus, and sets a flag in
child->bus_flags if not supported. This  flag is inherited by all children
buses of this child bus and then is checked to avoid this unsupported
accesses to every device on these buses.

Thanks

=====================
Gilles BULOZ
Senior software engineer
Kontron France
=====================


Signed-off-by: Gilles Buloz <gilles.buloz@kontron.com>

--- linux-4.17-rc1/include/linux/pci.h.orig	2018-04-16 01:24:20.000000000 +0000
+++ linux-4.17-rc1/include/linux/pci.h	2018-05-03 09:53:03.270000000 +0000
@@ -217,6 +217,7 @@ enum pci_bus_flags {
  	PCI_BUS_FLAGS_NO_MSI	= (__force pci_bus_flags_t) 1,
  	PCI_BUS_FLAGS_NO_MMRBC	= (__force pci_bus_flags_t) 2,
  	PCI_BUS_FLAGS_NO_AERSID	= (__force pci_bus_flags_t) 4,
+	PCI_BUS_FLAGS_NO_EXTCFG	= (__force pci_bus_flags_t) 8,
  };
  
  /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */
--- linux-4.17-rc1/drivers/pci/probe.c.orig	2018-05-03 09:45:21.110000000 +0000
+++ linux-4.17-rc1/drivers/pci/probe.c	2018-05-03 09:46:50.550000000 +0000
@@ -882,6 +882,24 @@ free:
  	return err;
  }
  
+static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bridge)
+{
+	int pos;
+	u32 status;
+
+	if (pci_is_pcie(bridge) &&
+	    (pci_pcie_type(bridge) != PCI_EXP_TYPE_PCIE_BRIDGE) &&
+	    (pci_pcie_type(bridge) != PCI_EXP_TYPE_PCI_BRIDGE))
+		return true;
+
+	/* PCI/PCI, or PCIe/PCI (forward), or PCI/PCIe (reverse) bridge */
+	pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
+	if (pos)
+		pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
+
+	return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ));
+}
+
  static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
  					   struct pci_dev *bridge, int busnr)
  {
@@ -930,6 +948,20 @@ static struct pci_bus *pci_alloc_child_b
  	}
  	bridge->subordinate = child;
  
+	/*
+	 * if bus_flags inherited from parent bus do not already report lack of
+	 * extended config space support, check if supported by child bus by
+	 * checking its parent bridge
+	 */
+	if (child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG) {
+		pci_info(child, "extended config space not accessible due to parent bus\n");
+	} else {
+		if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) {
+			child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
+			pci_info(child, "extended config space not accessible due to parent bridge\n");
+		}
+	}
+
  add_dev:
  	pci_set_bus_msi_domain(child);
  	ret = device_register(&child->dev);
@@ -1393,6 +1425,9 @@ int pci_cfg_space_size(struct pci_dev *d
  	u32 status;
  	u16 class;
  
+	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
+		return PCI_CFG_SPACE_SIZE;
+
  	class = dev->class >> 8;
  	if (class == PCI_CLASS_BRIDGE_HOST)
  		return pci_cfg_space_size_ext(dev);

[PATCH] PCI: Check whether bridges allow access to extended config space

[Bjorn Helgaas]

[+cc LKML]

On Thu, May 03, 2018 at 12:40:27PM +0000, Gilles Buloz wrote:
> Subject:    [PATCH] For exception at PCI probe due to bridge reporting UR
> 
> Even if a device supports extended config access, no such access must be
> done to this device If there's a bridge not supporting that in the path
> to this device. Doing such access with UR reporting enabled on the root
> bridge leads to an exception.
> 
> This is the case on a LS1043A CPU (NXP QorIQ Layerscape) platform with
> the following bus topology :
>   LS1043 PCIe root
>     -> PEX8112 PCIe-to-PCI bridge (not supporting ext cfg on PCI side)
>       -> PMC slot connector (for legacy PMC modules)
> With a PMC module topology as follows :
>   PMC connector
>     -> PCI-to-PCIe bridge
>       -> PCIe switch (4 ports)
>         -> 4 PCIe devices (one on each port)
> In this case all devices behind the PEX8112 are supporting extended config
> access but this is prohibited by the PEX8112. Without this patch, an
> exception (synchronous abort) occurs in pci_cfg_space_size_ext().
> 
> This patch checks the parent bridge of each allocated child bus to know if
> extended config access is supported on the child bus, and sets a flag in
> child->bus_flags if not supported. This  flag is inherited by all children
> buses of this child bus and then is checked to avoid this unsupported
> accesses to every device on these buses.

Hi Gilles,

Thanks for the patch!  I reworked it a little bit to simplify the code
in pci_alloc_child_bus().  Can you test it and make sure I didn't
break anything?


commit cbaaa85b558a8f974e301fa0364d568efaf491ce
Author: Gilles Buloz <Gilles.Buloz@kontron.com>
Date:   Thu May 3 15:21:44 2018 -0500

    PCI: Check whether bridges allow access to extended config space
    
    Even if a device supports extended config space, i.e., it is a PCI-X Mode 2
    or a PCI Express device, the extended space may not be accessible if
    there's a conventional PCI bus in the path to it.
    
    We currently figure that out in pci_cfg_space_size() by reading the first
    dword of extended config space.  On most platforms that returns ~0 data if
    the space is inaccessible, but it may set error bits in PCI status
    registers, and on some platforms it causes exceptions that we currently
    don't recover from.
    
    For example, a PCIe-to-conventional PCI bridge treats config transactions
    with a non-zero Extended Register Address as an Unsupported Request on PCIe
    and a received Master-Abort on the destination bus (see PCI Express to
    PCI/PCI-X Bridge spec, r1.0, sec 4.1.3).
    
    A sample case is a LS1043A CPU (NXP QorIQ Layerscape) platform with the
    following bus topology:
    
      LS1043 PCIe Root Port
        -> PEX8112 PCIe-to-PCI bridge (doesn't support ext cfg on PCI side)
          -> PMC slot connector (for legacy PMC modules)
    
    With a PMC module topology as follows:
    
      PMC connector
        -> PCI-to-PCIe bridge
          -> PCIe switch (4 ports)
            -> 4 PCIe devices (one on each port)
    
    The PCIe devices on the PMC module support extended config space, but we
    can't reach it because the PEX8112 can't generate accesses to the extended
    space on its secondary bus.  Attempts to access it cause Unsupported
    Request errors, which result in synchronous aborts on this platform.
    
    To avoid these errors, check whether bridges are capable of generating
    extended config space addresses on their secondary interfaces.  If they
    can't, we restrict devices below the bridge to only the 256-byte
    PCI-compatible config space.
    
    Signed-off-by: Gilles Buloz <gilles.buloz@kontron.com>
    [bhelgaas: changelog, rework patch so bus_flags testing is all in
    pci_bridge_child_ext_cfg_accessible()]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac91b6fd0bcd..7b1b7b2e01e4 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -882,6 +882,45 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	return err;
 }
 
+static bool pci_bridge_child_ext_cfg_accessible(struct pci_dev *bridge)
+{
+	int pos;
+	u32 status;
+
+	/*
+	 * If extended config space isn't accessible on a bridge's primary
+	 * bus, we certainly can't access it on the secondary bus.
+	 */
+	if (bridge->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
+		return false;
+
+	/*
+	 * PCIe Root Ports and switch ports are PCIe on both sides, so if
+	 * extended config space is accessible on the primary, it's also
+	 * accessible on the secondary.
+	 */
+	if (pci_is_pcie(bridge) &&
+	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT ||
+	     pci_pcie_type(bridge) == PCI_EXP_TYPE_UPSTREAM ||
+	     pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM))
+		return true;
+
+	/*
+	 * For the other bridge types:
+	 *   - PCI-to-PCI bridges
+	 *   - PCIe-to-PCI/PCI-X forward bridges
+	 *   - PCI/PCI-X-to-PCIe reverse bridges
+	 * extended config space on the secondary side is only accessible
+	 * if the bridge supports PCI-X Mode 2.
+	 */
+	pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
+	if (!pos)
+		return false;
+
+	pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
+	return status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ);
+}
+
 static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 					   struct pci_dev *bridge, int busnr)
 {
@@ -923,6 +962,16 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	pci_set_bus_of_node(child);
 	pci_set_bus_speed(child);
 
+	/*
+	 * Check whether extended config space is accessible on the child
+	 * bus.  Note that we currently assume it is always accessible on
+	 * the root bus.
+	 */
+	if (!pci_bridge_child_ext_cfg_accessible(bridge)) {
+		child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
+		pci_info(child, "extended config space not accessible on secondary bus\n");
+	}
+
 	/* Set up default resource pointers and names */
 	for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
 		child->resource[i] = &bridge->resource[PCI_BRIDGE_RESOURCES+i];
@@ -1393,6 +1442,9 @@ int pci_cfg_space_size(struct pci_dev *dev)
 	u32 status;
 	u16 class;
 
+	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
+		return PCI_CFG_SPACE_SIZE;
+
 	class = dev->class >> 8;
 	if (class == PCI_CLASS_BRIDGE_HOST)
 		return pci_cfg_space_size_ext(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 230615620a4a..f7aa6d9f8999 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -217,6 +217,7 @@ enum pci_bus_flags {
 	PCI_BUS_FLAGS_NO_MSI	= (__force pci_bus_flags_t) 1,
 	PCI_BUS_FLAGS_NO_MMRBC	= (__force pci_bus_flags_t) 2,
 	PCI_BUS_FLAGS_NO_AERSID	= (__force pci_bus_flags_t) 4,
+	PCI_BUS_FLAGS_NO_EXTCFG	= (__force pci_bus_flags_t) 8,
 };
 
 /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */

[PATCH] PCI: Check whether bridges allow access to extended config space

[Bjorn Helgaas]

[+cc LKML]

On Thu, May 03, 2018 at 12:40:27PM +0000, Gilles Buloz wrote:
> Subject:    [PATCH] For exception at PCI probe due to bridge reporting UR
> 
> Even if a device supports extended config access, no such access must be
> done to this device If there's a bridge not supporting that in the path
> to this device. Doing such access with UR reporting enabled on the root
> bridge leads to an exception.
> 
> This is the case on a LS1043A CPU (NXP QorIQ Layerscape) platform with
> the following bus topology :
>   LS1043 PCIe root
>     -> PEX8112 PCIe-to-PCI bridge (not supporting ext cfg on PCI side)
>       -> PMC slot connector (for legacy PMC modules)
> With a PMC module topology as follows :
>   PMC connector
>     -> PCI-to-PCIe bridge
>       -> PCIe switch (4 ports)
>         -> 4 PCIe devices (one on each port)
> In this case all devices behind the PEX8112 are supporting extended config
> access but this is prohibited by the PEX8112. Without this patch, an
> exception (synchronous abort) occurs in pci_cfg_space_size_ext().
> 
> This patch checks the parent bridge of each allocated child bus to know if
> extended config access is supported on the child bus, and sets a flag in
> child->bus_flags if not supported. This  flag is inherited by all children
> buses of this child bus and then is checked to avoid this unsupported
> accesses to every device on these buses.

Hi Gilles,

Thanks for the patch!  I reworked it a little bit to simplify the code
in pci_alloc_child_bus().  Can you test it and make sure I didn't
break anything?


commit cbaaa85b558a8f974e301fa0364d568efaf491ce
Author: Gilles Buloz <Gilles.Buloz@kontron.com>
Date:   Thu May 3 15:21:44 2018 -0500

    PCI: Check whether bridges allow access to extended config space
    
    Even if a device supports extended config space, i.e., it is a PCI-X Mode 2
    or a PCI Express device, the extended space may not be accessible if
    there's a conventional PCI bus in the path to it.
    
    We currently figure that out in pci_cfg_space_size() by reading the first
    dword of extended config space.  On most platforms that returns ~0 data if
    the space is inaccessible, but it may set error bits in PCI status
    registers, and on some platforms it causes exceptions that we currently
    don't recover from.
    
    For example, a PCIe-to-conventional PCI bridge treats config transactions
    with a non-zero Extended Register Address as an Unsupported Request on PCIe
    and a received Master-Abort on the destination bus (see PCI Express to
    PCI/PCI-X Bridge spec, r1.0, sec 4.1.3).
    
    A sample case is a LS1043A CPU (NXP QorIQ Layerscape) platform with the
    following bus topology:
    
      LS1043 PCIe Root Port
        -> PEX8112 PCIe-to-PCI bridge (doesn't support ext cfg on PCI side)
          -> PMC slot connector (for legacy PMC modules)
    
    With a PMC module topology as follows:
    
      PMC connector
        -> PCI-to-PCIe bridge
          -> PCIe switch (4 ports)
            -> 4 PCIe devices (one on each port)
    
    The PCIe devices on the PMC module support extended config space, but we
    can't reach it because the PEX8112 can't generate accesses to the extended
    space on its secondary bus.  Attempts to access it cause Unsupported
    Request errors, which result in synchronous aborts on this platform.
    
    To avoid these errors, check whether bridges are capable of generating
    extended config space addresses on their secondary interfaces.  If they
    can't, we restrict devices below the bridge to only the 256-byte
    PCI-compatible config space.
    
    Signed-off-by: Gilles Buloz <gilles.buloz@kontron.com>
    [bhelgaas: changelog, rework patch so bus_flags testing is all in
    pci_bridge_child_ext_cfg_accessible()]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac91b6fd0bcd..7b1b7b2e01e4 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -882,6 +882,45 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	return err;
 }
 
+static bool pci_bridge_child_ext_cfg_accessible(struct pci_dev *bridge)
+{
+	int pos;
+	u32 status;
+
+	/*
+	 * If extended config space isn't accessible on a bridge's primary
+	 * bus, we certainly can't access it on the secondary bus.
+	 */
+	if (bridge->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
+		return false;
+
+	/*
+	 * PCIe Root Ports and switch ports are PCIe on both sides, so if
+	 * extended config space is accessible on the primary, it's also
+	 * accessible on the secondary.
+	 */
+	if (pci_is_pcie(bridge) &&
+	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT ||
+	     pci_pcie_type(bridge) == PCI_EXP_TYPE_UPSTREAM ||
+	     pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM))
+		return true;
+
+	/*
+	 * For the other bridge types:
+	 *   - PCI-to-PCI bridges
+	 *   - PCIe-to-PCI/PCI-X forward bridges
+	 *   - PCI/PCI-X-to-PCIe reverse bridges
+	 * extended config space on the secondary side is only accessible
+	 * if the bridge supports PCI-X Mode 2.
+	 */
+	pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
+	if (!pos)
+		return false;
+
+	pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
+	return status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ);
+}
+
 static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 					   struct pci_dev *bridge, int busnr)
 {
@@ -923,6 +962,16 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	pci_set_bus_of_node(child);
 	pci_set_bus_speed(child);
 
+	/*
+	 * Check whether extended config space is accessible on the child
+	 * bus.  Note that we currently assume it is always accessible on
+	 * the root bus.
+	 */
+	if (!pci_bridge_child_ext_cfg_accessible(bridge)) {
+		child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
+		pci_info(child, "extended config space not accessible on secondary bus\n");
+	}
+
 	/* Set up default resource pointers and names */
 	for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
 		child->resource[i] = &bridge->resource[PCI_BRIDGE_RESOURCES+i];
@@ -1393,6 +1442,9 @@ int pci_cfg_space_size(struct pci_dev *dev)
 	u32 status;
 	u16 class;
 
+	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
+		return PCI_CFG_SPACE_SIZE;
+
 	class = dev->class >> 8;
 	if (class == PCI_CLASS_BRIDGE_HOST)
 		return pci_cfg_space_size_ext(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 230615620a4a..f7aa6d9f8999 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -217,6 +217,7 @@ enum pci_bus_flags {
 	PCI_BUS_FLAGS_NO_MSI	= (__force pci_bus_flags_t) 1,
 	PCI_BUS_FLAGS_NO_MMRBC	= (__force pci_bus_flags_t) 2,
 	PCI_BUS_FLAGS_NO_AERSID	= (__force pci_bus_flags_t) 4,
+	PCI_BUS_FLAGS_NO_EXTCFG	= (__force pci_bus_flags_t) 8,
 };
 
 /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */

[PATCH] PCI: Check whether bridges allow access to extended config space

[Bjorn Helgaas]

[+cc LKML]

On Thu, May 03, 2018 at 12:40:27PM +0000, Gilles Buloz wrote:
> Subject:    [PATCH] For exception at PCI probe due to bridge reporting UR
> 
> Even if a device supports extended config access, no such access must be
> done to this device If there's a bridge not supporting that in the path
> to this device. Doing such access with UR reporting enabled on the root
> bridge leads to an exception.
> 
> This is the case on a LS1043A CPU (NXP QorIQ Layerscape) platform with
> the following bus topology :
>   LS1043 PCIe root
>     -> PEX8112 PCIe-to-PCI bridge (not supporting ext cfg on PCI side)
>       -> PMC slot connector (for legacy PMC modules)
> With a PMC module topology as follows :
>   PMC connector
>     -> PCI-to-PCIe bridge
>       -> PCIe switch (4 ports)
>         -> 4 PCIe devices (one on each port)
> In this case all devices behind the PEX8112 are supporting extended config
> access but this is prohibited by the PEX8112. Without this patch, an
> exception (synchronous abort) occurs in pci_cfg_space_size_ext().
> 
> This patch checks the parent bridge of each allocated child bus to know if
> extended config access is supported on the child bus, and sets a flag in
> child->bus_flags if not supported. This  flag is inherited by all children
> buses of this child bus and then is checked to avoid this unsupported
> accesses to every device on these buses.

Hi Gilles,

Thanks for the patch!  I reworked it a little bit to simplify the code
in pci_alloc_child_bus().  Can you test it and make sure I didn't
break anything?


commit cbaaa85b558a8f974e301fa0364d568efaf491ce
Author: Gilles Buloz <Gilles.Buloz@kontron.com>
Date:   Thu May 3 15:21:44 2018 -0500

    PCI: Check whether bridges allow access to extended config space
    
    Even if a device supports extended config space, i.e., it is a PCI-X Mode 2
    or a PCI Express device, the extended space may not be accessible if
    there's a conventional PCI bus in the path to it.
    
    We currently figure that out in pci_cfg_space_size() by reading the first
    dword of extended config space.  On most platforms that returns ~0 data if
    the space is inaccessible, but it may set error bits in PCI status
    registers, and on some platforms it causes exceptions that we currently
    don't recover from.
    
    For example, a PCIe-to-conventional PCI bridge treats config transactions
    with a non-zero Extended Register Address as an Unsupported Request on PCIe
    and a received Master-Abort on the destination bus (see PCI Express to
    PCI/PCI-X Bridge spec, r1.0, sec 4.1.3).
    
    A sample case is a LS1043A CPU (NXP QorIQ Layerscape) platform with the
    following bus topology:
    
      LS1043 PCIe Root Port
        -> PEX8112 PCIe-to-PCI bridge (doesn't support ext cfg on PCI side)
          -> PMC slot connector (for legacy PMC modules)
    
    With a PMC module topology as follows:
    
      PMC connector
        -> PCI-to-PCIe bridge
          -> PCIe switch (4 ports)
            -> 4 PCIe devices (one on each port)
    
    The PCIe devices on the PMC module support extended config space, but we
    can't reach it because the PEX8112 can't generate accesses to the extended
    space on its secondary bus.  Attempts to access it cause Unsupported
    Request errors, which result in synchronous aborts on this platform.
    
    To avoid these errors, check whether bridges are capable of generating
    extended config space addresses on their secondary interfaces.  If they
    can't, we restrict devices below the bridge to only the 256-byte
    PCI-compatible config space.
    
    Signed-off-by: Gilles Buloz <gilles.buloz@kontron.com>
    [bhelgaas: changelog, rework patch so bus_flags testing is all in
    pci_bridge_child_ext_cfg_accessible()]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac91b6fd0bcd..7b1b7b2e01e4 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -882,6 +882,45 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	return err;
 }
 
+static bool pci_bridge_child_ext_cfg_accessible(struct pci_dev *bridge)
+{
+	int pos;
+	u32 status;
+
+	/*
+	 * If extended config space isn't accessible on a bridge's primary
+	 * bus, we certainly can't access it on the secondary bus.
+	 */
+	if (bridge->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
+		return false;
+
+	/*
+	 * PCIe Root Ports and switch ports are PCIe on both sides, so if
+	 * extended config space is accessible on the primary, it's also
+	 * accessible on the secondary.
+	 */
+	if (pci_is_pcie(bridge) &&
+	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT ||
+	     pci_pcie_type(bridge) == PCI_EXP_TYPE_UPSTREAM ||
+	     pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM))
+		return true;
+
+	/*
+	 * For the other bridge types:
+	 *   - PCI-to-PCI bridges
+	 *   - PCIe-to-PCI/PCI-X forward bridges
+	 *   - PCI/PCI-X-to-PCIe reverse bridges
+	 * extended config space on the secondary side is only accessible
+	 * if the bridge supports PCI-X Mode 2.
+	 */
+	pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
+	if (!pos)
+		return false;
+
+	pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
+	return status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ);
+}
+
 static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 					   struct pci_dev *bridge, int busnr)
 {
@@ -923,6 +962,16 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	pci_set_bus_of_node(child);
 	pci_set_bus_speed(child);
 
+	/*
+	 * Check whether extended config space is accessible on the child
+	 * bus.  Note that we currently assume it is always accessible on
+	 * the root bus.
+	 */
+	if (!pci_bridge_child_ext_cfg_accessible(bridge)) {
+		child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
+		pci_info(child, "extended config space not accessible on secondary bus\n");
+	}
+
 	/* Set up default resource pointers and names */
 	for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
 		child->resource[i] = &bridge->resource[PCI_BRIDGE_RESOURCES+i];
@@ -1393,6 +1442,9 @@ int pci_cfg_space_size(struct pci_dev *dev)
 	u32 status;
 	u16 class;
 
+	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
+		return PCI_CFG_SPACE_SIZE;
+
 	class = dev->class >> 8;
 	if (class == PCI_CLASS_BRIDGE_HOST)
 		return pci_cfg_space_size_ext(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 230615620a4a..f7aa6d9f8999 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -217,6 +217,7 @@ enum pci_bus_flags {
 	PCI_BUS_FLAGS_NO_MSI	= (__force pci_bus_flags_t) 1,
 	PCI_BUS_FLAGS_NO_MMRBC	= (__force pci_bus_flags_t) 2,
 	PCI_BUS_FLAGS_NO_AERSID	= (__force pci_bus_flags_t) 4,
+	PCI_BUS_FLAGS_NO_EXTCFG	= (__force pci_bus_flags_t) 8,
 };
 
 /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: Check whether bridges allow access to extended config space

[Gilles Buloz]

Le 04/05/2018 00:31, Bjorn Helgaas a écrit :
> [+cc LKML]
>
> On Thu, May 03, 2018 at 12:40:27PM +0000, Gilles Buloz wrote:
>> Subject:    [PATCH] For exception at PCI probe due to bridge reporting UR
>>
>> Even if a device supports extended config access, no such access must be
>> done to this device If there's a bridge not supporting that in the path
>> to this device. Doing such access with UR reporting enabled on the root
>> bridge leads to an exception.
>>
>> This is the case on a LS1043A CPU (NXP QorIQ Layerscape) platform with
>> the following bus topology :
>>    LS1043 PCIe root
>>      -> PEX8112 PCIe-to-PCI bridge (not supporting ext cfg on PCI side)
>>        -> PMC slot connector (for legacy PMC modules)
>> With a PMC module topology as follows :
>>    PMC connector
>>      -> PCI-to-PCIe bridge
>>        -> PCIe switch (4 ports)
>>          -> 4 PCIe devices (one on each port)
>> In this case all devices behind the PEX8112 are supporting extended config
>> access but this is prohibited by the PEX8112. Without this patch, an
>> exception (synchronous abort) occurs in pci_cfg_space_size_ext().
>>
>> This patch checks the parent bridge of each allocated child bus to know if
>> extended config access is supported on the child bus, and sets a flag in
>> child->bus_flags if not supported. This  flag is inherited by all children
>> buses of this child bus and then is checked to avoid this unsupported
>> accesses to every device on these buses.
> Hi Gilles,
>
> Thanks for the patch!  I reworked it a little bit to simplify the code
> in pci_alloc_child_bus().  Can you test it and make sure I didn't
> break anything?
>
Hi Bjorn,

Your rework works as expected. Tested on LS1043A platform with kernel 4.17-rc1, and with some backport on kernel 4.1.35

Suggestion : maybe change the pci_info() string to have :
     pci_bus 0000:xx: extended config space not accessible
instead of
     pci_bus 0000:xx: extended config space not accessible on secondary bus
as xx is already the number of the secondary bus

Info : with kernel 4.17-rc1, it turns out I need pcie_aspm=off to have the PMC devices behind the
PCI-to-PCIe bridge of the PMC safely detected/configured. But this is not caused by the patch.
Without pcie_aspm=off I saw this at one boot :
    "pci 0000:02:0e.0: ASPM: Could not configure common clock" for this bridge, but devices
    correctly detected/configured
but at most boots I get :
    no ASPM message but "pci 0000:04:02.0: bridge configuration invalid ([bus ff-ff]), reconfiguring "
    instead, and some devices are missing. Also lspci show "rev ff" for some devices.
I don't see this problem on 4.1.35 with the same backported patch.

Gilles
> commit cbaaa85b558a8f974e301fa0364d568efaf491ce
> Author: Gilles Buloz <Gilles.Buloz@kontron.com>
> Date:   Thu May 3 15:21:44 2018 -0500
>
>      PCI: Check whether bridges allow access to extended config space
>      
>      Even if a device supports extended config space, i.e., it is a PCI-X Mode 2
>      or a PCI Express device, the extended space may not be accessible if
>      there's a conventional PCI bus in the path to it.
>      
>      We currently figure that out in pci_cfg_space_size() by reading the first
>      dword of extended config space.  On most platforms that returns ~0 data if
>      the space is inaccessible, but it may set error bits in PCI status
>      registers, and on some platforms it causes exceptions that we currently
>      don't recover from.
>      
>      For example, a PCIe-to-conventional PCI bridge treats config transactions
>      with a non-zero Extended Register Address as an Unsupported Request on PCIe
>      and a received Master-Abort on the destination bus (see PCI Express to
>      PCI/PCI-X Bridge spec, r1.0, sec 4.1.3).
>      
>      A sample case is a LS1043A CPU (NXP QorIQ Layerscape) platform with the
>      following bus topology:
>      
>        LS1043 PCIe Root Port
>          -> PEX8112 PCIe-to-PCI bridge (doesn't support ext cfg on PCI side)
>            -> PMC slot connector (for legacy PMC modules)
>      
>      With a PMC module topology as follows:
>      
>        PMC connector
>          -> PCI-to-PCIe bridge
>            -> PCIe switch (4 ports)
>              -> 4 PCIe devices (one on each port)
>      
>      The PCIe devices on the PMC module support extended config space, but we
>      can't reach it because the PEX8112 can't generate accesses to the extended
>      space on its secondary bus.  Attempts to access it cause Unsupported
>      Request errors, which result in synchronous aborts on this platform.
>      
>      To avoid these errors, check whether bridges are capable of generating
>      extended config space addresses on their secondary interfaces.  If they
>      can't, we restrict devices below the bridge to only the 256-byte
>      PCI-compatible config space.
>      
>      Signed-off-by: Gilles Buloz <gilles.buloz@kontron.com>
>      [bhelgaas: changelog, rework patch so bus_flags testing is all in
>      pci_bridge_child_ext_cfg_accessible()]
>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac91b6fd0bcd..7b1b7b2e01e4 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -882,6 +882,45 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>   	return err;
>   }
>   
> +static bool pci_bridge_child_ext_cfg_accessible(struct pci_dev *bridge)
> +{
> +	int pos;
> +	u32 status;
> +
> +	/*
> +	 * If extended config space isn't accessible on a bridge's primary
> +	 * bus, we certainly can't access it on the secondary bus.
> +	 */
> +	if (bridge->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> +		return false;
> +
> +	/*
> +	 * PCIe Root Ports and switch ports are PCIe on both sides, so if
> +	 * extended config space is accessible on the primary, it's also
> +	 * accessible on the secondary.
> +	 */
> +	if (pci_is_pcie(bridge) &&
> +	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT ||
> +	     pci_pcie_type(bridge) == PCI_EXP_TYPE_UPSTREAM ||
> +	     pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM))
> +		return true;
> +
> +	/*
> +	 * For the other bridge types:
> +	 *   - PCI-to-PCI bridges
> +	 *   - PCIe-to-PCI/PCI-X forward bridges
> +	 *   - PCI/PCI-X-to-PCIe reverse bridges
> +	 * extended config space on the secondary side is only accessible
> +	 * if the bridge supports PCI-X Mode 2.
> +	 */
> +	pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
> +	if (!pos)
> +		return false;
> +
> +	pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
> +	return status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ);
> +}
> +
>   static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>   					   struct pci_dev *bridge, int busnr)
>   {
> @@ -923,6 +962,16 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>   	pci_set_bus_of_node(child);
>   	pci_set_bus_speed(child);
>   
> +	/*
> +	 * Check whether extended config space is accessible on the child
> +	 * bus.  Note that we currently assume it is always accessible on
> +	 * the root bus.
> +	 */
> +	if (!pci_bridge_child_ext_cfg_accessible(bridge)) {
> +		child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
> +		pci_info(child, "extended config space not accessible on secondary bus\n");
> +	}
> +
>   	/* Set up default resource pointers and names */
>   	for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
>   		child->resource[i] = &bridge->resource[PCI_BRIDGE_RESOURCES+i];
> @@ -1393,6 +1442,9 @@ int pci_cfg_space_size(struct pci_dev *dev)
>   	u32 status;
>   	u16 class;
>   
> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> +		return PCI_CFG_SPACE_SIZE;
> +
>   	class = dev->class >> 8;
>   	if (class == PCI_CLASS_BRIDGE_HOST)
>   		return pci_cfg_space_size_ext(dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 230615620a4a..f7aa6d9f8999 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -217,6 +217,7 @@ enum pci_bus_flags {
>   	PCI_BUS_FLAGS_NO_MSI	= (__force pci_bus_flags_t) 1,
>   	PCI_BUS_FLAGS_NO_MMRBC	= (__force pci_bus_flags_t) 2,
>   	PCI_BUS_FLAGS_NO_AERSID	= (__force pci_bus_flags_t) 4,
> +	PCI_BUS_FLAGS_NO_EXTCFG	= (__force pci_bus_flags_t) 8,
>   };
>   
>   /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */
>
> .
>

[PATCH] PCI: Check whether bridges allow access to extended config space

[Gilles Buloz]

Le 04/05/2018 00:31, Bjorn Helgaas a ?crit :
> [+cc LKML]
>
> On Thu, May 03, 2018 at 12:40:27PM +0000, Gilles Buloz wrote:
>> Subject:    [PATCH] For exception at PCI probe due to bridge reporting UR
>>
>> Even if a device supports extended config access, no such access must be
>> done to this device If there's a bridge not supporting that in the path
>> to this device. Doing such access with UR reporting enabled on the root
>> bridge leads to an exception.
>>
>> This is the case on a LS1043A CPU (NXP QorIQ Layerscape) platform with
>> the following bus topology :
>>    LS1043 PCIe root
>>      -> PEX8112 PCIe-to-PCI bridge (not supporting ext cfg on PCI side)
>>        -> PMC slot connector (for legacy PMC modules)
>> With a PMC module topology as follows :
>>    PMC connector
>>      -> PCI-to-PCIe bridge
>>        -> PCIe switch (4 ports)
>>          -> 4 PCIe devices (one on each port)
>> In this case all devices behind the PEX8112 are supporting extended config
>> access but this is prohibited by the PEX8112. Without this patch, an
>> exception (synchronous abort) occurs in pci_cfg_space_size_ext().
>>
>> This patch checks the parent bridge of each allocated child bus to know if
>> extended config access is supported on the child bus, and sets a flag in
>> child->bus_flags if not supported. This  flag is inherited by all children
>> buses of this child bus and then is checked to avoid this unsupported
>> accesses to every device on these buses.
> Hi Gilles,
>
> Thanks for the patch!  I reworked it a little bit to simplify the code
> in pci_alloc_child_bus().  Can you test it and make sure I didn't
> break anything?
>
Hi Bjorn,

Your rework works as expected. Tested on LS1043A platform with kernel 4.17-rc1, and with some backport on kernel 4.1.35

Suggestion : maybe change the pci_info() string to have :
     pci_bus 0000:xx: extended config space not accessible
instead of
     pci_bus 0000:xx: extended config space not accessible on secondary bus
as xx is already the number of the secondary bus

Info : with kernel 4.17-rc1, it turns out I need pcie_aspm=off to have the PMC devices behind the
PCI-to-PCIe bridge of the PMC safely detected/configured. But this is not caused by the patch.
Without pcie_aspm=off I saw this at one boot :
    "pci 0000:02:0e.0: ASPM: Could not configure common clock" for this bridge, but devices
    correctly detected/configured
but at most boots I get :
    no ASPM message but "pci 0000:04:02.0: bridge configuration invalid ([bus ff-ff]), reconfiguring "
    instead, and some devices are missing. Also lspci show "rev ff" for some devices.
I don't see this problem on 4.1.35 with the same backported patch.

Gilles
> commit cbaaa85b558a8f974e301fa0364d568efaf491ce
> Author: Gilles Buloz <Gilles.Buloz@kontron.com>
> Date:   Thu May 3 15:21:44 2018 -0500
>
>      PCI: Check whether bridges allow access to extended config space
>      
>      Even if a device supports extended config space, i.e., it is a PCI-X Mode 2
>      or a PCI Express device, the extended space may not be accessible if
>      there's a conventional PCI bus in the path to it.
>      
>      We currently figure that out in pci_cfg_space_size() by reading the first
>      dword of extended config space.  On most platforms that returns ~0 data if
>      the space is inaccessible, but it may set error bits in PCI status
>      registers, and on some platforms it causes exceptions that we currently
>      don't recover from.
>      
>      For example, a PCIe-to-conventional PCI bridge treats config transactions
>      with a non-zero Extended Register Address as an Unsupported Request on PCIe
>      and a received Master-Abort on the destination bus (see PCI Express to
>      PCI/PCI-X Bridge spec, r1.0, sec 4.1.3).
>      
>      A sample case is a LS1043A CPU (NXP QorIQ Layerscape) platform with the
>      following bus topology:
>      
>        LS1043 PCIe Root Port
>          -> PEX8112 PCIe-to-PCI bridge (doesn't support ext cfg on PCI side)
>            -> PMC slot connector (for legacy PMC modules)
>      
>      With a PMC module topology as follows:
>      
>        PMC connector
>          -> PCI-to-PCIe bridge
>            -> PCIe switch (4 ports)
>              -> 4 PCIe devices (one on each port)
>      
>      The PCIe devices on the PMC module support extended config space, but we
>      can't reach it because the PEX8112 can't generate accesses to the extended
>      space on its secondary bus.  Attempts to access it cause Unsupported
>      Request errors, which result in synchronous aborts on this platform.
>      
>      To avoid these errors, check whether bridges are capable of generating
>      extended config space addresses on their secondary interfaces.  If they
>      can't, we restrict devices below the bridge to only the 256-byte
>      PCI-compatible config space.
>      
>      Signed-off-by: Gilles Buloz <gilles.buloz@kontron.com>
>      [bhelgaas: changelog, rework patch so bus_flags testing is all in
>      pci_bridge_child_ext_cfg_accessible()]
>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac91b6fd0bcd..7b1b7b2e01e4 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -882,6 +882,45 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>   	return err;
>   }
>   
> +static bool pci_bridge_child_ext_cfg_accessible(struct pci_dev *bridge)
> +{
> +	int pos;
> +	u32 status;
> +
> +	/*
> +	 * If extended config space isn't accessible on a bridge's primary
> +	 * bus, we certainly can't access it on the secondary bus.
> +	 */
> +	if (bridge->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> +		return false;
> +
> +	/*
> +	 * PCIe Root Ports and switch ports are PCIe on both sides, so if
> +	 * extended config space is accessible on the primary, it's also
> +	 * accessible on the secondary.
> +	 */
> +	if (pci_is_pcie(bridge) &&
> +	    (pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT ||
> +	     pci_pcie_type(bridge) == PCI_EXP_TYPE_UPSTREAM ||
> +	     pci_pcie_type(bridge) == PCI_EXP_TYPE_DOWNSTREAM))
> +		return true;
> +
> +	/*
> +	 * For the other bridge types:
> +	 *   - PCI-to-PCI bridges
> +	 *   - PCIe-to-PCI/PCI-X forward bridges
> +	 *   - PCI/PCI-X-to-PCIe reverse bridges
> +	 * extended config space on the secondary side is only accessible
> +	 * if the bridge supports PCI-X Mode 2.
> +	 */
> +	pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX);
> +	if (!pos)
> +		return false;
> +
> +	pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
> +	return status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ);
> +}
> +
>   static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>   					   struct pci_dev *bridge, int busnr)
>   {
> @@ -923,6 +962,16 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>   	pci_set_bus_of_node(child);
>   	pci_set_bus_speed(child);
>   
> +	/*
> +	 * Check whether extended config space is accessible on the child
> +	 * bus.  Note that we currently assume it is always accessible on
> +	 * the root bus.
> +	 */
> +	if (!pci_bridge_child_ext_cfg_accessible(bridge)) {
> +		child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG;
> +		pci_info(child, "extended config space not accessible on secondary bus\n");
> +	}
> +
>   	/* Set up default resource pointers and names */
>   	for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
>   		child->resource[i] = &bridge->resource[PCI_BRIDGE_RESOURCES+i];
> @@ -1393,6 +1442,9 @@ int pci_cfg_space_size(struct pci_dev *dev)
>   	u32 status;
>   	u16 class;
>   
> +	if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> +		return PCI_CFG_SPACE_SIZE;
> +
>   	class = dev->class >> 8;
>   	if (class == PCI_CLASS_BRIDGE_HOST)
>   		return pci_cfg_space_size_ext(dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 230615620a4a..f7aa6d9f8999 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -217,6 +217,7 @@ enum pci_bus_flags {
>   	PCI_BUS_FLAGS_NO_MSI	= (__force pci_bus_flags_t) 1,
>   	PCI_BUS_FLAGS_NO_MMRBC	= (__force pci_bus_flags_t) 2,
>   	PCI_BUS_FLAGS_NO_AERSID	= (__force pci_bus_flags_t) 4,
> +	PCI_BUS_FLAGS_NO_EXTCFG	= (__force pci_bus_flags_t) 8,
>   };
>   
>   /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */
>
> .
>

Re: [PATCH] PCI: Check whether bridges allow access to extended config space

[Gilles Buloz]

Le 04/05/2018 00:31, Bjorn Helgaas a =E9crit :
> [+cc LKML]
>
> On Thu, May 03, 2018 at 12:40:27PM +0000, Gilles Buloz wrote:
>> Subject:    [PATCH] For exception at PCI probe due to bridge reporting U=
R
>>
>> Even if a device supports extended config access, no such access must be
>> done to this device If there's a bridge not supporting that in the path
>> to this device. Doing such access with UR reporting enabled on the root
>> bridge leads to an exception.
>>
>> This is the case on a LS1043A CPU (NXP QorIQ Layerscape) platform with
>> the following bus topology :
>>    LS1043 PCIe root
>>      -> PEX8112 PCIe-to-PCI bridge (not supporting ext cfg on PCI side)
>>        -> PMC slot connector (for legacy PMC modules)
>> With a PMC module topology as follows :
>>    PMC connector
>>      -> PCI-to-PCIe bridge
>>        -> PCIe switch (4 ports)
>>          -> 4 PCIe devices (one on each port)
>> In this case all devices behind the PEX8112 are supporting extended conf=
ig
>> access but this is prohibited by the PEX8112. Without this patch, an
>> exception (synchronous abort) occurs in pci_cfg_space_size_ext().
>>
>> This patch checks the parent bridge of each allocated child bus to know =
if
>> extended config access is supported on the child bus, and sets a flag in
>> child->bus_flags if not supported. This  flag is inherited by all childr=
en
>> buses of this child bus and then is checked to avoid this unsupported
>> accesses to every device on these buses.
> Hi Gilles,
>
> Thanks for the patch!  I reworked it a little bit to simplify the code
> in pci_alloc_child_bus().  Can you test it and make sure I didn't
> break anything?
>
Hi Bjorn,

Your rework works as expected. Tested on LS1043A platform with kernel 4.17-=
rc1, and with some backport on kernel 4.1.35

Suggestion : maybe change the pci_info() string to have :
     pci_bus 0000:xx: extended config space not accessible
instead of
     pci_bus 0000:xx: extended config space not accessible on secondary bus
as xx is already the number of the secondary bus

Info : with kernel 4.17-rc1, it turns out I need pcie_aspm=3Doff to have th=
e PMC devices behind the
PCI-to-PCIe bridge of the PMC safely detected/configured. But this is not c=
aused by the patch.
Without pcie_aspm=3Doff I saw this at one boot :
    "pci 0000:02:0e.0: ASPM: Could not configure common clock" for this bri=
dge, but devices
    correctly detected/configured
but at most boots I get :
    no ASPM message but "pci 0000:04:02.0: bridge configuration invalid ([b=
us ff-ff]), reconfiguring "
    instead, and some devices are missing. Also lspci show "rev ff" for som=
e devices.
I don't see this problem on 4.1.35 with the same backported patch.

Gilles
> commit cbaaa85b558a8f974e301fa0364d568efaf491ce
> Author: Gilles Buloz <Gilles.Buloz@kontron.com>
> Date:   Thu May 3 15:21:44 2018 -0500
>
>      PCI: Check whether bridges allow access to extended config space
>     =20
>      Even if a device supports extended config space, i.e., it is a PCI-X=
 Mode 2
>      or a PCI Express device, the extended space may not be accessible if
>      there's a conventional PCI bus in the path to it.
>     =20
>      We currently figure that out in pci_cfg_space_size() by reading the =
first
>      dword of extended config space.  On most platforms that returns ~0 d=
ata if
>      the space is inaccessible, but it may set error bits in PCI status
>      registers, and on some platforms it causes exceptions that we curren=
tly
>      don't recover from.
>     =20
>      For example, a PCIe-to-conventional PCI bridge treats config transac=
tions
>      with a non-zero Extended Register Address as an Unsupported Request =
on PCIe
>      and a received Master-Abort on the destination bus (see PCI Express =
to
>      PCI/PCI-X Bridge spec, r1.0, sec 4.1.3).
>     =20
>      A sample case is a LS1043A CPU (NXP QorIQ Layerscape) platform with =
the
>      following bus topology:
>     =20
>        LS1043 PCIe Root Port
>          -> PEX8112 PCIe-to-PCI bridge (doesn't support ext cfg on PCI si=
de)
>            -> PMC slot connector (for legacy PMC modules)
>     =20
>      With a PMC module topology as follows:
>     =20
>        PMC connector
>          -> PCI-to-PCIe bridge
>            -> PCIe switch (4 ports)
>              -> 4 PCIe devices (one on each port)
>     =20
>      The PCIe devices on the PMC module support extended config space, bu=
t we
>      can't reach it because the PEX8112 can't generate accesses to the ex=
tended
>      space on its secondary bus.  Attempts to access it cause Unsupported
>      Request errors, which result in synchronous aborts on this platform.
>     =20
>      To avoid these errors, check whether bridges are capable of generati=
ng
>      extended config space addresses on their secondary interfaces.  If t=
hey
>      can't, we restrict devices below the bridge to only the 256-byte
>      PCI-compatible config space.
>     =20
>      Signed-off-by: Gilles Buloz <gilles.buloz@kontron.com>
>      [bhelgaas: changelog, rework patch so bus_flags testing is all in
>      pci_bridge_child_ext_cfg_accessible()]
>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac91b6fd0bcd..7b1b7b2e01e4 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -882,6 +882,45 @@ static int pci_register_host_bridge(struct pci_host_=
bridge *bridge)
>   =09return err;
>   }
>  =20
> +static bool pci_bridge_child_ext_cfg_accessible(struct pci_dev *bridge)
> +{
> +=09int pos;
> +=09u32 status;
> +
> +=09/*
> +=09 * If extended config space isn't accessible on a bridge's primary
> +=09 * bus, we certainly can't access it on the secondary bus.
> +=09 */
> +=09if (bridge->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> +=09=09return false;
> +
> +=09/*
> +=09 * PCIe Root Ports and switch ports are PCIe on both sides, so if
> +=09 * extended config space is accessible on the primary, it's also
> +=09 * accessible on the secondary.
> +=09 */
> +=09if (pci_is_pcie(bridge) &&
> +=09    (pci_pcie_type(bridge) =3D=3D PCI_EXP_TYPE_ROOT_PORT ||
> +=09     pci_pcie_type(bridge) =3D=3D PCI_EXP_TYPE_UPSTREAM ||
> +=09     pci_pcie_type(bridge) =3D=3D PCI_EXP_TYPE_DOWNSTREAM))
> +=09=09return true;
> +
> +=09/*
> +=09 * For the other bridge types:
> +=09 *   - PCI-to-PCI bridges
> +=09 *   - PCIe-to-PCI/PCI-X forward bridges
> +=09 *   - PCI/PCI-X-to-PCIe reverse bridges
> +=09 * extended config space on the secondary side is only accessible
> +=09 * if the bridge supports PCI-X Mode 2.
> +=09 */
> +=09pos =3D pci_find_capability(bridge, PCI_CAP_ID_PCIX);
> +=09if (!pos)
> +=09=09return false;
> +
> +=09pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status);
> +=09return status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ);
> +}
> +
>   static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>   =09=09=09=09=09   struct pci_dev *bridge, int busnr)
>   {
> @@ -923,6 +962,16 @@ static struct pci_bus *pci_alloc_child_bus(struct pc=
i_bus *parent,
>   =09pci_set_bus_of_node(child);
>   =09pci_set_bus_speed(child);
>  =20
> +=09/*
> +=09 * Check whether extended config space is accessible on the child
> +=09 * bus.  Note that we currently assume it is always accessible on
> +=09 * the root bus.
> +=09 */
> +=09if (!pci_bridge_child_ext_cfg_accessible(bridge)) {
> +=09=09child->bus_flags |=3D PCI_BUS_FLAGS_NO_EXTCFG;
> +=09=09pci_info(child, "extended config space not accessible on secondary=
 bus\n");
> +=09}
> +
>   =09/* Set up default resource pointers and names */
>   =09for (i =3D 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
>   =09=09child->resource[i] =3D &bridge->resource[PCI_BRIDGE_RESOURCES+i];
> @@ -1393,6 +1442,9 @@ int pci_cfg_space_size(struct pci_dev *dev)
>   =09u32 status;
>   =09u16 class;
>  =20
> +=09if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)
> +=09=09return PCI_CFG_SPACE_SIZE;
> +
>   =09class =3D dev->class >> 8;
>   =09if (class =3D=3D PCI_CLASS_BRIDGE_HOST)
>   =09=09return pci_cfg_space_size_ext(dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 230615620a4a..f7aa6d9f8999 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -217,6 +217,7 @@ enum pci_bus_flags {
>   =09PCI_BUS_FLAGS_NO_MSI=09=3D (__force pci_bus_flags_t) 1,
>   =09PCI_BUS_FLAGS_NO_MMRBC=09=3D (__force pci_bus_flags_t) 2,
>   =09PCI_BUS_FLAGS_NO_AERSID=09=3D (__force pci_bus_flags_t) 4,
> +=09PCI_BUS_FLAGS_NO_EXTCFG=09=3D (__force pci_bus_flags_t) 8,
>   };
>  =20
>   /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */
>
> .
>