* [PATCH v2 0/3] Add new PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag @ 2017-06-03 4:04 Ding Tianhong 2017-06-03 4:04 ` [PATCH v2 1/3] PCI: Add new PCIe Fabric End Node flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING Ding Tianhong ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Ding Tianhong @ 2017-06-03 4:04 UTC (permalink / raw) To: leedom, ashok.raj, helgaas, werner, ganeshgr, asit.k.mallick, patrick.j.cramer, Suravee.Suthikulpanit, Bob.Shaw, l.stach, amira, gabriele.paoloni, David.Laight, jeffrey.t.kirsher, catalin.marinas, will.deacon, mark.rutland, robin.murphy, davem, alexander.duyck, linux-arm-kernel, netdev, linux-pci, linux-kernel Cc: Ding Tianhong Some devices have problems with Transaction Layer Packets with the Relaxed Ordering Attribute set. This patch set adds a new PCIe Device Flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING, a set of PCI Quirks to catch some known devices with Relaxed Ordering issues, and a use of this new flag by the cxgb4 driver to avoid using Relaxed Ordering with problematic Root Complex Ports. It's been years since I've submitted kernel.org patches, I appolgise for the almost certain submission errors. v2: Alexander point out that the v1 was only a part of the whole solution, some platform which has some issues could use the new flag to indicate that it is not safe to enable relaxed ordering attribute, then we need to clear the relaxed ordering enable bits in the PCI configuration when initializing the device. So add a new second patch to modify the PCI initialization code to clear the relaxed ordering enable bit in the event that the root complex doesn't want relaxed ordering enabled. The third patch was base on the v1's second patch and only be changed to query the relaxed ordering enable bit in the PCI configuration space to allow the Chelsio NIC to send TLPs with the relaxed ordering attributes set. This version didn't plan to drop the defines for Intel Drivers to use the new checking way to enable relaxed ordering because it is not the hardest part of the moment, we could fix it in next patchset when this patches reach the goal. Casey Leedom (2): PCI: Add new PCIe Fabric End Node flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING net/cxgb4: Use new PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag Ding Tianhong (1): PCI: Enable PCIe Relaxed Ordering if supported drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 1 + drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 17 ++++++++++ drivers/net/ethernet/chelsio/cxgb4/sge.c | 5 +-- drivers/pci/pci.c | 42 +++++++++++++++++++++++++ drivers/pci/probe.c | 11 +++++++ drivers/pci/quirks.c | 38 ++++++++++++++++++++++ include/linux/pci.h | 5 +++ 7 files changed, 117 insertions(+), 2 deletions(-) -- 1.9.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/3] PCI: Add new PCIe Fabric End Node flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING 2017-06-03 4:04 [PATCH v2 0/3] Add new PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag Ding Tianhong @ 2017-06-03 4:04 ` Ding Tianhong 2017-06-03 4:04 ` [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong 2017-06-03 4:04 ` [PATCH v2 3/3] net/cxgb4: Use new PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag Ding Tianhong 2 siblings, 0 replies; 10+ messages in thread From: Ding Tianhong @ 2017-06-03 4:04 UTC (permalink / raw) To: leedom, ashok.raj, helgaas, werner, ganeshgr, asit.k.mallick, patrick.j.cramer, Suravee.Suthikulpanit, Bob.Shaw, l.stach, amira, gabriele.paoloni, David.Laight, jeffrey.t.kirsher, catalin.marinas, will.deacon, mark.rutland, robin.murphy, davem, alexander.duyck, linux-arm-kernel, netdev, linux-pci, linux-kernel Cc: Ding Tianhong From: Casey Leedom <leedom@chelsio.com> The new flag PCI_DEV_FLAGS_NO_RELAXED_ORDERING indicates that the Relaxed Ordering Attribute should not be used on Transaction Layer Packets destined for the PCIe End Node so flagged. Initially flagged this way are Intel E5-26xx Root Complex Ports which suffer from a Flow Control Credit Performance Problem and AMD A1100 ARM ("SEATTLE") Root Complex Ports which don't obey PCIe 3.0 ordering rules which can lead to Data Corruption. Signed-off-by: Casey Leedom <leedom@chelsio.com> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> --- drivers/pci/quirks.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 2 ++ 2 files changed, 40 insertions(+) diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 085fb78..58bdd23 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3999,6 +3999,44 @@ static void quirk_tw686x_class(struct pci_dev *pdev) quirk_tw686x_class); /* + * Some devices have problems with Transaction Layer Packets with the Relaxed + * Ordering Attribute set. Such devices should mark themselves and other + * Device Drivers should check before sending TLPs with RO set. + */ +static void quirk_relaxedordering_disable(struct pci_dev *dev) +{ + dev->dev_flags |= PCI_DEV_FLAGS_NO_RELAXED_ORDERING; +} + +/* + * Intel E5-26xx Root Complex has a Flow Control Credit issue which can + * cause performance problems with Upstream Transaction Layer Packets with + * Relaxed Ordering set. + */ +DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, 0x6f02, PCI_CLASS_NOT_DEFINED, 8, + quirk_relaxedordering_disable); +DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, 0x6f04, PCI_CLASS_NOT_DEFINED, 8, + quirk_relaxedordering_disable); +DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, 0x6f08, PCI_CLASS_NOT_DEFINED, 8, + quirk_relaxedordering_disable); + +/* + * The AMD ARM A1100 (AKA "SEATTLE") SoC has a bug in its PCIe Root Complex + * where Upstream Transaction Layer Packets with the Relaxed Ordering + * Attribute clear are allowed to bypass earlier TLPs with Relaxed Ordering + * set. This is a violation of the PCIe 3.0 Transaction Ordering Rules + * outlined in Section 2.4.1 (PCI Express(r) Base Specification Revision 3.0 + * November 10, 2010). As a result, on this platform we can't use Relaxed + * Ordering for Upstream TLPs. + */ +DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_AMD, 0x1a00, PCI_CLASS_NOT_DEFINED, 8, + quirk_relaxedordering_disable); +DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_AMD, 0x1a01, PCI_CLASS_NOT_DEFINED, 8, + quirk_relaxedordering_disable); +DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_AMD, 0x1a02, PCI_CLASS_NOT_DEFINED, 8, + quirk_relaxedordering_disable); + +/* * Per PCIe r3.0, sec 2.2.9, "Completion headers must supply the same * values for the Attribute as were supplied in the header of the * corresponding Request, except as explicitly allowed when IDO is used." diff --git a/include/linux/pci.h b/include/linux/pci.h index 33c2b0b..e1e8428 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -183,6 +183,8 @@ enum pci_dev_flags { PCI_DEV_FLAGS_BRIDGE_XLATE_ROOT = (__force pci_dev_flags_t) (1 << 9), /* Do not use FLR even if device advertises PCI_AF_CAP */ PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10), + /* Don't use Relaxed Ordering for TLPs directed at this device */ + PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11), }; enum pci_irq_reroute_variant { -- 1.9.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported 2017-06-03 4:04 [PATCH v2 0/3] Add new PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag Ding Tianhong 2017-06-03 4:04 ` [PATCH v2 1/3] PCI: Add new PCIe Fabric End Node flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING Ding Tianhong @ 2017-06-03 4:04 ` Ding Tianhong 2017-06-03 6:02 ` kbuild test robot 2017-06-03 18:19 ` Alexander Duyck 2017-06-03 4:04 ` [PATCH v2 3/3] net/cxgb4: Use new PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag Ding Tianhong 2 siblings, 2 replies; 10+ messages in thread From: Ding Tianhong @ 2017-06-03 4:04 UTC (permalink / raw) To: leedom, ashok.raj, helgaas, werner, ganeshgr, asit.k.mallick, patrick.j.cramer, Suravee.Suthikulpanit, Bob.Shaw, l.stach, amira, gabriele.paoloni, David.Laight, jeffrey.t.kirsher, catalin.marinas, will.deacon, mark.rutland, robin.murphy, davem, alexander.duyck, linux-arm-kernel, netdev, linux-pci, linux-kernel Cc: Ding Tianhong The PCIe Device Control Register use the bit 4 to indicate that whether the device is permitted to enable relaxed ordering or not. But relaxed ordering is not safe for some platform which could only use strong write ordering, so devices are allowed (but not required) to enable relaxed ordering bit by default. If a platform support relaxed ordering but does not enable it by default, enable it in the PCIe configuration. This allows some device to send TLPs with the relaxed ordering attributes set, which may improve the performance. Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> --- drivers/pci/pci.c | 42 ++++++++++++++++++++++++++++++++++++++++++ drivers/pci/probe.c | 11 +++++++++++ include/linux/pci.h | 3 +++ 3 files changed, 56 insertions(+) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b01bd5b..f57a374 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4878,6 +4878,48 @@ int pcie_set_mps(struct pci_dev *dev, int mps) EXPORT_SYMBOL(pcie_set_mps); /** + * pcie_set_relaxed_ordering - set PCI Express relexed ordering bit + * @dev: PCI device to query + * + * If possible sets relaxed ordering + */ +int pcie_set_relaxed_ordering(struct pci_dev *dev) +{ + return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); +} +EXPORT_SYMBOL(pcie_set_relaxed_ordering); + +/** + * pcie_clear_relaxed_ordering - clear PCI Express relexed ordering bit + * @dev: PCI device to query + * + * If possible clear relaxed ordering + */ +int pcie_clear_relaxed_ordering(struct pci_dev *dev) +{ + return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); +} +EXPORT_SYMBOL(pcie_clear_relaxed_ordering); + +/** + * pcie_get_relaxed_ordering - check PCI Express relexed ordering bit + * @dev: PCI device to query + * + * Returns true if relaxed ordering is been set + */ +int pcie_get_relaxed_ordering(struct pci_dev *dev) +{ + u16 v; + + pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v); + + return (v & PCI_EXP_DEVCTL_RELAX_EN) >> 4; +} +EXPORT_SYMBOL(pcie_get_relaxed_ordering); + +/** + * pcie_set_mps - set PCI Express maximum payload size +/** * pcie_get_minimum_link - determine minimum link settings of a PCI device * @dev: PCI device to query * @speed: storage for minimum speed diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 19c8950..aeb22b5 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1701,6 +1701,16 @@ static void pci_configure_extended_tags(struct pci_dev *dev) PCI_EXP_DEVCTL_EXT_TAG); } +static void pci_configure_relaxed_ordering(struct pci_dev *dev) +{ + int ret; + + if (dev && (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING)) + pcie_set_relaxed_ordering(dev); + else + pcie_clear_relaxed_ordering(dev); +} + static void pci_configure_device(struct pci_dev *dev) { struct hotplug_params hpp; @@ -1708,6 +1718,7 @@ static void pci_configure_device(struct pci_dev *dev) pci_configure_mps(dev); pci_configure_extended_tags(dev); + pci_configure_relaxed_ordering(dev); memset(&hpp, 0, sizeof(hpp)); ret = pci_get_hp_params(dev, &hpp); diff --git a/include/linux/pci.h b/include/linux/pci.h index e1e8428..84bd6af 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1105,6 +1105,9 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, void pci_pme_wakeup_bus(struct pci_bus *bus); void pci_d3cold_enable(struct pci_dev *dev); void pci_d3cold_disable(struct pci_dev *dev); +int pcie_set_relaxed_ordering(struct pci_dev *dev); +int pcie_clear_relaxed_ordering(struct pci_dev *dev); +int pcie_get_relaxed_ordering(struct pci_dev *dev); static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable) -- 1.9.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported 2017-06-03 4:04 ` [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong @ 2017-06-03 6:02 ` kbuild test robot 2017-06-03 18:19 ` Alexander Duyck 1 sibling, 0 replies; 10+ messages in thread From: kbuild test robot @ 2017-06-03 6:02 UTC (permalink / raw) To: Ding Tianhong Cc: kbuild-all, leedom, ashok.raj, helgaas, werner, ganeshgr, asit.k.mallick, patrick.j.cramer, Suravee.Suthikulpanit, Bob.Shaw, l.stach, amira, gabriele.paoloni, David.Laight, jeffrey.t.kirsher, catalin.marinas, will.deacon, mark.rutland, robin.murphy, davem, alexander.duyck, linux-arm-kernel, netdev, linux-pci, linux-kernel, Ding Tianhong [-- Attachment #1: Type: text/plain, Size: 1741 bytes --] Hi Ding, [auto build test WARNING on pci/next] [cannot apply to v4.12-rc3 next-20170602] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Ding-Tianhong/Add-new-PCI_DEV_FLAGS_NO_RELAXED_ORDERING-flag/20170603-132448 base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next config: x86_64-randconfig-x019-201722 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): >> drivers//pci/pci.c:4922:1: warning: "/*" within comment [-Wcomment] /** vim +4922 drivers//pci/pci.c 4906 * @dev: PCI device to query 4907 * 4908 * Returns true if relaxed ordering is been set 4909 */ 4910 int pcie_get_relaxed_ordering(struct pci_dev *dev) 4911 { 4912 u16 v; 4913 4914 pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v); 4915 4916 return (v & PCI_EXP_DEVCTL_RELAX_EN) >> 4; 4917 } 4918 EXPORT_SYMBOL(pcie_get_relaxed_ordering); 4919 4920 /** 4921 * pcie_set_mps - set PCI Express maximum payload size > 4922 /** 4923 * pcie_get_minimum_link - determine minimum link settings of a PCI device 4924 * @dev: PCI device to query 4925 * @speed: storage for minimum speed 4926 * @width: storage for minimum width 4927 * 4928 * This function will walk up the PCI device chain and determine the minimum 4929 * link width and speed of the device. 4930 */ --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 23461 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported 2017-06-03 4:04 ` [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong 2017-06-03 6:02 ` kbuild test robot @ 2017-06-03 18:19 ` Alexander Duyck 2017-06-04 3:07 ` Ding Tianhong 2017-06-05 13:33 ` Ding Tianhong 1 sibling, 2 replies; 10+ messages in thread From: Alexander Duyck @ 2017-06-03 18:19 UTC (permalink / raw) To: Ding Tianhong Cc: Casey Leedom, Ashok Raj, Bjorn Helgaas, Michael Werner, Ganesh Goudar, Asit K Mallick, Patrick J Cramer, Suravee Suthikulpanit, Bob Shaw, h, Amir Ancel, Gabriele Paoloni, David Laight, Jeff Kirsher, Catalin Marinas, Will Deacon, Mark Rutland, Robin Murphy, David Miller, linux-arm-kernel, Netdev, linux-pci, linux-kernel On Fri, Jun 2, 2017 at 9:04 PM, Ding Tianhong <dingtianhong@huawei.com> wrote: > The PCIe Device Control Register use the bit 4 to indicate that > whether the device is permitted to enable relaxed ordering or not. > But relaxed ordering is not safe for some platform which could only > use strong write ordering, so devices are allowed (but not required) > to enable relaxed ordering bit by default. > > If a platform support relaxed ordering but does not enable it by > default, enable it in the PCIe configuration. This allows some device > to send TLPs with the relaxed ordering attributes set, which may > improve the performance. > > Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> > --- > drivers/pci/pci.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > drivers/pci/probe.c | 11 +++++++++++ > include/linux/pci.h | 3 +++ > 3 files changed, 56 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index b01bd5b..f57a374 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4878,6 +4878,48 @@ int pcie_set_mps(struct pci_dev *dev, int mps) > EXPORT_SYMBOL(pcie_set_mps); > > /** > + * pcie_set_relaxed_ordering - set PCI Express relexed ordering bit > + * @dev: PCI device to query > + * > + * If possible sets relaxed ordering > + */ > +int pcie_set_relaxed_ordering(struct pci_dev *dev) > +{ > + return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); > +} > +EXPORT_SYMBOL(pcie_set_relaxed_ordering); > + > +/** > + * pcie_clear_relaxed_ordering - clear PCI Express relexed ordering bit > + * @dev: PCI device to query > + * > + * If possible clear relaxed ordering > + */ > +int pcie_clear_relaxed_ordering(struct pci_dev *dev) > +{ > + return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); > +} > +EXPORT_SYMBOL(pcie_clear_relaxed_ordering); > + > +/** > + * pcie_get_relaxed_ordering - check PCI Express relexed ordering bit > + * @dev: PCI device to query > + * > + * Returns true if relaxed ordering is been set > + */ > +int pcie_get_relaxed_ordering(struct pci_dev *dev) > +{ > + u16 v; > + > + pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v); > + > + return (v & PCI_EXP_DEVCTL_RELAX_EN) >> 4; > +} > +EXPORT_SYMBOL(pcie_get_relaxed_ordering); > + > +/** > + * pcie_set_mps - set PCI Express maximum payload size > +/** > * pcie_get_minimum_link - determine minimum link settings of a PCI device > * @dev: PCI device to query > * @speed: storage for minimum speed > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 19c8950..aeb22b5 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1701,6 +1701,16 @@ static void pci_configure_extended_tags(struct pci_dev *dev) > PCI_EXP_DEVCTL_EXT_TAG); > } > > +static void pci_configure_relaxed_ordering(struct pci_dev *dev) > +{ > + int ret; > + > + if (dev && (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING)) So there is a minor issue here. The problem is this is only trying to modify relaxed ordering for the device itself. That isn't what we want. What we want is to modify it on all of the upstream port interfaces where there is something the path to the root complex that has an issue. So if the root complex has to set the NO_RELAXED_ORDERING flag on a root port, all of the interfaces below it that would be pushing traffic toward it should not have the relaxed ordering bit set. Also I am pretty sure this is a PCIe capability, not a PCI capability. You probably need to make sure you code is making this distinction which I don't know if it currently is. If you need an example of the kind of checks I am suggesting just take a look at pcie_configure_mps(). It is verifying the function is PCIe before attempting to make any updates. In your case you will probably also need to make sure there is a bus for you to walk up the chain of. Otherwise this shouldn't apply. > + pcie_set_relaxed_ordering(dev); > + else > + pcie_clear_relaxed_ordering(dev); > +} Also I am not a fan of the way this is handled currently. If you don't have relaxed ordering set then you don't need to do anything else, if you do have it set but there is no bus to walk up you shouldn't change it, and if there is a bus to walk up and you find that the root complex on that bus has the NO_RELAXED_ORDERING set you should clear it. Right now this code seems to be enabling relaxed ordering if the NO_RELAXED_ORDERING flag is set. > + > static void pci_configure_device(struct pci_dev *dev) > { > struct hotplug_params hpp; > @@ -1708,6 +1718,7 @@ static void pci_configure_device(struct pci_dev *dev) > > pci_configure_mps(dev); > pci_configure_extended_tags(dev); > + pci_configure_relaxed_ordering(dev); > > memset(&hpp, 0, sizeof(hpp)); > ret = pci_get_hp_params(dev, &hpp); > diff --git a/include/linux/pci.h b/include/linux/pci.h > index e1e8428..84bd6af 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1105,6 +1105,9 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, > void pci_pme_wakeup_bus(struct pci_bus *bus); > void pci_d3cold_enable(struct pci_dev *dev); > void pci_d3cold_disable(struct pci_dev *dev); > +int pcie_set_relaxed_ordering(struct pci_dev *dev); > +int pcie_clear_relaxed_ordering(struct pci_dev *dev); > +int pcie_get_relaxed_ordering(struct pci_dev *dev); > > static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state, > bool enable) > -- > 1.9.0 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported 2017-06-03 18:19 ` Alexander Duyck @ 2017-06-04 3:07 ` Ding Tianhong 2017-06-05 13:33 ` Ding Tianhong 1 sibling, 0 replies; 10+ messages in thread From: Ding Tianhong @ 2017-06-04 3:07 UTC (permalink / raw) To: Alexander Duyck Cc: Casey Leedom, Ashok Raj, Bjorn Helgaas, Michael Werner, Ganesh Goudar, Asit K Mallick, Patrick J Cramer, Suravee Suthikulpanit, Bob Shaw, h, Amir Ancel, Gabriele Paoloni, David Laight, Jeff Kirsher, Catalin Marinas, Will Deacon, Mark Rutland, Robin Murphy, David Miller, linux-arm-kernel, Netdev, linux-pci, linux-kernel On 2017/6/4 2:19, Alexander Duyck wrote: > On Fri, Jun 2, 2017 at 9:04 PM, Ding Tianhong <dingtianhong@huawei.com> wrote: >> The PCIe Device Control Register use the bit 4 to indicate that >> whether the device is permitted to enable relaxed ordering or not. >> But relaxed ordering is not safe for some platform which could only >> use strong write ordering, so devices are allowed (but not required) >> to enable relaxed ordering bit by default. >> >> If a platform support relaxed ordering but does not enable it by >> default, enable it in the PCIe configuration. This allows some device >> to send TLPs with the relaxed ordering attributes set, which may >> improve the performance. >> >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >> --- >> drivers/pci/pci.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> drivers/pci/probe.c | 11 +++++++++++ >> include/linux/pci.h | 3 +++ >> 3 files changed, 56 insertions(+) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index b01bd5b..f57a374 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -4878,6 +4878,48 @@ int pcie_set_mps(struct pci_dev *dev, int mps) >> EXPORT_SYMBOL(pcie_set_mps); >> >> /** >> + * pcie_set_relaxed_ordering - set PCI Express relexed ordering bit >> + * @dev: PCI device to query >> + * >> + * If possible sets relaxed ordering >> + */ >> +int pcie_set_relaxed_ordering(struct pci_dev *dev) >> +{ >> + return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); >> +} >> +EXPORT_SYMBOL(pcie_set_relaxed_ordering); >> + >> +/** >> + * pcie_clear_relaxed_ordering - clear PCI Express relexed ordering bit >> + * @dev: PCI device to query >> + * >> + * If possible clear relaxed ordering >> + */ >> +int pcie_clear_relaxed_ordering(struct pci_dev *dev) >> +{ >> + return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); >> +} >> +EXPORT_SYMBOL(pcie_clear_relaxed_ordering); >> + >> +/** >> + * pcie_get_relaxed_ordering - check PCI Express relexed ordering bit >> + * @dev: PCI device to query >> + * >> + * Returns true if relaxed ordering is been set >> + */ >> +int pcie_get_relaxed_ordering(struct pci_dev *dev) >> +{ >> + u16 v; >> + >> + pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v); >> + >> + return (v & PCI_EXP_DEVCTL_RELAX_EN) >> 4; >> +} >> +EXPORT_SYMBOL(pcie_get_relaxed_ordering); >> + >> +/** >> + * pcie_set_mps - set PCI Express maximum payload size >> +/** >> * pcie_get_minimum_link - determine minimum link settings of a PCI device >> * @dev: PCI device to query >> * @speed: storage for minimum speed >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 19c8950..aeb22b5 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -1701,6 +1701,16 @@ static void pci_configure_extended_tags(struct pci_dev *dev) >> PCI_EXP_DEVCTL_EXT_TAG); >> } >> >> +static void pci_configure_relaxed_ordering(struct pci_dev *dev) >> +{ >> + int ret; >> + >> + if (dev && (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING)) > > So there is a minor issue here. The problem is this is only trying to > modify relaxed ordering for the device itself. That isn't what we > want. What we want is to modify it on all of the upstream port > interfaces where there is something the path to the root complex that > has an issue. So if the root complex has to set the > NO_RELAXED_ORDERING flag on a root port, all of the interfaces below > it that would be pushing traffic toward it should not have the relaxed > ordering bit set. > > Also I am pretty sure this is a PCIe capability, not a PCI capability. > You probably need to make sure you code is making this distinction > which I don't know if it currently is. If you need an example of the > kind of checks I am suggesting just take a look at > pcie_configure_mps(). It is verifying the function is PCIe before > attempting to make any updates. In your case you will probably also > need to make sure there is a bus for you to walk up the chain of. > Otherwise this shouldn't apply. > Yes, I miss the upstream ports and the pcie/pci capability, will check the pcie_configure_mps() again and fix it, thanks. > >> + pcie_set_relaxed_ordering(dev); >> + else >> + pcie_clear_relaxed_ordering(dev); >> +} > > Also I am not a fan of the way this is handled currently. If you don't > have relaxed ordering set then you don't need to do anything else, if > you do have it set but there is no bus to walk up you shouldn't change > it, and if there is a bus to walk up and you find that the root > complex on that bus has the NO_RELAXED_ORDERING set you should clear > it. Right now this code seems to be enabling relaxed ordering if the > NO_RELAXED_ORDERING flag is set. > I'm not very clear about this, I check the PCIe Standard, it said that: Enable Relaxed Ordering – If this bit is set, the device is permitted to set the Relaxed Ordering bit in the Attributes field of transactions it initiates that do not require strong write ordering (see Sections 2.2.6.4 and 2.4). Default value of this bit is 1. This bit may be hardwired to 0 if a device never sets the Relaxed Ordering attribute in transactions it initiates as a requester. Is that means some platform may set the bit to 0 default but it didn't mean the hardware couldn't support Relaxed Ordering ? if so, I need to set the bit enable if relaxed ordering flag is set. If I didn't understand it correctly, I will fix it follow your opinion, thanks. Ding >> + >> static void pci_configure_device(struct pci_dev *dev) >> { >> struct hotplug_params hpp; >> @@ -1708,6 +1718,7 @@ static void pci_configure_device(struct pci_dev *dev) >> >> pci_configure_mps(dev); >> pci_configure_extended_tags(dev); >> + pci_configure_relaxed_ordering(dev); >> >> memset(&hpp, 0, sizeof(hpp)); >> ret = pci_get_hp_params(dev, &hpp); >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index e1e8428..84bd6af 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -1105,6 +1105,9 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, >> void pci_pme_wakeup_bus(struct pci_bus *bus); >> void pci_d3cold_enable(struct pci_dev *dev); >> void pci_d3cold_disable(struct pci_dev *dev); >> +int pcie_set_relaxed_ordering(struct pci_dev *dev); >> +int pcie_clear_relaxed_ordering(struct pci_dev *dev); >> +int pcie_get_relaxed_ordering(struct pci_dev *dev); >> >> static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state, >> bool enable) >> -- >> 1.9.0 >> >> > > . > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported 2017-06-03 18:19 ` Alexander Duyck 2017-06-04 3:07 ` Ding Tianhong @ 2017-06-05 13:33 ` Ding Tianhong 2017-06-06 0:28 ` Alexander Duyck 1 sibling, 1 reply; 10+ messages in thread From: Ding Tianhong @ 2017-06-05 13:33 UTC (permalink / raw) To: Alexander Duyck Cc: Casey Leedom, Ashok Raj, Bjorn Helgaas, Michael Werner, Ganesh Goudar, Asit K Mallick, Patrick J Cramer, Suravee Suthikulpanit, Bob Shaw, h, Amir Ancel, Gabriele Paoloni, David Laight, Jeff Kirsher, Catalin Marinas, Will Deacon, Mark Rutland, Robin Murphy, David Miller, linux-arm-kernel, Netdev, linux-pci, linux-kernel On 2017/6/4 2:19, Alexander Duyck wrote: > On Fri, Jun 2, 2017 at 9:04 PM, Ding Tianhong <dingtianhong@huawei.com> wrote: >> The PCIe Device Control Register use the bit 4 to indicate that >> whether the device is permitted to enable relaxed ordering or not. >> But relaxed ordering is not safe for some platform which could only >> use strong write ordering, so devices are allowed (but not required) >> to enable relaxed ordering bit by default. >> >> If a platform support relaxed ordering but does not enable it by >> default, enable it in the PCIe configuration. This allows some device >> to send TLPs with the relaxed ordering attributes set, which may >> improve the performance. >> >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >> --- >> drivers/pci/pci.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> drivers/pci/probe.c | 11 +++++++++++ >> include/linux/pci.h | 3 +++ >> 3 files changed, 56 insertions(+) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index b01bd5b..f57a374 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -4878,6 +4878,48 @@ int pcie_set_mps(struct pci_dev *dev, int mps) >> EXPORT_SYMBOL(pcie_set_mps); >> >> /** >> + * pcie_set_relaxed_ordering - set PCI Express relexed ordering bit >> + * @dev: PCI device to query >> + * >> + * If possible sets relaxed ordering >> + */ >> +int pcie_set_relaxed_ordering(struct pci_dev *dev) >> +{ >> + return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); >> +} >> +EXPORT_SYMBOL(pcie_set_relaxed_ordering); >> + >> +/** >> + * pcie_clear_relaxed_ordering - clear PCI Express relexed ordering bit >> + * @dev: PCI device to query >> + * >> + * If possible clear relaxed ordering >> + */ >> +int pcie_clear_relaxed_ordering(struct pci_dev *dev) >> +{ >> + return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); >> +} >> +EXPORT_SYMBOL(pcie_clear_relaxed_ordering); >> + >> +/** >> + * pcie_get_relaxed_ordering - check PCI Express relexed ordering bit >> + * @dev: PCI device to query >> + * >> + * Returns true if relaxed ordering is been set >> + */ >> +int pcie_get_relaxed_ordering(struct pci_dev *dev) >> +{ >> + u16 v; >> + >> + pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v); >> + >> + return (v & PCI_EXP_DEVCTL_RELAX_EN) >> 4; >> +} >> +EXPORT_SYMBOL(pcie_get_relaxed_ordering); >> + >> +/** >> + * pcie_set_mps - set PCI Express maximum payload size >> +/** >> * pcie_get_minimum_link - determine minimum link settings of a PCI device >> * @dev: PCI device to query >> * @speed: storage for minimum speed >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 19c8950..aeb22b5 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -1701,6 +1701,16 @@ static void pci_configure_extended_tags(struct pci_dev *dev) >> PCI_EXP_DEVCTL_EXT_TAG); >> } >> >> +static void pci_configure_relaxed_ordering(struct pci_dev *dev) >> +{ >> + int ret; >> + >> + if (dev && (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING)) > > So there is a minor issue here. The problem is this is only trying to > modify relaxed ordering for the device itself. That isn't what we > want. What we want is to modify it on all of the upstream port > interfaces where there is something the path to the root complex that > has an issue. So if the root complex has to set the > NO_RELAXED_ORDERING flag on a root port, all of the interfaces below > it that would be pushing traffic toward it should not have the relaxed > ordering bit set. > > Also I am pretty sure this is a PCIe capability, not a PCI capability. > You probably need to make sure you code is making this distinction > which I don't know if it currently is. If you need an example of the > kind of checks I am suggesting just take a look at > pcie_configure_mps(). It is verifying the function is PCIe before > attempting to make any updates. In your case you will probably also > need to make sure there is a bus for you to walk up the chain of. > Otherwise this shouldn't apply. > > >> + pcie_set_relaxed_ordering(dev); >> + else >> + pcie_clear_relaxed_ordering(dev); >> +} > > Also I am not a fan of the way this is handled currently. If you don't > have relaxed ordering set then you don't need to do anything else, if > you do have it set but there is no bus to walk up you shouldn't change > it, and if there is a bus to walk up and you find that the root > complex on that bus has the NO_RELAXED_ORDERING set you should clear > it. Right now this code seems to be enabling relaxed ordering if the > NO_RELAXED_ORDERING flag is set. > Hi Alexander: I reconsidered your suggestion and found I miss something here, decide to modify the configure police as your solution, I think it is close to our goal. Thanks Ding diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 19c8950..68dee05 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1701,6 +1701,45 @@ static void pci_configure_extended_tags(struct pci_dev *dev) PCI_EXP_DEVCTL_EXT_TAG); } +static int pcie_clearing_relaxed_ordering(struct pci_dev *dev, void *data) +{ + int origin_ero; + + if (!pci_is_pcie(dev)) + return 0; + + origin_ero = pcie_get_relaxed_ordering(dev); + + /* If the releaxed ordering enable bit is not set, do nothing. */ + if (!origin_ero) + return 0; + + pcie_clear_relaxed_ordering(dev); + + dev_info(&dev->dev, "Disable Relaxed Ordering\n"); + + return 0; +} + +static void pci_configure_relaxed_ordering(struct pci_dev *dev) +{ + int origin_ero; + + if (!pci_is_pcie(dev)) + return; + + origin_ero = pcie_get_relaxed_ordering(dev); + /* If the releaxed ordering enable bit is not set, do nothing. */ + if (!origin_ero) + return; + + if (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING) { + pcie_clear_relaxed_ordering(dev); + pci_walk_bus(dev->bus, pcie_clearing_relaxed_ordering, NULL); + dev_info(&dev->dev, "Disable Relaxed Ordering\n"); + } +} + static void pci_configure_device(struct pci_dev *dev) { struct hotplug_params hpp; @@ -1708,6 +1747,7 @@ static void pci_configure_device(struct pci_dev *dev) pci_configure_mps(dev); pci_configure_extended_tags(dev); + pci_configure_relaxed_ordering(dev); memset(&hpp, 0, sizeof(hpp)); ret = pci_get_hp_params(dev, &hpp); diff --git a/include/linux/pci.h b/include/linux/pci.h >> + >> static void pci_configure_device(struct pci_dev *dev) >> { >> struct hotplug_params hpp; >> @@ -1708,6 +1718,7 @@ static void pci_configure_device(struct pci_dev *dev) >> >> pci_configure_mps(dev); >> pci_configure_extended_tags(dev); >> + pci_configure_relaxed_ordering(dev); >> >> memset(&hpp, 0, sizeof(hpp)); >> ret = pci_get_hp_params(dev, &hpp); >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index e1e8428..84bd6af 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -1105,6 +1105,9 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, >> void pci_pme_wakeup_bus(struct pci_bus *bus); >> void pci_d3cold_enable(struct pci_dev *dev); >> void pci_d3cold_disable(struct pci_dev *dev); >> +int pcie_set_relaxed_ordering(struct pci_dev *dev); >> +int pcie_clear_relaxed_ordering(struct pci_dev *dev); >> +int pcie_get_relaxed_ordering(struct pci_dev *dev); >> >> static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state, >> bool enable) >> -- >> 1.9.0 >> >> > > . > ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported 2017-06-05 13:33 ` Ding Tianhong @ 2017-06-06 0:28 ` Alexander Duyck 2017-06-06 6:09 ` Ding Tianhong 0 siblings, 1 reply; 10+ messages in thread From: Alexander Duyck @ 2017-06-06 0:28 UTC (permalink / raw) To: Ding Tianhong Cc: Casey Leedom, Ashok Raj, Bjorn Helgaas, Michael Werner, Ganesh Goudar, Asit K Mallick, Patrick J Cramer, Suravee Suthikulpanit, Bob Shaw, h, Amir Ancel, Gabriele Paoloni, David Laight, Jeff Kirsher, Catalin Marinas, Will Deacon, Mark Rutland, Robin Murphy, David Miller, linux-arm-kernel, Netdev, linux-pci, linux-kernel On Mon, Jun 5, 2017 at 6:33 AM, Ding Tianhong <dingtianhong@huawei.com> wrote: > > > On 2017/6/4 2:19, Alexander Duyck wrote: >> On Fri, Jun 2, 2017 at 9:04 PM, Ding Tianhong <dingtianhong@huawei.com> wrote: >>> The PCIe Device Control Register use the bit 4 to indicate that >>> whether the device is permitted to enable relaxed ordering or not. >>> But relaxed ordering is not safe for some platform which could only >>> use strong write ordering, so devices are allowed (but not required) >>> to enable relaxed ordering bit by default. >>> >>> If a platform support relaxed ordering but does not enable it by >>> default, enable it in the PCIe configuration. This allows some device >>> to send TLPs with the relaxed ordering attributes set, which may >>> improve the performance. >>> >>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >>> --- >>> drivers/pci/pci.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >>> drivers/pci/probe.c | 11 +++++++++++ >>> include/linux/pci.h | 3 +++ >>> 3 files changed, 56 insertions(+) >>> >>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>> index b01bd5b..f57a374 100644 >>> --- a/drivers/pci/pci.c >>> +++ b/drivers/pci/pci.c >>> @@ -4878,6 +4878,48 @@ int pcie_set_mps(struct pci_dev *dev, int mps) >>> EXPORT_SYMBOL(pcie_set_mps); >>> >>> /** >>> + * pcie_set_relaxed_ordering - set PCI Express relexed ordering bit >>> + * @dev: PCI device to query >>> + * >>> + * If possible sets relaxed ordering >>> + */ >>> +int pcie_set_relaxed_ordering(struct pci_dev *dev) >>> +{ >>> + return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); >>> +} >>> +EXPORT_SYMBOL(pcie_set_relaxed_ordering); >>> + >>> +/** >>> + * pcie_clear_relaxed_ordering - clear PCI Express relexed ordering bit >>> + * @dev: PCI device to query >>> + * >>> + * If possible clear relaxed ordering >>> + */ >>> +int pcie_clear_relaxed_ordering(struct pci_dev *dev) >>> +{ >>> + return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); >>> +} >>> +EXPORT_SYMBOL(pcie_clear_relaxed_ordering); >>> + >>> +/** >>> + * pcie_get_relaxed_ordering - check PCI Express relexed ordering bit >>> + * @dev: PCI device to query >>> + * >>> + * Returns true if relaxed ordering is been set >>> + */ >>> +int pcie_get_relaxed_ordering(struct pci_dev *dev) >>> +{ >>> + u16 v; >>> + >>> + pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v); >>> + >>> + return (v & PCI_EXP_DEVCTL_RELAX_EN) >> 4; >>> +} >>> +EXPORT_SYMBOL(pcie_get_relaxed_ordering); >>> + >>> +/** >>> + * pcie_set_mps - set PCI Express maximum payload size >>> +/** >>> * pcie_get_minimum_link - determine minimum link settings of a PCI device >>> * @dev: PCI device to query >>> * @speed: storage for minimum speed >>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>> index 19c8950..aeb22b5 100644 >>> --- a/drivers/pci/probe.c >>> +++ b/drivers/pci/probe.c >>> @@ -1701,6 +1701,16 @@ static void pci_configure_extended_tags(struct pci_dev *dev) >>> PCI_EXP_DEVCTL_EXT_TAG); >>> } >>> >>> +static void pci_configure_relaxed_ordering(struct pci_dev *dev) >>> +{ >>> + int ret; >>> + >>> + if (dev && (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING)) >> >> So there is a minor issue here. The problem is this is only trying to >> modify relaxed ordering for the device itself. That isn't what we >> want. What we want is to modify it on all of the upstream port >> interfaces where there is something the path to the root complex that >> has an issue. So if the root complex has to set the >> NO_RELAXED_ORDERING flag on a root port, all of the interfaces below >> it that would be pushing traffic toward it should not have the relaxed >> ordering bit set. >> >> Also I am pretty sure this is a PCIe capability, not a PCI capability. >> You probably need to make sure you code is making this distinction >> which I don't know if it currently is. If you need an example of the >> kind of checks I am suggesting just take a look at >> pcie_configure_mps(). It is verifying the function is PCIe before >> attempting to make any updates. In your case you will probably also >> need to make sure there is a bus for you to walk up the chain of. >> Otherwise this shouldn't apply. >> >> >>> + pcie_set_relaxed_ordering(dev); >>> + else >>> + pcie_clear_relaxed_ordering(dev); >>> +} >> >> Also I am not a fan of the way this is handled currently. If you don't >> have relaxed ordering set then you don't need to do anything else, if >> you do have it set but there is no bus to walk up you shouldn't change >> it, and if there is a bus to walk up and you find that the root >> complex on that bus has the NO_RELAXED_ORDERING set you should clear >> it. Right now this code seems to be enabling relaxed ordering if the >> NO_RELAXED_ORDERING flag is set. >> > > Hi Alexander: > > I reconsidered your suggestion and found I miss something here, > decide to modify the configure police as your solution, I think > it is close to our goal. > > Thanks > Ding > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 19c8950..68dee05 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1701,6 +1701,45 @@ static void pci_configure_extended_tags(struct pci_dev *dev) > PCI_EXP_DEVCTL_EXT_TAG); > } > > +static int pcie_clearing_relaxed_ordering(struct pci_dev *dev, void *data) > +{ > + int origin_ero; > + > + if (!pci_is_pcie(dev)) > + return 0; > + > + origin_ero = pcie_get_relaxed_ordering(dev); > + > + /* If the releaxed ordering enable bit is not set, do nothing. */ > + if (!origin_ero) > + return 0; > + > + pcie_clear_relaxed_ordering(dev); > + > + dev_info(&dev->dev, "Disable Relaxed Ordering\n"); > + > + return 0; > +} > + > +static void pci_configure_relaxed_ordering(struct pci_dev *dev) > +{ > + int origin_ero; > + > + if (!pci_is_pcie(dev)) > + return; > + > + origin_ero = pcie_get_relaxed_ordering(dev); > + /* If the releaxed ordering enable bit is not set, do nothing. */ > + if (!origin_ero) > + return; > + > + if (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING) { > + pcie_clear_relaxed_ordering(dev); > + pci_walk_bus(dev->bus, pcie_clearing_relaxed_ordering, NULL); > + dev_info(&dev->dev, "Disable Relaxed Ordering\n"); > + } > +} > + This is kind of backwards from what I was thinking. Basically what I would like to see is at probe time we should work our way up the PCIe buses checking to see if any device has PCI_DEV_FLAGS_NO_RELAXED_ORDERING set. The assumption is if we can't use relaxed ordering with the downstream facing port we probably shouldn't be enabling it on our upstream facing port. We don't want to be writing to other devices and such since we don't know what they need, we will only know what our device needs and if the root complex reports that it can't support relaxed ordering we should disable it for the device we are initializing and move on. You might use pcie_get_minimum_link as an example of what I am thinking. Basically what we should do is add a function that will return true if any of the devices above us do not support relaxed ordering, otherwise return false. Then based on that result if we get a return that indicates that relaxed ordering is not supported we should update our device to disable relaxed ordering. If the device above us doesn't exist, or isn't PCIe we should just exit and skip updating relaxed ordering since we are probably running in a guest. > static void pci_configure_device(struct pci_dev *dev) > { > struct hotplug_params hpp; > @@ -1708,6 +1747,7 @@ static void pci_configure_device(struct pci_dev *dev) > > pci_configure_mps(dev); > pci_configure_extended_tags(dev); > + pci_configure_relaxed_ordering(dev); > > memset(&hpp, 0, sizeof(hpp)); > ret = pci_get_hp_params(dev, &hpp); > diff --git a/include/linux/pci.h b/include/linux/pci.h > > > >>> + >>> static void pci_configure_device(struct pci_dev *dev) >>> { >>> struct hotplug_params hpp; >>> @@ -1708,6 +1718,7 @@ static void pci_configure_device(struct pci_dev *dev) >>> >>> pci_configure_mps(dev); >>> pci_configure_extended_tags(dev); >>> + pci_configure_relaxed_ordering(dev); >>> >>> memset(&hpp, 0, sizeof(hpp)); >>> ret = pci_get_hp_params(dev, &hpp); >>> diff --git a/include/linux/pci.h b/include/linux/pci.h >>> index e1e8428..84bd6af 100644 >>> --- a/include/linux/pci.h >>> +++ b/include/linux/pci.h >>> @@ -1105,6 +1105,9 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, >>> void pci_pme_wakeup_bus(struct pci_bus *bus); >>> void pci_d3cold_enable(struct pci_dev *dev); >>> void pci_d3cold_disable(struct pci_dev *dev); >>> +int pcie_set_relaxed_ordering(struct pci_dev *dev); >>> +int pcie_clear_relaxed_ordering(struct pci_dev *dev); >>> +int pcie_get_relaxed_ordering(struct pci_dev *dev); >>> >>> static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state, >>> bool enable) >>> -- >>> 1.9.0 >>> >>> >> >> . >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported 2017-06-06 0:28 ` Alexander Duyck @ 2017-06-06 6:09 ` Ding Tianhong 0 siblings, 0 replies; 10+ messages in thread From: Ding Tianhong @ 2017-06-06 6:09 UTC (permalink / raw) To: Alexander Duyck Cc: Casey Leedom, Ashok Raj, Bjorn Helgaas, Michael Werner, Ganesh Goudar, Asit K Mallick, Patrick J Cramer, Suravee Suthikulpanit, Bob Shaw, h, Amir Ancel, Gabriele Paoloni, David Laight, Jeff Kirsher, Catalin Marinas, Will Deacon, Mark Rutland, Robin Murphy, David Miller, linux-arm-kernel, Netdev, linux-pci, linux-kernel On 2017/6/6 8:28, Alexander Duyck wrote: > On Mon, Jun 5, 2017 at 6:33 AM, Ding Tianhong <dingtianhong@huawei.com> wrote: >> >> >> On 2017/6/4 2:19, Alexander Duyck wrote: >>> On Fri, Jun 2, 2017 at 9:04 PM, Ding Tianhong <dingtianhong@huawei.com> wrote: >>>> The PCIe Device Control Register use the bit 4 to indicate that >>>> whether the device is permitted to enable relaxed ordering or not. >>>> But relaxed ordering is not safe for some platform which could only >>>> use strong write ordering, so devices are allowed (but not required) >>>> to enable relaxed ordering bit by default. >>>> >>>> If a platform support relaxed ordering but does not enable it by >>>> default, enable it in the PCIe configuration. This allows some device >>>> to send TLPs with the relaxed ordering attributes set, which may >>>> improve the performance. >>>> >>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >>>> --- >>>> drivers/pci/pci.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >>>> drivers/pci/probe.c | 11 +++++++++++ >>>> include/linux/pci.h | 3 +++ >>>> 3 files changed, 56 insertions(+) >>>> >>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>>> index b01bd5b..f57a374 100644 >>>> --- a/drivers/pci/pci.c >>>> +++ b/drivers/pci/pci.c >>>> @@ -4878,6 +4878,48 @@ int pcie_set_mps(struct pci_dev *dev, int mps) >>>> EXPORT_SYMBOL(pcie_set_mps); >>>> >>>> /** >>>> + * pcie_set_relaxed_ordering - set PCI Express relexed ordering bit >>>> + * @dev: PCI device to query >>>> + * >>>> + * If possible sets relaxed ordering >>>> + */ >>>> +int pcie_set_relaxed_ordering(struct pci_dev *dev) >>>> +{ >>>> + return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); >>>> +} >>>> +EXPORT_SYMBOL(pcie_set_relaxed_ordering); >>>> + >>>> +/** >>>> + * pcie_clear_relaxed_ordering - clear PCI Express relexed ordering bit >>>> + * @dev: PCI device to query >>>> + * >>>> + * If possible clear relaxed ordering >>>> + */ >>>> +int pcie_clear_relaxed_ordering(struct pci_dev *dev) >>>> +{ >>>> + return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN); >>>> +} >>>> +EXPORT_SYMBOL(pcie_clear_relaxed_ordering); >>>> + >>>> +/** >>>> + * pcie_get_relaxed_ordering - check PCI Express relexed ordering bit >>>> + * @dev: PCI device to query >>>> + * >>>> + * Returns true if relaxed ordering is been set >>>> + */ >>>> +int pcie_get_relaxed_ordering(struct pci_dev *dev) >>>> +{ >>>> + u16 v; >>>> + >>>> + pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v); >>>> + >>>> + return (v & PCI_EXP_DEVCTL_RELAX_EN) >> 4; >>>> +} >>>> +EXPORT_SYMBOL(pcie_get_relaxed_ordering); >>>> + >>>> +/** >>>> + * pcie_set_mps - set PCI Express maximum payload size >>>> +/** >>>> * pcie_get_minimum_link - determine minimum link settings of a PCI device >>>> * @dev: PCI device to query >>>> * @speed: storage for minimum speed >>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>>> index 19c8950..aeb22b5 100644 >>>> --- a/drivers/pci/probe.c >>>> +++ b/drivers/pci/probe.c >>>> @@ -1701,6 +1701,16 @@ static void pci_configure_extended_tags(struct pci_dev *dev) >>>> PCI_EXP_DEVCTL_EXT_TAG); >>>> } >>>> >>>> +static void pci_configure_relaxed_ordering(struct pci_dev *dev) >>>> +{ >>>> + int ret; >>>> + >>>> + if (dev && (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING)) >>> >>> So there is a minor issue here. The problem is this is only trying to >>> modify relaxed ordering for the device itself. That isn't what we >>> want. What we want is to modify it on all of the upstream port >>> interfaces where there is something the path to the root complex that >>> has an issue. So if the root complex has to set the >>> NO_RELAXED_ORDERING flag on a root port, all of the interfaces below >>> it that would be pushing traffic toward it should not have the relaxed >>> ordering bit set. >>> >>> Also I am pretty sure this is a PCIe capability, not a PCI capability. >>> You probably need to make sure you code is making this distinction >>> which I don't know if it currently is. If you need an example of the >>> kind of checks I am suggesting just take a look at >>> pcie_configure_mps(). It is verifying the function is PCIe before >>> attempting to make any updates. In your case you will probably also >>> need to make sure there is a bus for you to walk up the chain of. >>> Otherwise this shouldn't apply. >>> >>> >>>> + pcie_set_relaxed_ordering(dev); >>>> + else >>>> + pcie_clear_relaxed_ordering(dev); >>>> +} >>> >>> Also I am not a fan of the way this is handled currently. If you don't >>> have relaxed ordering set then you don't need to do anything else, if >>> you do have it set but there is no bus to walk up you shouldn't change >>> it, and if there is a bus to walk up and you find that the root >>> complex on that bus has the NO_RELAXED_ORDERING set you should clear >>> it. Right now this code seems to be enabling relaxed ordering if the >>> NO_RELAXED_ORDERING flag is set. >>> >> >> Hi Alexander: >> >> I reconsidered your suggestion and found I miss something here, >> decide to modify the configure police as your solution, I think >> it is close to our goal. >> >> Thanks >> Ding >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 19c8950..68dee05 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -1701,6 +1701,45 @@ static void pci_configure_extended_tags(struct pci_dev *dev) >> PCI_EXP_DEVCTL_EXT_TAG); >> } >> >> +static int pcie_clearing_relaxed_ordering(struct pci_dev *dev, void *data) >> +{ >> + int origin_ero; >> + >> + if (!pci_is_pcie(dev)) >> + return 0; >> + >> + origin_ero = pcie_get_relaxed_ordering(dev); >> + >> + /* If the releaxed ordering enable bit is not set, do nothing. */ >> + if (!origin_ero) >> + return 0; >> + >> + pcie_clear_relaxed_ordering(dev); >> + >> + dev_info(&dev->dev, "Disable Relaxed Ordering\n"); >> + >> + return 0; >> +} >> + >> +static void pci_configure_relaxed_ordering(struct pci_dev *dev) >> +{ >> + int origin_ero; >> + >> + if (!pci_is_pcie(dev)) >> + return; >> + >> + origin_ero = pcie_get_relaxed_ordering(dev); >> + /* If the releaxed ordering enable bit is not set, do nothing. */ >> + if (!origin_ero) >> + return; >> + >> + if (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING) { >> + pcie_clear_relaxed_ordering(dev); >> + pci_walk_bus(dev->bus, pcie_clearing_relaxed_ordering, NULL); >> + dev_info(&dev->dev, "Disable Relaxed Ordering\n"); >> + } >> +} >> + > > This is kind of backwards from what I was thinking. Basically what I > would like to see is at probe time we should work our way up the PCIe > buses checking to see if any device has > PCI_DEV_FLAGS_NO_RELAXED_ORDERING set. The assumption is if we can't > use relaxed ordering with the downstream facing port we probably > shouldn't be enabling it on our upstream facing port. We don't want to > be writing to other devices and such since we don't know what they > need, we will only know what our device needs and if the root complex > reports that it can't support relaxed ordering we should disable it > for the device we are initializing and move on. > > You might use pcie_get_minimum_link as an example of what I am > thinking. Basically what we should do is add a function that will > return true if any of the devices above us do not support relaxed > ordering, otherwise return false. Then based on that result if we get > a return that indicates that relaxed ordering is not supported we > should update our device to disable relaxed ordering. If the device > above us doesn't exist, or isn't PCIe we should just exit and skip > updating relaxed ordering since we are probably running in a guest. > I think I understand what you mean this time, if no obviously problem, I will send next version base on this code, thanks. diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 19c8950..777fe5c 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1701,6 +1701,48 @@ static void pci_configure_extended_tags(struct pci_dev *dev) PCI_EXP_DEVCTL_EXT_TAG); } +/** + * pci_dev_relaxed_ordering_disabled - check if the PCI device + * should disable the relaxed ordering attribute. + * @dev: PCI device + * + * Return true if any of the PCI devices above us do not support + * relaxed ordering. + */ +static int pcie_check_relaxed_ordering_status(struct pci_dev *dev) +{ + int ro_disabled = 0; + + while(dev) { + if (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING) { + ro_disabled = 1; + break; + } + dev = dev->bus->self; + } + + return ro_disabled; +} + +static void pci_configure_relaxed_ordering(struct pci_dev *dev) +{ + struct pci_dev *bridge = pci_upstream_bridge(dev); + int origin_ero; + + if (!pci_is_pcie(dev) || !bridge || !pci_is_pcie(bridge)) + return; + + origin_ero = pcie_get_relaxed_ordering(dev); + /* If the releaxed ordering enable bit is not set, do nothing. */ + if (!origin_ero) + return; + + if (pcie_check_relaxed_ordering_status(dev)) { + pcie_clear_relaxed_ordering(dev); + dev_info(&dev->dev, "Disable Relaxed Ordering\n"); + } +} + >> static void pci_configure_device(struct pci_dev *dev) >> { >> struct hotplug_params hpp; >> @@ -1708,6 +1747,7 @@ static void pci_configure_device(struct pci_dev *dev) >> >> pci_configure_mps(dev); >> pci_configure_extended_tags(dev); >> + pci_configure_relaxed_ordering(dev); >> >> memset(&hpp, 0, sizeof(hpp)); >> ret = pci_get_hp_params(dev, &hpp); >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> >> >> >>>> + >>>> static void pci_configure_device(struct pci_dev *dev) >>>> { >>>> struct hotplug_params hpp; >>>> @@ -1708,6 +1718,7 @@ static void pci_configure_device(struct pci_dev *dev) >>>> >>>> pci_configure_mps(dev); >>>> pci_configure_extended_tags(dev); >>>> + pci_configure_relaxed_ordering(dev); >>>> >>>> memset(&hpp, 0, sizeof(hpp)); >>>> ret = pci_get_hp_params(dev, &hpp); >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h >>>> index e1e8428..84bd6af 100644 >>>> --- a/include/linux/pci.h >>>> +++ b/include/linux/pci.h >>>> @@ -1105,6 +1105,9 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state, >>>> void pci_pme_wakeup_bus(struct pci_bus *bus); >>>> void pci_d3cold_enable(struct pci_dev *dev); >>>> void pci_d3cold_disable(struct pci_dev *dev); >>>> +int pcie_set_relaxed_ordering(struct pci_dev *dev); >>>> +int pcie_clear_relaxed_ordering(struct pci_dev *dev); >>>> +int pcie_get_relaxed_ordering(struct pci_dev *dev); >>>> >>>> static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state, >>>> bool enable) >>>> -- >>>> 1.9.0 >>>> >>>> >>> >>> . >>> >> > > . > ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] net/cxgb4: Use new PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag 2017-06-03 4:04 [PATCH v2 0/3] Add new PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag Ding Tianhong 2017-06-03 4:04 ` [PATCH v2 1/3] PCI: Add new PCIe Fabric End Node flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING Ding Tianhong 2017-06-03 4:04 ` [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong @ 2017-06-03 4:04 ` Ding Tianhong 2 siblings, 0 replies; 10+ messages in thread From: Ding Tianhong @ 2017-06-03 4:04 UTC (permalink / raw) To: leedom, ashok.raj, helgaas, werner, ganeshgr, asit.k.mallick, patrick.j.cramer, Suravee.Suthikulpanit, Bob.Shaw, l.stach, amira, gabriele.paoloni, David.Laight, jeffrey.t.kirsher, catalin.marinas, will.deacon, mark.rutland, robin.murphy, davem, alexander.duyck, linux-arm-kernel, netdev, linux-pci, linux-kernel Cc: Ding Tianhong From: Casey Leedom <leedom@chelsio.com> cxgb4 Ethernet driver now queries Root Complex Port to determine if it can send TLPs to it with the Relaxed Ordering Attribute set. Signed-off-by: Casey Leedom <leedom@chelsio.com> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> --- drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 1 + drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 17 +++++++++++++++++ drivers/net/ethernet/chelsio/cxgb4/sge.c | 5 +++-- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h index e88c180..478f25a 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h @@ -521,6 +521,7 @@ enum { /* adapter flags */ USING_SOFT_PARAMS = (1 << 6), MASTER_PF = (1 << 7), FW_OFLD_CONN = (1 << 9), + ROOT_NO_RELAXED_ORDERING = (1 << 10), }; enum { diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c index 38a5c67..fbfe341 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c @@ -4628,6 +4628,7 @@ static int init_one(struct pci_dev *pdev, const struct pci_device_id *ent) #ifdef CONFIG_PCI_IOV u32 v, port_vec; #endif + struct pci_dev *root; printk_once(KERN_INFO "%s - version %s\n", DRV_DESC, DRV_VERSION); @@ -4726,6 +4727,22 @@ static int init_one(struct pci_dev *pdev, const struct pci_device_id *ent) adapter->msg_enable = DFLT_MSG_ENABLE; memset(adapter->chan_map, 0xff, sizeof(adapter->chan_map)); + /* If possible, we use PCIe Relaxed Ordering Attribute to deliver + * Ingress Packet Data to Free List Buffers in order to allow for + * chipset performance optimizations between the Root Complex and + * Memory Controllers. (Messages to the associated Ingress Queue + * notifying new Packet Placement in the Free Lists Buffers will be + * send without the Relaxed Ordering Attribute thus guaranteing that + * all preceding PCIe Transaction Layer Packets will be processed + * first.) But some Root Complexes have various issues with Upstream + * Transaction Layer Packets with the Relaxed Ordering Attribute set. + * So we check our Root Complex to see if it's flaged with advice + * against using Relaxed Ordering. + */ + root = pci_find_pcie_root_port(adapter->pdev); + if (pcie_get_relaxed_ordering(root)) + adapter->flags |= ROOT_NO_RELAXED_ORDERING; + spin_lock_init(&adapter->stats_lock); spin_lock_init(&adapter->tid_release_lock); spin_lock_init(&adapter->win0_lock); diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c index f05f0d4..ac229a3 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/sge.c +++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c @@ -2571,6 +2571,7 @@ int t4_sge_alloc_rxq(struct adapter *adap, struct sge_rspq *iq, bool fwevtq, struct fw_iq_cmd c; struct sge *s = &adap->sge; struct port_info *pi = netdev_priv(dev); + int relaxed = !(adap->flags & ROOT_NO_RELAXED_ORDERING); /* Size needs to be multiple of 16, including status entry. */ iq->size = roundup(iq->size, 16); @@ -2624,8 +2625,8 @@ int t4_sge_alloc_rxq(struct adapter *adap, struct sge_rspq *iq, bool fwevtq, flsz = fl->size / 8 + s->stat_len / sizeof(struct tx_desc); c.iqns_to_fl0congen |= htonl(FW_IQ_CMD_FL0PACKEN_F | - FW_IQ_CMD_FL0FETCHRO_F | - FW_IQ_CMD_FL0DATARO_F | + FW_IQ_CMD_FL0FETCHRO_V(relaxed) | + FW_IQ_CMD_FL0DATARO_V(relaxed) | FW_IQ_CMD_FL0PADEN_F); if (cong >= 0) c.iqns_to_fl0congen |= -- 1.9.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-06-06 6:21 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-03 4:04 [PATCH v2 0/3] Add new PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag Ding Tianhong 2017-06-03 4:04 ` [PATCH v2 1/3] PCI: Add new PCIe Fabric End Node flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING Ding Tianhong 2017-06-03 4:04 ` [PATCH v2 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong 2017-06-03 6:02 ` kbuild test robot 2017-06-03 18:19 ` Alexander Duyck 2017-06-04 3:07 ` Ding Tianhong 2017-06-05 13:33 ` Ding Tianhong 2017-06-06 0:28 ` Alexander Duyck 2017-06-06 6:09 ` Ding Tianhong 2017-06-03 4:04 ` [PATCH v2 3/3] net/cxgb4: Use new PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag Ding Tianhong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).