[PATCH v6 0/2] update device mps

[PATCH v6 0/2] update device mps

[Yijing Wang]

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.



Yijing Wang (2):
  PCI: remove the unnecessary check in pcie_find_smpss()
  PCI: update device mps when doing pci hotplug

 drivers/pci/probe.c |   60 ++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 52 insertions(+), 8 deletions(-)

[PATCH v6 1/2] PCI: remove the unnecessary check in pcie_find_smpss()

[Yijing Wang]

In "pci=pcie_bus_safe" mode, if find a hotplug slot, we will set
all devices which in the path mps value to the minimum mps
support(128B). Unless the slot is directly connected to root port,
and there are not other devices on the fabric. In the latter case,
we will set root port and device mps to the minmum mpss support
for performace. Currently we use list_is_singular() to identify
whether slot is the only one connected to root port, it's not
necessary, because root port always connected to only one slot.

Following is test info:
-+-[0000:40]-+-00.0-[0000:41]--
	................
 |           +-05.0-[0000:45]--
 |           +-07.0-[0000:46]--+-00.0  Intel Corporation 82576 Gigabit Network Connection
 |           |                 \-00.1  Intel Corporation 82576 Gigabit Network Connection

root port (40:07.0 mps=256 mpss=256)
EndPoing deivce (46:00.0/1 mps=256 mpss=512)

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

result is good, root port and device mps were wrote to 256, original code wrote to 128

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Jon Mason <jdmason@kudzu.us>
---
 drivers/pci/probe.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cf57fe7..160ae38 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1501,14 +1501,11 @@ static int pcie_find_smpss(struct pci_dev *dev, void *data)
 	 * 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.
+	 * slot is directly connected to the root port, then this is
+	 * not an issue and MPS discovery 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)))
+	if (dev->is_hotplug_bridge &&
+	      pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
 		*smpss = 0;
 
 	if (*smpss > dev->pcie_mpss)
-- 
1.7.1

[PATCH v6 2/2] PCI: update device mps when doing pci hotplug

[Yijing Wang]

Currently we don't update device's mps value when doing
pci device hot-add. The hot-added device's mps will be set
to default value (128B). But the upstream port device's mps
may be larger than 128B which was set by firmware during
system bootup. In this case the new added device may not
work normally. This patch try to update the hot added device
mps equal to its parent mps, if device mpss < parent mps,
print warning.

References: https://bugzilla.kernel.org/show_bug.cgi?id=60671
Reported-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Jon Mason <jdmason@kudzu.us>
Cc: stable@vger.kernel.org # 3.4+
---
 drivers/pci/probe.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 160ae38..a18fa3b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1600,6 +1600,44 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
 	return 0;
 }
 
+static int pcie_bus_update_set(struct pci_dev *dev, void *data)
+{
+	int mps, p_mps, mpss;
+	struct pci_dev *parent;
+
+	if (!pci_is_pcie(dev) || !dev->bus->self)
+		return 0;
+
+	parent = dev->bus->self;
+	mps = pcie_get_mps(dev);
+	p_mps = pcie_get_mps(dev->bus->self);
+
+	if (mps >= p_mps)
+		return 0;
+
+	/* we only update the device mps, unless its parent device is root port,
+	 * and it is the only slot directly connected to root port.
+	 */
+	mpss = 128 << dev->pcie_mpss;
+	if (mpss >= p_mps) {
+		pcie_write_mps(dev, p_mps);
+	} else if (pci_pcie_type(parent) == PCI_EXP_TYPE_ROOT_PORT &&
+			pci_only_one_slot(dev->bus)) {
+		pcie_write_mps(parent, mpss);
+		pcie_write_mps(dev, mpss);
+	} else
+		dev_warn(&dev->dev, "MPS %d MPSS %d both smaller than upstream MPS %d\n"
+				"If necessary, use \"pci=pcie_bus_peer2peer\" boot parameter to avoid this problem\n",
+				mps, 128 << dev->pcie_mpss, p_mps);
+	return 0;
+}
+
+static void pcie_bus_update_setting(struct pci_bus *bus)
+{
+	if (bus->self->is_hotplug_bridge)
+		pci_walk_bus(bus, pcie_bus_update_set, NULL);
+}
+
 /* pcie_bus_configure_settings requires that pci_walk_bus work in a top-down,
  * parents then children fashion.  If this changes, then this code will not
  * work as designed.
@@ -1611,8 +1649,17 @@ void pcie_bus_configure_settings(struct pci_bus *bus, u8 mpss)
 	if (!pci_is_pcie(bus->self))
 		return;
 
-	if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
+	if (pcie_bus_config == PCIE_BUS_TUNE_OFF) {
+		/* Sometimes we should update device mps here,
+		 * eg. after hot add, device mps value will be
+		 * set to default(128B), but the upstream port
+		 * mps value may be larger than 128B, if we do
+		 * not update the device mps, it maybe can not
+		 * work normally.
+		 */
+		pcie_bus_update_setting(bus);
 		return;
+	}
 
 	/* FIXME - Peer to peer DMA is possible, though the endpoint would need
 	 * to be aware to the MPS of the destination.  To work around this,
-- 
1.7.1

Re: [PATCH v6 2/2] PCI: update device mps when doing pci hotplug

[Bjorn Helgaas]

On Wed, Aug 21, 2013 at 10:25:37AM +0800, Yijing Wang wrote:
> Currently we don't update device's mps value when doing
> pci device hot-add. The hot-added device's mps will be set
> to default value (128B). But the upstream port device's mps
> may be larger than 128B which was set by firmware during
> system bootup. In this case the new added device may not
> work normally. This patch try to update the hot added device
> mps equal to its parent mps, if device mpss < parent mps,
> print warning.
> 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=60671
> Reported-by: Yijing Wang <wangyijing@huawei.com>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> Cc: Jon Mason <jdmason@kudzu.us>
> Cc: stable@vger.kernel.org # 3.4+
> ---
>  drivers/pci/probe.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 48 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 160ae38..a18fa3b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1600,6 +1600,44 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
>  	return 0;
>  }
>  
> +static int pcie_bus_update_set(struct pci_dev *dev, void *data)
> +{
> +	int mps, p_mps, mpss;
> +	struct pci_dev *parent;
> +
> +	if (!pci_is_pcie(dev) || !dev->bus->self)
> +		return 0;
> +
> +	parent = dev->bus->self;
> +	mps = pcie_get_mps(dev);
> +	p_mps = pcie_get_mps(dev->bus->self);
> +
> +	if (mps >= p_mps)
> +		return 0;
> +
> +	/* we only update the device mps, unless its parent device is root port,
> +	 * and it is the only slot directly connected to root port.
> +	 */
> +	mpss = 128 << dev->pcie_mpss;
> +	if (mpss >= p_mps) {
> +		pcie_write_mps(dev, p_mps);
> +	} else if (pci_pcie_type(parent) == PCI_EXP_TYPE_ROOT_PORT &&
> +			pci_only_one_slot(dev->bus)) {

Good grief.  You apparently didn't even compile this, because
pci_only_one_slot() doesn't exist.

I reworked a couple of your previous patches and added a cleanup or two
of my own.  I'll post those as v7, and you can fix this one and post it
as a v8.  That way you and Jon can fix errors in my comments and changelogs
at the same time.

> +		pcie_write_mps(parent, mpss);
> +		pcie_write_mps(dev, mpss);
> +	} else
> +		dev_warn(&dev->dev, "MPS %d MPSS %d both smaller than upstream MPS %d\n"
> +				"If necessary, use \"pci=pcie_bus_peer2peer\" boot parameter to avoid this problem\n",
> +				mps, 128 << dev->pcie_mpss, p_mps);
> +	return 0;
> +}
> +
> +static void pcie_bus_update_setting(struct pci_bus *bus)
> +{
> +	if (bus->self->is_hotplug_bridge)
> +		pci_walk_bus(bus, pcie_bus_update_set, NULL);
> +}
> +
>  /* pcie_bus_configure_settings requires that pci_walk_bus work in a top-down,
>   * parents then children fashion.  If this changes, then this code will not
>   * work as designed.
> @@ -1611,8 +1649,17 @@ void pcie_bus_configure_settings(struct pci_bus *bus, u8 mpss)
>  	if (!pci_is_pcie(bus->self))
>  		return;
>  
> -	if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
> +	if (pcie_bus_config == PCIE_BUS_TUNE_OFF) {
> +		/* Sometimes we should update device mps here,
> +		 * eg. after hot add, device mps value will be
> +		 * set to default(128B), but the upstream port
> +		 * mps value may be larger than 128B, if we do
> +		 * not update the device mps, it maybe can not
> +		 * work normally.
> +		 */
> +		pcie_bus_update_setting(bus);
>  		return;
> +	}
>  
>  	/* FIXME - Peer to peer DMA is possible, though the endpoint would need
>  	 * to be aware to the MPS of the destination.  To work around this,
> -- 
> 1.7.1
> 
> 

Re: [PATCH v6 2/2] PCI: update device mps when doing pci hotplug

[Yijing Wang]

>> +	mpss = 128 << dev->pcie_mpss;
>> +	if (mpss >= p_mps) {
>> +		pcie_write_mps(dev, p_mps);
>> +	} else if (pci_pcie_type(parent) == PCI_EXP_TYPE_ROOT_PORT &&
>> +			pci_only_one_slot(dev->bus)) {
> 
> Good grief.  You apparently didn't even compile this, because
> pci_only_one_slot() doesn't exist.

I wanna shoot myself, :(, so sorry.

> 
> I reworked a couple of your previous patches and added a cleanup or two
> of my own.  I'll post those as v7, and you can fix this one and post it
> as a v8.  That way you and Jon can fix errors in my comments and changelogs
> at the same time.

OK, I will update this patch and try to test all this series in my machine.


Thanks!
Yijing.

> 
>> +		pcie_write_mps(parent, mpss);
>> +		pcie_write_mps(dev, mpss);
>> +	} else
>> +		dev_warn(&dev->dev, "MPS %d MPSS %d both smaller than upstream MPS %d\n"
>> +				"If necessary, use \"pci=pcie_bus_peer2peer\" boot parameter to avoid this problem\n",
>> +				mps, 128 << dev->pcie_mpss, p_mps);
>> +	return 0;
>> +}
>> +
>> +static void pcie_bus_update_setting(struct pci_bus *bus)
>> +{
>> +	if (bus->self->is_hotplug_bridge)
>> +		pci_walk_bus(bus, pcie_bus_update_set, NULL);
>> +}
>> +
>>  /* pcie_bus_configure_settings requires that pci_walk_bus work in a top-down,
>>   * parents then children fashion.  If this changes, then this code will not
>>   * work as designed.
>> @@ -1611,8 +1649,17 @@ void pcie_bus_configure_settings(struct pci_bus *bus, u8 mpss)
>>  	if (!pci_is_pcie(bus->self))
>>  		return;
>>  
>> -	if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
>> +	if (pcie_bus_config == PCIE_BUS_TUNE_OFF) {
>> +		/* Sometimes we should update device mps here,
>> +		 * eg. after hot add, device mps value will be
>> +		 * set to default(128B), but the upstream port
>> +		 * mps value may be larger than 128B, if we do
>> +		 * not update the device mps, it maybe can not
>> +		 * work normally.
>> +		 */
>> +		pcie_bus_update_setting(bus);
>>  		return;
>> +	}
>>  
>>  	/* FIXME - Peer to peer DMA is possible, though the endpoint would need
>>  	 * to be aware to the MPS of the destination.  To work around this,
>> -- 
>> 1.7.1
>>
>>
> 
> .
> 


-- 
Thanks!
Yijing