[v2,2/5] spi: spi-fsl-dspi: Exit the ISR with IRQ_NONE when it's not ours
diff mbox series

Message ID 20190822211514.19288-3-olteanv@gmail.com
State Accepted
Commit 37b4100180641968056cb4e034cebc38338e8652
Headers show
Series
  • Poll mode for NXP DSPI driver
Related show

Commit Message

Vladimir Oltean Aug. 22, 2019, 9:15 p.m. UTC
The DSPI interrupt can be shared between two controllers at least on the
LX2160A. In that case, the driver for one controller might misbehave and
consume the other's interrupt. Fix this by actually checking if any of
the bits in the status register have been asserted.

Fixes: 13aed2392741 ("spi: spi-fsl-dspi: use IRQF_SHARED mode to request IRQ")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/spi/spi-fsl-dspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mark Brown Aug. 23, 2019, 10:28 a.m. UTC | #1
On Fri, Aug 23, 2019 at 12:15:11AM +0300, Vladimir Oltean wrote:
> The DSPI interrupt can be shared between two controllers at least on the
> LX2160A. In that case, the driver for one controller might misbehave and
> consume the other's interrupt. Fix this by actually checking if any of
> the bits in the status register have been asserted.

It would be better to have done this as the first patch before
the restructuring, that way we could send this as a fix - the
refactoring while good doesn't really fit with stable.
Vladimir Oltean Aug. 23, 2019, 10:30 a.m. UTC | #2
Hi Mark,

On Fri, 23 Aug 2019 at 13:28, Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Aug 23, 2019 at 12:15:11AM +0300, Vladimir Oltean wrote:
> > The DSPI interrupt can be shared between two controllers at least on the
> > LX2160A. In that case, the driver for one controller might misbehave and
> > consume the other's interrupt. Fix this by actually checking if any of
> > the bits in the status register have been asserted.
>
> It would be better to have done this as the first patch before
> the restructuring, that way we could send this as a fix - the
> refactoring while good doesn't really fit with stable.

Did you see this?
https://lkml.org/lkml/2019/8/22/1542

Regards,
-Vladimir
Mark Brown Aug. 23, 2019, 10:50 a.m. UTC | #3
On Fri, Aug 23, 2019 at 01:30:27PM +0300, Vladimir Oltean wrote:
> On Fri, 23 Aug 2019 at 13:28, Mark Brown <broonie@kernel.org> wrote:

> > It would be better to have done this as the first patch before
> > the restructuring, that way we could send this as a fix - the
> > refactoring while good doesn't really fit with stable.

> Did you see this?
> https://lkml.org/lkml/2019/8/22/1542

I'm not online enough to readily follow that link right now, I
did apply another patch for a similar issue though.  If that's
a different version of the same change please don't do that,
sending multiple conflicting versions of the same thing creates
conflicts and makes everything harder to work with.

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.
Mark Brown Aug. 23, 2019, 10:59 a.m. UTC | #4
On Fri, Aug 23, 2019 at 11:50:44AM +0100, Mark Brown wrote:
> On Fri, Aug 23, 2019 at 01:30:27PM +0300, Vladimir Oltean wrote:

> > Did you see this?
> > https://lkml.org/lkml/2019/8/22/1542

> I'm not online enough to readily follow that link right now, I
> did apply another patch for a similar issue though.  If that's
> a different version of the same change please don't do that,
> sending multiple conflicting versions of the same thing creates
> conflicts and makes everything harder to work with.

Oh, I guess this was due to there being an existing refactoring
in -next that meant the fix wouldn't apply directly.  I sorted
that out now I think, but in general the same thing applies -
it's better to put fixes before anything else in the series,
it'll flag up when reviewing.
Vladimir Oltean Aug. 23, 2019, 12:06 p.m. UTC | #5
Hi Mark,

On Fri, 23 Aug 2019 at 13:59, Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Aug 23, 2019 at 11:50:44AM +0100, Mark Brown wrote:
> > On Fri, Aug 23, 2019 at 01:30:27PM +0300, Vladimir Oltean wrote:
>
> > > Did you see this?
> > > https://lkml.org/lkml/2019/8/22/1542
>
> > I'm not online enough to readily follow that link right now, I
> > did apply another patch for a similar issue though.  If that's
> > a different version of the same change please don't do that,
> > sending multiple conflicting versions of the same thing creates
> > conflicts and makes everything harder to work with.
>
> Oh, I guess this was due to there being an existing refactoring
> in -next that meant the fix wouldn't apply directly.  I sorted
> that out now I think, but in general the same thing applies -
> it's better to put fixes before anything else in the series,
> it'll flag up when reviewing.

I try to require as little attention span as possible from you and I
apologize that I'm sending patches like a noob, but I'm not used to
this sort of interaction with a maintainer. It's taking me some time
to adjust expectations.
- You left change requests in the initial patchset I submitted, but
you partially applied the series anyway. You didn't give me a chance
to respin the whole series and put the shared IRQ fix on top, so it
applies on old trees as well. No problem, I sent two versions of the
patch.
- On my previous series you left this comment:

> Please don't include all the extra tags you've got in your subject
> lines.  In my inbox this series looks like:
>
>    790 N T 08/18 Vladimir Oltean ( 16K) ├─>[PATCH spi for-5.4 01/14] spi: spi-f
>
> so I can't tell what it's actually about just from looking at it.  Just
> [PATCH 01/14] would be enough, putting a target version in or versioning
> the patch series can be OK but you usually don't use a target version
> for -next and adding spi in there is redundant given that it's also in
> the changelog.

So I didn't put any target version in the patch titles this time,
although arguably it would have been clearer to you that there's a
patch for-5.4 and another version of it for-4.20 (which i *think* is
how I should submit a fix, I don't see any branch for inclusion in
stable trees per se).
No problem, I explained in the cover letters that one patchset is for
-next and the other is for inclusion in stable trees. Maintainers do
read cover letters, right?
Message from the -next cover letter:

> The series also contains a bug fix for the shared IRQ of the DSPI
> driver. I am going to respin a version of it as a separate patch for
> inclusion in stable trees, independent of this patchset.

Message from the fix's cover letter:

> This patch is taken out of the "Poll mode for NXP DSPI driver" series
> and respun against the "for-4.20" branch.
> $(git describe --tags 13aed2392741) shows:
> v4.20-rc1-18-g13aed2392741

Yes, I did send a cover letter for a single patch. I thought it's
harder to miss than a note hidden under patch 2/5 of one series, and
in the note section of the other's. I think you could have also made
an argument about me not doing it the other way around. In the end,
you can not read a note just as well as not read a cover letter, and
there's little I can do.

No problem, you missed the link between the two. I sent you a link to
the lkml archive. You said "I'm not online enough to readily follow
that link right now". Please teach me - I really don't know - how can
I make links between patchsets easier for you to follow, if you don't
read cover letters and you can't access lkml? I promise I'll use that
method next time.

> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.

Maybe you simply should do something else while traveling, just saying.

Regards,
-Vladimir
Mark Brown Aug. 23, 2019, 9:03 p.m. UTC | #6
On Fri, Aug 23, 2019 at 03:06:52PM +0300, Vladimir Oltean wrote:

> - You left change requests in the initial patchset I submitted, but
> you partially applied the series anyway. You didn't give me a chance
> to respin the whole series and put the shared IRQ fix on top, so it
> applies on old trees as well. No problem, I sent two versions of the
> patch.

Right, and this is fine.  A big part of this is that it's just
generally bad practice to not have fixes at the front of the
series, I'd flag this up as a problem even if the code was all
new and there was no question of applying as a bug fix.  It's
something that's noticable just at the level of looking at the
shape of the series without even looking at the contents of the
patches, if the fix is actually a good one or anything like that.
In the context of this it made it look like the reason you'd had
to do two versions.

> So I didn't put any target version in the patch titles this time,
> although arguably it would have been clearer to you that there's a
> patch for-5.4 and another version of it for-4.20 (which i *think* is
> how I should submit a fix, I don't see any branch for inclusion in
> stable trees per se).

Not for 4.20, for v5.3 - we basically only fix Linus' tree
directly, anything else gets backported from there unless it's
super important.  I don't think anyone is updating v4.20 at all
these days, the version number change from v4 to v5 was totally
arbatrary.

> Yes, I did send a cover letter for a single patch. I thought it's
> harder to miss than a note hidden under patch 2/5 of one series, and
> in the note section of the other's. I think you could have also made

If you're sending a multi-patch series it's of course good to
send a cover letter, it's just single patches where it's adding
overhead.

> No problem, you missed the link between the two. I sent you a link to
> the lkml archive. You said "I'm not online enough to readily follow
> that link right now". Please teach me - I really don't know - how can

It's not that I missed the link between them, it's that what I'd
expected to see was the fix being the first patch in the series
for -next and for that fix to look substantially the same with at
most some context difference.  I wasn't expecting to see a
completely different patch that wasn't at the start of the
series, had the fix been at the start of the series it'd have
been fairly clear what was going on but the refactoring patch
looked like the main reason you'd needed different versions (it's
certainly why they don't visually resemble each other).

In other words it looked like you'd sent a different fix because
the fix you'd done for -next was based on the first patch in the
series rather than there also being some context changes.

> I make links between patchsets easier for you to follow, if you don't
> read cover letters and you can't access lkml? I promise I'll use that
> method next time.

Like I said include a plain text description of what you're
linking to (eg, the subject line from a mail).

> > I do frequently catch up on my mail on flights or while otherwise
> > travelling so this is even more pressing for me than just being about
> > making things a bit easier to read.

> Maybe you simply should do something else while traveling, just saying.

I could also add in the coffee shop I sometimes work from which
doesn't have WiFi or mobile coverage.  Besides, like that part of
the text does say it's also a usability thing, having to fire up
a web browser to figure out what's being described is a stumbling
block.

Patch
diff mbox series

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index c90db7db4121..6ef2279a3699 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -659,7 +659,7 @@  static irqreturn_t dspi_interrupt(int irq, void *dev_id)
 	regmap_write(dspi->regmap, SPI_SR, spi_sr);
 
 	if (!(spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)))
-		return IRQ_HANDLED;
+		return IRQ_NONE;
 
 	/* Get transfer counter (in number of SPI transfers). It was
 	 * reset to 0 when transfer(s) were started.