linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] PCI: aardvark: Don't rely on jiffies while holding spinlock
@ 2019-09-27  8:31 Remi Pommarel
  2019-09-27  8:34 ` Thomas Petazzoni
  0 siblings, 1 reply; 3+ messages in thread
From: Remi Pommarel @ 2019-09-27  8:31 UTC (permalink / raw)
  To: Thomas Petazzoni, Lorenzo Pieralisi, Bjorn Helgaas
  Cc: linux-pci, linux-arm-kernel, linux-kernel, Remi Pommarel

advk_pcie_wait_pio() can be called while holding a spinlock (from
pci_bus_read_config_dword()), then depends on jiffies in order to
timeout while polling on PIO state registers. In the case the PIO
transaction failed, the timeout will never happen and will also cause
the cpu to stall.

This decrements a variable and wait instead of using jiffies.

Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
Changes since v1:
  - Reduce polling delay
  - Change size_t into int for loop counter
---
 drivers/pci/controller/pci-aardvark.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index fc0fe4d4de49..ee05ccb2b686 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -175,7 +175,8 @@
 	(PCIE_CONF_BUS(bus) | PCIE_CONF_DEV(PCI_SLOT(devfn))	| \
 	 PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where))
 
-#define PIO_TIMEOUT_MS			1
+#define PIO_RETRY_CNT			10
+#define PIO_RETRY_DELAY			2 /* 2 us*/
 
 #define LINK_WAIT_MAX_RETRIES		10
 #define LINK_WAIT_USLEEP_MIN		90000
@@ -383,17 +384,16 @@ static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
 static int advk_pcie_wait_pio(struct advk_pcie *pcie)
 {
 	struct device *dev = &pcie->pdev->dev;
-	unsigned long timeout;
+	int i;
 
-	timeout = jiffies + msecs_to_jiffies(PIO_TIMEOUT_MS);
-
-	while (time_before(jiffies, timeout)) {
+	for (i = 0; i < PIO_RETRY_CNT; i++) {
 		u32 start, isr;
 
 		start = advk_readl(pcie, PIO_START);
 		isr = advk_readl(pcie, PIO_ISR);
 		if (!start && isr)
 			return 0;
+		udelay(PIO_RETRY_DELAY);
 	}
 
 	dev_err(dev, "config read/write timed out\n");
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] PCI: aardvark: Don't rely on jiffies while holding spinlock
  2019-09-27  8:31 [PATCH v2] PCI: aardvark: Don't rely on jiffies while holding spinlock Remi Pommarel
@ 2019-09-27  8:34 ` Thomas Petazzoni
  2019-09-27  8:50   ` Remi Pommarel
  0 siblings, 1 reply; 3+ messages in thread
From: Thomas Petazzoni @ 2019-09-27  8:34 UTC (permalink / raw)
  To: Remi Pommarel
  Cc: Lorenzo Pieralisi, Bjorn Helgaas, linux-pci, linux-arm-kernel,
	linux-kernel

Hello Remi,

Thanks for the new iteration!

On Fri, 27 Sep 2019 10:31:42 +0200
Remi Pommarel <repk@triplefau.lt> wrote:

> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index fc0fe4d4de49..ee05ccb2b686 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -175,7 +175,8 @@
>  	(PCIE_CONF_BUS(bus) | PCIE_CONF_DEV(PCI_SLOT(devfn))	| \
>  	 PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where))
>  
> -#define PIO_TIMEOUT_MS			1
> +#define PIO_RETRY_CNT			10
> +#define PIO_RETRY_DELAY			2 /* 2 us*/

So this changes the timeout from 1ms to just 20us, a division by 50
from the previous timeout value. From my measurements, it could
sometime take up to 6us from a single PIO read operation to complete,
which is getting close to the 20us timeout.

Shouldn't PIO_RETRY_CNT be kept at 500, so that we keep using a 1ms
timeout ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] PCI: aardvark: Don't rely on jiffies while holding spinlock
  2019-09-27  8:34 ` Thomas Petazzoni
@ 2019-09-27  8:50   ` Remi Pommarel
  0 siblings, 0 replies; 3+ messages in thread
From: Remi Pommarel @ 2019-09-27  8:50 UTC (permalink / raw)
  To: Thomas Petazzoni
  Cc: Lorenzo Pieralisi, Bjorn Helgaas, linux-pci, linux-arm-kernel,
	linux-kernel

Hi Thomas,

On Fri, Sep 27, 2019 at 10:34:20AM +0200, Thomas Petazzoni wrote:
> Hello Remi,
> 
> Thanks for the new iteration!
> 
> On Fri, 27 Sep 2019 10:31:42 +0200
> Remi Pommarel <repk@triplefau.lt> wrote:
> 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index fc0fe4d4de49..ee05ccb2b686 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -175,7 +175,8 @@
> >  	(PCIE_CONF_BUS(bus) | PCIE_CONF_DEV(PCI_SLOT(devfn))	| \
> >  	 PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where))
> >  
> > -#define PIO_TIMEOUT_MS			1
> > +#define PIO_RETRY_CNT			10
> > +#define PIO_RETRY_DELAY			2 /* 2 us*/
> 
> So this changes the timeout from 1ms to just 20us, a division by 50
> from the previous timeout value. From my measurements, it could
> sometime take up to 6us from a single PIO read operation to complete,
> which is getting close to the 20us timeout.
> 
> Shouldn't PIO_RETRY_CNT be kept at 500, so that we keep using a 1ms
> timeout ?

Damn. You right of course, sorry about that.

Thanks

-- 
Remi

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-09-27  8:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-27  8:31 [PATCH v2] PCI: aardvark: Don't rely on jiffies while holding spinlock Remi Pommarel
2019-09-27  8:34 ` Thomas Petazzoni
2019-09-27  8:50   ` Remi Pommarel

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