Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAD=FV=Xgw+33pCycHyaMPsk64Qs+oh8e-RtJaM1yn0F27qZRVQ@mail.gmail.com>
References: <20201203074459.13078-1-rojay@codeaurora.org> <CAD=FV=WtU3cnRe6pDKFMA9_0cnQFtSOyohY_bJwZObK+KrbhVQ@mail.gmail.com> <160764107797.1580929.14768824290834396298@swboyd.mtv.corp.google.com> <CAD=FV=WuQjKC6GHy8d2nuqS-fgsUfxYrJosg3eyC9JU1FPCcjw@mail.gmail.com> <160764316821.1580929.18177257779550490986@swboyd.mtv.corp.google.com> <CAD=FV=WvG085orLqnvg9WUobL7iyxwgoxh-8RvOaRdi9rLeDUg@mail.gmail.com> <160764785500.1580929.4255309510717807485@swboyd.mtv.corp.google.com> <CAD=FV=VD78fmSRciFf38AbZG=EFPzDiT_e7QkEC08zA9iL1vTw@mail.gmail.com> <160764967649.1580929.3992720095789306793@swboyd.mtv.corp.google.com> <CAD=FV=Xgw+33pCycHyaMPsk64Qs+oh8e-RtJaM1yn0F27qZRVQ@mail.gmail.com>
Subject: Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     Roja Rani Yarubandi <rojay@codeaurora.org>,
        Mark Brown <broonie@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        linux-spi <linux-spi@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Akash Asthana <akashast@codeaurora.org>,
        msavaliy@qti.qualcomm.com
To:     Doug Anderson <dianders@chromium.org>
Date:   Thu, 10 Dec 2020 17:39:38 -0800
Message-ID: <160765077856.1580929.643282739071441296@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9.1
Precedence: bulk

Quoting Doug Anderson (2020-12-10 17:30:17)
> On Thu, Dec 10, 2020 at 5:21 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Yeah and so if it comes way later because it timed out then what's the
> > point of calling synchronize_irq() again? To make the completion
> > variable set when it won't be tested again until it is reinitialized?
>=20
> Presumably the idea is to try to recover to a somewhat usable state
> again?  We're not rebooting the machine so, even though this transfer
> failed, we will undoubtedly do another transfer later.  If that
> "abort" interrupt comes way later while we're setting up the next
> transfer we'll really confuse ourselves.

The interrupt handler just sets a completion variable. What does that
confuse?

>=20
> I guess you could go the route of adding a synchronize_irq() at the
> start of the next transfer, but I'd rather add the overhead in the
> exceptional case (the timeout) than the normal case.  In the normal
> case we don't need to worry about random IRQs from the past transfer
> suddenly showing up.
>=20

How does adding synchronize_irq() at the end guarantee that the abort is
cleared out of the hardware though? It seems to assume that the abort is
pending at the GIC when it could still be running through the hardware
and not executed yet. It seems like a synchronize_irq() for that is
wishful thinking that the irq is merely pending even though it timed
out and possibly never ran. Maybe it's stuck in a write buffer in the
CPU?