Patchwork [v5,1/2] PCI: fix the only slot identification in pcie_find_smpss()

login
register
mail settings
Submitter Yijing Wang
Date Aug. 15, 2013, 2:18 a.m.
Message ID <1376533135-36708-2-git-send-email-wangyijing@huawei.com>
Download mbox | patch
Permalink /patch/267302/
State Changes Requested
Headers show

Comments

Yijing Wang - Aug. 15, 2013, 2:18 a.m.
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

Acked-by: Jon Mason <jdmason@kudzu.us>
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(-)
Bjorn Helgaas - Aug. 16, 2013, 10:17 p.m.
On Thu, Aug 15, 2013 at 10:18:54AM +0800, Yijing Wang 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.

This is a perfect example of a changelog that looks like it has a lot
of information but makes absolutely no sense.  It describes the lowest
possible level of detail, but nothing to motivate or justify the
change.

I assume we want this patch because it allows us to set larger MPS
values for hot-added devices, which increases performance.  Or maybe
the hot-added devices just don't work because we set their MPS wrong.
Whatever it is, you should mention it.

Apparently this change has to do with multi-function devices, but you
don't say why that's important.  You should include a reference to
whatever spec section this is related to.

I wonder whether pci_only_one_slot() does what you intend for ARI
devices, where a multi-function device may have up to 256 functions.
PCI_SLOT() will return different "device" numbers for those functions
even though they're actually part of the same device.  Please explain.

In fact, I'm not sure the idea of "pci_only_one_slot()" even makes
sense for PCIe.  A conventional PCI bus may have several slots on it,
so you can have multiple devices (each of which may be a multi-
function device) on the bus.  But PCIe is inherently point-to-point,
so there's no way a link (which is really the analog of a conventional
PCI bus) can lead to multiple slots unless it first leads to a switch
or bridge that then fans out to multiple slots.

Bjorn

> -+-[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
> 
> Acked-by: Jon Mason <jdmason@kudzu.us>
> 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
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yijing Wang - Aug. 19, 2013, 2:49 a.m.
Hi Jon, do you think check the slot is only connected to upstream port is necessary?
As Bjorn's comment, we don't know whether there is a case of a port connected to more than one slot.

On 2013/8/17 6:17, Bjorn Helgaas wrote:
> On Thu, Aug 15, 2013 at 10:18:54AM +0800, Yijing Wang 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.
> 
> This is a perfect example of a changelog that looks like it has a lot
> of information but makes absolutely no sense.  It describes the lowest
> possible level of detail, but nothing to motivate or justify the
> change.

I'm so sorry. I update the log as following:

PCI: use correct interface to identify the only-slot connection

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(which seems to
be the most common case). But list_is_singular() can only identify
whether the function device is only one in parent bus. This patch
introduce pci_only_one_slot() funcntion to fix this issue.

> 
> I assume we want this patch because it allows us to set larger MPS
> values for hot-added devices, which increases performance. 

Right.

> Or maybe
> the hot-added devices just don't work because we set their MPS wrong.
> Whatever it is, you should mention it.
> 
> Apparently this change has to do with multi-function devices, but you
> don't say why that's important.  You should include a reference to
> whatever spec section this is related to.

In the most common case, we hot add device support multi-function.
In the original patch, if we insert multi-function device, the code
which use list_is_singular() function always think there are more than one
slot connected to parent port. And this is not match the comment in pcie_find_smpss().

> 
> I wonder whether pci_only_one_slot() does what you intend for ARI
> devices, where a multi-function device may have up to 256 functions.
> PCI_SLOT() will return different "device" numbers for those functions
> even though they're actually part of the same device.  Please explain.

No, But I should consider the ARI device case, if ARI device found,
pci_only_one_slot() should always return true.
Exactly, I've never seen root port connected to more than one slot.
Here I just fix the original logic, maybe Jon know whether this is necessary.

> 
> In fact, I'm not sure the idea of "pci_only_one_slot()" even makes
> sense for PCIe.  A conventional PCI bus may have several slots on it,
> so you can have multiple devices (each of which may be a multi-
> function device) on the bus.  But PCIe is inherently point-to-point,
> so there's no way a link (which is really the analog of a conventional
> PCI bus) can lead to multiple slots unless it first leads to a switch
> or bridge that then fans out to multiple slots.

As mentioned above, I don't know Whether there is a case of a port to connected to
multiple devices.

HI Jon, any comments about this?

In my idea, we should fix this issue by using like pci_only_one_slot() or delete the list_is_singular() check (if we think it's unnecessary).

Thanks!
Yijing.

> 
>> -+-[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
>>
>> Acked-by: Jon Mason <jdmason@kudzu.us>
>> 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
>>
>>
> 
> .
>
Jon Mason - Aug. 19, 2013, 5:42 p.m.
On Sun, Aug 18, 2013 at 7:49 PM, Yijing Wang <wangyijing@huawei.com> wrote:
> Hi Jon, do you think check the slot is only connected to upstream port is necessary?

Yes.

> As Bjorn's comment, we don't know whether there is a case of a port connected to more than one slot.

My understanding of Bjorn's comment is that there may be multiple
children of a single device, which may or may not be one or more
slots.  My response to that is that he is correct and that it is a
poor name, but the functionality it necessary (though perhaps
incomplete).

>
> On 2013/8/17 6:17, Bjorn Helgaas wrote:
>> On Thu, Aug 15, 2013 at 10:18:54AM +0800, Yijing Wang 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.
>>
>> This is a perfect example of a changelog that looks like it has a lot
>> of information but makes absolutely no sense.  It describes the lowest
>> possible level of detail, but nothing to motivate or justify the
>> change.
>
> I'm so sorry. I update the log as following:
>
> PCI: use correct interface to identify the only-slot connection
>
> 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(which seems to
> be the most common case). But list_is_singular() can only identify
> whether the function device is only one in parent bus. This patch
> introduce pci_only_one_slot() funcntion to fix this issue.
>
>>
>> I assume we want this patch because it allows us to set larger MPS
>> values for hot-added devices, which increases performance.
>
> Right.
>
>> Or maybe
>> the hot-added devices just don't work because we set their MPS wrong.
>> Whatever it is, you should mention it.
>>
>> Apparently this change has to do with multi-function devices, but you
>> don't say why that's important.  You should include a reference to
>> whatever spec section this is related to.
>
> In the most common case, we hot add device support multi-function.
> In the original patch, if we insert multi-function device, the code
> which use list_is_singular() function always think there are more than one
> slot connected to parent port. And this is not match the comment in pcie_find_smpss().
>
>>
>> I wonder whether pci_only_one_slot() does what you intend for ARI
>> devices, where a multi-function device may have up to 256 functions.
>> PCI_SLOT() will return different "device" numbers for those functions
>> even though they're actually part of the same device.  Please explain.
>
> No, But I should consider the ARI device case, if ARI device found,
> pci_only_one_slot() should always return true.
> Exactly, I've never seen root port connected to more than one slot.
> Here I just fix the original logic, maybe Jon know whether this is necessary.
>
>>
>> In fact, I'm not sure the idea of "pci_only_one_slot()" even makes
>> sense for PCIe.  A conventional PCI bus may have several slots on it,
>> so you can have multiple devices (each of which may be a multi-
>> function device) on the bus.  But PCIe is inherently point-to-point,
>> so there's no way a link (which is really the analog of a conventional
>> PCI bus) can lead to multiple slots unless it first leads to a switch
>> or bridge that then fans out to multiple slots.
>
> As mentioned above, I don't know Whether there is a case of a port to connected to
> multiple devices.
>
> HI Jon, any comments about this?
>
> In my idea, we should fix this issue by using like pci_only_one_slot() or delete the list_is_singular() check (if we think it's unnecessary).

It is necessary.  Per the other patch, we need to make sure that no
device can be hot-plugged into an already running fabric.  If the
fabric is already running, then the MPS cannot be configured except to
conform to the MPS already present.  If this is not possible, then the
device either needs to disabled or the fabic needs to be already at
the minimum size so that any device can be added without needing to
modify the MPS of any of the other devices.  The point of this check
is to see if this is a possibility.  It checks the parent device and
sees if there are any other children (at least that is the intent).

I believe what we need to do is rename the checking function something
else, perhaps "pci_multiple_children".  Then check to see if the
parent has multiple devices hanging off of it (per the original code)
or there are multiple devices being added to the slot.

>
> Thanks!
> Yijing.
>
>>
>>> -+-[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
>>>
>>> Acked-by: Jon Mason <jdmason@kudzu.us>
>>> 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
>>>
>>>
>>
>> .
>>
>
>
> --
> Thanks!
> Yijing
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Aug. 19, 2013, 6:17 p.m.
On Mon, Aug 19, 2013 at 11:42 AM, Jon Mason <jdmason@kudzu.us> wrote:
> On Sun, Aug 18, 2013 at 7:49 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>> Hi Jon, do you think check the slot is only connected to upstream port is necessary?
>
> Yes.
>
>> As Bjorn's comment, we don't know whether there is a case of a port connected to more than one slot.
>
> My understanding of Bjorn's comment is that there may be multiple
> children of a single device, which may or may not be one or more
> slots.

I don't understand this.  A PCIe link leads to a single device.  That
device may be a multi-function device, but a link can not lead to
multiple devices or multiple slots.

If the device is a multi-function device, obviously there will be
several pci_devs on the bus->devices list.  But I don't think looking
at PCI_SLOT() of each of those pci_devs is useful.  Those pci_devs may
have different PCI_SLOT() values, but that doesn't mean they are in
different physical slots.  I think it would mean that the
multi-function device at the end of the link has ARI enabled and it
has a function number larger than 7.

So I don't think we learned anything by looking at PCI_SLOT() for each
device on bus->devices.  I think the only useful information is
whether there's a device present or not.  Maybe you want to use
list_empty() or something instead of the list_is_singular() test in
the current code.

>  My response to that is that he is correct and that it is a
> poor name, but the functionality it necessary (though perhaps
> incomplete).
>
>>
>> On 2013/8/17 6:17, Bjorn Helgaas wrote:
>>> On Thu, Aug 15, 2013 at 10:18:54AM +0800, Yijing Wang 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.
>>>
>>> This is a perfect example of a changelog that looks like it has a lot
>>> of information but makes absolutely no sense.  It describes the lowest
>>> possible level of detail, but nothing to motivate or justify the
>>> change.
>>
>> I'm so sorry. I update the log as following:
>>
>> PCI: use correct interface to identify the only-slot connection
>>
>> 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(which seems to
>> be the most common case). But list_is_singular() can only identify
>> whether the function device is only one in parent bus. This patch
>> introduce pci_only_one_slot() funcntion to fix this issue.
>>
>>>
>>> I assume we want this patch because it allows us to set larger MPS
>>> values for hot-added devices, which increases performance.
>>
>> Right.
>>
>>> Or maybe
>>> the hot-added devices just don't work because we set their MPS wrong.
>>> Whatever it is, you should mention it.
>>>
>>> Apparently this change has to do with multi-function devices, but you
>>> don't say why that's important.  You should include a reference to
>>> whatever spec section this is related to.
>>
>> In the most common case, we hot add device support multi-function.
>> In the original patch, if we insert multi-function device, the code
>> which use list_is_singular() function always think there are more than one
>> slot connected to parent port. And this is not match the comment in pcie_find_smpss().
>>
>>>
>>> I wonder whether pci_only_one_slot() does what you intend for ARI
>>> devices, where a multi-function device may have up to 256 functions.
>>> PCI_SLOT() will return different "device" numbers for those functions
>>> even though they're actually part of the same device.  Please explain.
>>
>> No, But I should consider the ARI device case, if ARI device found,
>> pci_only_one_slot() should always return true.
>> Exactly, I've never seen root port connected to more than one slot.
>> Here I just fix the original logic, maybe Jon know whether this is necessary.
>>
>>>
>>> In fact, I'm not sure the idea of "pci_only_one_slot()" even makes
>>> sense for PCIe.  A conventional PCI bus may have several slots on it,
>>> so you can have multiple devices (each of which may be a multi-
>>> function device) on the bus.  But PCIe is inherently point-to-point,
>>> so there's no way a link (which is really the analog of a conventional
>>> PCI bus) can lead to multiple slots unless it first leads to a switch
>>> or bridge that then fans out to multiple slots.
>>
>> As mentioned above, I don't know Whether there is a case of a port to connected to
>> multiple devices.
>>
>> HI Jon, any comments about this?
>>
>> In my idea, we should fix this issue by using like pci_only_one_slot() or delete the list_is_singular() check (if we think it's unnecessary).
>
> It is necessary.  Per the other patch, we need to make sure that no
> device can be hot-plugged into an already running fabric.

If there's a device in a PCIe hotplug slot, it is impossible to add
another device there.  If the slot is empty, you have to assume any
device added in the future is capable of only MPS=128.

> If the
> fabric is already running, then the MPS cannot be configured except to
> conform to the MPS already present.  If this is not possible, then the
> device either needs to disabled or the fabic needs to be already at
> the minimum size so that any device can be added without needing to
> modify the MPS of any of the other devices.  The point of this check
> is to see if this is a possibility.

Sorry, I'm not clear on what possibility you're looking for.  The
possibility of adding a new device?  If so, something like
"dev->is_hotplug_bridge && list_empty(dev->subordinate->devices)"
should tell us that.

> It checks the parent device and
> sees if there are any other children (at least that is the intent).
>
> I believe what we need to do is rename the checking function something
> else, perhaps "pci_multiple_children".  Then check to see if the
> parent has multiple devices hanging off of it (per the original code)
> or there are multiple devices being added to the slot.
>
>>>> -+-[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
>>>>
>>>> Acked-by: Jon Mason <jdmason@kudzu.us>
>>>> 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
>>>>
>>>>
>>>
>>> .
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Aug. 19, 2013, 7:39 p.m.
On Mon, Aug 19, 2013 at 12:17 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Aug 19, 2013 at 11:42 AM, Jon Mason <jdmason@kudzu.us> wrote:
>> On Sun, Aug 18, 2013 at 7:49 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>> Hi Jon, do you think check the slot is only connected to upstream port is necessary?
>>
>> Yes.
>>
>>> As Bjorn's comment, we don't know whether there is a case of a port connected to more than one slot.
>>
>> My understanding of Bjorn's comment is that there may be multiple
>> children of a single device, which may or may not be one or more
>> slots.
>
> I don't understand this.  A PCIe link leads to a single device.  That
> device may be a multi-function device, but a link can not lead to
> multiple devices or multiple slots.
>
> If the device is a multi-function device, obviously there will be
> several pci_devs on the bus->devices list.  But I don't think looking
> at PCI_SLOT() of each of those pci_devs is useful.  Those pci_devs may
> have different PCI_SLOT() values, but that doesn't mean they are in
> different physical slots.  I think it would mean that the
> multi-function device at the end of the link has ARI enabled and it
> has a function number larger than 7.
>
> So I don't think we learned anything by looking at PCI_SLOT() for each
> device on bus->devices.  I think the only useful information is
> whether there's a device present or not.  Maybe you want to use
> list_empty() or something instead of the list_is_singular() test in
> the current code.
>
>>  My response to that is that he is correct and that it is a
>> poor name, but the functionality it necessary (though perhaps
>> incomplete).
>>
>>>
>>> On 2013/8/17 6:17, Bjorn Helgaas wrote:
>>>> On Thu, Aug 15, 2013 at 10:18:54AM +0800, Yijing Wang 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.
>>>>
>>>> This is a perfect example of a changelog that looks like it has a lot
>>>> of information but makes absolutely no sense.  It describes the lowest
>>>> possible level of detail, but nothing to motivate or justify the
>>>> change.
>>>
>>> I'm so sorry. I update the log as following:
>>>
>>> PCI: use correct interface to identify the only-slot connection
>>>
>>> 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(which seems to
>>> be the most common case). But list_is_singular() can only identify
>>> whether the function device is only one in parent bus. This patch
>>> introduce pci_only_one_slot() funcntion to fix this issue.
>>>
>>>>
>>>> I assume we want this patch because it allows us to set larger MPS
>>>> values for hot-added devices, which increases performance.
>>>
>>> Right.
>>>
>>>> Or maybe
>>>> the hot-added devices just don't work because we set their MPS wrong.
>>>> Whatever it is, you should mention it.
>>>>
>>>> Apparently this change has to do with multi-function devices, but you
>>>> don't say why that's important.  You should include a reference to
>>>> whatever spec section this is related to.
>>>
>>> In the most common case, we hot add device support multi-function.
>>> In the original patch, if we insert multi-function device, the code
>>> which use list_is_singular() function always think there are more than one
>>> slot connected to parent port. And this is not match the comment in pcie_find_smpss().
>>>
>>>>
>>>> I wonder whether pci_only_one_slot() does what you intend for ARI
>>>> devices, where a multi-function device may have up to 256 functions.
>>>> PCI_SLOT() will return different "device" numbers for those functions
>>>> even though they're actually part of the same device.  Please explain.
>>>
>>> No, But I should consider the ARI device case, if ARI device found,
>>> pci_only_one_slot() should always return true.
>>> Exactly, I've never seen root port connected to more than one slot.
>>> Here I just fix the original logic, maybe Jon know whether this is necessary.
>>>
>>>>
>>>> In fact, I'm not sure the idea of "pci_only_one_slot()" even makes
>>>> sense for PCIe.  A conventional PCI bus may have several slots on it,
>>>> so you can have multiple devices (each of which may be a multi-
>>>> function device) on the bus.  But PCIe is inherently point-to-point,
>>>> so there's no way a link (which is really the analog of a conventional
>>>> PCI bus) can lead to multiple slots unless it first leads to a switch
>>>> or bridge that then fans out to multiple slots.
>>>
>>> As mentioned above, I don't know Whether there is a case of a port to connected to
>>> multiple devices.
>>>
>>> HI Jon, any comments about this?
>>>
>>> In my idea, we should fix this issue by using like pci_only_one_slot() or delete the list_is_singular() check (if we think it's unnecessary).
>>
>> It is necessary.  Per the other patch, we need to make sure that no
>> device can be hot-plugged into an already running fabric.
>
> If there's a device in a PCIe hotplug slot, it is impossible to add
> another device there.  If the slot is empty, you have to assume any
> device added in the future is capable of only MPS=128.
>
>> If the
>> fabric is already running, then the MPS cannot be configured except to
>> conform to the MPS already present.  If this is not possible, then the
>> device either needs to disabled or the fabic needs to be already at
>> the minimum size so that any device can be added without needing to
>> modify the MPS of any of the other devices.  The point of this check
>> is to see if this is a possibility.
>
> Sorry, I'm not clear on what possibility you're looking for.  The
> possibility of adding a new device?  If so, something like
> "dev->is_hotplug_bridge && list_empty(dev->subordinate->devices)"
> should tell us that.

Actually, I don't think it matters whether the slot is currently
occupied or not.  If the bridge supports hot-plug, we have to handle
the current device (if any) as well as any other device that might be
added after removing the current device.

>> It checks the parent device and
>> sees if there are any other children (at least that is the intent).
>>
>> I believe what we need to do is rename the checking function something
>> else, perhaps "pci_multiple_children".  Then check to see if the
>> parent has multiple devices hanging off of it (per the original code)
>> or there are multiple devices being added to the slot.
>>
>>>>> -+-[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
>>>>>
>>>>> Acked-by: Jon Mason <jdmason@kudzu.us>
>>>>> 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
>>>>>
>>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>> --
>>> Thanks!
>>> Yijing
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jon Mason - Aug. 19, 2013, 11:34 p.m.
On Mon, Aug 19, 2013 at 11:17 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Aug 19, 2013 at 11:42 AM, Jon Mason <jdmason@kudzu.us> wrote:
>> On Sun, Aug 18, 2013 at 7:49 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>> Hi Jon, do you think check the slot is only connected to upstream port is necessary?
>>
>> Yes.
>>
>>> As Bjorn's comment, we don't know whether there is a case of a port connected to more than one slot.
>>
>> My understanding of Bjorn's comment is that there may be multiple
>> children of a single device, which may or may not be one or more
>> slots.
>
> I don't understand this.  A PCIe link leads to a single device.  That
> device may be a multi-function device, but a link can not lead to
> multiple devices or multiple slots.

Perhaps I am not wording it properly, but I am trying to say that a
root port may have multiple devices under it.  Those devices,
connected via PCIE switch, must all have the same MPS (or not, per the
PERFORMANCE option, but I don't want to get off point).

> If the device is a multi-function device, obviously there will be
> several pci_devs on the bus->devices list.  But I don't think looking
> at PCI_SLOT() of each of those pci_devs is useful.  Those pci_devs may
> have different PCI_SLOT() values, but that doesn't mean they are in
> different physical slots.  I think it would mean that the
> multi-function device at the end of the link has ARI enabled and it
> has a function number larger than 7.
>
> So I don't think we learned anything by looking at PCI_SLOT() for each
> device on bus->devices.  I think the only useful information is
> whether there's a device present or not.  Maybe you want to use
> list_empty() or something instead of the list_is_singular() test in
> the current code.

Agreed

>>  My response to that is that he is correct and that it is a
>> poor name, but the functionality it necessary (though perhaps
>> incomplete).
>>
>>>
>>> On 2013/8/17 6:17, Bjorn Helgaas wrote:
>>>> On Thu, Aug 15, 2013 at 10:18:54AM +0800, Yijing Wang 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.
>>>>
>>>> This is a perfect example of a changelog that looks like it has a lot
>>>> of information but makes absolutely no sense.  It describes the lowest
>>>> possible level of detail, but nothing to motivate or justify the
>>>> change.
>>>
>>> I'm so sorry. I update the log as following:
>>>
>>> PCI: use correct interface to identify the only-slot connection
>>>
>>> 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(which seems to
>>> be the most common case). But list_is_singular() can only identify
>>> whether the function device is only one in parent bus. This patch
>>> introduce pci_only_one_slot() funcntion to fix this issue.
>>>
>>>>
>>>> I assume we want this patch because it allows us to set larger MPS
>>>> values for hot-added devices, which increases performance.
>>>
>>> Right.
>>>
>>>> Or maybe
>>>> the hot-added devices just don't work because we set their MPS wrong.
>>>> Whatever it is, you should mention it.
>>>>
>>>> Apparently this change has to do with multi-function devices, but you
>>>> don't say why that's important.  You should include a reference to
>>>> whatever spec section this is related to.
>>>
>>> In the most common case, we hot add device support multi-function.
>>> In the original patch, if we insert multi-function device, the code
>>> which use list_is_singular() function always think there are more than one
>>> slot connected to parent port. And this is not match the comment in pcie_find_smpss().
>>>
>>>>
>>>> I wonder whether pci_only_one_slot() does what you intend for ARI
>>>> devices, where a multi-function device may have up to 256 functions.
>>>> PCI_SLOT() will return different "device" numbers for those functions
>>>> even though they're actually part of the same device.  Please explain.
>>>
>>> No, But I should consider the ARI device case, if ARI device found,
>>> pci_only_one_slot() should always return true.
>>> Exactly, I've never seen root port connected to more than one slot.
>>> Here I just fix the original logic, maybe Jon know whether this is necessary.
>>>
>>>>
>>>> In fact, I'm not sure the idea of "pci_only_one_slot()" even makes
>>>> sense for PCIe.  A conventional PCI bus may have several slots on it,
>>>> so you can have multiple devices (each of which may be a multi-
>>>> function device) on the bus.  But PCIe is inherently point-to-point,
>>>> so there's no way a link (which is really the analog of a conventional
>>>> PCI bus) can lead to multiple slots unless it first leads to a switch
>>>> or bridge that then fans out to multiple slots.
>>>
>>> As mentioned above, I don't know Whether there is a case of a port to connected to
>>> multiple devices.
>>>
>>> HI Jon, any comments about this?
>>>
>>> In my idea, we should fix this issue by using like pci_only_one_slot() or delete the list_is_singular() check (if we think it's unnecessary).
>>
>> It is necessary.  Per the other patch, we need to make sure that no
>> device can be hot-plugged into an already running fabric.
>
> If there's a device in a PCIe hotplug slot, it is impossible to add
> another device there.  If the slot is empty, you have to assume any
> device added in the future is capable of only MPS=128.

To be clear, I was referring to a number of devices already running
under a root port, which also can have devices hotplugged to it.  In
this case you are correct.

>> If the
>> fabric is already running, then the MPS cannot be configured except to
>> conform to the MPS already present.  If this is not possible, then the
>> device either needs to disabled or the fabic needs to be already at
>> the minimum size so that any device can be added without needing to
>> modify the MPS of any of the other devices.  The point of this check
>> is to see if this is a possibility.
>
> Sorry, I'm not clear on what possibility you're looking for.  The
> possibility of adding a new device?  If so, something like
> "dev->is_hotplug_bridge && list_empty(dev->subordinate->devices)"
> should tell us that.

Yijing, can you confirm this for the mulit-func adapter?

>> It checks the parent device and
>> sees if there are any other children (at least that is the intent).
>>
>> I believe what we need to do is rename the checking function something
>> else, perhaps "pci_multiple_children".  Then check to see if the
>> parent has multiple devices hanging off of it (per the original code)
>> or there are multiple devices being added to the slot.
>>
>>>>> -+-[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
>>>>>
>>>>> Acked-by: Jon Mason <jdmason@kudzu.us>
>>>>> 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
>>>>>
>>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>> --
>>> Thanks!
>>> Yijing
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jon Mason - Aug. 19, 2013, 11:45 p.m.
On Mon, Aug 19, 2013 at 12:39 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Aug 19, 2013 at 12:17 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Mon, Aug 19, 2013 at 11:42 AM, Jon Mason <jdmason@kudzu.us> wrote:
>>> On Sun, Aug 18, 2013 at 7:49 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>>> Hi Jon, do you think check the slot is only connected to upstream port is necessary?
>>>
>>> Yes.
>>>
>>>> As Bjorn's comment, we don't know whether there is a case of a port connected to more than one slot.
>>>
>>> My understanding of Bjorn's comment is that there may be multiple
>>> children of a single device, which may or may not be one or more
>>> slots.
>>
>> I don't understand this.  A PCIe link leads to a single device.  That
>> device may be a multi-function device, but a link can not lead to
>> multiple devices or multiple slots.
>>
>> If the device is a multi-function device, obviously there will be
>> several pci_devs on the bus->devices list.  But I don't think looking
>> at PCI_SLOT() of each of those pci_devs is useful.  Those pci_devs may
>> have different PCI_SLOT() values, but that doesn't mean they are in
>> different physical slots.  I think it would mean that the
>> multi-function device at the end of the link has ARI enabled and it
>> has a function number larger than 7.
>>
>> So I don't think we learned anything by looking at PCI_SLOT() for each
>> device on bus->devices.  I think the only useful information is
>> whether there's a device present or not.  Maybe you want to use
>> list_empty() or something instead of the list_is_singular() test in
>> the current code.
>>
>>>  My response to that is that he is correct and that it is a
>>> poor name, but the functionality it necessary (though perhaps
>>> incomplete).
>>>
>>>>
>>>> On 2013/8/17 6:17, Bjorn Helgaas wrote:
>>>>> On Thu, Aug 15, 2013 at 10:18:54AM +0800, Yijing Wang 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.
>>>>>
>>>>> This is a perfect example of a changelog that looks like it has a lot
>>>>> of information but makes absolutely no sense.  It describes the lowest
>>>>> possible level of detail, but nothing to motivate or justify the
>>>>> change.
>>>>
>>>> I'm so sorry. I update the log as following:
>>>>
>>>> PCI: use correct interface to identify the only-slot connection
>>>>
>>>> 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(which seems to
>>>> be the most common case). But list_is_singular() can only identify
>>>> whether the function device is only one in parent bus. This patch
>>>> introduce pci_only_one_slot() funcntion to fix this issue.
>>>>
>>>>>
>>>>> I assume we want this patch because it allows us to set larger MPS
>>>>> values for hot-added devices, which increases performance.
>>>>
>>>> Right.
>>>>
>>>>> Or maybe
>>>>> the hot-added devices just don't work because we set their MPS wrong.
>>>>> Whatever it is, you should mention it.
>>>>>
>>>>> Apparently this change has to do with multi-function devices, but you
>>>>> don't say why that's important.  You should include a reference to
>>>>> whatever spec section this is related to.
>>>>
>>>> In the most common case, we hot add device support multi-function.
>>>> In the original patch, if we insert multi-function device, the code
>>>> which use list_is_singular() function always think there are more than one
>>>> slot connected to parent port. And this is not match the comment in pcie_find_smpss().
>>>>
>>>>>
>>>>> I wonder whether pci_only_one_slot() does what you intend for ARI
>>>>> devices, where a multi-function device may have up to 256 functions.
>>>>> PCI_SLOT() will return different "device" numbers for those functions
>>>>> even though they're actually part of the same device.  Please explain.
>>>>
>>>> No, But I should consider the ARI device case, if ARI device found,
>>>> pci_only_one_slot() should always return true.
>>>> Exactly, I've never seen root port connected to more than one slot.
>>>> Here I just fix the original logic, maybe Jon know whether this is necessary.
>>>>
>>>>>
>>>>> In fact, I'm not sure the idea of "pci_only_one_slot()" even makes
>>>>> sense for PCIe.  A conventional PCI bus may have several slots on it,
>>>>> so you can have multiple devices (each of which may be a multi-
>>>>> function device) on the bus.  But PCIe is inherently point-to-point,
>>>>> so there's no way a link (which is really the analog of a conventional
>>>>> PCI bus) can lead to multiple slots unless it first leads to a switch
>>>>> or bridge that then fans out to multiple slots.
>>>>
>>>> As mentioned above, I don't know Whether there is a case of a port to connected to
>>>> multiple devices.
>>>>
>>>> HI Jon, any comments about this?
>>>>
>>>> In my idea, we should fix this issue by using like pci_only_one_slot() or delete the list_is_singular() check (if we think it's unnecessary).
>>>
>>> It is necessary.  Per the other patch, we need to make sure that no
>>> device can be hot-plugged into an already running fabric.
>>
>> If there's a device in a PCIe hotplug slot, it is impossible to add
>> another device there.  If the slot is empty, you have to assume any
>> device added in the future is capable of only MPS=128.
>>
>>> If the
>>> fabric is already running, then the MPS cannot be configured except to
>>> conform to the MPS already present.  If this is not possible, then the
>>> device either needs to disabled or the fabic needs to be already at
>>> the minimum size so that any device can be added without needing to
>>> modify the MPS of any of the other devices.  The point of this check
>>> is to see if this is a possibility.
>>
>> Sorry, I'm not clear on what possibility you're looking for.  The
>> possibility of adding a new device?  If so, something like
>> "dev->is_hotplug_bridge && list_empty(dev->subordinate->devices)"
>> should tell us that.
>
> Actually, I don't think it matters whether the slot is currently
> occupied or not.  If the bridge supports hot-plug, we have to handle
> the current device (if any) as well as any other device that might be
> added after removing the current device.

Yes, this is true.  So, we want to see if there is the ability to:
1. have a device hotplugged
and
2a. there are other devices under the same root port
or
2b. there are multiple hotplug slots under the same root port

The patch covers part 2b (unless I missunderstand and it only covers
the devices present and not the slots available), but we need part 2a
to be covered.  I believe your suggested code above covers 2a.  Am I
off my rocker?

Thanks,
Jon

>>> It checks the parent device and
>>> sees if there are any other children (at least that is the intent).
>>>
>>> I believe what we need to do is rename the checking function something
>>> else, perhaps "pci_multiple_children".  Then check to see if the
>>> parent has multiple devices hanging off of it (per the original code)
>>> or there are multiple devices being added to the slot.
>>>
>>>>>> -+-[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
>>>>>>
>>>>>> Acked-by: Jon Mason <jdmason@kudzu.us>
>>>>>> 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
>>>>>>
>>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>> --
>>>> Thanks!
>>>> Yijing
>>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yijing Wang - Aug. 20, 2013, 2:55 a.m.
>> Actually, I don't think it matters whether the slot is currently
>> occupied or not.  If the bridge supports hot-plug, we have to handle
>> the current device (if any) as well as any other device that might be
>> added after removing the current device.
> 
> Yes, this is true.  So, we want to see if there is the ability to:
> 1. have a device hotplugged
> and
> 2a. there are other devices under the same root port
> or
> 2b. there are multiple hotplug slots under the same root port
> 
> The patch covers part 2b (unless I missunderstand and it only covers
> the devices present and not the slots available), but we need part 2a
> to be covered.  I believe your suggested code above covers 2a.  Am I
> off my rocker?

I'm a little confused, let us list the possible pcie topo here:

1.
root port -------------->[slot]-(function device a)
			       -(function device b)

Here there is only one slot directly connected to root port,
this slot is inserted a physical device which contain function device a and b.
After we insert physical device into this slot, hotplug driver will call
pci_configure_slot()---->pcie_bus_configure_settings() to set device mps.

As Jon's original patch, in this case we should update root port and slot device
mps to the minmum mpss of root port and slot device. But in pcie_find_smpss()
list_is_singular() is used to check whether there are other devices on the fabric.
list_is_singular() in this case return false. because more than one devices found in devices list.
But we expect the result is true.

And Bjorn think root port always connected to only one hot-plug slot, so use list_is_singular() or pci_only_one_slot()
to check other devices on the fabric make no sense. I agree with him if there is no broken hardware platform that
a root port connected to more than one slot.

2.		(is_hotplug_bridge)
root port --------------->Port A--------->[slot]
		bus a	 |
			 |Port or devices ?

If Jon's original patch is correct, I guess the topo is like above.
In this case, Port A is hotplug bridge and connected directly to root port.
So if there is no other port or devices in bus a, we can configure mps to the minmum
mpss of root port and Port A and slot device.

But does this topo exist? root port has only one link, the pcie link is point to point.

3.                                                (hotplug bridge)
root port ---------------Up steam port----------->Downsteam port A -------->[slot]
						 |
						 |Downsteam port B -------->[slot]  ?

This topo is common, but DP A is not directly connected to root port, so the original patch
can not cover this case. If DP B is not exist. there is only one Downstream port A, like:

                                                (hotplug bridge)
root port ---------------Up steam port----------->Downsteam port A -------->[slot]

In this case, we can also configure mps after a pcie card inserted into slot, because
although the system is running, because root port UP port DP port A and slot are not running.
But this case is rare.

So, Jon, does your original patch want to cover the case 1 or case 2 ?
For case 1, we can remove the list_is_singular() check.
For case 2, do you have some example hardware platform ?
Or there are other pcie topos not exist here?

Thanks!
Yijing.


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Aug. 20, 2013, 4:44 a.m.
On Tue, Aug 20, 2013 at 10:55:00AM +0800, Yijing Wang wrote:
> >> Actually, I don't think it matters whether the slot is currently
> >> occupied or not.  If the bridge supports hot-plug, we have to handle
> >> the current device (if any) as well as any other device that might be
> >> added after removing the current device.
> > 
> > Yes, this is true.  So, we want to see if there is the ability to:
> > 1. have a device hotplugged
> > and
> > 2a. there are other devices under the same root port
> > or
> > 2b. there are multiple hotplug slots under the same root port
> > 
> > The patch covers part 2b (unless I missunderstand and it only covers
> > the devices present and not the slots available), but we need part 2a
> > to be covered.  I believe your suggested code above covers 2a.  Am I
> > off my rocker?
> 
> I'm a little confused, let us list the possible pcie topo here:

I'm *very* confused; thanks for helping out with some pictures!

> 1.
> root port -------------->[slot]-(function device a)
> 			       -(function device b)
> 
> Here there is only one slot directly connected to root port,
> this slot is inserted a physical device which contain function
> device a and b.  After we insert physical device into this
> slot, hotplug driver will call
> pci_configure_slot()---->pcie_bus_configure_settings() to set
> device mps.
> 
> As Jon's original patch, in this case we should update root
> port and slot device mps to the minmum mpss of root port and
> slot device. But in pcie_find_smpss() list_is_singular() is
> used to check whether there are other devices on the fabric.
> list_is_singular() in this case return false. because more than
> one devices found in devices list.  But we expect the result is
> true.
> 
> And Bjorn think root port always connected to only one hot-plug
> slot, so use list_is_singular() or pci_only_one_slot() to check
> other devices on the fabric make no sense. I agree with him if
> there is no broken hardware platform that a root port connected
> to more than one slot.

It makes no sense for a single PCIe link to connect directly to
more than one slot.  A link has two ends, connecting two devices.
There's no way it could connect three devices.

> 2.		(is_hotplug_bridge)
> root port --------------->Port A--------->[slot]
> 		bus a	 |
> 			 |Port or devices ?
> 
> If Jon's original patch is correct, I guess the topo is like
> above.  In this case, Port A is hotplug bridge and connected
> directly to root port.  So if there is no other port or devices
> in bus a, we can configure mps to the minmum mpss of root port
> and Port A and slot device.
> 
> But does this topo exist? root port has only one link, the pcie
> link is point to point.

The topology above does not exist.  Since Port A is connected
directly to a Root Port, Port A must be an Upstream Port.  But in
order for Port A to connect to a slot, it must also be a
Downstream Port.  It can't be both.  

There would have to be a switch between the Root Port and the
slot, and the switch would contain both an Upstream Port and a
Downstream Port.

> 3.                              (hotplug bridge)
> root port ----Upstream port---->Downstream port A -------->[slot]
> 				 |
> 				 |Downstream port B -------->[slot]  ?
> 
> This topo is common, but DP A is not directly connected to root
> port, so the original patch can not cover this case. 

This is a very good observation: only Root Ports and Downstream
Ports can have slots.  That means the following existing code:

    if (dev->is_hotplug_bridge &&
        ((dev->bus->self &&
          pci_pcie_type(dev->bus->self) != PCI_EXP_TYPE_ROOT_PORT)))

doesn't make sense.  "dev->bus->self" is the bridge immediately
upstream from "dev".  If "dev" is a Port with a slot, it must be
either a Root Port or a Downstream Port.  A Root Port *has* no
upstream bridge, and the bridge immediately upstream from a
Downstream Port is always an Upstream Port.

So we should just write this instead:

    if (dev->is_hotplug_bridge &&
        pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)))
            *smpss = 0;

That means we'll set MPS=128 for Downstream Ports leading to
slots, which is safe for anything we can plug in.

Apparently we don't enforce MPS=128 for Root Ports because we
assume either (a) there is no peer-to-peer traffic, or (b)
peer-to-peer traffic is not routed between Root Ports.  I don't
know if those are valid assumptions, but you're not changing
anything there, so I guess we can keep them.

I don't think we should look at the dev->bus->devices list at all
because that only applies to the current configuration and
doesn't account for any future hot-plugs.
 
> If DP B is
> not exist. there is only one Downstream port A, like:

> 
>                                (hotplug bridge)
> root port ----Upstream port--->Downstream port A -------->[slot]
> 
> In this case, we can also configure mps after a pcie card
> inserted into slot, because although the system is running,
> because root port UP port DP port A and slot are not running.
> But this case is rare.

> So, Jon, does your original patch want to cover the case 1 or case 2 ?
> For case 1, we can remove the list_is_singular() check.
> For case 2, do you have some example hardware platform ?
> Or there are other pcie topos not exist here?
> 
> Thanks!
> Yijing.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yijing Wang - Aug. 20, 2013, 12:23 p.m.
>> 3.                              (hotplug bridge)
>> root port ----Upstream port---->Downstream port A -------->[slot]
>> 				 |
>> 				 |Downstream port B -------->[slot]  ?
>>
>> This topo is common, but DP A is not directly connected to root
>> port, so the original patch can not cover this case. 
> 
> This is a very good observation: only Root Ports and Downstream
> Ports can have slots.  That means the following existing code:
> 
>     if (dev->is_hotplug_bridge &&
>         ((dev->bus->self &&
>           pci_pcie_type(dev->bus->self) != PCI_EXP_TYPE_ROOT_PORT)))
> 
> doesn't make sense.  "dev->bus->self" is the bridge immediately
> upstream from "dev".  If "dev" is a Port with a slot, it must be
> either a Root Port or a Downstream Port.  A Root Port *has* no
> upstream bridge, and the bridge immediately upstream from a
> Downstream Port is always an Upstream Port.
> 
> So we should just write this instead:
> 
>     if (dev->is_hotplug_bridge &&
>         pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)))
>             *smpss = 0;
> 
> That means we'll set MPS=128 for Downstream Ports leading to
> slots, which is safe for anything we can plug in.

I agree, Jon, what do you think?
And I test this code in my machine, result is ok.


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

> 
> Apparently we don't enforce MPS=128 for Root Ports because we
> assume either (a) there is no peer-to-peer traffic, or (b)
> peer-to-peer traffic is not routed between Root Ports.  I don't
> know if those are valid assumptions, but you're not changing
> anything there, so I guess we can keep them.
> 
> I don't think we should look at the dev->bus->devices list at all
> because that only applies to the current configuration and
> doesn't account for any future hot-plugs.

I think so.

If Jon agree too , I will update this patch.

>  
>> If DP B is
>> not exist. there is only one Downstream port A, like:
> 
>>
>>                                (hotplug bridge)
>> root port ----Upstream port--->Downstream port A -------->[slot]
>>
>> In this case, we can also configure mps after a pcie card
>> inserted into slot, because although the system is running,
>> because root port UP port DP port A and slot are not running.
>> But this case is rare.
> 
>> So, Jon, does your original patch want to cover the case 1 or case 2 ?
>> For case 1, we can remove the list_is_singular() check.
>> For case 2, do you have some example hardware platform ?
>> Or there are other pcie topos not exist here?
>>
>> Thanks!
>> Yijing.
> 
> .
>
Bjorn Helgaas - Aug. 20, 2013, 8:12 p.m.
On Tue, Aug 20, 2013 at 6:23 AM, Yijing Wang <wangyijing@huawei.com> wrote:

>> I don't think we should look at the dev->bus->devices list at all
>> because that only applies to the current configuration and
>> doesn't account for any future hot-plugs.
>
> I think so.
>
> If Jon agree too , I will update this patch.

No need to wait for Jon; this is a tiny patch, and things will go
faster if you just go ahead and update the patch, test it, and post
it.  Then we can all see exactly what change you propose, and it's
always easier to comment on a concrete patch rather than an English
description of it.  If Jon disagrees with the patch, we can discuss
some more and go from there.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;