[PATCH] spi: fsl-espi: Only process interrupts for expected events

The SPIE register contains counts for the TX FIFO so any time the irq
handler was invoked we would attempt to process the RX/TX fifos. Use the
SPIM value to mask the events so that we only process interrupts that
were expected.

This was a latent issue exposed by commit 3282a3da25bd ("powerpc/64:
Implement soft interrupt replay in C").

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: stable@vger.kernel.org
---

Notes:
    I've tested this on a T2080RDB and a custom board using the T2081 SoC. With
    this change I don't see any spurious instances of the "Transfer done but
    SPIE_DON isn't set!" or "Transfer done but rx/tx fifo's aren't empty!" messages
    and the updates to spi flash are successful.
    
    I think this should go into the stable trees that contain 3282a3da25bd but I
    haven't added a Fixes: tag because I think 3282a3da25bd exposed the issue as
    opposed to causing it.

 drivers/spi/spi-fsl-espi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c
index 7e7c92cafdbb..cb120b68c0e2 100644
--- a/drivers/spi/spi-fsl-espi.c
+++ b/drivers/spi/spi-fsl-espi.c

@@ -574,13 +574,14 @@ static void fsl_espi_cpu_irq(struct fsl_espi *espi, u32 events)
 static irqreturn_t fsl_espi_irq(s32 irq, void *context_data)
 {
 	struct fsl_espi *espi = context_data;
-	u32 events;
+	u32 events, mask;
 
 	spin_lock(&espi->lock);
 
 	/* Get interrupt events(tx/rx) */
 	events = fsl_espi_read_reg(espi, ESPI_SPIE);
-	if (!events) {
+	mask = fsl_espi_read_reg(espi, ESPI_SPIM);
+	if (!(events & mask)) {
 		spin_unlock(&espi->lock);
 		return IRQ_NONE;
 	}

-- 
2.28.0

Hi All,

On 4/09/20 12:28 pm, Chris Packham wrote:

The SPIE register contains counts for the TX FIFO so any time the irq
handler was invoked we would attempt to process the RX/TX fifos. Use the
SPIM value to mask the events so that we only process interrupts that
were expected.

This was a latent issue exposed by commit 3282a3da25bd ("powerpc/64:
Implement soft interrupt replay in C").

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: stable@vger.kernel.org
---

ping?

Excerpts from Chris Packham's message of September 14, 2020 8:03 am:

Hi All,

On 4/09/20 12:28 pm, Chris Packham wrote:

quoted

The SPIE register contains counts for the TX FIFO so any time the irq
handler was invoked we would attempt to process the RX/TX fifos. Use the
SPIM value to mask the events so that we only process interrupts that
were expected.

This was a latent issue exposed by commit 3282a3da25bd ("powerpc/64:
Implement soft interrupt replay in C").

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: stable@vger.kernel.org
---

ping?

I don't know the code/hardware but thanks for tracking this down.

Was there anything more to be done with Jocke's observations, or would 
that be a follow-up patch if anything?

If this patch fixes your problem it should probably go in, unless there 
are any objections.

Thanks,
Nick

quoted

Notes:
     I've tested this on a T2080RDB and a custom board using the T2081 SoC. With
     this change I don't see any spurious instances of the "Transfer done but
     SPIE_DON isn't set!" or "Transfer done but rx/tx fifo's aren't empty!" messages
     and the updates to spi flash are successful.
     
     I think this should go into the stable trees that contain 3282a3da25bd but I
     haven't added a Fixes: tag because I think 3282a3da25bd exposed the issue as
     opposed to causing it.

  drivers/spi/spi-fsl-espi.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c
index 7e7c92cafdbb..cb120b68c0e2 100644
--- a/drivers/spi/spi-fsl-espi.c
+++ b/drivers/spi/spi-fsl-espi.c

@@ -574,13 +574,14 @@ static void fsl_espi_cpu_irq(struct fsl_espi *espi, u32 events)
  static irqreturn_t fsl_espi_irq(s32 irq, void *context_data)
  {
  	struct fsl_espi *espi = context_data;
-	u32 events;
+	u32 events, mask;
  
  	spin_lock(&espi->lock);
  
  	/* Get interrupt events(tx/rx) */
  	events = fsl_espi_read_reg(espi, ESPI_SPIE);
-	if (!events) {
+	mask = fsl_espi_read_reg(espi, ESPI_SPIM);
+	if (!(events & mask)) {
  		spin_unlock(&espi->lock);
  		return IRQ_NONE;
  	}

On Mon, 2020-09-14 at 12:28 +1000, Nicholas Piggin wrote:

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.


Excerpts from Chris Packham's message of September 14, 2020 8:03 am:

quoted

Hi All,

On 4/09/20 12:28 pm, Chris Packham wrote:

quoted

The SPIE register contains counts for the TX FIFO so any time the irq
handler was invoked we would attempt to process the RX/TX fifos. Use the
SPIM value to mask the events so that we only process interrupts that
were expected.

This was a latent issue exposed by commit 3282a3da25bd ("powerpc/64:
Implement soft interrupt replay in C").

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: stable@vger.kernel.org
---

ping?

I don't know the code/hardware but thanks for tracking this down.

Was there anything more to be done with Jocke's observations, or would
that be a follow-up patch if anything?

Patch is good IMHO, there may be more to fix w.r.t clearing the IRQs

If this patch fixes your problem it should probably go in, unless there
are any objections.

It should go in I think.

 Jocke

Thanks,
Nick

quoted

quoted

Notes:
     I've tested this on a T2080RDB and a custom board using the T2081 SoC. With
     this change I don't see any spurious instances of the "Transfer done but
     SPIE_DON isn't set!" or "Transfer done but rx/tx fifo's aren't empty!" messages
     and the updates to spi flash are successful.

     I think this should go into the stable trees that contain 3282a3da25bd but I
     haven't added a Fixes: tag because I think 3282a3da25bd exposed the issue as
     opposed to causing it.

  drivers/spi/spi-fsl-espi.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c
index 7e7c92cafdbb..cb120b68c0e2 100644
--- a/drivers/spi/spi-fsl-espi.c
+++ b/drivers/spi/spi-fsl-espi.c

@@ -574,13 +574,14 @@ static void fsl_espi_cpu_irq(struct fsl_espi *espi, u32 events)

  static irqreturn_t fsl_espi_irq(s32 irq, void *context_data)
  {
     struct fsl_espi *espi = context_data;
-    u32 events;
+    u32 events, mask;

     spin_lock(&espi->lock);

     /* Get interrupt events(tx/rx) */
     events = fsl_espi_read_reg(espi, ESPI_SPIE);
-    if (!events) {
+    mask = fsl_espi_read_reg(espi, ESPI_SPIM);
+    if (!(events & mask)) {
             spin_unlock(&espi->lock);
             return IRQ_NONE;
     }

On Fri, 4 Sep 2020 12:28:12 +1200, Chris Packham wrote:

The SPIE register contains counts for the TX FIFO so any time the irq
handler was invoked we would attempt to process the RX/TX fifos. Use the
SPIM value to mask the events so that we only process interrupts that
were expected.

This was a latent issue exposed by commit 3282a3da25bd ("powerpc/64:
Implement soft interrupt replay in C").

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: fsl-espi: Only process interrupts for expected events
      commit: b867eef4cf548cd9541225aadcdcee644669b9e1

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

On 04.09.2020 02:28, Chris Packham wrote:

Sorry, I was on vacation and therefore couldn't comment earlier.
I'm fine with the change, just one thing could be improved IMO.
If we skip an unneeded interrupt now, then returning IRQ_NONE
causes reporting this interrupt as spurious. This isn't too nice
as spurious interrupts typically are seen as a problem indicator.
Therefore returning IRQ_HANDLED should be more appropriate.
This would just require a comment in the code explaining why we
do this, and why it can happen that we receive interrupts
we're not interested in.

 	}

On 24/09/20 8:27 am, Heiner Kallweit wrote:

On 04.09.2020 02:28, Chris Packham wrote:

quoted

The SPIE register contains counts for the TX FIFO so any time the irq
handler was invoked we would attempt to process the RX/TX fifos. Use the
SPIM value to mask the events so that we only process interrupts that
were expected.

This was a latent issue exposed by commit 3282a3da25bd ("powerpc/64:
Implement soft interrupt replay in C").

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: stable@vger.kernel.org
---

Notes:
     I've tested this on a T2080RDB and a custom board using the T2081 SoC. With
     this change I don't see any spurious instances of the "Transfer done but
     SPIE_DON isn't set!" or "Transfer done but rx/tx fifo's aren't empty!" messages
     and the updates to spi flash are successful.
     
     I think this should go into the stable trees that contain 3282a3da25bd but I
     haven't added a Fixes: tag because I think 3282a3da25bd exposed the issue as
     opposed to causing it.

  drivers/spi/spi-fsl-espi.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c
index 7e7c92cafdbb..cb120b68c0e2 100644
--- a/drivers/spi/spi-fsl-espi.c
+++ b/drivers/spi/spi-fsl-espi.c

@@ -574,13 +574,14 @@ static void fsl_espi_cpu_irq(struct fsl_espi *espi, u32 events)
  static irqreturn_t fsl_espi_irq(s32 irq, void *context_data)
  {
  	struct fsl_espi *espi = context_data;
-	u32 events;
+	u32 events, mask;
  
  	spin_lock(&espi->lock);
  
  	/* Get interrupt events(tx/rx) */
  	events = fsl_espi_read_reg(espi, ESPI_SPIE);
-	if (!events) {
+	mask = fsl_espi_read_reg(espi, ESPI_SPIM);
+	if (!(events & mask)) {
  		spin_unlock(&espi->lock);
  		return IRQ_NONE;

Sorry, I was on vacation and therefore couldn't comment earlier.
I'm fine with the change, just one thing could be improved IMO.
If we skip an unneeded interrupt now, then returning IRQ_NONE
causes reporting this interrupt as spurious. This isn't too nice
as spurious interrupts typically are seen as a problem indicator.
Therefore returning IRQ_HANDLED should be more appropriate.
This would just require a comment in the code explaining why we
do this, and why it can happen that we receive interrupts
we're not interested in.

I'd be happy to send a follow-up to change IRQ_NONE to IRQ_HANDLED. I 
don't think the old code could have ever hit the IRQ_NONE (because event 
will always be non-zero) so it won't really be a change in behaviour. 
With the patch (that is now in spi/for-next) so far I do see a low 
number of spurious interrupts on the test setup where previously I would 
have seen failure to talk to the spi-flash.