| Message ID | 20200904002812.7300-1-chris.packham@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b867eef4cf548cd9541225aadcdcee644669b9e1 |
| Headers | show |
| Series | spi: fsl-espi: Only process interrupts for expected events | expand |
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; > }
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; >> }
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; > > > }
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: > 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. > } >
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.
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; }
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(-)