[PATCH 0/3] MPS tuning

[PATCH 0/3] MPS tuning

[Bjorn Helgaas]

Hi Keith, et al,

This is essentially your patch, tweaked a little to move it to the
pci_device_add() path instead of the pcie_bus_configure_settings() path.

One interesting wrinkle is the quirk_intel_mc_errata() quirk, which
was previously essentially disabled because it did nothing in
PCIE_BUS_TUNE_OFF mode, which was the default.  If we change the mode to
something else (as I did in the last patch), that quirk will now be active
when it wasn't before.  I'd like to chat with Jon about whether that's the
right thing.
---

Bjorn Helgaas (2):
      PCI: Move MPS configuration check to pci_configure_device()
      PCI: Change MPS default to "match upstream bridge"

Keith Busch (1):
      PCI: Set MPS to match upstream bridge


 drivers/pci/pci.c   |    2 +-
 drivers/pci/probe.c |   54 +++++++++++++++++++++++++++++++++------------------
 include/linux/pci.h |    9 +++++----
 3 files changed, 41 insertions(+), 24 deletions(-)

[PATCH 1/3] PCI: Move MPS configuration check to pci_configure_device()

[Bjorn Helgaas]

Previously we checked for invalid MPS settings, i.e., a device with MPS
different than its upstream bridge, in pcie_bus_detect_mps().  We only did
this if the arch or hotplug driver called pcie_bus_configure_settings(),
and then only if PCIe bus tuning was disabled (PCIE_BUS_TUNE_OFF).

Move the MPS checking code to pci_configure_device(), so we do it in the
pci_device_add() path for every device.

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

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 9ff4df0..eb32395 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1275,6 +1275,27 @@ int pci_setup_device(struct pci_dev *dev)
 	return 0;
 }
 
+static void pci_configure_mps(struct pci_dev *dev)
+{
+	struct pci_dev *bridge = pci_upstream_bridge(dev);
+	int mps, p_mps;
+
+	if (!pci_is_pcie(dev) || !bridge || !pci_is_pcie(bridge))
+		return;
+
+	mps = pcie_get_mps(dev);
+	p_mps = pcie_get_mps(bridge);
+
+	if (mps == p_mps)
+		return;
+
+	if (pcie_bus_config == PCIE_BUS_TUNE_OFF) {
+		dev_warn(&dev->dev, "Max Payload Size %d, but upstream %s set to %d; if necessary, use \"pci=pcie_bus_safe\" and report a bug\n",
+			 mps, pci_name(bridge), p_mps);
+		return;
+	}
+}
+
 static struct hpp_type0 pci_default_type0 = {
 	.revision = 1,
 	.cache_line_size = 8,
@@ -1396,6 +1417,8 @@ static void pci_configure_device(struct pci_dev *dev)
 	struct hotplug_params hpp;
 	int ret;
 
+	pci_configure_mps(dev);
+
 	memset(&hpp, 0, sizeof(hpp));
 	ret = pci_get_hp_params(dev, &hpp);
 	if (ret)
@@ -1791,22 +1814,6 @@ static void pcie_write_mrrs(struct pci_dev *dev)
 		dev_err(&dev->dev, "MRRS was unable to be configured with a safe value.  If problems are experienced, try running with pci=pcie_bus_safe\n");
 }
 
-static void pcie_bus_detect_mps(struct pci_dev *dev)
-{
-	struct pci_dev *bridge = dev->bus->self;
-	int mps, p_mps;
-
-	if (!bridge)
-		return;
-
-	mps = pcie_get_mps(dev);
-	p_mps = pcie_get_mps(bridge);
-
-	if (mps != p_mps)
-		dev_warn(&dev->dev, "Max Payload Size %d, but upstream %s set to %d; if necessary, use \"pci=pcie_bus_safe\" and report a bug\n",
-			 mps, pci_name(bridge), p_mps);
-}
-
 static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
 {
 	int mps, orig_mps;
@@ -1814,10 +1821,8 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
 	if (!pci_is_pcie(dev))
 		return 0;
 
-	if (pcie_bus_config == PCIE_BUS_TUNE_OFF) {
-		pcie_bus_detect_mps(dev);
+	if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
 		return 0;
-	}
 
 	mps = 128 << *(u8 *)data;
 	orig_mps = pcie_get_mps(dev);

[PATCH 2/3] PCI: Set MPS to match upstream bridge

[Bjorn Helgaas]

From: Keith Busch <keith.busch@intel.com>

Firmware typically configures the PCIe fabric with a consistent Max Payload
Size setting based on the devices present at boot.  A hot-added device
typically has the power-on default MPS setting (128 bytes), which may not
match the fabric.

When adding a new device via pci_device_add(), set its Max Payload Size to
match the upstream bridge (unless PCIe bus tuning is disabled).  Note that
pcie_bus_configure_settings() may further change MPS based on the tuning
policy.

This makes it more likely that a hot-added device will work in a system
with optimized MPS configuration.

If we hot-add a device that only supports 128-byte MPS, it still likely
won't work because we don't reconfigure the rest of the fabric.  Booting
with "pci=pcie_bus_peer2peer" is a workaround for this because it sets MPS
to 128 for everything.

Signed-off-by: Keith Busch <keith.busch@intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index eb32395..f73dd7a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1278,7 +1278,7 @@ int pci_setup_device(struct pci_dev *dev)
 static void pci_configure_mps(struct pci_dev *dev)
 {
 	struct pci_dev *bridge = pci_upstream_bridge(dev);
-	int mps, p_mps;
+	int mps, p_mps, rc;
 
 	if (!pci_is_pcie(dev) || !bridge || !pci_is_pcie(bridge))
 		return;
@@ -1294,6 +1294,16 @@ static void pci_configure_mps(struct pci_dev *dev)
 			 mps, pci_name(bridge), p_mps);
 		return;
 	}
+
+	rc = pcie_set_mps(dev, p_mps);
+	if (rc) {
+		dev_warn(&dev->dev, "can't set Max Payload Size to %d; if necessary, use \"pci=pcie_bus_safe\" and report a bug\n",
+			 p_mps);
+		return;
+	}
+
+	dev_info(&dev->dev, "Max Payload Size set to %d (was %d, max %d)\n",
+		 p_mps, mps, 128 << dev->pcie_mpss);
 }
 
 static struct hpp_type0 pci_default_type0 = {

[PATCH 3/3] PCI: Change MPS default to "match upstream bridge"

[Bjorn Helgaas]

Change the default MPS tuning policy to "default," which means we set every
device's MPS to match its upstream bridge.  Typically this means we'll
preserve the fabric configuration done by firmware, even for hot-added
devices.

N.B. This effectively re-enables quirk_intel_mc_errata(), which turns off
read completion coalescing on Intel 5000 and 5100 memory controllers.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c   |    2 +-
 drivers/pci/probe.c |    3 ++-
 include/linux/pci.h |    9 +++++----
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0008c95..b96b4cc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -81,7 +81,7 @@ unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
 unsigned long pci_hotplug_io_size  = DEFAULT_HOTPLUG_IO_SIZE;
 unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
 
-enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_TUNE_OFF;
+enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_DEFAULT;
 
 /*
  * The default CLS is used if arch didn't set CLS explicitly and not
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index f73dd7a..9da27af 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1831,7 +1831,8 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
 	if (!pci_is_pcie(dev))
 		return 0;
 
-	if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
+	if (pcie_bus_config == PCIE_BUS_TUNE_OFF ||
+	    pcie_bus_config == PCIE_BUS_DEFAULT)
 		return 0;
 
 	mps = 128 << *(u8 *)data;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4d4f9d2..faf06a1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -738,10 +738,11 @@ struct pci_driver {
 void pcie_bus_configure_settings(struct pci_bus *bus);
 
 enum pcie_bus_config_types {
-	PCIE_BUS_TUNE_OFF,
-	PCIE_BUS_SAFE,
-	PCIE_BUS_PERFORMANCE,
-	PCIE_BUS_PEER2PEER,
+	PCIE_BUS_TUNE_OFF,	/* don't touch MPS at all */
+	PCIE_BUS_DEFAULT,	/* MPS matches upstream bridge */
+	PCIE_BUS_SAFE,		/* largest MPS supported by boot-time devices */
+	PCIE_BUS_PERFORMANCE,	/* use MPS and MRRS for best performance */
+	PCIE_BUS_PEER2PEER,	/* MPS = 128 for all devices */
 };
 
 extern enum pcie_bus_config_types pcie_bus_config;

Re: [PATCH 2/3] PCI: Set MPS to match upstream bridge

[Yinghai Lu]

On Fri, Aug 21, 2015 at 8:07 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> From: Keith Busch <keith.busch@intel.com>
>
> Firmware typically configures the PCIe fabric with a consistent Max Payload
> Size setting based on the devices present at boot.  A hot-added device
> typically has the power-on default MPS setting (128 bytes), which may not
> match the fabric.
>
> When adding a new device via pci_device_add(), set its Max Payload Size to
> match the upstream bridge (unless PCIe bus tuning is disabled).  Note that
> pcie_bus_configure_settings() may further change MPS based on the tuning
> policy.
>
> This makes it more likely that a hot-added device will work in a system
> with optimized MPS configuration.
>
> If we hot-add a device that only supports 128-byte MPS, it still likely
> won't work because we don't reconfigure the rest of the fabric.  Booting
> with "pci=pcie_bus_peer2peer" is a workaround for this because it sets MPS
> to 128 for everything.
>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/probe.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index eb32395..f73dd7a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1278,7 +1278,7 @@ int pci_setup_device(struct pci_dev *dev)
>  static void pci_configure_mps(struct pci_dev *dev)
>  {
>         struct pci_dev *bridge = pci_upstream_bridge(dev);
> -       int mps, p_mps;
> +       int mps, p_mps, rc;
>
>         if (!pci_is_pcie(dev) || !bridge || !pci_is_pcie(bridge))
>                 return;
> @@ -1294,6 +1294,16 @@ static void pci_configure_mps(struct pci_dev *dev)
>                          mps, pci_name(bridge), p_mps);
>                 return;
>         }
> +

you may need add if the pcie_bus_config == PCIE_BUS_DEFAULT here.

we should not  could set mps extra for _SAFE, _PERFORMANCE, _PEER2PEER

other than that.

For those patches:
Acked-by: Yinghai Lu <yinghai@kernel.org>

> +       rc = pcie_set_mps(dev, p_mps);
> +       if (rc) {
> +               dev_warn(&dev->dev, "can't set Max Payload Size to %d; if necessary, use \"pci=pcie_bus_safe\" and report a bug\n",
> +                        p_mps);
> +               return;
> +       }
> +
> +       dev_info(&dev->dev, "Max Payload Size set to %d (was %d, max %d)\n",
> +                p_mps, mps, 128 << dev->pcie_mpss);
>  }

Re: [PATCH 3/3] PCI: Change MPS default to "match upstream bridge"

[Bjorn Helgaas]

On Fri, Aug 21, 2015 at 8:08 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Change the default MPS tuning policy to "default," which means we set every
> device's MPS to match its upstream bridge.  Typically this means we'll
> preserve the fabric configuration done by firmware, even for hot-added
> devices.
>
> N.B. This effectively re-enables quirk_intel_mc_errata(), which turns off
> read completion coalescing on Intel 5000 and 5100 memory controllers.

I think this patch might be the wrong thing for quirk_intel_mc_errata().

Currently, the default is PCIE_BUS_TUNE_OFF.  In that case, the quirk
does nothing, Linux doesn't touch MPS at all, and the BIOS is
responsible for avoiding the erratum, either by disabling read
coalescing or by avoiding MPS=256.

This patch (as-is) means that even if the BIOS set MPS=128, we would
disable read coalescing here, which is unnecessary.  The new
PCIE_BUS_DEFAULT basically means Linux will set MPS for hot-added
devices the same way as for devices present at boot.  That means we
should be able to rely on the BIOS workaround and shouldn't have to
disable read coalescing.

So I think maybe we should bail out of quirk_intel_mc_errata() for
both TUNE_OFF and DEFAULT, e.g.,

--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2862,7 +2862,8 @@ static void quirk_intel_mc_errata(struct pci_dev *dev)
        int err;
        u16 rcc;

-       if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
+       if (pcie_bus_config == PCIE_BUS_TUNE_OFF ||
+           pcie_bus_config == PCIE_BUS_DEFAULT)
                return;


> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci.c   |    2 +-
>  drivers/pci/probe.c |    3 ++-
>  include/linux/pci.h |    9 +++++----
>  3 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 0008c95..b96b4cc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -81,7 +81,7 @@ unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
>  unsigned long pci_hotplug_io_size  = DEFAULT_HOTPLUG_IO_SIZE;
>  unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
>
> -enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_TUNE_OFF;
> +enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_DEFAULT;
>
>  /*
>   * The default CLS is used if arch didn't set CLS explicitly and not
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index f73dd7a..9da27af 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1831,7 +1831,8 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
>         if (!pci_is_pcie(dev))
>                 return 0;
>
> -       if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
> +       if (pcie_bus_config == PCIE_BUS_TUNE_OFF ||
> +           pcie_bus_config == PCIE_BUS_DEFAULT)
>                 return 0;
>
>         mps = 128 << *(u8 *)data;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 4d4f9d2..faf06a1 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -738,10 +738,11 @@ struct pci_driver {
>  void pcie_bus_configure_settings(struct pci_bus *bus);
>
>  enum pcie_bus_config_types {
> -       PCIE_BUS_TUNE_OFF,
> -       PCIE_BUS_SAFE,
> -       PCIE_BUS_PERFORMANCE,
> -       PCIE_BUS_PEER2PEER,
> +       PCIE_BUS_TUNE_OFF,      /* don't touch MPS at all */
> +       PCIE_BUS_DEFAULT,       /* MPS matches upstream bridge */
> +       PCIE_BUS_SAFE,          /* largest MPS supported by boot-time devices */
> +       PCIE_BUS_PERFORMANCE,   /* use MPS and MRRS for best performance */
> +       PCIE_BUS_PEER2PEER,     /* MPS = 128 for all devices */
>  };
>
>  extern enum pcie_bus_config_types pcie_bus_config;
>

Re: [PATCH 2/3] PCI: Set MPS to match upstream bridge

[Bjorn Helgaas]

On Fri, Aug 21, 2015 at 12:14:40PM -0700, Yinghai Lu wrote:
> On Fri, Aug 21, 2015 at 8:07 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > From: Keith Busch <keith.busch@intel.com>
> >
> > Firmware typically configures the PCIe fabric with a consistent Max Payload
> > Size setting based on the devices present at boot.  A hot-added device
> > typically has the power-on default MPS setting (128 bytes), which may not
> > match the fabric.
> >
> > When adding a new device via pci_device_add(), set its Max Payload Size to
> > match the upstream bridge (unless PCIe bus tuning is disabled).  Note that
> > pcie_bus_configure_settings() may further change MPS based on the tuning
> > policy.
> >
> > This makes it more likely that a hot-added device will work in a system
> > with optimized MPS configuration.
> >
> > If we hot-add a device that only supports 128-byte MPS, it still likely
> > won't work because we don't reconfigure the rest of the fabric.  Booting
> > with "pci=pcie_bus_peer2peer" is a workaround for this because it sets MPS
> > to 128 for everything.
> >
> > Signed-off-by: Keith Busch <keith.busch@intel.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/probe.c |   12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index eb32395..f73dd7a 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1278,7 +1278,7 @@ int pci_setup_device(struct pci_dev *dev)
> >  static void pci_configure_mps(struct pci_dev *dev)
> >  {
> >         struct pci_dev *bridge = pci_upstream_bridge(dev);
> > -       int mps, p_mps;
> > +       int mps, p_mps, rc;
> >
> >         if (!pci_is_pcie(dev) || !bridge || !pci_is_pcie(bridge))
> >                 return;
> > @@ -1294,6 +1294,16 @@ static void pci_configure_mps(struct pci_dev *dev)
> >                          mps, pci_name(bridge), p_mps);
> >                 return;
> >         }
> > +
> 
> you may need add if the pcie_bus_config == PCIE_BUS_DEFAULT here.
> 
> we should not  could set mps extra for _SAFE, _PERFORMANCE, _PEER2PEER

How about this?

commit f2082d6c5e92ceedd1ed53f2d41fe50edd859557
Author: Keith Busch <keith.busch@intel.com>
Date:   Fri Aug 21 08:46:28 2015 -0500

    PCI: Set MPS to match upstream bridge
    
    Firmware typically configures the PCIe fabric with a consistent Max Payload
    Size setting based on the devices present at boot.  A hot-added device
    typically has the power-on default MPS setting (128 bytes), which may not
    match the fabric.
    
    When adding a new device via pci_device_add(), set its Max Payload Size to
    match the upstream bridge (unless PCIe bus tuning is disabled).  Note that
    pcie_bus_configure_settings() may further change MPS based on the tuning
    policy.
    
    This makes it more likely that a hot-added device will work in a system
    with optimized MPS configuration.
    
    If we hot-add a device that only supports 128-byte MPS, it still likely
    won't work because we don't reconfigure the rest of the fabric.  Booting
    with "pci=pcie_bus_peer2peer" is a workaround for this because it sets MPS
    to 128 for everything.
    
    Signed-off-by: Keith Busch <keith.busch@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index eb32395..0e5d22d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1278,7 +1278,7 @@ int pci_setup_device(struct pci_dev *dev)
 static void pci_configure_mps(struct pci_dev *dev)
 {
 	struct pci_dev *bridge = pci_upstream_bridge(dev);
-	int mps, p_mps;
+	int mps, p_mps, rc;
 
 	if (!pci_is_pcie(dev) || !bridge || !pci_is_pcie(bridge))
 		return;
@@ -1294,6 +1294,23 @@ static void pci_configure_mps(struct pci_dev *dev)
 			 mps, pci_name(bridge), p_mps);
 		return;
 	}
+
+	/*
+	 * Fancier MPS configuration is done later by
+	 * pcie_bus_configure_settings()
+	 */
+	if (pcie_bus_config != PCIE_BUS_DEFAULT)
+		return;
+
+	rc = pcie_set_mps(dev, p_mps);
+	if (rc) {
+		dev_warn(&dev->dev, "can't set Max Payload Size to %d; if necessary, use \"pci=pcie_bus_safe\" and report a bug\n",
+			 p_mps);
+		return;
+	}
+
+	dev_info(&dev->dev, "Max Payload Size set to %d (was %d, max %d)\n",
+		 p_mps, mps, 128 << dev->pcie_mpss);
 }
 
 static struct hpp_type0 pci_default_type0 = {

Re: [PATCH 2/3] PCI: Set MPS to match upstream bridge

[Yinghai Lu]

On Fri, Aug 21, 2015 at 1:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Aug 21, 2015 at 12:14:40PM -0700, Yinghai Lu wrote:
>> On Fri, Aug 21, 2015 at 8:07 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> > From: Keith Busch <keith.busch@intel.com>
>> >
>> > Firmware typically configures the PCIe fabric with a consistent Max Payload
>> > Size setting based on the devices present at boot.  A hot-added device
>> > typically has the power-on default MPS setting (128 bytes), which may not
>> > match the fabric.
>> >
>> > When adding a new device via pci_device_add(), set its Max Payload Size to
>> > match the upstream bridge (unless PCIe bus tuning is disabled).  Note that
>> > pcie_bus_configure_settings() may further change MPS based on the tuning
>> > policy.
>> >
>> > This makes it more likely that a hot-added device will work in a system
>> > with optimized MPS configuration.
>> >
>> > If we hot-add a device that only supports 128-byte MPS, it still likely
>> > won't work because we don't reconfigure the rest of the fabric.  Booting
>> > with "pci=pcie_bus_peer2peer" is a workaround for this because it sets MPS
>> > to 128 for everything.
>> >
>> > Signed-off-by: Keith Busch <keith.busch@intel.com>
>> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> > ---
>> >  drivers/pci/probe.c |   12 +++++++++++-
>> >  1 file changed, 11 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> > index eb32395..f73dd7a 100644
>> > --- a/drivers/pci/probe.c
>> > +++ b/drivers/pci/probe.c
>> > @@ -1278,7 +1278,7 @@ int pci_setup_device(struct pci_dev *dev)
>> >  static void pci_configure_mps(struct pci_dev *dev)
>> >  {
>> >         struct pci_dev *bridge = pci_upstream_bridge(dev);
>> > -       int mps, p_mps;
>> > +       int mps, p_mps, rc;
>> >
>> >         if (!pci_is_pcie(dev) || !bridge || !pci_is_pcie(bridge))
>> >                 return;
>> > @@ -1294,6 +1294,16 @@ static void pci_configure_mps(struct pci_dev *dev)
>> >                          mps, pci_name(bridge), p_mps);
>> >                 return;
>> >         }
>> > +
>>
>> you may need add if the pcie_bus_config == PCIE_BUS_DEFAULT here.
>>
>> we should not  could set mps extra for _SAFE, _PERFORMANCE, _PEER2PEER
>
> How about this?

> @@ -1294,6 +1294,23 @@ static void pci_configure_mps(struct pci_dev *dev)
>                          mps, pci_name(bridge), p_mps);
>                 return;
>         }
> +
> +       /*
> +        * Fancier MPS configuration is done later by
> +        * pcie_bus_configure_settings()
> +        */
> +       if (pcie_bus_config != PCIE_BUS_DEFAULT)
> +               return;
> +
> +       rc = pcie_set_mps(dev, p_mps);
> +       if (rc) {
> +               dev_warn(&dev->dev, "can't set Max Payload Size to %d; if necessary, use \"pci=pcie_bus_safe\" and report a bug\n",
> +                        p_mps);
> +               return;
> +       }
> +
> +       dev_info(&dev->dev, "Max Payload Size set to %d (was %d, max %d)\n",
> +                p_mps, mps, 128 << dev->pcie_mpss);
>  }

Good to me.

Thanks

Yinghai

Re: [PATCH 3/3] PCI: Change MPS default to "match upstream bridge"

[Keith Busch]

On Fri, 21 Aug 2015, Bjorn Helgaas wrote:
> On Fri, Aug 21, 2015 at 8:08 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> Change the default MPS tuning policy to "default," which means we set every
>> device's MPS to match its upstream bridge.  Typically this means we'll
>> preserve the fabric configuration done by firmware, even for hot-added
>> devices.
>>
>> N.B. This effectively re-enables quirk_intel_mc_errata(), which turns off
>> read completion coalescing on Intel 5000 and 5100 memory controllers.
>
> I think this patch might be the wrong thing for quirk_intel_mc_errata().
>
> Currently, the default is PCIE_BUS_TUNE_OFF.  In that case, the quirk
> does nothing, Linux doesn't touch MPS at all, and the BIOS is
> responsible for avoiding the erratum, either by disabling read
> coalescing or by avoiding MPS=256.
>
> This patch (as-is) means that even if the BIOS set MPS=128, we would
> disable read coalescing here, which is unnecessary.  The new
> PCIE_BUS_DEFAULT basically means Linux will set MPS for hot-added
> devices the same way as for devices present at boot.  That means we
> should be able to rely on the BIOS workaround and shouldn't have to
> disable read coalescing.
>
> So I think maybe we should bail out of quirk_intel_mc_errata() for
> both TUNE_OFF and DEFAULT, e.g.,
>
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2862,7 +2862,8 @@ static void quirk_intel_mc_errata(struct pci_dev *dev)
>        int err;
>        u16 rcc;
>
> -       if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
> +       if (pcie_bus_config == PCIE_BUS_TUNE_OFF ||
> +           pcie_bus_config == PCIE_BUS_DEFAULT)
>                return;

Hi Bjorn,

I've successfully tested the previously failing hot-plug tests with your
pci/enumeration tree.

I don't understand the reported kbuild failure, though. It built
successfully for me and the error didn't make sense just from looking
at the patch either, so I hope it was a false alarm.

Thank you for working on this feature with us. I've no further concerns
with the most recent implementation, think it looks great and all the
better for the effort.

Tested-by: Keith Busch <keith.busch@intel.com>

>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Re: [PATCH 3/3] PCI: Change MPS default to "match upstream bridge"

[Bjorn Helgaas]

On Mon, Aug 24, 2015 at 9:13 AM, Keith Busch <keith.busch@intel.com> wrote:
> On Fri, 21 Aug 2015, Bjorn Helgaas wrote:
>>
>> On Fri, Aug 21, 2015 at 8:08 AM, Bjorn Helgaas <bhelgaas@google.com>
>> wrote:
>>>
>>> Change the default MPS tuning policy to "default," which means we set
>>> every
>>> device's MPS to match its upstream bridge.  Typically this means we'll
>>> preserve the fabric configuration done by firmware, even for hot-added
>>> devices.
>>>
>>> N.B. This effectively re-enables quirk_intel_mc_errata(), which turns off
>>> read completion coalescing on Intel 5000 and 5100 memory controllers.
>>
>>
>> I think this patch might be the wrong thing for quirk_intel_mc_errata().
>>
>> Currently, the default is PCIE_BUS_TUNE_OFF.  In that case, the quirk
>> does nothing, Linux doesn't touch MPS at all, and the BIOS is
>> responsible for avoiding the erratum, either by disabling read
>> coalescing or by avoiding MPS=256.
>>
>> This patch (as-is) means that even if the BIOS set MPS=128, we would
>> disable read coalescing here, which is unnecessary.  The new
>> PCIE_BUS_DEFAULT basically means Linux will set MPS for hot-added
>> devices the same way as for devices present at boot.  That means we
>> should be able to rely on the BIOS workaround and shouldn't have to
>> disable read coalescing.
>>
>> So I think maybe we should bail out of quirk_intel_mc_errata() for
>> both TUNE_OFF and DEFAULT, e.g.,
>>
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -2862,7 +2862,8 @@ static void quirk_intel_mc_errata(struct pci_dev
>> *dev)
>>        int err;
>>        u16 rcc;
>>
>> -       if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
>> +       if (pcie_bus_config == PCIE_BUS_TUNE_OFF ||
>> +           pcie_bus_config == PCIE_BUS_DEFAULT)
>>                return;
>
>
> Hi Bjorn,
>
> I've successfully tested the previously failing hot-plug tests with your
> pci/enumeration tree.
>
> I don't understand the reported kbuild failure, though. It built
> successfully for me and the error didn't make sense just from looking
> at the patch either, so I hope it was a false alarm.

I couldn't figure that out either, but I reworked them slightly to
squash the last two patches together, and I just got the build success
email.

> Thank you for working on this feature with us. I've no further concerns
> with the most recent implementation, think it looks great and all the
> better for the effort.
>
> Tested-by: Keith Busch <keith.busch@intel.com>

Thanks for testing it!  I'll put it into -next today.

Bjorn