pci: octeon: Remove udelay() causing huge IRQ latency
PATCH 1/1 pci: octeon: Remove udelay() causing huge IRQ latency
udelay() in PCI/PCIe read/write callbacks cause 30ms IRQ latency on Octeon platforms because these operations are called from PCI_OP_READ() and PCI_OP_WRITE() under raw_spin_lock_irqsave().
--- arch/mips/include/asm/octeon/pci-octeon.h | 3 --- arch/mips/pci/pci-octeon.c | 6 ------ arch/mips/pci/pcie-octeon.c | 8 -------- 3 files changed, 0 insertions(+), 17 deletions(-)
arch/mips/include/asm/octeon/pci-octeon.h +0 -3
@@ -11,9 +11,6 @@#include <linux/pci.h> -/* Some PCI cards require delays when accessing config space. */ -#define PCI_CONFIG_SPACE_DELAY 10000 -/* * The physical memory base mapped by BAR1. 256MB at the end of the * first 4GB.
arch/mips/pci/pci-octeon.c +0 -6
@@ -271,9 +271,6 @@ static int octeon_read_config(struct pci_bus *bus, unsigned int devfn, pci_addr.s.func = devfn &0x7 ; pci_addr.s.reg = reg; -#if PCI_CONFIG_SPACE_DELAY -udelay (PCI_CONFIG_SPACE_DELAY ); -#endif switch (size) {case 4 : *val =le32_to_cpu (cvmx_read64_uint32 (pci_addr.u64));
@@ -308,9 +305,6 @@ static int octeon_write_config(struct pci_bus *bus, unsigned int devfn, pci_addr.s.func = devfn &0x7 ; pci_addr.s.reg = reg; -#if PCI_CONFIG_SPACE_DELAY -udelay (PCI_CONFIG_SPACE_DELAY ); -#endif switch (size) {case 4 :cvmx_write64_uint32 (pci_addr.u64,cpu_to_le32 (val));
arch/mips/pci/pcie-octeon.c +0 -8
@@ -1762,14 +1762,6 @@ static int octeon_pcie_write_config(unsigned int pcie_port, struct pci_bus *bus,default :return PCIBIOS_FUNC_NOT_SUPPORTED ; } -#if PCI_CONFIG_SPACE_DELAY -/* -* Delay on writes so that devices have time to come up. Some -* bridges need this to allow time for the secondary busses to -* work -*/ -udelay (PCI_CONFIG_SPACE_DELAY ); -#endif return PCIBIOS_SUCCESSFUL ; }
Quoted 5 lines · Alexander Sverdlin
udelay()in PCI/PCIe read/write callbacks cause 30ms IRQ latency on Octeon platforms because these operations are called fromPCI_OP_READ()andPCI_OP_WRITE()underraw_spin_lock_irqsave().Signed-off-by Alexander Sverdlin <[email protected]>
I have no idea about the octeon platform, but how do you deal with the fact that is stated by the comment you are removing? I.e.
/* Some PCI cards require delays when accessing config space. */
Is that not the case any more for some reason? If not, it really needs to be explained in the changelog.
Signature · 2 lines
Jiri Kosina SUSE Labs
Hi!
Quoted 4 lines · ext Jiri Kosina
Quoted 4 lines
udelay()in PCI/PCIe read/write callbacks cause 30ms IRQ latency on OcteonQuoted 3 lines
platforms because these operations are called from
PCI_OP_READ()andPCI_OP_WRITE()underraw_spin_lock_irqsave().
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Signed-off-by Alexander Sverdlin <[email protected]>I have no idea about the octeon platform, but how do you deal with the fact that is stated by the comment you are removing? I.e.
/* Some PCI cards require delays when accessing config space. */Is that not the case any more for some reason? If not, it really needs to be explained in the changelog.
This udelay() should not have made its way upstream, someone overseen it during code review. This is simply not allowed to have delayed read/write in the current Linux PCI subsystem. And maybe there is no place for it at all.
Signature · 2 lines
Best regards, Alexander Sverdlin.
Quoted 5 lines · Alexander Sverdlin
udelay()in PCI/PCIe read/write callbacks cause 30ms IRQ latency on Octeon platforms because these operations are called fromPCI_OP_READ()andPCI_OP_WRITE()underraw_spin_lock_irqsave().Signed-off-by Alexander Sverdlin <[email protected]>
Can you say how it was tested. In principle I have no objections, but it would be nice to know how it was validated.
Thanks,
David Daney
Quoted 64 lines
--- arch/mips/include/asm/octeon/pci-octeon.h | 3 --- arch/mips/pci/pci-octeon.c | 6 ------ arch/mips/pci/pcie-octeon.c | 8 -------- 3 files changed, 0 insertions(+), 17 deletions(-)arch/mips/include/asm/octeon/pci-octeon.h +0
-3@@ -11,9 +11,6 @@#include <linux/pci.h> -/* Some PCI cards require delays when accessing config space. */ -#define PCI_CONFIG_SPACE_DELAY 10000 -/* * The physical memory base mapped by BAR1. 256MB at the end of the * first 4GB. arch/mips/pci/pci-octeon.c +0
-6@@ -271,9 +271,6 @@ static int octeon_read_config(struct pci_bus *bus, unsigned int devfn, pci_addr.s.func = devfn &0x7 ; pci_addr.s.reg = reg; -#if PCI_CONFIG_SPACE_DELAY -udelay (PCI_CONFIG_SPACE_DELAY ); -#endif switch (size) {case 4 : *val =le32_to_cpu (cvmx_read64_uint32 (pci_addr.u64));@@ -308,9 +305,6 @@ static int octeon_write_config(struct pci_bus *bus, unsigned int devfn, pci_addr.s.func = devfn &0x7 ; pci_addr.s.reg = reg; -#if PCI_CONFIG_SPACE_DELAY -udelay (PCI_CONFIG_SPACE_DELAY ); -#endif switch (size) {case 4 :cvmx_write64_uint32 (pci_addr.u64,cpu_to_le32 (val));arch/mips/pci/pcie-octeon.c +0
-8@@ -1762,14 +1762,6 @@ static int octeon_pcie_write_config(unsigned int pcie_port, struct pci_bus *bus,default :return PCIBIOS_FUNC_NOT_SUPPORTED ; } -#if PCI_CONFIG_SPACE_DELAY -/* -* Delay on writes so that devices have time to come up. Some -* bridges need this to allow time for the secondary busses to -* work -*/ -udelay (PCI_CONFIG_SPACE_DELAY ); -#endif return PCIBIOS_SUCCESSFUL ; }
Hello David,
Quoted 7 lines · ext David Daney
Quoted 5 lines
udelay()in PCI/PCIe read/write callbacks cause 30ms IRQ latency on Octeon platforms because these operations are called fromPCI_OP_READ()andPCI_OP_WRITE()underraw_spin_lock_irqsave().Signed-off-by Alexander Sverdlin <[email protected]>Can you say how it was tested. In principle I have no objections, but it would be nice to know how it was validated.
What do you want to know, how we've debugged IRQ latency and found the root cause or how we figured out that delay is not necessary? I'm pretty sure that there is HW which requires it. Maybe it's even Octeon itself... But putting udelay() in this callbacks is wrong wrong wrong.
Signature · 2 lines
Best regards, Alexander Sverdlin.
Hi,
Quoted 5 lines · Alexander Sverdlin
Quoted 4 lines · ext David Daney
Quoted 4 lines
udelay()in PCI/PCIe read/write callbacks cause 30ms IRQ latency on Octeon platforms because these operations are called fromPCI_OP_READ()andPCI_OP_WRITE()underraw_spin_lock_irqsave().
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Alexander Sverdlin <[email protected]>Can you say how it was tested. In principle I have no objections, but it would be nice to know how it was validated.
What do you want to know, how we've debugged IRQ latency and found the root cause or how we figured out that delay is not necessary?
You could at least say on which OCTEON platforms (e.g. OCTEON II or older) you tested the patch on, and tell about tested PCI bus topology and devices.
Quoted 3 lines
I'm pretty sure that there is HW which requires it. Maybe it's even Octeon itself... But putting
udelay()in this callbacks is wrong wrong wrong.
Still, you should not break HW that works with current kernel.
A.
Hi!
Quoted 3 lines · ext Aaro Koskinen
You could at least say on which OCTEON platforms (e.g. OCTEON II or older) you tested the patch on, and tell about tested PCI bus topology and devices.
Octeon II CN68xx with Marvell XGE switch connected to PCIe.
Signature · 2 lines
Best regards, Alexander Sverdlin.