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

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

[Chris Packham]

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

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

[Chris Packham]

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?
>
> 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;
>   	}

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

[Nicholas Piggin]

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

>>
>> 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;
>>   	}

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

[Joakim Tjernlund]

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:
> > 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?
> 
> 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
> 
> > > 
> > > 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;
> > >      }

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

[Mark Brown]

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

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

[Heiner Kallweit]

On 04.09.2020 02:28, 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
> ---
> 
> 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.

>  	}
> 

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

[Chris Packham]

On 24/09/20 8:27 am, Heiner Kallweit wrote:
> On 04.09.2020 02:28, 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
>> ---
>>
>> 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.