[PATCH v7 0/5] Update device MPS

[PATCH v7 0/5] Update device MPS

[Bjorn Helgaas]

v6->v7: Bjorn's rework and additions of other minor cleanups
v5->v6: rework the patch 1/2, remove the unnecessary check, pointed out by Bjorn.
        remove the patch 1/2 cc stable tag, because it's not a serious bug.
v4->v5: Fix some spelling problems and move mpss = 128 << dev->pcie_mpss above to reuse
        it, also remove the else braces for code style. thanks for Jon's review and comments.
v3->v4: Call pcie_bus_update_set() only when pcie_bus_config == PCIE_BUS_TUNE_OFF
        suggested by Jon Mason, try to change parent mps when parent device is
        root port and only one slot connected to it when parent mps > child device
        mpss. Other add a patch to fix a issue in pcie_find_smpss() during use
        "pci=pcie_bus_safe".
v2->v3: Update CC stable tag suggested by Li Zefan.
v1->v2: Update patch log, remove Joe's reported-by, because his problem
        was mainly caused by BIOS incorrect setting. But this patch mainly
        to fix the bug caused by device hot add. Conservatively, this
        version only update the mps problem when hot add. When the device
        mps < parent mps found, this patch try to update device mps.
        It seems unlikely device mps > parent mps after hot add device.
        So we don't care that situation.

---

Bjorn Helgaas (3):
      PCI: Drop "PCI-E" prefix from Max Payload Size message
      PCI: Simplify pcie_bus_configure_settings() interface
      PCI: Simplify MPS test for Downstream Port

Yijing Wang (2):
      PCI: Remove unnecessary check for pcie_get_mps() failure
      PCI: Don't restrict MPS for slots below Root Ports


 arch/powerpc/kernel/pci-common.c |    8 ++-----
 arch/tile/kernel/pci_gx.c        |    9 ++------
 arch/x86/pci/acpi.c              |    9 ++------
 drivers/pci/hotplug/pcihp_slot.c |    5 ++---
 drivers/pci/pci.c                |    3 ---
 drivers/pci/probe.c              |   42 ++++++++++++++++++++------------------
 include/linux/pci.h              |    2 +-
 7 files changed, 31 insertions(+), 47 deletions(-)

[PATCH v7 1/5] PCI: Drop "PCI-E" prefix from Max Payload Size message

[Bjorn Helgaas]

The conventional spelling is "PCIe", but I think even that is superfluous,
so remove the whole thing.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 46ada5c..9a33430 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1596,7 +1596,7 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
 	pcie_write_mps(dev, mps);
 	pcie_write_mrrs(dev);
 
-	dev_info(&dev->dev, "PCI-E Max Payload Size set to %4d/%4d (was %4d), "
+	dev_info(&dev->dev, "Max Payload Size set to %4d/%4d (was %4d), "
 		 "Max Read Rq %4d\n", pcie_get_mps(dev), 128 << dev->pcie_mpss,
 		 orig_mps, pcie_get_readrq(dev));
 

[PATCH v7 2/5] PCI: Simplify pcie_bus_configure_settings() interface

[Bjorn Helgaas]

Based on a patch by Jon Mason (see URL below).

All users of pcie_bus_configure_settings() pass arguments of the form
"bus, bus->self->pcie_mpss".  The "mpss" argument is redundant since we
can easily look it up internally.  In addition, all callers check
"bus->self" for NULL, which we can also do internally.

This patch simplifies the interface and the callers.  No functional change.

Reference: http://lkml.kernel.org/r/1317048850-30728-2-git-send-email-mason@myri.com
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/powerpc/kernel/pci-common.c |    8 ++------
 arch/tile/kernel/pci_gx.c        |    9 ++-------
 arch/x86/pci/acpi.c              |    9 ++-------
 drivers/pci/hotplug/pcihp_slot.c |    5 ++---
 drivers/pci/probe.c              |    7 +++++--
 include/linux/pci.h              |    2 +-
 6 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index f46914a..d35ec34 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1672,12 +1672,8 @@ void pcibios_scan_phb(struct pci_controller *hose)
 	/* Configure PCI Express settings */
 	if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
 		struct pci_bus *child;
-		list_for_each_entry(child, &bus->children, node) {
-			struct pci_dev *self = child->self;
-			if (!self)
-				continue;
-			pcie_bus_configure_settings(child, self->pcie_mpss);
-		}
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
 	}
 }
 
diff --git a/arch/tile/kernel/pci_gx.c b/arch/tile/kernel/pci_gx.c
index 1142563..6640e7b 100644
--- a/arch/tile/kernel/pci_gx.c
+++ b/arch/tile/kernel/pci_gx.c
@@ -508,13 +508,8 @@ static void fixup_read_and_payload_sizes(struct pci_controller *controller)
 						rc_dev_cap.word);
 
 	/* Configure PCI Express MPS setting. */
-	list_for_each_entry(child, &root_bus->children, node) {
-		struct pci_dev *self = child->self;
-		if (!self)
-			continue;
-
-		pcie_bus_configure_settings(child, self->pcie_mpss);
-	}
+	list_for_each_entry(child, &root_bus->children, node)
+		pcie_bus_configure_settings(child);
 
 	/*
 	 * Set the mac_config register in trio based on the MPS/MRS of the link.
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index d641897..b30e937 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -568,13 +568,8 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	 */
 	if (bus) {
 		struct pci_bus *child;
-		list_for_each_entry(child, &bus->children, node) {
-			struct pci_dev *self = child->self;
-			if (!self)
-				continue;
-
-			pcie_bus_configure_settings(child, self->pcie_mpss);
-		}
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
 	}
 
 	if (bus && node != -1) {
diff --git a/drivers/pci/hotplug/pcihp_slot.c b/drivers/pci/hotplug/pcihp_slot.c
index fec2d5b..16f9203 100644
--- a/drivers/pci/hotplug/pcihp_slot.c
+++ b/drivers/pci/hotplug/pcihp_slot.c
@@ -160,9 +160,8 @@ void pci_configure_slot(struct pci_dev *dev)
 			(dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)))
 		return;
 
-	if (dev->bus && dev->bus->self)
-		pcie_bus_configure_settings(dev->bus,
-					    dev->bus->self->pcie_mpss);
+	if (dev->bus)
+		pcie_bus_configure_settings(dev->bus);
 
 	memset(&hpp, 0, sizeof(hpp));
 	ret = pci_get_hp_params(dev, &hpp);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 9a33430..ecae7f2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1607,10 +1607,13 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
  * parents then children fashion.  If this changes, then this code will not
  * work as designed.
  */
-void pcie_bus_configure_settings(struct pci_bus *bus, u8 mpss)
+void pcie_bus_configure_settings(struct pci_bus *bus)
 {
 	u8 smpss;
 
+	if (!bus->self)
+		return;
+
 	if (!pci_is_pcie(bus->self))
 		return;
 
@@ -1625,7 +1628,7 @@ void pcie_bus_configure_settings(struct pci_bus *bus, u8 mpss)
 		smpss = 0;
 
 	if (pcie_bus_config == PCIE_BUS_SAFE) {
-		smpss = mpss;
+		smpss = bus->self->pcie_mpss;
 
 		pcie_find_smpss(bus->self, &smpss);
 		pci_walk_bus(bus, pcie_find_smpss, &smpss);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0fd1f15..57062b7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -675,7 +675,7 @@ struct pci_driver {
 /* these external functions are only available when PCI support is enabled */
 #ifdef CONFIG_PCI
 
-void pcie_bus_configure_settings(struct pci_bus *bus, u8 smpss);
+void pcie_bus_configure_settings(struct pci_bus *bus);
 
 enum pcie_bus_config_types {
 	PCIE_BUS_TUNE_OFF,

[PATCH v7 3/5] PCI: Remove unnecessary check for pcie_get_mps() failure

[Bjorn Helgaas]

From: Yijing Wang <wangyijing@huawei.com>

After 59875ae489 ("PCI/core: Use PCI Express Capability accessors"),
pcie_get_mps() never returns an error, so don't bother to check for it.

No functional change.

[bhelgaas: changelog, fix pcie_get_mps() doc]
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e37fea6..5bb97ee 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3525,8 +3525,6 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
 	if (pcie_bus_config == PCIE_BUS_PERFORMANCE) {
 		int mps = pcie_get_mps(dev);
 
-		if (mps < 0)
-			return mps;
 		if (mps < rq)
 			rq = mps;
 	}
@@ -3543,7 +3541,6 @@ EXPORT_SYMBOL(pcie_set_readrq);
  * @dev: PCI device to query
  *
  * Returns maximum payload size in bytes
- *    or appropriate error value.
  */
 int pcie_get_mps(struct pci_dev *dev)
 {

[PATCH v7 4/5] PCI: Simplify MPS test for Downstream Port

[Bjorn Helgaas]

PCIe hotplug bridges are always either Root Ports or Downstream Ports.  No
other device type can have a PCIe link leading downstream to a slot.

Root Ports don't have an upstream bridge, so "dev->is_hotplug_bridge &&
dev->bus->self" is true if and only if "dev" is a Downstream Port.  That
means we can simplify this by looking at the type of "dev" itself, without
looking upstream at all.

No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ecae7f2..0591b08 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1507,8 +1507,7 @@ static int pcie_find_smpss(struct pci_dev *dev, void *data)
 	 * will occur as normal.
 	 */
 	if (dev->is_hotplug_bridge && (!list_is_singular(&dev->bus->devices) ||
-	     (dev->bus->self &&
-	      pci_pcie_type(dev->bus->self) != PCI_EXP_TYPE_ROOT_PORT)))
+	    pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT))
 		*smpss = 0;
 
 	if (*smpss > dev->pcie_mpss)

[PATCH v7 5/5] PCI: Don't restrict MPS for slots below Root Ports

[Bjorn Helgaas]

From: Yijing Wang <wangyijing@huawei.com>

When booting with "pci=pcie_bus_safe", we previously limited the
fabric MPS to 128 when we found:

  (1) A hotplug-capable Downstream Port ("dev->is_hotplug_bridge &&
      pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT"), or

  (2) A hotplug-capable Root Port with a slot that was either empty or
      contained a multi-function device ("dev->is_hotplug_bridge &&
      !list_is_singular(&dev->bus->devices)")

Part (1) is valid, but part (2) is not.

After a hot-add in the slot below a Root Port, we can reconfigure all
MPS values in the fabric below the Root Port because the new device is
the only thing below the Root Port and there are no active drivers.
Therefore, there's no reason to limit the MPS for Root Ports, no
matter what's in the slot.

Test info:

    -+-[0000:40]-+-07.0-[0000:46]--+-00.0  Intel 82576 NIC
                                   \-00.1  Intel 82576 NIC

    0000:40:07.0 Root Port bridge to [bus 46] (MPS supported=256)
    0000:46:00.0 Endpoint                     (MPS supported=512)
    0000:46:00.1 Endpoint                     (MPS supported=512)

    # echo 0 > /sys/bus/pci/slots/7/power
    # echo 1 > /sys/bus/pci/slots/7/power
    # dmesg
    ...
    pcieport 0000:40:07.0: PCI-E Max Payload Size set to 256/ 256 (was 256)
    pci 0000:46:00.0:      PCI-E Max Payload Size set to 256/ 512 (was 128)
    pci 0000:46:00.1:      PCI-E Max Payload Size set to 256/ 512 (was 128)

Before this change, we set MPS to 128 for the Root Port and both NICs
because the slot contained a multi-function device and

    dev->is_hotplug_bridge && !list_is_singular(&dev->bus->devices)

was true.  After this change, we set it to 256.

[bhelgaas: changelog, comments, split out upstream bridge check]
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Jon Mason <jdmason@kudzu.us>
---
 drivers/pci/probe.c |   32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 0591b08..94b1d22 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1491,23 +1491,23 @@ static int pcie_find_smpss(struct pci_dev *dev, void *data)
 	if (!pci_is_pcie(dev))
 		return 0;
 
-	/* For PCIE hotplug enabled slots not connected directly to a
-	 * PCI-E root port, there can be problems when hotplugging
-	 * devices.  This is due to the possibility of hotplugging a
-	 * device into the fabric with a smaller MPS that the devices
-	 * currently running have configured.  Modifying the MPS on the
-	 * running devices could cause a fatal bus error due to an
-	 * incoming frame being larger than the newly configured MPS.
-	 * To work around this, the MPS for the entire fabric must be
-	 * set to the minimum size.  Any devices hotplugged into this
-	 * fabric will have the minimum MPS set.  If the PCI hotplug
-	 * slot is directly connected to the root port and there are not
-	 * other devices on the fabric (which seems to be the most
-	 * common case), then this is not an issue and MPS discovery
-	 * will occur as normal.
+	/*
+	 * We don't have a way to change MPS settings on devices that have
+	 * drivers attached.  A hot-added device might support only the minimum
+	 * MPS setting (MPS=128).  Therefore, if the fabric contains a bridge
+	 * where devices may be hot-added, we limit the fabric MPS to 128 so
+	 * hot-added devices will work correctly.
+	 *
+	 * However, if we hot-add a device to a slot directly below a Root
+	 * Port, it's impossible for there to be other existing devices below
+	 * the port.  We don't limit the MPS in this case because we can
+	 * reconfigure MPS on both the Root Port and the hot-added device,
+	 * and there are no other devices involved.
+	 *
+	 * Note that this PCIE_BUS_SAFE path assumes no peer-to-peer DMA.
 	 */
-	if (dev->is_hotplug_bridge && (!list_is_singular(&dev->bus->devices) ||
-	    pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT))
+	if (dev->is_hotplug_bridge &&
+	    pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
 		*smpss = 0;
 
 	if (*smpss > dev->pcie_mpss)