[PATCH v4 0/2] update device mps

[PATCH v4 0/2] update device mps

[Yijing Wang]

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 if 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: fix the only slot identification in pcie_find_smpss()
  PCI: update device mps when doing pci hotplug

 drivers/pci/probe.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 68 insertions(+), 2 deletions(-)

[PATCH v4 1/2] PCI: fix the only slot identification in pcie_find_smpss()

[Yijing Wang]

We use list_is_singular() to identify the slot whether is
only slot directly connected to the root port in
pcie_find_smpss(). It's not correct, if we have only slot
connected to root port, and this slot has two function devices.
list_is_singular() return false. This patch introduces
pci_only_one_slot() to fix this issue. In addition, we should
pass subordinate bus devices list to list_is_singular(), not
its parent bus devices list.

-+-[0000:40]-+-00.0-[0000:41]--
    ......................
 |           +-07.0-[0000:46]--+-00.0  Intel Corporation 82576 Gigabit Network Connection
 |           |                 \-00.1  Intel Corporation 82576 Gigabit Network Connection

MPS configure after boot up, with boot command "pci=pcie_bus_safe"

linux-ha2:~ # lspci -vvv -s 40:07.0
40:07.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root Port 7 (rev 22) (prog-if 00 [Normal decode])
	...............
	Capabilities: [90] Express (v2) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 256 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
			ExtTag+ RBE+ FLReset-
		DevCtl:	Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+
			RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
			MaxPayload 256 bytes, MaxReadReq 128 bytes

linux-ha2:~ # lspci -vvv -s 46:00.0
46:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network Connection (rev 01)
	...............
	Capabilities: [a0] Express (v2) Endpoint, MSI 00
		DevCap:	MaxPayload 512 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us
			ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset+
		DevCtl:	Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+ FLReset-
			MaxPayload 256 bytes, MaxReadReq 512 bytes

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

Because 46:00.0 and 46:00.1 function devices are directly connected to root port 40:07.0.
After slot hot plug, root port mps is 256, slot fun devices(46:00.0/1) mps is 128.
We should both change root port and slot mps to 256, but now kernel change mps to 128.


After applied this patch, dmesg after hot plug:
..............
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

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 |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cf57fe7..0699ec0 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1484,6 +1484,24 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
 	return nr;
 }
 
+static bool pci_only_one_slot(struct pci_bus *pbus)
+{
+	u8 device;
+	struct pci_dev *pdev;
+
+	if (!pbus || list_empty(&pbus->devices))
+		return false;
+
+	pdev = list_entry(pbus->devices.next,
+			struct pci_dev, bus_list);
+	device = PCI_SLOT(pdev->devfn);
+	list_for_each_entry(pdev, &pbus->devices, bus_list)
+		if (PCI_SLOT(pdev->devfn) != device)
+			return false;
+
+	return true;
+}
+
 static int pcie_find_smpss(struct pci_dev *dev, void *data)
 {
 	u8 *smpss = data;
@@ -1506,7 +1524,7 @@ static int pcie_find_smpss(struct pci_dev *dev, void *data)
 	 * common case), then this is not an issue and MPS discovery
 	 * will occur as normal.
 	 */
-	if (dev->is_hotplug_bridge && (!list_is_singular(&dev->bus->devices) ||
+	if (dev->is_hotplug_bridge && (!pci_only_one_slot(dev->subordinate) ||
 	     (dev->bus->self &&
 	      pci_pcie_type(dev->bus->self) != PCI_EXP_TYPE_ROOT_PORT)))
 		*smpss = 0;
-- 
1.7.1

Re: [PATCH v4 1/2] PCI: fix the only slot identification in pcie_find_smpss()

[Jon Mason]

On Wed, Aug 14, 2013 at 2:01 AM, Yijing Wang <wangyijing@huawei.com> wrote:
> We use list_is_singular() to identify the slot whether is
> only slot directly connected to the root port in
> pcie_find_smpss(). It's not correct, if we have only slot
> connected to root port, and this slot has two function devices.
> list_is_singular() return false. This patch introduces
> pci_only_one_slot() to fix this issue. In addition, we should
> pass subordinate bus devices list to list_is_singular(), not
> its parent bus devices list.
>
> -+-[0000:40]-+-00.0-[0000:41]--
>     ......................
>  |           +-07.0-[0000:46]--+-00.0  Intel Corporation 82576 Gigabit Network Connection
>  |           |                 \-00.1  Intel Corporation 82576 Gigabit Network Connection
>
> MPS configure after boot up, with boot command "pci=pcie_bus_safe"
>
> linux-ha2:~ # lspci -vvv -s 40:07.0
> 40:07.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root Port 7 (rev 22) (prog-if 00 [Normal decode])
>         ...............
>         Capabilities: [90] Express (v2) Root Port (Slot+), MSI 00
>                 DevCap: MaxPayload 256 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
>                         ExtTag+ RBE+ FLReset-
>                 DevCtl: Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+
>                         RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
>                         MaxPayload 256 bytes, MaxReadReq 128 bytes
>
> linux-ha2:~ # lspci -vvv -s 46:00.0
> 46:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network Connection (rev 01)
>         ...............
>         Capabilities: [a0] Express (v2) Endpoint, MSI 00
>                 DevCap: MaxPayload 512 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us
>                         ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset+
>                 DevCtl: Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+
>                         RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+ FLReset-
>                         MaxPayload 256 bytes, MaxReadReq 512 bytes
>
> linux-ha2:/sys/bus/pci/slots/7 # echo 0 > power     ------>power off slot
> linux-ha2:/sys/bus/pci/slots/7 # echo 1 > power     ------>power on slot
> linux-ha2:/sys/bus/pci/slots/7 # dmesg
> ................
> pcieport 0000:40:07.0: PCI-E Max Payload Size set to  128/ 256 (was  256), Max Read Rq  128
> pci 0000:46:00.0: PCI-E Max Payload Size set to  128/ 512 (was  128), Max Read Rq  512
> pci 0000:46:00.1: PCI-E Max Payload Size set to  128/ 512 (was  128), Max Read Rq  512
> pcieport 0000:40:07.0: PCI-E Max Payload Size set to  128/ 256 (was  128), Max Read Rq  128
> pci 0000:46:00.0: PCI-E Max Payload Size set to  128/ 512 (was  128), Max Read Rq  512
> pci 0000:46:00.1: PCI-E Max Payload Size set to  128/ 512 (was  128), Max Read Rq  512
> .....
>
> Because 46:00.0 and 46:00.1 function devices are directly connected to root port 40:07.0.
> After slot hot plug, root port mps is 256, slot fun devices(46:00.0/1) mps is 128.
> We should both change root port and slot mps to 256, but now kernel change mps to 128.
>
>
> After applied this patch, dmesg after hot plug:
> ..............
> 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
>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>

Good catch!
Acked-by: Jon Mason <jdmason@kudzu.us>

> Cc: Jon Mason <jdmason@kudzu.us>
> Cc: stable@vger.kernel.org # 3.4+
> ---
>  drivers/pci/probe.c |   20 +++++++++++++++++++-
>  1 files changed, 19 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index cf57fe7..0699ec0 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1484,6 +1484,24 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
>         return nr;
>  }
>
> +static bool pci_only_one_slot(struct pci_bus *pbus)
> +{
> +       u8 device;
> +       struct pci_dev *pdev;
> +
> +       if (!pbus || list_empty(&pbus->devices))
> +               return false;
> +
> +       pdev = list_entry(pbus->devices.next,
> +                       struct pci_dev, bus_list);
> +       device = PCI_SLOT(pdev->devfn);
> +       list_for_each_entry(pdev, &pbus->devices, bus_list)
> +               if (PCI_SLOT(pdev->devfn) != device)
> +                       return false;
> +
> +       return true;
> +}
> +
>  static int pcie_find_smpss(struct pci_dev *dev, void *data)
>  {
>         u8 *smpss = data;
> @@ -1506,7 +1524,7 @@ static int pcie_find_smpss(struct pci_dev *dev, void *data)
>          * common case), then this is not an issue and MPS discovery
>          * will occur as normal.
>          */
> -       if (dev->is_hotplug_bridge && (!list_is_singular(&dev->bus->devices) ||
> +       if (dev->is_hotplug_bridge && (!pci_only_one_slot(dev->subordinate) ||
>              (dev->bus->self &&
>               pci_pcie_type(dev->bus->self) != PCI_EXP_TYPE_ROOT_PORT)))
>                 *smpss = 0;
> --
> 1.7.1
>
>

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

[Yijing Wang]

Currently we don't update device's mps vaule 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 euqal 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 |   50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 49 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 0699ec0..0d47b4a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1621,6 +1621,45 @@ 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.
+	 */
+	if ((128 << dev->pcie_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)) {
+		mpss = 128 << dev->pcie_mpss;
+		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.
@@ -1632,8 +1671,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 v4 2/2] PCI: update device mps when doing pci hotplug

[Jon Mason]

On Wed, Aug 14, 2013 at 2:01 AM, Yijing Wang <wangyijing@huawei.com> wrote:
> Currently we don't update device's mps vaule when doing

Spelling nit, "value"

> 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 euqal to its parent mps, if device mpss < parent mps,

Spelling nit, "equal".

> 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 |   50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 49 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 0699ec0..0d47b4a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1621,6 +1621,45 @@ 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)

I'm probably overly paranoid, but I worry what would happen in the >
case.  Should we try and force the device MPS to a sane value or are
we confident that this will never happen?

> +               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.
> +        */
> +       if ((128 << dev->pcie_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)) {
> +               mpss = 128 << dev->pcie_mpss;

OCD nit, you could move the above line up 4 lines and reuse the
variable in the first if-statement.

> +               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);
> +       }

Nit, braces are unnecessary in the else.  I think
scripts/checkpatch.pl would've complained if you ran it on the patch.

If you/Bjorn want to disregard my paranoia and nits, it looks fine to me.

> +       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.
> @@ -1632,8 +1671,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 v4 2/2] PCI: update device mps when doing pci hotplug

[Yijing Wang]

Hi Jon,
   Thanks for your review and comments!

On 2013/8/15 0:59, Jon Mason wrote:
> On Wed, Aug 14, 2013 at 2:01 AM, Yijing Wang <wangyijing@huawei.com> wrote:
>> Currently we don't update device's mps vaule when doing
> 
> Spelling nit, "value"

Will update, thanks!

> 
>> 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 euqal to its parent mps, if device mpss < parent mps,
> 
> Spelling nit, "equal".

Will update.

> 
>> 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 |   50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 49 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 0699ec0..0d47b4a 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -1621,6 +1621,45 @@ 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)
> 
> I'm probably overly paranoid, but I worry what would happen in the >
> case.  Should we try and force the device MPS to a sane value or are
> we confident that this will never happen?

In the original patch, this patch will update the mps if mps != p_mps.
For conservative, this patch only to fix the issue occurs in hot-plug(mps will never larger than p_mps).
I don't know whether BIOS will set mps like mps > p_mps for certain purposes.:)

> 
>> +               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.
>> +        */
>> +       if ((128 << dev->pcie_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)) {
>> +               mpss = 128 << dev->pcie_mpss;
> 
> OCD nit, you could move the above line up 4 lines and reuse the
> variable in the first if-statement.

Good, thanks!

> 
>> +               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);
>> +       }
> 
> Nit, braces are unnecessary in the else.  I think
> scripts/checkpatch.pl would've complained if you ran it on the patch.

I used scripts/checkpatch.pl to check patch, result is ok.
So if I use scripts/checkpatch.pl to check patch without the pair braces, and result is ok,
I will remove it. Thanks!

> 
> If you/Bjorn want to disregard my paranoia and nits, it looks fine to me.
> 
>> +       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.
>> @@ -1632,8 +1671,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
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 


-- 
Thanks!
Yijing