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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 19, 2013 at 11:17 AM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
> On Mon, Aug 19, 2013 at 11:42 AM, Jon Mason <jdmason@xxxxxxxx> wrote:
>> On Sun, Aug 18, 2013 at 7:49 PM, Yijing Wang <wangyijing@xxxxxxxxxx> 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@xxxxxxxx>
>>>>> Signed-off-by: Yijing Wang <wangyijing@xxxxxxxxxx>
>>>>> Cc: Jon Mason <jdmason@xxxxxxxx>
>>>>> Cc: stable@xxxxxxxxxxxxxxx # 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux