Re: [PATCH] spi: spi-geni-qcom: Speculative fix of "nobody cared" about interrupt

From: Mark Brown <broonie@kernel.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Alok Chauhan <alokc@codeaurora.org>,
	Dilip Kota <dkota@codeaurora.org>,
	skakit@codeaurora.org, Girish Mahadevan <girishm@codeaurora.org>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Stephen Boyd <swboyd@chromium.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-spi <linux-spi@vger.kernel.org>
Subject: Re: [PATCH] spi: spi-geni-qcom: Speculative fix of "nobody cared" about interrupt
Date: Tue, 17 Mar 2020 16:44:17 +0000	[thread overview]
Message-ID: <20200317164417.GJ3971@sirena.org.uk> (raw)
In-Reply-To: <CAD=FV=VAeOYAG-R6aeAAoo7TsuvDBgNnqxn3XE2Mw3hwL1H7Ew@mail.gmail.com>

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

On Tue, Mar 17, 2020 at 08:12:30AM -0700, Doug Anderson wrote:
> On Tue, Mar 17, 2020 at 5:10 AM Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Mar 16, 2020 at 03:20:01PM -0700, Douglas Anderson wrote:

> >
> > Does this mean that there was an actual concrete message of type
> > CMD_NONE or does it mean that there was no message waiting?  If there
> > was no message then isn't the interrupt spurious?

> There is no message of type "CMD_NONE".  The "cur_mcmd" field is
> basically where in the software state machine we're at:

...

> ...so certainly if we see "cur_mcmd == CMD_NONE" in the interrupt
> handler we're in an unexpected situation.  We don't expect interrupts
> while idle.  I wouldn't necessarily say it was a spurious interrupt,
> though.  To say that I'd rather look at the result of this line in the
> IRQ handler:

>     m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS);

> ...if that line returns 0 then I would be willing to say it is a
> spurious interrupt.

Right, that should return IRQ_NONE if there's nothing actually asserted.
I think you're right about the state machine though.

> C) Do we care to try to detect spurious interrupts (by checking
> SE_GENI_M_IRQ_STATUS) and return IRQ_NONE?  Right now a spurious
> interrupt will be harmless because all of the logic in geni_spi_isr()
> doesn't do anything if SE_GENI_M_IRQ_STATUS has no bits set.  ...but
> it will still return IRQ_HANDLED.  I can't imagine anyone ever putting
> this device on a shared interrupt, but if it's important I can detect
> this and return IRQ_NONE in this case in a v2 of this patch.

It's a good idea to return IRQ_NONE not just for the shared interrupt
case but also because it lets the error handling code in the genirq core
do it's thing and detect bugs - as seems to have happened here.  This
one was fairly benign but you can also see things like interrupts that
are constantly asserted by the hardware where we end up spinning in
interrupt handlers.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]