[PATCH 1/3] spi: spi-xilinx: Remove ISR race condition

[PATCH 1/3] spi: spi-xilinx: Remove ISR race condition

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 4297 bytes --]

From: Peter Crosthwaite <peter.crosthwaite@petalogix.com>

The ISR currently consumes the rx buffer data and re-enables transmission
from within interrupt context. This is bad because if the interrupt
occurs again before the ISR exits, the new interrupt will be erroneously
cleared by the still completing ISR.

Simplified the ISR by just setting the completion variable and exiting with
no action. Then just looped the transmit functionality in
xilinx_spi_txrx_bufs().

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/spi/spi-xilinx.c | 74 +++++++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 39 deletions(-)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index e1d7696..34d18dc 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -267,7 +267,6 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 {
 	struct xilinx_spi *xspi = spi_master_get_devdata(spi->master);
 	u32 ipif_ier;
-	u16 cr;

 	/* We get here with transmitter inhibited */

@@ -276,7 +275,6 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 	xspi->remaining_bytes = t->len;
 	INIT_COMPLETION(xspi->done);

-	xilinx_spi_fill_tx_fifo(xspi);

 	/* Enable the transmit empty interrupt, which we use to determine
 	 * progress on the transmission.
@@ -285,12 +283,41 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 	xspi->write_fn(ipif_ier | XSPI_INTR_TX_EMPTY,
 		xspi->regs + XIPIF_V123B_IIER_OFFSET);

-	/* Start the transfer by not inhibiting the transmitter any longer */
-	cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET) &
-		~XSPI_CR_TRANS_INHIBIT;
-	xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
+	for (;;) {
+		u16 cr;
+		u8 sr;
+
+		xilinx_spi_fill_tx_fifo(xspi);
+
+		/* Start the transfer by not inhibiting the transmitter any
+		 * longer
+		 */
+		cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET) &
+							~XSPI_CR_TRANS_INHIBIT;
+		xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
+
+		wait_for_completion(&xspi->done);
+
+		/* A transmit has just completed. Process received data and
+		 * check for more data to transmit. Always inhibit the
+		 * transmitter while the Isr refills the transmit register/FIFO,
+		 * or make sure it is stopped if we're done.
+		 */
+		cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
+		xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT,
+			       xspi->regs + XSPI_CR_OFFSET);
+
+		/* Read out all the data from the Rx FIFO */
+		sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
+		while ((sr & XSPI_SR_RX_EMPTY_MASK) == 0) {
+			xspi->rx_fn(xspi);
+			sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
+		}

-	wait_for_completion(&xspi->done);
+		/* See if there is more data to send */
+		if (!xspi->remaining_bytes > 0)
+			break;
+	}

 	/* Disable the transmit empty interrupt */
 	xspi->write_fn(ipif_ier, xspi->regs + XIPIF_V123B_IIER_OFFSET);
@@ -314,38 +341,7 @@ static irqreturn_t xilinx_spi_irq(int irq, void *dev_id)
 	xspi->write_fn(ipif_isr, xspi->regs + XIPIF_V123B_IISR_OFFSET);

 	if (ipif_isr & XSPI_INTR_TX_EMPTY) {	/* Transmission completed */
-		u16 cr;
-		u8 sr;
-
-		/* A transmit has just completed. Process received data and
-		 * check for more data to transmit. Always inhibit the
-		 * transmitter while the Isr refills the transmit register/FIFO,
-		 * or make sure it is stopped if we're done.
-		 */
-		cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
-		xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT,
-			xspi->regs + XSPI_CR_OFFSET);
-
-		/* Read out all the data from the Rx FIFO */
-		sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
-		while ((sr & XSPI_SR_RX_EMPTY_MASK) == 0) {
-			xspi->rx_fn(xspi);
-			sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
-		}
-
-		/* See if there is more data to send */
-		if (xspi->remaining_bytes > 0) {
-			xilinx_spi_fill_tx_fifo(xspi);
-			/* Start the transfer by not inhibiting the
-			 * transmitter any longer
-			 */
-			xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
-		} else {
-			/* No more data to send.
-			 * Indicate the transfer is completed.
-			 */
-			complete(&xspi->done);
-		}
+		complete(&xspi->done);
 	}

 	return IRQ_HANDLED;
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 2/3] spi: spi-xilinx: Clear dma_mask for xilinx spi controller

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

From: Michal Simek <monstr@monstr.eu>

mmc_spi driver tests if dma is available
through spi->master->dev.parent->dma_mask.
Microblaze supports DMA but xilinx_spi IP doesn't.
That's why clear dma_mask in the driver.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/spi/spi-xilinx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 34d18dc..d690756 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -365,6 +365,9 @@ struct spi_master *xilinx_spi_init(struct device *dev, struct resource *mem,
 	if (!master)
 		return NULL;

+	/* clear the dma_mask, to try to disable use of dma */
+	master->dev.dma_mask = 0;
+
 	/* the spi->mode bits understood by this driver: */
 	master->mode_bits = SPI_CPOL | SPI_CPHA;

--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 3/3] spi: spi-xilinx: Add run run-time endian detection

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 3638 bytes --]

Do not load endian value from platform data
and rather autodetect it.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/mfd/timberdale.c       |  1 -
 drivers/spi/spi-xilinx.c       | 29 +++++++++++++++++++++--------
 include/linux/spi/xilinx_spi.h |  1 -
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
index 59e0ee2..0c1fcbc 100644
--- a/drivers/mfd/timberdale.c
+++ b/drivers/mfd/timberdale.c
@@ -145,7 +145,6 @@ static struct spi_board_info timberdale_spi_8bit_board_info[] = {

 static struct xspi_platform_data timberdale_xspi_platform_data = {
 	.num_chipselect = 3,
-	.little_endian = true,
 	/* bits per word and devices will be filled in runtime depending
 	 * on the HW config
 	 */
diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index d690756..0b7b8d7 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -30,6 +30,7 @@
  */
 #define XSPI_CR_OFFSET		0x60	/* Control Register */

+#define XSPI_CR_LOOP		0x01
 #define XSPI_CR_ENABLE		0x02
 #define XSPI_CR_MASTER_MODE	0x04
 #define XSPI_CR_CPOL		0x08
@@ -355,11 +356,12 @@ static const struct of_device_id xilinx_spi_of_match[] = {
 MODULE_DEVICE_TABLE(of, xilinx_spi_of_match);

 struct spi_master *xilinx_spi_init(struct device *dev, struct resource *mem,
-	u32 irq, s16 bus_num, int num_cs, int little_endian, int bits_per_word)
+	u32 irq, s16 bus_num, int num_cs, int bits_per_word)
 {
 	struct spi_master *master;
 	struct xilinx_spi *xspi;
 	int ret;
+	u32 tmp;

 	master = spi_alloc_master(dev, sizeof(struct xilinx_spi));
 	if (!master)
@@ -395,13 +397,25 @@ struct spi_master *xilinx_spi_init(struct device *dev, struct resource *mem,

 	xspi->mem = *mem;
 	xspi->irq = irq;
-	if (little_endian) {
-		xspi->read_fn = xspi_read32;
-		xspi->write_fn = xspi_write32;
-	} else {
+
+	/*
+	 * Detect endianess on the IP via loop bit in CR. Detection
+	 * must be done before reset is sent because incorrect reset
+	 * value generates error interrupt.
+	 * Setup little endian helper functions first and try to use them
+	 * and check if bit was correctly setup or not.
+	 */
+	xspi->read_fn = xspi_read32;
+	xspi->write_fn = xspi_write32;
+
+	xspi->write_fn(XSPI_CR_LOOP, xspi->regs + XSPI_CR_OFFSET);
+	tmp = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
+	tmp &= XSPI_CR_LOOP;
+	if (tmp != XSPI_CR_LOOP) {
 		xspi->read_fn = xspi_read32_be;
 		xspi->write_fn = xspi_write32_be;
 	}
+
 	xspi->bits_per_word = bits_per_word;
 	if (xspi->bits_per_word == 8) {
 		xspi->tx_fn = xspi_tx8;
@@ -465,14 +479,13 @@ static int xilinx_spi_probe(struct platform_device *dev)
 {
 	struct xspi_platform_data *pdata;
 	struct resource *r;
-	int irq, num_cs = 0, little_endian = 0, bits_per_word = 8;
+	int irq, num_cs = 0, bits_per_word = 8;
 	struct spi_master *master;
 	u8 i;

 	pdata = dev->dev.platform_data;
 	if (pdata) {
 		num_cs = pdata->num_chipselect;
-		little_endian = pdata->little_endian;
 		bits_per_word = pdata->bits_per_word;
 	}

@@ -504,7 +517,7 @@ static int xilinx_spi_probe(struct platform_device *dev)
 		return -ENXIO;

 	master = xilinx_spi_init(&dev->dev, r, irq, dev->id, num_cs,
-				 little_endian, bits_per_word);
+				 bits_per_word);
 	if (!master)
 		return -ENODEV;

diff --git a/include/linux/spi/xilinx_spi.h b/include/linux/spi/xilinx_spi.h
index 6f17278..333ecdf 100644
--- a/include/linux/spi/xilinx_spi.h
+++ b/include/linux/spi/xilinx_spi.h
@@ -11,7 +11,6 @@
  */
 struct xspi_platform_data {
 	u16 num_chipselect;
-	bool little_endian;
 	u8 bits_per_word;
 	struct spi_board_info *devices;
 	u8 num_devices;
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/3] spi: spi-xilinx: Remove ISR race condition

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

On Tue, Jun 04, 2013 at 04:02:34PM +0200, Michal Simek wrote:

> The ISR currently consumes the rx buffer data and re-enables transmission
> from within interrupt context. This is bad because if the interrupt
> occurs again before the ISR exits, the new interrupt will be erroneously
> cleared by the still completing ISR.

> Simplified the ISR by just setting the completion variable and exiting with
> no action. Then just looped the transmit functionality in
> xilinx_spi_txrx_bufs().

Applied but this is a bit sad, having to defer the refill to process
context means that we're adding extra latency which takes us further
away from being able to saturate the bus.  There ought to be a way to
avoid the issue though I can't think of a non-racy one - I guess level
triggered interrupts aren't an option?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] spi: spi-xilinx: Clear dma_mask for xilinx spi controller

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]

On Tue, Jun 04, 2013 at 04:02:35PM +0200, Michal Simek wrote:
> From: Michal Simek <monstr@monstr.eu>
> 
> mmc_spi driver tests if dma is available
> through spi->master->dev.parent->dma_mask.
> Microblaze supports DMA but xilinx_spi IP doesn't.
> That's why clear dma_mask in the driver.

> +	/* clear the dma_mask, to try to disable use of dma */
> +	master->dev.dma_mask = 0;
> +

This looks like a bodge in the wrong place.  Either the device
registration is incorrect in that it advertises DMA when none is
available or the SPI driver ought to be offering the MMC driver a more
sensible way of advertising this limitation.  My first thought is the
former but I didn't check where dma_mask is getting set.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] spi: spi-xilinx: Add run run-time endian detection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 154 bytes --]

On Tue, Jun 04, 2013 at 04:02:36PM +0200, Michal Simek wrote:
> Do not load endian value from platform data
> and rather autodetect it.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] spi: spi-xilinx: Clear dma_mask for xilinx spi controller

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]

On 06/04/2013 07:36 PM, Mark Brown wrote:
> On Tue, Jun 04, 2013 at 04:02:35PM +0200, Michal Simek wrote:
>> From: Michal Simek <monstr@monstr.eu>
>>
>> mmc_spi driver tests if dma is available
>> through spi->master->dev.parent->dma_mask.
>> Microblaze supports DMA but xilinx_spi IP doesn't.
>> That's why clear dma_mask in the driver.
> 
>> +	/* clear the dma_mask, to try to disable use of dma */
>> +	master->dev.dma_mask = 0;
>> +
> 
> This looks like a bodge in the wrong place.  Either the device
> registration is incorrect in that it advertises DMA when none is
> available or the SPI driver ought to be offering the MMC driver a more
> sensible way of advertising this limitation.  My first thought is the
> former but I didn't check where dma_mask is getting set.

I have looked at history of this change and we have done it 3 years ago
based on one custom configuration.

It is shame that I don't have hw to test this but there was
something wrong in connection to mmc_spi.c.
I will try to find out hw for this to test but probably
won't be available. :-( And I will just revert this change in my tree.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/3] spi: spi-xilinx: Remove ISR race condition

[Jean-Francois Dagenais]

> On Jun 4, 2013, at 1:32 PM, Mark Brown <broonie@kernel.org> wrote:
> 
> Applied but this is a bit sad, having to defer the refill to process
> context means that we're adding extra latency which takes us further
> away from being able to saturate the bus.  There ought to be a way to
> avoid the issue though I can't think of a non-racy one - I guess level
> triggered interrupts aren't an option?

Hi all,

We've been using an improved version of this driver in production for years here
in the 3.4 tree. I had to intervene in order to improve performance. I managed
to double it (not bad).

Like the other threads I've seen about this, my strategy involved limiting the
reads to the registers, which, through a pci-e link were kind of long. Here's
the yet un-submitted commit header I carry in my clone:

=============
spi: xilinx - minimize iomem reads
    
    If this IP core is accessed through bridges like PCI-e, reads are rather
    costly. Doing many reads or read-modify-writes is thus long and strenuous
    on the CPU (active waiting).
    
    The transfer workflow of this driver allows some assumptions to be made and
    exploited to minimize reads as much as possible.
    
    These two assumptions are made:
    - since we are in control of the CR register, cache it so we don't have to
      read it all the time to modify it.
    - FIFO (either depth of 1 or 16) are always maxed out, unless the remaining
      bytes are less than the FIFO depth. For this reason, we can predict the
      state of the FIFOs without checking with the status register to check if
      they are empty or full.
    
    Reading 32Mb flash at 32MHz on a core with 8bits/word and FIFO enabled (16),
    has dropped from ~60s to ~30s. Still high, but better. Most of the delay still
    comes from emptying the RX FIFO one "word" at a time.
===============

Today I am merging from 3.4 to 3.14 and then 3.19 and am dealing with merge
conflicts.

I have to say we have never seen any race here and am trying to understand how
the race would have happened. The spi master is locked to concurrent users and
the only time the interrupt is enabled is from the txrx_bufs method. As I
understand it, the interrupt is not declared as IRQ_SHARED, so the driver's ISR
does run because of an actual TX empty event (the only interrupt flag we
enable).

This is where I suspect the interrupt controller's behaviour *might* come into
play. I have not worked with many types of IRQ so please feel free to educate me
here. We are using level based interrupt. When the ISR runs, the irq line is
masked so any new interrupt event is pending. When the ISR returns IRQ_HANDLED,
the IRQ is unmasked and the ISR is re-invoked as needed.

In this context, I fail to see how when disabling the TX inhibitor, i.e.
enabling TX, the sudden interrupt event could cause a race. The ISR invocation
would only be deferred until the current ISR complete.

I guess if you were to use edge interrupt without a latched register you could
miss the event, but why would anyone use such an interrupt controller? (if you
can call it that :-P)

So if we go back to the commit msg about the race:
> On Jun 4, 2013, at 10:02 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> 
> The ISR currently consumes the rx buffer data and re-enables transmission
> from within interrupt context. This is bad because if the interrupt
> occurs again before the ISR exits, the new interrupt will be erroneously
> cleared by the still completing ISR.

So regardless of the interrupt controller, I can't see how this event is even
possible. Given that the the ISR clears the interrupt immediately upon entry and
the only code left to execute by the ISR after the tx fifo is filled and the TX
inhibitor is disabled, is "return IRQ_SHARED"... ???

Thank you for your help!
/jfd

Re: [PATCH 1/3] spi: spi-xilinx: Remove ISR race condition

[Shubhrajyoti Datta]

On Thu, Oct 8, 2015 at 8:40 PM, Jean-Francois Dagenais
<jeff.dagenais@gmail.com> wrote:
>
>> On Jun 4, 2013, at 1:32 PM, Mark Brown <broonie@kernel.org> wrote:
>>
>> Applied but this is a bit sad, having to defer the refill to process
>> context means that we're adding extra latency which takes us further
>> away from being able to saturate the bus.  There ought to be a way to
>> avoid the issue though I can't think of a non-racy one - I guess level
>> triggered interrupts aren't an option?
>
> Hi all,
>
> We've been using an improved version of this driver in production for years here
> in the 3.4 tree. I had to intervene in order to improve performance. I managed
> to double it (not bad).
Thats cool.



>
> Like the other threads I've seen about this, my strategy involved limiting the
> reads to the registers, which, through a pci-e link were kind of long. Here's
> the yet un-submitted commit header I carry in my clone:

Why not send the patches.

>
> =============
> spi: xilinx - minimize iomem reads
>
>     If this IP core is accessed through bridges like PCI-e, reads are rather
>     costly. Doing many reads or read-modify-writes is thus long and strenuous
>     on the CPU (active waiting).
>
>     The transfer workflow of this driver allows some assumptions to be made and
>     exploited to minimize reads as much as possible.
>
>     These two assumptions are made:
>     - since we are in control of the CR register, cache it so we don't have to
>       read it all the time to modify it.

Makes sense.

>     - FIFO (either depth of 1 or 16) are always maxed out, unless the remaining
>       bytes are less than the FIFO depth. For this reason, we can predict the
>       state of the FIFOs without checking with the status register to check if
>       they are empty or full.

Can you explain more details.
>
>     Reading 32Mb flash at 32MHz on a core with 8bits/word and FIFO enabled (16),
>     has dropped from ~60s to ~30s. Still high, but better. Most of the delay still
>     comes from emptying the RX FIFO one "word" at a time.
> ===============
>

Re: [PATCH 1/3] spi: spi-xilinx: Remove ISR race condition

[Shubhrajyoti Datta]

>
>>
>> =============
>> spi: xilinx - minimize iomem reads
>>
>>     If this IP core is accessed through bridges like PCI-e, reads are rather
>>     costly. Doing many reads or read-modify-writes is thus long and strenuous
>>     on the CPU (active waiting).
>>
>>     The transfer workflow of this driver allows some assumptions to be made and
>>     exploited to minimize reads as much as possible.
>>
>>     These two assumptions are made:
>>     - since we are in control of the CR register, cache it so we don't have to
>>       read it all the time to modify it.
>
> Makes sense.

I have made an attempt at it can you check if you get any performance
improvemets
on your setup.

http://www.spinics.net/lists/linux-spi/msg05963.html


Thanks,
Shubhrajyoti