[PATCH] PCI: Check whether bridges allow access to extended config space
Gilles Buloz
Gilles.Buloz at kontron.com
Fri May 4 08:45:07 PDT 2018
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 at 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 at 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 at 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 */
>
> .
>
More information about the linux-arm-kernel
mailing list