[PATCH] pci: octeon: Remove udelay() causing huge IRQ latency

[PATCH] pci: octeon: Remove udelay() causing huge IRQ latency

[Alexander Sverdlin]

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().

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 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(-)

diff --git a/arch/mips/include/asm/octeon/pci-octeon.h b/arch/mips/include/asm/octeon/pci-octeon.h
index 64ba56a..1884609 100644
--- a/arch/mips/include/asm/octeon/pci-octeon.h
+++ b/arch/mips/include/asm/octeon/pci-octeon.h
@@ -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.
diff --git a/arch/mips/pci/pci-octeon.c b/arch/mips/pci/pci-octeon.c
index a04af55..01c604a 100644
--- a/arch/mips/pci/pci-octeon.c
+++ b/arch/mips/pci/pci-octeon.c
@@ -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));
diff --git a/arch/mips/pci/pcie-octeon.c b/arch/mips/pci/pcie-octeon.c
index 1bb0b2b..99f3db4 100644
--- a/arch/mips/pci/pcie-octeon.c
+++ b/arch/mips/pci/pcie-octeon.c
@@ -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;
 }
 

Re: [PATCH] pci: octeon: Remove udelay() causing huge IRQ latency

[Jiri Kosina]

On Wed, 18 Mar 2015, Alexander Sverdlin wrote:

> 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().
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

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.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] pci: octeon: Remove udelay() causing huge IRQ latency

[Alexander Sverdlin]

Hi!

On 18/03/15 14:11, ext Jiri Kosina wrote:
>> 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().
>> > 
>> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 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.

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] pci: octeon: Remove udelay() causing huge IRQ latency

[David Daney]

On 03/18/2015 06:05 AM, Alexander Sverdlin wrote:
> 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().
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

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


> ---
>   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(-)
>
> diff --git a/arch/mips/include/asm/octeon/pci-octeon.h b/arch/mips/include/asm/octeon/pci-octeon.h
> index 64ba56a..1884609 100644
> --- a/arch/mips/include/asm/octeon/pci-octeon.h
> +++ b/arch/mips/include/asm/octeon/pci-octeon.h
> @@ -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.
> diff --git a/arch/mips/pci/pci-octeon.c b/arch/mips/pci/pci-octeon.c
> index a04af55..01c604a 100644
> --- a/arch/mips/pci/pci-octeon.c
> +++ b/arch/mips/pci/pci-octeon.c
> @@ -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));
> diff --git a/arch/mips/pci/pcie-octeon.c b/arch/mips/pci/pcie-octeon.c
> index 1bb0b2b..99f3db4 100644
> --- a/arch/mips/pci/pcie-octeon.c
> +++ b/arch/mips/pci/pcie-octeon.c
> @@ -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;
>   }
>
>

Re: [PATCH] pci: octeon: Remove udelay() causing huge IRQ latency

[David Daney]

On 03/18/2015 06:05 AM, Alexander Sverdlin wrote:
> 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().
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

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


> ---
>   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(-)
>
> diff --git a/arch/mips/include/asm/octeon/pci-octeon.h b/arch/mips/include/asm/octeon/pci-octeon.h
> index 64ba56a..1884609 100644
> --- a/arch/mips/include/asm/octeon/pci-octeon.h
> +++ b/arch/mips/include/asm/octeon/pci-octeon.h
> @@ -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.
> diff --git a/arch/mips/pci/pci-octeon.c b/arch/mips/pci/pci-octeon.c
> index a04af55..01c604a 100644
> --- a/arch/mips/pci/pci-octeon.c
> +++ b/arch/mips/pci/pci-octeon.c
> @@ -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));
> diff --git a/arch/mips/pci/pcie-octeon.c b/arch/mips/pci/pcie-octeon.c
> index 1bb0b2b..99f3db4 100644
> --- a/arch/mips/pci/pcie-octeon.c
> +++ b/arch/mips/pci/pcie-octeon.c
> @@ -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;
>   }
>
>

Re: [PATCH] pci: octeon: Remove udelay() causing huge IRQ latency

[Alexander Sverdlin]

Hello David,

On 18/03/15 17:11, ext David Daney wrote:
>> 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().
>>
>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> 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. 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] pci: octeon: Remove udelay() causing huge IRQ latency

[Aaro Koskinen]

Hi,

On Wed, Mar 18, 2015 at 05:17:04PM +0100, Alexander Sverdlin wrote:
> On 18/03/15 17:11, ext David Daney wrote:
> >> 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().
> >>
> >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> > 
> > 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.

> 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.

Re: [PATCH] pci: octeon: Remove udelay() causing huge IRQ latency

[Alexander Sverdlin]

Hi!

On 18/03/15 19:06, ext Aaro Koskinen wrote:
> 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.

-- 
Best regards,
Alexander Sverdlin.