>> Hi Jon, >> Thanks for your comments! >> I think we can improve this logic a little . As your comments in pcie_find_smpss(), if the PCI hotplug >> slot is directly connected to the root port and there are not other devices on the fabric, then this >> is not an issue.. So I think in this case, we can first update the newly hot added device mps as above logic. >> if newly hot added device mpss < root port mps, then we can modify both root port device and newly hot added device >> mps to device mpss. What do you think? >> >> eg. >> Root port --------------> slot (mps is default 128,assume mpss is 256) >> (mps 512) >> >> Only in this case, I think we try to update the parent device is safe. >> >> after update: >> Root port --------------> slot (mps is default 128,assume mpss is 256) >> (mps 512-->256) mps 128--->256 > > > Yes, but this is where it get difficult. You can only do it if the > parent bus is the root port. Otherwise, the other devices on the bus > will already have their MPS configured and changing the root port MPS > will do bad things. That is why I have the check in pcie_find_smpss() > of > > if (dev->is_hotplug_bridge && (!list_is_singular(&dev->bus->devices) || > (dev->bus->self && > pci_pcie_type(dev->bus->self) != PCI_EXP_TYPE_ROOT_PORT))) > > So, you either need to mimic this code in your new function or make > PCIE_BUS_SAFE the default option. I'm leaning towards the latter, but > if you need something for the stable tree then we should temporarily > have a patch which does it the former. > > Thoughts? Yes, I need one patch fix this issue in the stable tree. Will you send out the series patch recently? Bjorn now began to review this mps code, so this is a good chance to rework mps related codes I think. > >> >>> >>>> But if we try to configure DP 1 mps to 128B, it's not safe. its parent Upstream port still is 256. >>> >>> This is exactly the case that the "PERFORMANCE" option is trying to >>> allow. If the MRRS is set to the MPS of the device, it should work. >>> If not, then we should rip out all of the "PERFORMANCE" code. Is this >>> something you can verify? >> >> Hi Jon, >> I am not very clear about what the role of MRRS. So if I understand wrong, please correct me, thanks. >> >> eg. >> DP1 ----------------> EP A >> mps=256 mps=128 >> >> MRRS can control the read request TLP size when the Function as a Requester. >> So if we set the EP A MRRS to mps value(128), EP A won't generate >> TLP larger than 128, so Request stream from EP A to DP1 is safe. >> >> But if EP A is as a receiver, DP 1 generate completion TLP (larger than 128) to EP A. >> these TLPs will be discarded by EP A, right? > > Yes > >> So in my idea, If we set both mps and mrrs of EP A to 128, we can insure TLPs stream from EP A is safe. >> But How do we guarantee that DP1 won't generate TLPs to EP A is larger than 128? > > The device will never do any reads of larger than the MRRS, but writes > to the device are an issue. Assuming that all I/O is between CPU/RAM > and the device and there are no peer-to-peer transfers, we should be > safe (but is that a safe assumption?). So my question to you is that > the setup you have that is failing due to MPS sizes being off, can you > set the only MRRS to 128 and get it working? I don't have PCIe analyzer, so I can not catch TLP, I'm not sure the TLPs size in stream. my test is using ping $dest_ip -s 65500 local machine remote machine IP: 128.5.160.31 IP: 128.5.160.28 Root port ---------------->Endpoint device(bnx2 NIC) 00:01.0 01:00.1 default setting Root Port(00:01.0): 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 128 bytes, MaxReadReq 128 bytes Endpoint device(01:00.1) Capabilities: [ac] Express (v2) Endpoint, MSI 00 DevCap: MaxPayload 512 bytes, PhantFunc 0, Latency L0s <4us, L1 <64us ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset- DevCtl: Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+ RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+ MaxPayload 128 bytes, MaxReadReq 512 bytes test1: change root port mps to 256 Root port ---------------->EndPoint device(bnx2 NIC) mps=256 (default 128) mps=128 mrrs=128 mrrs=512(default) result: ping 128.5.160.28(remote ip) -s 65500 ----->fail ping 128.5.160.31(remove machine ping local ip)------->fail test2: change root port mps to 256, EP device mrrs to 128 Root port ---------------->EndPoint device(bnx2 NIC) mps=256 (default 128) mps=128 mrrs=128 mrrs=128(default 512) result: ping 128.5.160.28(remote ip) -s 65500 ----->success ping 128.5.160.31(remove machine ping local ip)------->success It seems change mrrs to 128 take effect. Thanks! Yijing. -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html