PCI: Workaround wrong flags completions for IDT switch
PATCH v4 1/1 PCI: Workaround wrong flags completions for IDT switch
From: James Puthukattukaran <[email protected]>
The IDT switch incorrectly flags an ACS source violation on a read config request to an end point device on the completion (IDT 89H32H8G3-YC, errata #36) even though the PCI Express spec states that completions are never affected by ACS source violation (PCI Spec 3.1, Section 6.12.1.1).
The suggested workaround by IDT is to issue a configuration write to the downstream device before issuing the first config read. This allows the downstream device to capture its bus number, thus avoiding the ACS violation on the completion.
The patch does the following -
1. Disable ACS source violation if enabled
2. Wait for config space access to become available by reading vendor id
3. Do a config write to the end point (errata workaround)
4. Enable ACS source validation (if it was enabled to begin with)
-v2: move workaround to pci_bus_read_dev_vendor_id() from pci_bus_check_dev()
and move enable_acs_sv to drivers/pci/pci.c -- by Yinghai
-v3: add bus->self check for root bus and virtual bus for sriov vfs. -v4: only do workaround for IDT switches
--
drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ drivers/pci/pci.h | 1 + drivers/pci/probe.c | 38 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 70 insertions(+), 2 deletions(-)
drivers/pci/pci.c +33 -0
@@ -2835,6 +2835,39 @@ static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) }/** +* pci_std_enable_acs_sv - enable/disable ACS source validation if supported by the switch +* @dev - pcie switch/RP +* @enable - enable (1) or disable (0) source validation +* +* Returns : < 0 on failure +* previous acs_sv state +*/ +int pci_std_enable_acs_sv (struct pci_dev *dev,bool enable) +{ +int pos; +u16 cap; +u16 ctrl; +int retval; + + pos =pci_find_ext_capability (dev,PCI_EXT_CAP_ID_ACS ); +if (!pos) +return -ENODEV ; + +pci_read_config_word (dev, pos +PCI_ACS_CAP , &cap); +pci_read_config_word (dev, pos +PCI_ACS_CTRL , &ctrl); + + retval = !!(ctrl & cap &PCI_ACS_SV ); +if (enable) + ctrl |= (cap &PCI_ACS_SV ); +else + ctrl &= ~(cap &PCI_ACS_SV ); + +pci_write_config_word (dev, pos +PCI_ACS_CTRL , ctrl); + +return retval; +} + +/** * pci_acs_enabled - test ACS against required flags for a given device * @pdev: device to test * @acs_flags: required PCI ACS flags
drivers/pci/pci.h +1 -0
@@ -343,6 +343,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev, }void pci_enable_acs (struct pci_dev *dev); +int pci_std_enable_acs_sv (struct pci_dev *dev,bool enable);#ifdef CONFIG_PCIE_PTM void pci_ptm_init (struct pci_dev *dev);
drivers/pci/probe.c +36 -2
@@ -1763,8 +1763,8 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus) }EXPORT_SYMBOL (pci_alloc_dev); -bool pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn,u32 *l, -int crs_timeout) +static bool __pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn, +u32 *l,int crs_timeout) {int delay =1 ;
@@ -1801,6 +1801,40 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,return true; } + +bool pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn,u32 *l, +int crs_timeout) +{ +int found; +int enable =-1 ; +int idt_workaround = (bus->self && (bus->self->vendor ==PCI_VENDOR_ID_IDT )); +/* +* Some IDT switches flag an ACS violation for config reads +* even though the PCI spec allows for it (PCIe 3.1, 6.1.12.1) +* It flags it because the bus number is not properly set in the +* completion. The workaround is to do a dummy write to properly +* latch number once the device is ready for config operations +*/ + +if (idt_workaround) + enable =pci_std_enable_acs_sv (bus->self, false); + + found =__pci_bus_read_dev_vendor_id (bus, devfn, l, crs_timeout); + +/* +* The fact that we can read the vendor id indicates that the device +* is ready for config operations. Do the write as part of the errata +* workaround. +*/ +if (idt_workaround) { +if (found) +pci_bus_write_config_word (bus, devfn,PCI_VENDOR_ID ,0 ); +if (enable >0 ) +pci_std_enable_acs_sv (bus->self, enable); + } + +return found; +}EXPORT_SYMBOL (pci_bus_read_dev_vendor_id);
James,
Quoted 45 lines · James Puthukattukaran
From: James Puthukattukaran <[email protected]>
The IDT switch incorrectly flags an ACS source violation on a read config request to an end point device on the completion (IDT 89H32H8G3-YC, errata #36) even though the PCI Express spec states that completions are never affected by ACS source violation (PCI Spec 3.1, Section 6.12.1.1).
The suggested workaround by IDT is to issue a configuration write to the downstream device before issuing the first config read. This allows the downstream device to capture its bus number, thus avoiding the ACS violation on the completion.
The patch does the following -
1. Disable ACS source violation if enabled
2. Wait for config space access to become available by reading vendor id
3. Do a config write to the end point (errata workaround)
4. Enable ACS source validation (if it was enabled to begin with)-v2: move workaround to
pci_bus_read_dev_vendor_id()frompci_bus_check_dev()
and moveenable_acs_svtodrivers/pci/pci.c-- by Yinghai(quoted diff) +0
-2-v3: add bus->self check for root bus and virtual bus for sriov vfs. -v4: only do workaround for IDT switchesSigned-off-by James Puthukattukaran <[email protected]>Signed-off-by Yinghai Lu <[email protected]>--
drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ drivers/pci/pci.h | 1 + drivers/pci/probe.c | 38 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 70 insertions(+), 2 deletions(-)drivers/pci/pci.c +1
-0@@ -2835,6 +2835,39 @@ static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) }/** +* pci_std_enable_acs_sv - enable/disable ACS source validation if
Quoted 5 lines
supported by the switch
+ * @dev - pcie switch/RP
+ * @enable - enable (1) or disable (0) source validation
+ *
+ * Returns : < 0 on failure
You didn't define the meaning of 0 and >0, but you check it later against >0, Then what does it mean 0 and >0 ?
Quoted 7 lines
+ * previous
acs_svstate
+ */drivers/pci/pci.c +5
-0+int pci_std_enable_acs_sv (struct pci_dev *dev,bool enable) +{ +int pos; +u16 cap; +u16 ctrl;
drivers/pci/pci.c static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) · line 2859
Quoted 10 lines
drivers/pci/pci.c +10
-0+int retval; + + pos =pci_find_ext_capability (dev,PCI_EXT_CAP_ID_ACS ); +if (!pos) +return -ENODEV ; + +pci_read_config_word (dev, pos +PCI_ACS_CAP , &cap); +pci_read_config_word (dev, pos +PCI_ACS_CTRL , &ctrl); + + retval = !!(ctrl & cap &PCI_ACS_SV );
If the device's ACS SV( ACS Source Validation) capability wasn't implemented, the return value of this function will still tell us the operation of enabling is successful ? though it might be rare case.
Thanks,
Ethan
Quoted 86 lines
drivers/pci/pci.c +11
-0+if (enable) + ctrl |= (cap &PCI_ACS_SV ); +else + ctrl &= ~(cap &PCI_ACS_SV ); + +pci_write_config_word (dev, pos +PCI_ACS_CTRL , ctrl); + +return retval; +} + +/** * pci_acs_enabled - test ACS against required flags for a given device * @pdev: device to test * @acs_flags: required PCI ACS flags drivers/pci/pci.h +1
-0@@ -343,6 +343,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev, } void pci_enable_acs(struct pci_dev *dev); +int pci_std_enable_acs_sv (struct pci_dev *dev,bool enable);#ifdef CONFIG_PCIE_PTM void
pci_ptm_init(struct pci_dev *dev);drivers/pci/probe.c +25
-2@@ -1763,8 +1763,8 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus) }EXPORT_SYMBOL (pci_alloc_dev); -bool pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn,u32 *l, -int crs_timeout) +static bool __pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn, +u32 *l,int crs_timeout) {int delay =1 ;@@ -1801,6 +1801,40 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, return true; } + +bool pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn,u32 *l, +int crs_timeout) +{ +int found; +int enable =-1 ; +int idt_workaround = (bus->self && (bus->self->vendor ==PCI_VENDOR_ID_IDT )); +/* +* Some IDT switches flag an ACS violation for config reads +* even though the PCI spec allows for it (PCIe 3.1, 6.1.12.1) +* It flags it because the bus number is not properly set in the +* completion. The workaround is to do a dummy write to properly +* latch number once the device is ready for config operations +*/ + +if (idt_workaround) + enable =pci_std_enable_acs_sv (bus->self, false); + + found =__pci_bus_read_dev_vendor_id (bus, devfn, l, crs_timeout); + +/* +* The fact that we can read the vendor id indicates that the device +* is ready for config operations. Do the write as part of the errata
drivers/pci/probe.c +5
-0+* workaround. +*/ +if (idt_workaround) { +if (found) + pci_bus_write_config_word(bus, devfn, PCI_VENDOR_ID,0);
drivers/pci/probe.c +6
-0+if (enable >0 ) +pci_std_enable_acs_sv (bus->self, enable); + } + +return found; +}EXPORT_SYMBOL (pci_bus_read_dev_vendor_id);
Ethan -
Quoted 47 lines · Ethan Zhao
James,
Quoted 43 lines · James Puthukattukaran
From: James Puthukattukaran <[email protected]>
The IDT switch incorrectly flags an ACS source violation on a read config request to an end point device on the completion (IDT 89H32H8G3-YC, errata #36) even though the PCI Express spec states that completions are never affected by ACS source violation (PCI Spec 3.1, Section 6.12.1.1).
The suggested workaround by IDT is to issue a configuration write to the downstream device before issuing the first config read. This allows the downstream device to capture its bus number, thus avoiding the ACS violation on the completion.
The patch does the following -
1. Disable ACS source violation if enabled
2. Wait for config space access to become available by reading vendor id
3. Do a config write to the end point (errata workaround)
4. Enable ACS source validation (if it was enabled to begin with)-v2: move workaround to
pci_bus_read_dev_vendor_id()frompci_bus_check_dev()
and moveenable_acs_svtodrivers/pci/pci.c-- by Yinghai(quoted diff) +0
-2-v3: add bus->self check for root bus and virtual bus for sriov vfs. -v4: only do workaround for IDT switchesSigned-off-by James Puthukattukaran <[email protected]>Signed-off-by Yinghai Lu <[email protected]>--
drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ drivers/pci/pci.h | 1 + drivers/pci/probe.c | 38 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 70 insertions(+), 2 deletions(-)drivers/pci/pci.c +0
-0@@ -2835,6 +2835,39 @@ static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) }
Quoted 9 lines
Quoted 7 lines
drivers/pci/pci.c +1
-0/** +* pci_std_enable_acs_sv - enable/disable ACS source validation if supported by the switch
+ * @dev - pcie switch/RP
+ * @enable - enable (1) or disable (0) source validation
+ *
+ * Returns : < 0 on failureYou didn't define the meaning of 0 and >0, but you check it later against >0, Then what does it mean 0 and >0 ?
see below..
Quoted 2 lines
Quoted 2 lines
+ * previous
acs_svstate
It returns the previous acs_sv state (0 or 1).
Quoted 11 lines
Quoted 11 lines
+ */
drivers/pci/pci.c +10
-0+int pci_std_enable_acs_sv (struct pci_dev *dev,bool enable) +{ +int pos; +u16 cap; +u16 ctrl; +int retval; + + pos =pci_find_ext_capability (dev,PCI_EXT_CAP_ID_ACS ); +if (!pos) +return -ENODEV ;
Quoted 8 lines
drivers/pci/pci.c static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) · line 2859
Quoted 5 lines
drivers/pci/pci.c +5
-0+ +pci_read_config_word (dev, pos +PCI_ACS_CAP , &cap); +pci_read_config_word (dev, pos +PCI_ACS_CTRL , &ctrl); + + retval = !!(ctrl & cap &PCI_ACS_SV );If the device's ACS SV( ACS Source Validation) capability wasn't implemented, the return value of this function will still tell us the operation of enabling is successful ? though it might be rare case.
If the ACS capability is implemented, then all bits are expected to have meaning and are valid. If SV is not implemented by the switch, the control bit for it should return zero (no source validation done). This is the PCI specification. The onus is on the switch designer to keep it so.
thanks,
James
Quoted 86 lines
Quoted 86 lines
drivers/pci/pci.c +11
-0+if (enable) + ctrl |= (cap &PCI_ACS_SV ); +else + ctrl &= ~(cap &PCI_ACS_SV ); + +pci_write_config_word (dev, pos +PCI_ACS_CTRL , ctrl); + +return retval; +} + +/** * pci_acs_enabled - test ACS against required flags for a given device * @pdev: device to test * @acs_flags: required PCI ACS flags drivers/pci/pci.h +1
-0@@ -343,6 +343,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev, } void pci_enable_acs(struct pci_dev *dev); +int pci_std_enable_acs_sv (struct pci_dev *dev,bool enable);#ifdef CONFIG_PCIE_PTM void pci_ptm_init(struct pci_dev *dev);drivers/pci/probe.c +25
-2@@ -1763,8 +1763,8 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus) }EXPORT_SYMBOL (pci_alloc_dev); -bool pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn,u32 *l, -int crs_timeout) +static bool __pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn, +u32 *l,int crs_timeout) {int delay =1 ;@@ -1801,6 +1801,40 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, return true; } + +bool pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn,u32 *l, +int crs_timeout) +{ +int found; +int enable =-1 ; +int idt_workaround = (bus->self && (bus->self->vendor ==PCI_VENDOR_ID_IDT )); +/* +* Some IDT switches flag an ACS violation for config reads +* even though the PCI spec allows for it (PCIe 3.1, 6.1.12.1) +* It flags it because the bus number is not properly set in the +* completion. The workaround is to do a dummy write to properly +* latch number once the device is ready for config operations +*/ + +if (idt_workaround) + enable =pci_std_enable_acs_sv (bus->self, false); + + found =__pci_bus_read_dev_vendor_id (bus, devfn, l, crs_timeout); + +/* +* The fact that we can read the vendor id indicates that the device +* is ready for config operations. Do the write as part of the errata
drivers/pci/probe.c +5
-0+* workaround. +*/ +if (idt_workaround) { +if (found) + pci_bus_write_config_word(bus, devfn, PCI_VENDOR_ID,0);
drivers/pci/probe.c +6
-0+if (enable >0 ) +pci_std_enable_acs_sv (bus->self, enable); + } + +return found; +}EXPORT_SYMBOL (pci_bus_read_dev_vendor_id);
James,
Quoted 14 lines · james puthukattukaran
Ethan -
Quoted 9 lines · Ethan Zhao
James,
Quoted 4 lines · James Puthukattukaran
From: James Puthukattukaran <[email protected]>
The IDT switch incorrectly flags an ACS source violation on a read conf=
ig
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
request to an end point device on the completion (IDT 89H32H8G3-YC, errata #36) even though the PCI Express spec states that completions ar=
e
Quoted 1 line
Quoted 1 line
Quoted 1 line
never affected by ACS source violation (PCI Spec 3.1, Section 6.12.1.1)=
.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
The suggested workaround by IDT is to issue a configuration write to th=
e
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
downstream device before issuing the first config read. This allows the downstream device to capture its bus number, thus avoiding the ACS violation on the completion.
The patch does the following -
1. Disable ACS source violation if enabled
2. Wait for config space access to become available by reading vendor i=
d
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
3. Do a config write to the end point (errata workaround)
4. Enable ACS source validation (if it was enabled to begin with)-v2: move workaround to
pci_bus_read_dev_vendor_id()frompci_bus_check_dev()
and moveenable_acs_svtodrivers/pci/pci.c-- by Yinghai(quoted diff) +0
-2-v3: add bus->self check for root bus and virtual bus for sriov vfs. -v4: only do workaround for IDT switchesSigned-off-by James Puthukattukaran <[email protected]>Signed-off-by Yinghai Lu <[email protected]>--
drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ drivers/pci/pci.h | 1 + drivers/pci/probe.c | 38 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 70 insertions(+), 2 deletions(-)drivers/pci/pci.c +0
-0@@ -2835,6 +2835,39 @@ static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) }
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
drivers/pci/pci.c +1
-0/** +* pci_std_enable_acs_sv - enable/disable ACS source validation if supported by the switch
+ * @dev - pcie switch/RP
+ * @enable - enable (1) or disable (0) source validation
+ *
+ * Returns : < 0 on failureYou didn't define the meaning of 0 and >0, but you check it later agains=
t
Quoted 10 lines
Quoted 2 lines
Quoted 1 line
0,
Then what does it mean 0 and >0 ?
see below..
Quoted 3 lines
Quoted 3 lines
+ * previous
acs_svstateIt returns the previous
acs_svstate (0 or 1).
You didn't clarify the meaning of previous acs_sv state, or possible value, you check it later with >0 also confused the possibility.
Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
+ */
drivers/pci/pci.c +11
-0+int pci_std_enable_acs_sv (struct pci_dev *dev,bool enable) +{ +int pos; +u16 cap; +u16 ctrl; +int retval; + + pos =3D pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS); +if (!pos) +return -ENODEV ; +Quoted 8 lines
drivers/pci/pci.c static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
Quoted 4 lines
drivers/pci/pci.c +4
-0+pci_read_config_word (dev, pos +PCI_ACS_CAP , &cap); +pci_read_config_word (dev, pos +PCI_ACS_CTRL , &ctrl); + + retval =3D !!(ctrl & cap & PCI_ACS_SV);If the device's ACS SV( ACS Source Validation) capability wasn't implemented, the return value of this function will still tell us the operation of enabling is successful ? though it might be rare case.
Quoted 2 lines
If the ACS capability is implemented, then all bits are expected to have meaning and are valid. If SV is not implemented by the switch, the contro=
l
Quoted 1 line
bit for it should return zero (no source validation done). This is the PC=
I
Quoted 1 line
specification. The onus is on the switch designer to keep it so.
PCI spec doesn't say SV must be implemented in every device even it has ACS Cap, see also:
"6.12.1.2. ACS Functions in Multi-Function Devices This section applies to multi-Function device ACS Functions, with the exception of Downstream Port Functions, which are covered in the preceding section. =EF=81=B1 ACS Source Validation: must not be implemented. 20 =EF=81=B1 ACS = Translation
Blocking: must not be implemented. =EF=81=B1 ACS P2P Request Redirect: must=
be
implemented by Functions that support peer-to-peer traffic with other Functions.
"
Here pci_std_enable_acs_sv() is common function, once implemented, possible be used by other code to enable acs beside this workaround.
Then how about it is called with a MF device ?
Thanks,
Ethan
Quoted 5 lines
thanks,
James
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
drivers/pci/pci.c +4
-0+if (enable) + ctrl |=3D (cap &PCI_ACS_SV ); +else + ctrl &=3D ~(cap & PCI_ACS_SV);Quoted 8 lines
drivers/pci/pci.c static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
Quoted 8 lines
drivers/pci/pci.c +7
-0+ +pci_write_config_word (dev, pos +PCI_ACS_CTRL , ctrl); + +return retval; +} + +/** * pci_acs_enabled - test ACS against required flags for a given devi=
ce
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
* @pdev: device to test
* @acs_flags: required PCI ACS flagsdrivers/pci/pci.h +1
-0@@ -343,6 +343,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev, } void pci_enable_acs(struct pci_dev *dev); +int pci_std_enable_acs_sv (struct pci_dev *dev,bool enable);
Quoted 7 lines
Quoted 7 lines
drivers/pci/probe.c struct pci_dev *pci_alloc_dev(struct pci_bus *bus=
Quoted 7 lines
drivers/pci/pci.h +0
-0#ifdef CONFIG_PCIE_PTM void pci_ptm_init(struct pci_dev *dev);drivers/pci/probe.c +0
-0@@ -1763,8 +1763,8 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus=
)
Quoted 4 lines
Quoted 4 lines
drivers/pci/probe.c struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
Quoted 4 lines
drivers/pci/probe.c +0
-1}EXPORT_SYMBOL (pci_alloc_dev); -bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l=
,
Quoted 2 lines
Quoted 2 lines
drivers/pci/probe.c struct pci_dev *pci_alloc_dev(struct pci_bus *bus= · line 1766
Quoted 2 lines
drivers/pci/probe.c +1
-1- int crs_timeout) +static bool __pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devf=
n,
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/pci/probe.c +1
-0+ u32 *l, int crs_timeout) { int delay =3D 1;
Quoted 8 lines
Quoted 8 lines
drivers/pci/probe.c bool pci_bus_read_dev_vendor_id(struct pci_bus · line 1807
Quoted 8 lines
drivers/pci/probe.c +2
-0@@ -1801,6 +1801,40 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, return true; } + +bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l=
,
Quoted 10 lines
Quoted 10 lines
drivers/pci/probe.c bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
Quoted 10 lines
drivers/pci/probe.c +9
-0+ int crs_timeout) +{ +int found; +int enable =3D -1 ; + int idt_workaround =3D (bus->self && (bus->self->vendor =3D=3D PCI_VENDOR_ID_IDT)); +/* +* Some IDT switches flag an ACS violation for config reads +* even though the PCI spec allows for it (PCIe 3.1, 6.1.12.1) +* It flags it because the bus number is not properly set in th=
e
Quoted 1 line
Quoted 1 line
drivers/pci/probe.c bool pci_bus_read_dev_vendor_id(struct pci_bus · line 1818
Quoted 1 line
drivers/pci/probe.c +1
-0+* completion. The workaround is to do a dummy write to properl=
y
Quoted 7 lines
Quoted 7 lines
drivers/pci/probe.c bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
Quoted 7 lines
drivers/pci/probe.c +7
-0+* latch number once the device is ready for config operations +*/ + +if (idt_workaround) + enable =3D pci_std_enable_acs_sv(bus->self, false); + + found =3D __pci_bus_read_dev_vendor_id(bus, devfn, l, crs_timeo=
ut);
Quoted 22 lines
Quoted 21 lines
Quoted 21 lines
drivers/pci/probe.c +3
-0+ +/* +* The fact that we can read the vendor id indicates that the device
+ * is ready for config operations. Do the write as part of the
erratadrivers/pci/probe.c +5
-0+* workaround. +*/ +if (idt_workaround) { +if (found) + pci_bus_write_config_word(bus, devfn,
PCI_VENDOR_ID,
0);drivers/pci/probe.c +6
-0+if (enable >0 ) +pci_std_enable_acs_sv (bus->self, enable); + } + +return found; +}EXPORT_SYMBOL (pci_bus_read_dev_vendor_id);
Quoted 124 lines · Ethan Zhao
James,
Quoted 66 lines · james puthukattukaran
Ethan -
Quoted 58 lines · Ethan Zhao
James,
Quoted 46 lines · James Puthukattukaran
From: James Puthukattukaran <[email protected]>
The IDT switch incorrectly flags an ACS source violation on a read config request to an end point device on the completion (IDT 89H32H8G3-YC, errata #36) even though the PCI Express spec states that completions are never affected by ACS source violation (PCI Spec 3.1, Section 6.12.1.1).
The suggested workaround by IDT is to issue a configuration write to the downstream device before issuing the first config read. This allows the downstream device to capture its bus number, thus avoiding the ACS violation on the completion.
The patch does the following -
1. Disable ACS source violation if enabled
2. Wait for config space access to become available by reading vendor id
3. Do a config write to the end point (errata workaround)
4. Enable ACS source validation (if it was enabled to begin with)-v2: move workaround to
pci_bus_read_dev_vendor_id()frompci_bus_check_dev()
and moveenable_acs_svtodrivers/pci/pci.c-- by Yinghai(quoted diff) +0
-2-v3: add bus->self check for root bus and virtual bus for sriov vfs. -v4: only do workaround for IDT switchesSigned-off-by James Puthukattukaran <[email protected]>Signed-off-by Yinghai Lu <[email protected]>--
drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ drivers/pci/pci.h | 1 + drivers/pci/probe.c | 38 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 70 insertions(+), 2 deletions(-)drivers/pci/pci.c +1
-0@@ -2835,6 +2835,39 @@ static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) }/** +* pci_std_enable_acs_sv - enable/disable ACS source validation if Quoted 5 lines
supported by the switch
+ * @dev - pcie switch/RP
+ * @enable - enable (1) or disable (0) source validation
+ *
+ * Returns : < 0 on failureYou didn't define the meaning of 0 and >0, but you check it later against
Quoted 2 lines
Quoted 1 line
0,
Then what does it mean 0 and >0 ?
Quoted 7 lines
see below..
Quoted 3 lines
Quoted 3 lines
+ * previous
acs_svstateIt returns the previous
acs_svstate (0 or 1).You didn't clarify the meaning of previous
acs_svstate, or possible value, you check it later with >0 also confused the possibility.Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
+ */
drivers/pci/pci.c +11
-0+int pci_std_enable_acs_sv (struct pci_dev *dev,bool enable) +{ +int pos; +u16 cap; +u16 ctrl; +int retval; + + pos =pci_find_ext_capability (dev,PCI_EXT_CAP_ID_ACS ); +if (!pos) +return -ENODEV ; +Quoted 8 lines
drivers/pci/pci.c static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags) · line 2859
Quoted 4 lines
drivers/pci/pci.c +4
-0+pci_read_config_word (dev, pos +PCI_ACS_CAP , &cap); +pci_read_config_word (dev, pos +PCI_ACS_CTRL , &ctrl); + + retval = !!(ctrl & cap &PCI_ACS_SV );If the device's ACS SV( ACS Source Validation) capability wasn't implemented, the return value of this function will still tell us the operation of enabling is successful ? though it might be rare case.
Quoted 4 lines
If the ACS capability is implemented, then all bits are expected to have meaning and are valid. If SV is not implemented by the switch, the control bit for it should return zero (no source validation done). This is the PCI specification. The onus is on the switch designer to keep it so.
PCI spec doesn't say SV must be implemented in every device even it has ACS Cap, see also:
"6.12.1.2. ACS Functions in Multi-Function Devices This section applies to multi-Function device ACS Functions, with the exception of Downstream Port Functions, which are covered in the preceding section. ACS Source Validation: must not be implemented. 20 ACS Translation
Blocking: must not be implemented. ACS P2P Request Redirect: must be implemented by Functions that support peer-to-peer traffic with other Functions.
"
Herepci_std_enable_acs_sv()is common function, once implemented, possible be used by other code to enable acs beside this workaround.
Quoted 1 line
Then how about it is called with a MF device ?
I will add code to check if SV is implemented and return failure (< 0) in case it is not. thanks
James
Quoted 99 lines
Thanks,
EthanQuoted 95 lines
thanks,
JamesQuoted 89 lines
Quoted 89 lines
drivers/pci/pci.c +11
-0+if (enable) + ctrl |= (cap &PCI_ACS_SV ); +else + ctrl &= ~(cap &PCI_ACS_SV ); + +pci_write_config_word (dev, pos +PCI_ACS_CTRL , ctrl); + +return retval; +} + +/** * pci_acs_enabled - test ACS against required flags for a given device * @pdev: device to test * @acs_flags: required PCI ACS flags drivers/pci/pci.h +1
-0@@ -343,6 +343,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev, } void pci_enable_acs(struct pci_dev *dev); +int pci_std_enable_acs_sv (struct pci_dev *dev,bool enable);#ifdef CONFIG_PCIE_PTM void pci_ptm_init(struct pci_dev *dev);drivers/pci/probe.c +24
-2@@ -1763,8 +1763,8 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus) }EXPORT_SYMBOL (pci_alloc_dev); -bool pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn,u32 *l, -int crs_timeout) +static bool __pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn, +u32 *l,int crs_timeout) {int delay =1 ;@@ -1801,6 +1801,40 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, return true; } + +bool pci_bus_read_dev_vendor_id (struct pci_bus *bus,int devfn,u32 *l, +int crs_timeout) +{ +int found; +int enable =-1 ; +int idt_workaround = (bus->self && (bus->self->vendor ==PCI_VENDOR_ID_IDT )); +/* +* Some IDT switches flag an ACS violation for config reads +* even though the PCI spec allows for it (PCIe 3.1, 6.1.12.1) +* It flags it because the bus number is not properly set in the +* completion. The workaround is to do a dummy write to properly +* latch number once the device is ready for config operations +*/ + +if (idt_workaround) + enable =pci_std_enable_acs_sv (bus->self, false); + + found =__pci_bus_read_dev_vendor_id (bus, devfn, l, crs_timeout); + +/* +* The fact that we can read the vendor id indicates that the device
+ * is ready for config operations. Do the write as part of the
erratadrivers/pci/probe.c +5
-0+* workaround. +*/ +if (idt_workaround) { +if (found) + pci_bus_write_config_word(bus, devfn,
PCI_VENDOR_ID,
0);drivers/pci/probe.c +6
-0+if (enable >0 ) +pci_std_enable_acs_sv (bus->self, enable); + } + +return found; +}EXPORT_SYMBOL (pci_bus_read_dev_vendor_id);