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.
v3: Redesigned the logic for pci_configure_relaxed_ordering when configuration,
If a PCIe device didn't enable the relaxed ordering attribute default,
we should not do anything in the PCIe configuration, otherwise we
should check if any of the devices above us do not support relaxed
ordering by the PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag, then base on
the result if we get a return that indicate that the relaxed ordering
is not supported we should update our device to disable relaxed ordering
in configuration space. If the device above us doesn't exist or isn't
the PCIe device, we shouldn't do anything and skip updating relaxed ordering
because we are probably running in a guest.
v4: Rename the functions pcie_get_relaxed_ordering and pcie_disable_relaxed_ordering
according John's suggestion, and modify the description, use the true/false
as the return value.
We shouldn't enable relaxed ordering attribute by the setting in the root
complex configuration space for PCIe device, so fix it for cxgb4.
Fix some format issues.
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 | 32 +++++++++++++++++++
drivers/pci/probe.c | 41 +++++++++++++++++++++++++
drivers/pci/quirks.c | 38 +++++++++++++++++++++++
include/linux/pci.h | 4 +++
7 files changed, 136 insertions(+), 2 deletions(-)
--
1.9.0
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 PCIe device didn't enable the relaxed ordering attribute default,
we should not do anything in the PCIe configuration, otherwise we
should check if any of the devices above us do not support relaxed
ordering by the PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag, then base on
the result if we get a return that indicate that the relaxed ordering
is not supported we should update our device to disable relaxed ordering
in configuration space. If the device above us doesn't exist or isn't
the PCIe device, we shouldn't do anything and skip updating relaxed ordering
because we are probably running in a guest machine.
Signed-off-by: Ding Tianhong <redacted>
---
drivers/pci/pci.c | 32 ++++++++++++++++++++++++++++++++
drivers/pci/probe.c | 41 +++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 2 ++
3 files changed, 75 insertions(+)
@@ -4878,6 +4878,38 @@ int pcie_set_mps(struct pci_dev *dev, int mps)EXPORT_SYMBOL(pcie_set_mps);/**+*pcie_clear_relaxed_ordering-clearPCIExpressrelaxedorderingbit+*@dev:PCIdevicetoquery+*+*Ifpossibleclearrelaxedordering+*/+intpcie_clear_relaxed_ordering(structpci_dev*dev)+{+returnpcie_capability_clear_word(dev,PCI_EXP_DEVCTL,+PCI_EXP_DEVCTL_RELAX_EN);+}+EXPORT_SYMBOL(pcie_clear_relaxed_ordering);++/**+*pcie_relaxed_ordering_supported-ProbeforPCIerelexedorderingsupport+*@dev:PCIdevicetoquery+*+*Returnstrueifthedevicesupportrelaxedorderingattribute.+*/+boolpcie_relaxed_ordering_supported(structpci_dev*dev)+{+boolro_supported=false;+u16v;++pcie_capability_read_word(dev,PCI_EXP_DEVCTL,&v);+if((v&PCI_EXP_DEVCTL_RELAX_EN)>>4)+ro_supported=true;++returnro_supported;+}+EXPORT_SYMBOL(pcie_relaxed_ordering_supported);++/***pcie_get_minimum_link-determineminimumlinksettingsofaPCIdevice*@dev:PCIdevicetoquery*@speed:storageforminimumspeed
@@ -1701,6 +1701,46 @@ static void pci_configure_extended_tags(struct pci_dev *dev)PCI_EXP_DEVCTL_EXT_TAG);}+/**+*pci_dev_should_disable_relaxed_ordering-checkifthePCIdevice+*shoulddisabletherelaxedorderingattribute.+*@dev:PCIdevice+*+*ReturntrueifanyofthePCIdevicesaboveusdonotsupport+*relaxedordering.+*/+staticboolpci_dev_should_disable_relaxed_ordering(structpci_dev*dev)+{+boolro_disabled=false;++while(dev){+if(dev->dev_flags&PCI_DEV_FLAGS_NO_RELAXED_ORDERING){+ro_disabled=true;+break;+}+dev=dev->bus->self;+}++returnro_disabled;+}++staticvoidpci_configure_relaxed_ordering(structpci_dev*dev)+{+structpci_dev*bridge=pci_upstream_bridge(dev);++if(!pci_is_pcie(dev)||!bridge||!pci_is_pcie(bridge))+return;++/* If the releaxed ordering enable bit is not set, do nothing. */+if(!pcie_relaxed_ordering_supported(dev))+return;++if(pci_dev_should_disable_relaxed_ordering(dev)){+pcie_clear_relaxed_ordering(dev);+dev_info(&dev->dev,"Disable Relaxed Ordering\n");+}+}+staticvoidpci_configure_device(structpci_dev*dev){structhotplug_paramshpp;
From: Casey Leedom <redacted>
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 <redacted>
Signed-off-by: Ding Tianhong <redacted>
---
drivers/pci/quirks.c | 38 ++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 2 ++
2 files changed, 40 insertions(+)
@@ -183,6 +183,8 @@ enum pci_dev_flags {PCI_DEV_FLAGS_BRIDGE_XLATE_ROOT=(__forcepci_dev_flags_t)(1<<9),/* Do not use FLR even if device advertises PCI_AF_CAP */PCI_DEV_FLAGS_NO_FLR_RESET=(__forcepci_dev_flags_t)(1<<10),+/* Don't use Relaxed Ordering for TLPs directed@this device */+PCI_DEV_FLAGS_NO_RELAXED_ORDERING=(__forcepci_dev_flags_t)(1<<11),};enumpci_irq_reroute_variant{
@@ -4726,6 +4726,23 @@ 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+*IngressPacketDatatoFreeListBuffersinordertoallowfor+*chipsetperformanceoptimizationsbetweentheRootComplexand+*MemoryControllers.(MessagestotheassociatedIngressQueue+*notifyingnewPacketPlacementintheFreeListsBufferswillbe+*sendwithouttheRelaxedOrderingAttributethusguaranteeingthat+*allprecedingPCIeTransactionLayerPacketswillbeprocessed+*first.)ButsomeRootComplexeshavevariousissueswithUpstream+*TransactionLayerPacketswiththeRelaxedOrderingAttributeset.+*ThePCIedeviceswhichundertheRootComplexeswillbeclearedthe+*RelaxedOrderingbitintheconfigurationspace,Sowecheckour+*PCIeconfigurationspacetoseeifit'sflaggedwithadviceagainst+*usingRelaxedOrdering.+*/+if(pcie_relaxed_ordering_supported(pdev))+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);
@@ -2571,6 +2571,7 @@ int t4_sge_alloc_rxq(struct adapter *adap, struct sge_rspq *iq, bool fwevtq,structfw_iq_cmdc;structsge*s=&adap->sge;structport_info*pi=netdev_priv(dev);+intrelaxed=!(adap->flags&ROOT_NO_RELAXED_ORDERING);/* Size needs to be multiple of 16, including status entry. */iq->size=roundup(iq->size,16);
From: Alexander Duyck <hidden> Date: 2017-06-12 21:28:16
On Mon, Jun 12, 2017 at 4:05 AM, Ding Tianhong [off-list ref] wrote:
quoted hunk
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 PCIe device didn't enable the relaxed ordering attribute default,
we should not do anything in the PCIe configuration, otherwise we
should check if any of the devices above us do not support relaxed
ordering by the PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag, then base on
the result if we get a return that indicate that the relaxed ordering
is not supported we should update our device to disable relaxed ordering
in configuration space. If the device above us doesn't exist or isn't
the PCIe device, we shouldn't do anything and skip updating relaxed ordering
because we are probably running in a guest machine.
Signed-off-by: Ding Tianhong <redacted>
---
drivers/pci/pci.c | 32 ++++++++++++++++++++++++++++++++
drivers/pci/probe.c | 41 +++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 2 ++
3 files changed, 75 insertions(+)
@@ -4878,6 +4878,38 @@ int pcie_set_mps(struct pci_dev *dev, int mps)EXPORT_SYMBOL(pcie_set_mps);/**+*pcie_clear_relaxed_ordering-clearPCIExpressrelaxedorderingbit+*@dev:PCIdevicetoquery+*+*Ifpossibleclearrelaxedordering+*/+intpcie_clear_relaxed_ordering(structpci_dev*dev)+{+returnpcie_capability_clear_word(dev,PCI_EXP_DEVCTL,+PCI_EXP_DEVCTL_RELAX_EN);+}+EXPORT_SYMBOL(pcie_clear_relaxed_ordering);++/**+*pcie_relaxed_ordering_supported-ProbeforPCIerelexedorderingsupport+*@dev:PCIdevicetoquery+*+*Returnstrueifthedevicesupportrelaxedorderingattribute.+*/+boolpcie_relaxed_ordering_supported(structpci_dev*dev)+{+boolro_supported=false;+u16v;++pcie_capability_read_word(dev,PCI_EXP_DEVCTL,&v);+if((v&PCI_EXP_DEVCTL_RELAX_EN)>>4)+ro_supported=true;
Instead of "return ro_supported" why not just "return !!(v &
PCIE_EXP_DEVCTL_RELAX_EN)"? You can cut out the extra steps and save
yourself some extra steps this way since the shift by 4 shouldn't even
really be needed since you are just testing for a bit anyway.
quoted hunk
++ return ro_supported;+}+EXPORT_SYMBOL(pcie_relaxed_ordering_supported);++/** * pcie_get_minimum_link - determine minimum link settings of a PCI device * @dev: PCI device to query * @speed: storage for minimum speed
Same thing here. I would suggest just returning either true or false,
and drop the ro_disabled value. It will return the lines of code and
make things a bit bit more direct.
The pci_is_pcie check is actually redundant based on the
pcie_relaxed_ordering_supported check using pcie_capability_read_word.
Also I am not sure what the point is of the pci_upstream_bridge()
check is, it seems like you should be able to catch all the same stuff
in your pci_dev_should_disable_relaxed_ordering() call. Though it did
give me a thought. I don't think we can alter this for a VF, so you
might want to add a check for dev->is_virtfn to the list of checks and
if it is a virtual function just return since I don't think there are
any VFs that would let you alter this bit anyway.
quoted hunk
+ /* If the releaxed ordering enable bit is not set, do nothing. */+ if (!pcie_relaxed_ordering_supported(dev))+ return;++ if (pci_dev_should_disable_relaxed_ordering(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;
On Mon, Jun 12, 2017 at 4:05 AM, Ding Tianhong [off-list ref] wrote:
...
quoted
/**+ * pcie_clear_relaxed_ordering - clear PCI Express relaxed 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_relaxed_ordering_supported - Probe for PCIe relexed ordering support+ * @dev: PCI device to query+ *+ * Returns true if the device support relaxed ordering attribute.+ */+bool pcie_relaxed_ordering_supported(struct pci_dev *dev)+{+ bool ro_supported = false;+ u16 v;++ pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v);+ if ((v & PCI_EXP_DEVCTL_RELAX_EN) >> 4)+ ro_supported = true;
Instead of "return ro_supported" why not just "return !!(v &
PCIE_EXP_DEVCTL_RELAX_EN)"? You can cut out the extra steps and save
yourself some extra steps this way since the shift by 4 shouldn't even
really be needed since you are just testing for a bit anyway.
OK.
quoted
++ return ro_supported;+}+EXPORT_SYMBOL(pcie_relaxed_ordering_supported);++/** * pcie_get_minimum_link - determine minimum link settings of a PCI device * @dev: PCI device to query * @speed: storage for minimum speed
Same thing here. I would suggest just returning either true or false,
and drop the ro_disabled value. It will return the lines of code and
make things a bit bit more direct.
Also I am not sure what the point is of the pci_upstream_bridge()
check is, it seems like you should be able to catch all the same stuff
in your pci_dev_should_disable_relaxed_ordering() call. Though it did
give me a thought. I don't think we can alter this for a VF, so you
might want to add a check for dev->is_virtfn to the list of checks and
if it is a virtual function just return since I don't think there are
any VFs that would let you alter this bit anyway.
If the upstream device is null, does it mean that it is in a guest OS device? maybe I miss something.
also I will check the dev->is_virtfn to avoid trying to change the configuration space for VF.
Another question: Because it looks like that maybe the Casey is too busy these days, should we
delay the modification of the cxgb4 and instead to update the ixgbe? what do you think about it. :)
Thanks.
Ding
quoted
+ /* If the releaxed ordering enable bit is not set, do nothing. */+ if (!pcie_relaxed_ordering_supported(dev))+ return;++ if (pci_dev_should_disable_relaxed_ordering(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;
From: Alexander Duyck <hidden> Date: 2017-06-16 14:39:33
On Thu, Jun 15, 2017 at 6:10 PM, Ding Tianhong [off-list ref] wrote:
On 2017/6/13 5:28, Alexander Duyck wrote:
quoted
On Mon, Jun 12, 2017 at 4:05 AM, Ding Tianhong [off-list ref] wrote:
...
quoted
quoted
/**+ * pcie_clear_relaxed_ordering - clear PCI Express relaxed 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_relaxed_ordering_supported - Probe for PCIe relexed ordering support+ * @dev: PCI device to query+ *+ * Returns true if the device support relaxed ordering attribute.+ */+bool pcie_relaxed_ordering_supported(struct pci_dev *dev)+{+ bool ro_supported = false;+ u16 v;++ pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v);+ if ((v & PCI_EXP_DEVCTL_RELAX_EN) >> 4)+ ro_supported = true;
Instead of "return ro_supported" why not just "return !!(v &
PCIE_EXP_DEVCTL_RELAX_EN)"? You can cut out the extra steps and save
yourself some extra steps this way since the shift by 4 shouldn't even
really be needed since you are just testing for a bit anyway.
OK.
quoted
quoted
++ return ro_supported;+}+EXPORT_SYMBOL(pcie_relaxed_ordering_supported);++/** * pcie_get_minimum_link - determine minimum link settings of a PCI device * @dev: PCI device to query * @speed: storage for minimum speed
Same thing here. I would suggest just returning either true or false,
and drop the ro_disabled value. It will return the lines of code and
make things a bit bit more direct.
Also I am not sure what the point is of the pci_upstream_bridge()
check is, it seems like you should be able to catch all the same stuff
in your pci_dev_should_disable_relaxed_ordering() call. Though it did
give me a thought. I don't think we can alter this for a VF, so you
might want to add a check for dev->is_virtfn to the list of checks and
if it is a virtual function just return since I don't think there are
any VFs that would let you alter this bit anyway.
If the upstream device is null, does it mean that it is in a guest OS device? maybe I miss something.
also I will check the dev->is_virtfn to avoid trying to change the configuration space for VF.
Yes, usually the upstream device is NULL in guest setups where all the
devices are hung off of a single PCI bus.
Another question: Because it looks like that maybe the Casey is too busy these days, should we
delay the modification of the cxgb4 and instead to update the ixgbe? what do you think about it. :)
I would still submit the cxgb4 changes with the one change we have
made. It should work as is. We can just leave any follow-up work to
Casey in terms of enabling the peer-to-peer mode if the bits related
to relaxed ordering are cleared.
Thanks.
Ding
quoted
quoted
+ /* If the releaxed ordering enable bit is not set, do nothing. */+ if (!pcie_relaxed_ordering_supported(dev))+ return;++ if (pci_dev_should_disable_relaxed_ordering(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;
On Thu, Jun 15, 2017 at 6:10 PM, Ding Tianhong [off-list ref] wrote:
quoted
On 2017/6/13 5:28, Alexander Duyck wrote:
quoted
On Mon, Jun 12, 2017 at 4:05 AM, Ding Tianhong [off-list ref] wrote:
...
quoted
quoted
/**+ * pcie_clear_relaxed_ordering - clear PCI Express relaxed 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_relaxed_ordering_supported - Probe for PCIe relexed ordering support+ * @dev: PCI device to query+ *+ * Returns true if the device support relaxed ordering attribute.+ */+bool pcie_relaxed_ordering_supported(struct pci_dev *dev)+{+ bool ro_supported = false;+ u16 v;++ pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v);+ if ((v & PCI_EXP_DEVCTL_RELAX_EN) >> 4)+ ro_supported = true;
Instead of "return ro_supported" why not just "return !!(v &
PCIE_EXP_DEVCTL_RELAX_EN)"? You can cut out the extra steps and save
yourself some extra steps this way since the shift by 4 shouldn't even
really be needed since you are just testing for a bit anyway.
OK.
quoted
quoted
++ return ro_supported;+}+EXPORT_SYMBOL(pcie_relaxed_ordering_supported);++/** * pcie_get_minimum_link - determine minimum link settings of a PCI device * @dev: PCI device to query * @speed: storage for minimum speed
Same thing here. I would suggest just returning either true or false,
and drop the ro_disabled value. It will return the lines of code and
make things a bit bit more direct.
Also I am not sure what the point is of the pci_upstream_bridge()
check is, it seems like you should be able to catch all the same stuff
in your pci_dev_should_disable_relaxed_ordering() call. Though it did
give me a thought. I don't think we can alter this for a VF, so you
might want to add a check for dev->is_virtfn to the list of checks and
if it is a virtual function just return since I don't think there are
any VFs that would let you alter this bit anyway.
If the upstream device is null, does it mean that it is in a guest OS device? maybe I miss something.
also I will check the dev->is_virtfn to avoid trying to change the configuration space for VF.
Yes, usually the upstream device is NULL in guest setups where all the
devices are hung off of a single PCI bus.
OK, no need for the pci_upstream_bridge, the pci_dev_should_disable_relaxed_ordering() will check and return result
as we need.
quoted
Another question: Because it looks like that maybe the Casey is too busy these days, should we
delay the modification of the cxgb4 and instead to update the ixgbe? what do you think about it. :)
I would still submit the cxgb4 changes with the one change we have
made. It should work as is. We can just leave any follow-up work to
Casey in terms of enabling the peer-to-peer mode if the bits related
to relaxed ordering are cleared.
OK, thanks.
quoted
Thanks.
Ding
quoted
quoted
+ /* If the releaxed ordering enable bit is not set, do nothing. */+ if (!pcie_relaxed_ordering_supported(dev))+ return;++ if (pci_dev_should_disable_relaxed_ordering(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;