[PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Roja Rani Yarubandi]

Here, there is a chance of race condition occurrence which leads to
NULL pointer dereference with struct spi_geni_master member 'cur_xfer'
between setup_fifo_xfer() and handle_fifo_timeout() functions.

Fix this race condition with guarding the 'cur_xfer' where it gets updated,
with spin_lock_irq/spin_unlock_irq in setup_fifo_xfer() as we do in
handle_fifo_timeout() function.

Call trace:
 geni_spi_isr+0x114/0x34c
 __handle_irq_event_percpu+0xe0/0x23c
 handle_irq_event_percpu+0x34/0x8c
 handle_irq_event+0x48/0x94
 handle_fasteoi_irq+0xd0/0x140
 __handle_domain_irq+0x8c/0xcc
 gic_handle_irq+0x114/0x1dc
 el1_irq+0xcc/0x180
 geni_spi a80000.spi: Failed to cancel/abort m_cmd
 dev_watchdog+0x348/0x354
 call_timer_fn+0xc4/0x220
 __run_timers+0x228/0x2d4
 spi_master spi6: failed to transfer one message from queue
 run_timer_softirq+0x24/0x44
 __do_softirq+0x16c/0x344
 irq_exit+0xa8/0xac
 __handle_domain_irq+0x94/0xcc
 gic_handle_irq+0x114/0x1dc
 el1_irq+0xcc/0x180
 cpuidle_enter_state+0xf8/0x204
 cpuidle_enter+0x38/0x4c
 cros-ec-spi spi6.0: spi transfer failed: -110
 ...

Fixes: 2ee471a1e28e ("spi: spi-geni-qcom: Mo' betta locking")
Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
---
 drivers/spi/spi-geni-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 25810a7eef10..e65d6676602b 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -457,7 +457,6 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 		len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
 	len &= TRANS_LEN_MSK;
 
-	mas->cur_xfer = xfer;
 	if (xfer->tx_buf) {
 		m_cmd |= SPI_TX_ONLY;
 		mas->tx_rem_bytes = xfer->len;
@@ -475,6 +474,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 	 * interrupt could come in at any time now.
 	 */
 	spin_lock_irq(&mas->lock);
+	mas->cur_xfer = xfer;
 	geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);
 
 	/*
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Doug Anderson]

Hi,

On Wed, Dec 2, 2020 at 11:45 PM Roja Rani Yarubandi
<rojay@codeaurora.org> wrote:
>
> Here, there is a chance of race condition occurrence which leads to
> NULL pointer dereference with struct spi_geni_master member 'cur_xfer'
> between setup_fifo_xfer() and handle_fifo_timeout() functions.
>
> Fix this race condition with guarding the 'cur_xfer' where it gets updated,
> with spin_lock_irq/spin_unlock_irq in setup_fifo_xfer() as we do in
> handle_fifo_timeout() function.
>
> Call trace:
>  geni_spi_isr+0x114/0x34c
>  __handle_irq_event_percpu+0xe0/0x23c
>  handle_irq_event_percpu+0x34/0x8c
>  handle_irq_event+0x48/0x94
>  handle_fasteoi_irq+0xd0/0x140
>  __handle_domain_irq+0x8c/0xcc
>  gic_handle_irq+0x114/0x1dc
>  el1_irq+0xcc/0x180
>  geni_spi a80000.spi: Failed to cancel/abort m_cmd
>  dev_watchdog+0x348/0x354
>  call_timer_fn+0xc4/0x220
>  __run_timers+0x228/0x2d4
>  spi_master spi6: failed to transfer one message from queue
>  run_timer_softirq+0x24/0x44
>  __do_softirq+0x16c/0x344
>  irq_exit+0xa8/0xac
>  __handle_domain_irq+0x94/0xcc
>  gic_handle_irq+0x114/0x1dc
>  el1_irq+0xcc/0x180
>  cpuidle_enter_state+0xf8/0x204
>  cpuidle_enter+0x38/0x4c
>  cros-ec-spi spi6.0: spi transfer failed: -110

Thanks for the patch!

I'm not convinced about your explanation, though.

The overall assumption in setup_fifo_xfer(), as indicated by the big
comment at the start of the function, is that once the spin_lock_irq()
/ spin_unlock_irq() dance happens at the start of that function that
no more interrupts will come in until the later lock.  If that
assumption is not true then we need to look at the whole function, not
just bandaid where we're setting "cur_xfer".  Specifically if
transfers are still happening all the other things that
setup_fifo_xfer() is doing are also all bad ideas.

I guess the first question is: why are you even getting a timeout?
SPI is clocked by the master and there's nothing the slave can do to
delay a transfer.  If you're getting a timeout in the first place it
means there's something pretty wrong.  Maybe that would be the first
thing to look at.  I guess the most likely case is that something else
in the system is somehow blocking our interrupt handler from running?

The second question is: what can we do about the "Failed to
cancel/abort m_cmd" message above.  I guess if our interrupt handler
is blocked that would explain why the cancel and abort didn't work.
It's pretty hard to figure out what to do to recover at this point.
When the function exits it assumes that the transfer has been canceled
/ aborted, but it hasn't.  This seems like it has the ability to throw
everything for a loop.  If an interrupt can come in for the previous
transfer after handle_fifo_timeout() exits it could cause all sorts of
problems, right?

It seems like one thing that might help would be to add this to the
start of handle_fifo_timeout():
  dev_warn(mas->dev, "Timeout; synchronizing IRQ\n");
  synchronize_irq(mas->irq);

Maybe also add a synchronize_irq() at the end of the function too?  If
my theory is correct and the whole problem here is that our interrupt
is blocked, I think that will make us block, which is probably the
best we can do and better than just returning and getting the
interrupt later.


That all being said, looking at all this makes me believe that the
NULL dereference is because of commit 7ba9bdcb91f6 ("spi:
spi-geni-qcom: Don't keep a local state variable").  There, I added
"mas->cur_xfer = NULL" into handle_fifo_timeout().  That should be the
right thing to do (without that we might have been reading bogus data
/ writing to memory we shouldn't), but I think geni_spi_handle_rx()
and geni_spi_handle_tx() don't handle it properly.  I think we'll
dereference it without checking.  :(  It would probably be good to fix
this too?  I would guess that if "mas->cur_xfer" is NULL then
geni_spi_handle_rx() should read all data in the FIFO and throw it
away and geni_spi_handle_tx() should set SE_GENI_TX_WATERMARK_REG to
0.  NOTE: I _think_ that with the synchronize_irq() I'm suggesting
above we'll avoid this case, but it never hurts to be defensive.


Does that all make sense?  So the summary is that instead of your patch:

1. Add synchronize_irq() at the start and end of
handle_fifo_timeout().  Not under lock.

2. In geni_spi_handle_rx(), check for NULL "mas->cur_xfer".  Read all
data in the FIFO (don't cap at rx_rem_bytes), but throw it away.

3. In geni_spi_handle_tx(), check for NULL "mas->cur_xfer".  Don't
write any data.  Just write 0 to SE_GENI_TX_WATERMARK_REG.

I think #1 is the real fix, but #2 and #3 will avoid crashes in case
there's another bug somewhere.


-Doug

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Stephen Boyd]

Quoting Doug Anderson (2020-12-03 08:40:46)

> I would guess that if "mas->cur_xfer" is NULL then
> geni_spi_handle_rx() should read all data in the FIFO and throw it
> away and geni_spi_handle_tx() should set SE_GENI_TX_WATERMARK_REG to
> 0.  NOTE: I _think_ that with the synchronize_irq() I'm suggesting
> above we'll avoid this case, but it never hurts to be defensive.
> 
> 
> Does that all make sense?  So the summary is that instead of your patch:

Can we get a CPU diagram describing the race and scenario where this
happens? Something like:

  CPU0                                CPU1
  ----                                ----
  setup_fifo_xfer()
   spin_lock_irq(&mas->lock);
   spin_unlock_irq(&mas->lock);
   mas->cur_xfer = xfer
   ...
   <IRQ>
                                      geni_spi_isr()
				       geni_spi_handle_rx()
				        <NULL deref boom explosion!>

But obviously this example diagram is incorrect and some timeout happens
instead? Sorry, I'm super lazy and don't want to read many paragraphs of
text. :) I'd rather have a diagram like above that clearly points out
the steps taken to the NULL pointer deref.

> 
> 1. Add synchronize_irq() at the start and end of
> handle_fifo_timeout().  Not under lock.
> 
> 2. In geni_spi_handle_rx(), check for NULL "mas->cur_xfer".  Read all
> data in the FIFO (don't cap at rx_rem_bytes), but throw it away.
> 
> 3. In geni_spi_handle_tx(), check for NULL "mas->cur_xfer".  Don't
> write any data.  Just write 0 to SE_GENI_TX_WATERMARK_REG.
> 
> I think #1 is the real fix, but #2 and #3 will avoid crashes in case
> there's another bug somewhere.
> 

Aren't 2 and 3 papering over some weird problem though where irqs are
coming in unexpectedly?

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Doug Anderson]

Hi,

On Wed, Dec 9, 2020 at 7:17 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2020-12-03 08:40:46)
>
> > I would guess that if "mas->cur_xfer" is NULL then
> > geni_spi_handle_rx() should read all data in the FIFO and throw it
> > away and geni_spi_handle_tx() should set SE_GENI_TX_WATERMARK_REG to
> > 0.  NOTE: I _think_ that with the synchronize_irq() I'm suggesting
> > above we'll avoid this case, but it never hurts to be defensive.
> >
> >
> > Does that all make sense?  So the summary is that instead of your patch:
>
> Can we get a CPU diagram describing the race and scenario where this
> happens? Something like:
>
>   CPU0                                CPU1
>   ----                                ----
>   setup_fifo_xfer()
>    spin_lock_irq(&mas->lock);
>    spin_unlock_irq(&mas->lock);
>    mas->cur_xfer = xfer
>    ...
>    <IRQ>
>                                       geni_spi_isr()
>                                        geni_spi_handle_rx()
>                                         <NULL deref boom explosion!>
>
> But obviously this example diagram is incorrect and some timeout happens
> instead? Sorry, I'm super lazy and don't want to read many paragraphs of
> text. :) I'd rather have a diagram like above that clearly points out
> the steps taken to the NULL pointer deref.

This is my untested belief of what's happening

 CPU0                                CPU1
 ----                                ----
                                     setup_fifo_xfer()
                                      ...
                                      geni_se_setup_m_cmd()
                                      <hardware starts transfer>
 <unrelated interrupt storm>          spin_unlock_irq()
 <continued interrupt storm>         <time passes>
 <continued interrupt storm>         <transfer complets in hardware>
 <continued interrupt storm>         <hardware sets M_RX_FIFO_WATERMARK_EN>
 <continued interrupt storm>         <time passes>
 <continued interrupt storm>         handle_fifo_timeout()
 <continued interrupt storm>          spin_lock_irq()
 <continued interrupt storm>          mas->cur_xfer = NULL
 <continued interrupt storm>          geni_se_cancel_m_cmd()
 <continued interrupt storm>          spin_unlock_irq()
 <continued interrupt storm>          wait_for_completion_timeout() => timeout
 <continued interrupt storm>          spin_lock_irq()
 <continued interrupt storm>          geni_se_abort_m_cmd()
 <continued interrupt storm>          spin_unlock_irq()
 <continued interrupt storm>          wait_for_completion_timeout() => timeout
 <interrupt storm ends>
 geni_spi_isr()
  spin_lock()
  if (m_irq & M_RX_FIFO_WATERMARK_EN)
   geni_spi_handle_rx()
    mas->cur_xfer NULL derefrence

With my proposed fix, I believe that would transform into:

 CPU0                                CPU1
 ----                                ----
                                     setup_fifo_xfer()
                                      ...
                                      geni_se_setup_m_cmd()
                                      <hardware starts transfer>
 <unrelated interrupt storm>          spin_unlock_irq()
 <continued interrupt storm>         <time passes>
 <continued interrupt storm>         <transfer complets in hardware>
 <continued interrupt storm>         <hardware sets M_RX_FIFO_WATERMARK_EN>
 <continued interrupt storm>         <time passes>
 <continued interrupt storm>         handle_fifo_timeout()
 <continued interrupt storm>          synchronize_irq()
 <continued interrupt storm>           <time passes>
 <interrupt storm ends>
 geni_spi_isr()
  ...
                                       <synchronize_irq() finishes>
                                      spin_lock_irq()
                                      mas->cur_xfer = NULL
                                      geni_se_cancel_m_cmd()
                                      spin_unlock_irq()
 geni_spi_isr()
   ...
                                      wait_for_completion_timeout() => success

The extra synchronize_irq() I was suggesting at the end of the
function would be an extra bit of paranoia.  Maybe a new storm showed
up while we were processing the timeout?


> > 1. Add synchronize_irq() at the start and end of
> > handle_fifo_timeout().  Not under lock.
> >
> > 2. In geni_spi_handle_rx(), check for NULL "mas->cur_xfer".  Read all
> > data in the FIFO (don't cap at rx_rem_bytes), but throw it away.
> >
> > 3. In geni_spi_handle_tx(), check for NULL "mas->cur_xfer".  Don't
> > write any data.  Just write 0 to SE_GENI_TX_WATERMARK_REG.
> >
> > I think #1 is the real fix, but #2 and #3 will avoid crashes in case
> > there's another bug somewhere.
>
> Aren't 2 and 3 papering over some weird problem though where irqs are
> coming in unexpectedly?

I think that's what I said but in different words?  #1 is the real fix
but #2 and #3 will keep us from crashing (AKA paper over) if we have
some other (unexpected) bug.  We'll already have an error in the log
in this case "Failed to cancel/abort m_cmd" so it doesn't feel
necessary to crash with a NULL dereference...

-Doug

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Stephen Boyd]

Quoting Doug Anderson (2020-12-10 09:14:15)
> 
> This is my untested belief of what's happening
> 
>  CPU0                                CPU1
>  ----                                ----
>                                      setup_fifo_xfer()
>                                       ...
>                                       geni_se_setup_m_cmd()
>                                       <hardware starts transfer>
>  <unrelated interrupt storm>          spin_unlock_irq()
>  <continued interrupt storm>         <time passes>
>  <continued interrupt storm>         <transfer complets in hardware>
>  <continued interrupt storm>         <hardware sets M_RX_FIFO_WATERMARK_EN>
>  <continued interrupt storm>         <time passes>
>  <continued interrupt storm>         handle_fifo_timeout()
>  <continued interrupt storm>          spin_lock_irq()
>  <continued interrupt storm>          mas->cur_xfer = NULL
>  <continued interrupt storm>          geni_se_cancel_m_cmd()
>  <continued interrupt storm>          spin_unlock_irq()
>  <continued interrupt storm>          wait_for_completion_timeout() => timeout
>  <continued interrupt storm>          spin_lock_irq()
>  <continued interrupt storm>          geni_se_abort_m_cmd()
>  <continued interrupt storm>          spin_unlock_irq()
>  <continued interrupt storm>          wait_for_completion_timeout() => timeout
>  <interrupt storm ends>
>  geni_spi_isr()
>   spin_lock()
>   if (m_irq & M_RX_FIFO_WATERMARK_EN)
>    geni_spi_handle_rx()
>     mas->cur_xfer NULL derefrence

Ok so the one line summary is "If geni_spi_isr() is sufficiently delayed
then we may deref NULL in the irq handler because the handler tries to
deref mas->cur_xfer after the timeout handling code has set it to NULL".

  CPU0                                CPU1
  ----                                ----
                                      setup_fifo_xfer()
                                       ...
                                       geni_se_setup_m_cmd()
                                       <hardware starts transfer>
  unrelated_irq_handler()              spin_unlock_irq()
   ...                                
                                        <transfer completes in hardware>
                                        <hardware sets M_RX_FIFO_WATERMARK_EN>
                                       
                                       handle_fifo_timeout()
                                        spin_lock_irq()
                                        mas->cur_xfer = NULL
                                        geni_se_cancel_m_cmd()
                                        spin_unlock_irq()
                                        wait_for_completion_timeout() => timeout
                                        spin_lock_irq()
                                        geni_se_abort_m_cmd()
                                        spin_unlock_irq()
                                        wait_for_completion_timeout() => timeout
   return IRQ_HANDLED;
  gic_handle_irq()
   geni_spi_isr()
    spin_lock()
    if (m_irq & M_RX_FIFO_WATERMARK_EN)
     geni_spi_handle_rx()
      rx_buf = mas->cur_xfer->rx_buf <--- OOPS!
 
> With my proposed fix, I believe that would transform into:
> 
>  CPU0                                CPU1
>  ----                                ----
>                                      setup_fifo_xfer()
>                                       ...
>                                       geni_se_setup_m_cmd()
>                                       <hardware starts transfer>
>  <unrelated interrupt storm>          spin_unlock_irq()
>  <continued interrupt storm>         <time passes>
>  <continued interrupt storm>         <transfer complets in hardware>
>  <continued interrupt storm>         <hardware sets M_RX_FIFO_WATERMARK_EN>
>  <continued interrupt storm>         <time passes>
>  <continued interrupt storm>         handle_fifo_timeout()
>  <continued interrupt storm>          synchronize_irq()
>  <continued interrupt storm>           <time passes>
>  <interrupt storm ends>
>  geni_spi_isr()
>   ...
>                                        <synchronize_irq() finishes>
>                                       spin_lock_irq()
>                                       mas->cur_xfer = NULL
>                                       geni_se_cancel_m_cmd()
>                                       spin_unlock_irq()
>  geni_spi_isr()
>    ...
>                                       wait_for_completion_timeout() => success
> 
> The extra synchronize_irq() I was suggesting at the end of the
> function would be an extra bit of paranoia.  Maybe a new storm showed
> up while we were processing the timeout?

Shouldn't we check in the timeout logic to see if m_irq has
M_RX_FIFO_WATERMARK_EN or M_TX_FIFO_WATERMARK_EN set instead? Similarly
for the CS assert/deassert stuff. If the timeout hits but one of those
bits are set then it seems we've run into some poor irqsoff section but
the hardware is still working. Calling synchronize_irq() wouldn't help
if the CPU handling the irqs (i.e. CPU0) had irqs off for a long time,
right? It will only ensure that other irq handlers have completed, which
may be a problem, but not the only one.

TL;DR: Peek at the irq status register in the timeout logic and skip it
if the irq is pending?

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Doug Anderson]

Hi,

On Thu, Dec 10, 2020 at 2:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2020-12-10 09:14:15)
> >
> > This is my untested belief of what's happening
> >
> >  CPU0                                CPU1
> >  ----                                ----
> >                                      setup_fifo_xfer()
> >                                       ...
> >                                       geni_se_setup_m_cmd()
> >                                       <hardware starts transfer>
> >  <unrelated interrupt storm>          spin_unlock_irq()
> >  <continued interrupt storm>         <time passes>
> >  <continued interrupt storm>         <transfer complets in hardware>
> >  <continued interrupt storm>         <hardware sets M_RX_FIFO_WATERMARK_EN>
> >  <continued interrupt storm>         <time passes>
> >  <continued interrupt storm>         handle_fifo_timeout()
> >  <continued interrupt storm>          spin_lock_irq()
> >  <continued interrupt storm>          mas->cur_xfer = NULL
> >  <continued interrupt storm>          geni_se_cancel_m_cmd()
> >  <continued interrupt storm>          spin_unlock_irq()
> >  <continued interrupt storm>          wait_for_completion_timeout() => timeout
> >  <continued interrupt storm>          spin_lock_irq()
> >  <continued interrupt storm>          geni_se_abort_m_cmd()
> >  <continued interrupt storm>          spin_unlock_irq()
> >  <continued interrupt storm>          wait_for_completion_timeout() => timeout
> >  <interrupt storm ends>
> >  geni_spi_isr()
> >   spin_lock()
> >   if (m_irq & M_RX_FIFO_WATERMARK_EN)
> >    geni_spi_handle_rx()
> >     mas->cur_xfer NULL derefrence
>
> Ok so the one line summary is "If geni_spi_isr() is sufficiently delayed
> then we may deref NULL in the irq handler because the handler tries to
> deref mas->cur_xfer after the timeout handling code has set it to NULL".

Sure.


>   CPU0                                CPU1
>   ----                                ----
>                                       setup_fifo_xfer()
>                                        ...
>                                        geni_se_setup_m_cmd()
>                                        <hardware starts transfer>
>   unrelated_irq_handler()              spin_unlock_irq()
>    ...
>                                         <transfer completes in hardware>
>                                         <hardware sets M_RX_FIFO_WATERMARK_EN>
>
>                                        handle_fifo_timeout()
>                                         spin_lock_irq()
>                                         mas->cur_xfer = NULL
>                                         geni_se_cancel_m_cmd()
>                                         spin_unlock_irq()
>                                         wait_for_completion_timeout() => timeout
>                                         spin_lock_irq()
>                                         geni_se_abort_m_cmd()
>                                         spin_unlock_irq()
>                                         wait_for_completion_timeout() => timeout
>    return IRQ_HANDLED;
>   gic_handle_irq()
>    geni_spi_isr()
>     spin_lock()
>     if (m_irq & M_RX_FIFO_WATERMARK_EN)
>      geni_spi_handle_rx()
>       rx_buf = mas->cur_xfer->rx_buf <--- OOPS!
>
> > With my proposed fix, I believe that would transform into:
> >
> >  CPU0                                CPU1
> >  ----                                ----
> >                                      setup_fifo_xfer()
> >                                       ...
> >                                       geni_se_setup_m_cmd()
> >                                       <hardware starts transfer>
> >  <unrelated interrupt storm>          spin_unlock_irq()
> >  <continued interrupt storm>         <time passes>
> >  <continued interrupt storm>         <transfer complets in hardware>
> >  <continued interrupt storm>         <hardware sets M_RX_FIFO_WATERMARK_EN>
> >  <continued interrupt storm>         <time passes>
> >  <continued interrupt storm>         handle_fifo_timeout()
> >  <continued interrupt storm>          synchronize_irq()
> >  <continued interrupt storm>           <time passes>
> >  <interrupt storm ends>
> >  geni_spi_isr()
> >   ...
> >                                        <synchronize_irq() finishes>
> >                                       spin_lock_irq()
> >                                       mas->cur_xfer = NULL
> >                                       geni_se_cancel_m_cmd()
> >                                       spin_unlock_irq()
> >  geni_spi_isr()
> >    ...
> >                                       wait_for_completion_timeout() => success
> >
> > The extra synchronize_irq() I was suggesting at the end of the
> > function would be an extra bit of paranoia.  Maybe a new storm showed
> > up while we were processing the timeout?
>
> Shouldn't we check in the timeout logic to see if m_irq has
> M_RX_FIFO_WATERMARK_EN or M_TX_FIFO_WATERMARK_EN set instead? Similarly
> for the CS assert/deassert stuff. If the timeout hits but one of those
> bits are set then it seems we've run into some poor irqsoff section but
> the hardware is still working. Calling synchronize_irq() wouldn't help
> if the CPU handling the irqs (i.e. CPU0) had irqs off for a long time,
> right? It will only ensure that other irq handlers have completed, which
> may be a problem, but not the only one.
>
> TL;DR: Peek at the irq status register in the timeout logic and skip it
> if the irq is pending?

I don't have tons of experience with synchronize_irq(), but the
function comment seems to indicate that as long as the interrupt is
pending synchronize_irq() will do what we want even if the CPU that
should handle the interrupt is in an irqsoff section.  Digging a
little bit I guess it relies upon the interrupt controller being able
to read this state, but (hopefully) the GIC can?

If it doesn't work like I think it does, I'd be OK with peeking in the
IRQ status register, but we shouldn't _skip_ the logic IMO.  As long
as we believe that an interrupt could happen in the future we
shouldn't return from handle_fifo_timeout().  It's impossible to
reason about how future transfers would work if the pending interrupt
from the previous transfer could fire at any point.

-Doug

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Stephen Boyd]

Quoting Doug Anderson (2020-12-10 15:07:39)
> Hi,
> 
> On Thu, Dec 10, 2020 at 2:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > right? It will only ensure that other irq handlers have completed, which
> > may be a problem, but not the only one.
> >
> > TL;DR: Peek at the irq status register in the timeout logic and skip it
> > if the irq is pending?
> 
> I don't have tons of experience with synchronize_irq(), but the
> function comment seems to indicate that as long as the interrupt is
> pending synchronize_irq() will do what we want even if the CPU that
> should handle the interrupt is in an irqsoff section.  Digging a
> little bit I guess it relies upon the interrupt controller being able
> to read this state, but (hopefully) the GIC can?

I didn't read synchronize_irq() more than the single line summary. I
thought it would only make sure other irq handlers have finished, which
is beside the point of some long section of code that has disabled irqs
on CPU0 with local_irq_disable(). And further more, presumably the irq
handler could be threaded, and then we could put a sufficiently large
msleep() at the start of geni_spi_isr() and see the same problem?

> 
> If it doesn't work like I think it does, I'd be OK with peeking in the
> IRQ status register, but we shouldn't _skip_ the logic IMO.  As long
> as we believe that an interrupt could happen in the future we
> shouldn't return from handle_fifo_timeout().  It's impossible to
> reason about how future transfers would work if the pending interrupt
> from the previous transfer could fire at any point.

Right. I just meant skip the timeout handling logic. We'd have to go
back to the timeout and keep waiting until the irq handler can run and
complete the completion variable.

I forgot that this is half handled in the spi core though. Peeking at
m_irq doesn't look very easy to implement. It certainly seems like this
means the timeout handler is busted and the diagram earlier could
indicate that spi core is driving this logic from
spi_transfer_one_message().

So why don't we check for cur_xfer being NULL in the rx/tx handling
paths too and bail out there? Does the FIFO need to be cleared out in
such a situation that spi core thinks a timeout happened but there's RX
data according to m_irq? Do we need to read it all and throw it away? Or
does the abort/cancel clear out the RX fifo?

----8<-----
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 25810a7eef10..651b1720401a 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -522,10 +522,12 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
 	spin_lock(&mas->lock);
 
 	if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN))
-		geni_spi_handle_rx(mas);
+		if (mas->cur_xfer)
+			geni_spi_handle_rx(mas);
 
 	if (m_irq & M_TX_FIFO_WATERMARK_EN)
-		geni_spi_handle_tx(mas);
+		if (mas->cur_xfer)
+			geni_spi_handle_tx(mas);
 
 	if (m_irq & M_CMD_DONE_EN) {
 		if (mas->cur_xfer) {

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Doug Anderson]

Hi,

On Thu, Dec 10, 2020 at 3:32 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2020-12-10 15:07:39)
> > Hi,
> >
> > On Thu, Dec 10, 2020 at 2:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > right? It will only ensure that other irq handlers have completed, which
> > > may be a problem, but not the only one.
> > >
> > > TL;DR: Peek at the irq status register in the timeout logic and skip it
> > > if the irq is pending?
> >
> > I don't have tons of experience with synchronize_irq(), but the
> > function comment seems to indicate that as long as the interrupt is
> > pending synchronize_irq() will do what we want even if the CPU that
> > should handle the interrupt is in an irqsoff section.  Digging a
> > little bit I guess it relies upon the interrupt controller being able
> > to read this state, but (hopefully) the GIC can?
>
> I didn't read synchronize_irq() more than the single line summary. I
> thought it would only make sure other irq handlers have finished, which
> is beside the point of some long section of code that has disabled irqs
> on CPU0 with local_irq_disable(). And further more, presumably the irq
> handler could be threaded, and then we could put a sufficiently large
> msleep() at the start of geni_spi_isr() and see the same problem?

As I understand it synchronize_irq():
1. If the interrupt is not running but is pending at a hardware level,
it'll wait.
2. If the interrupt is currently running it waits for it to finish.

That should handle all the cases you're talking about including
waiting for the threaded IRQ handler.  There's an explicit comment
about the threaded IRQ being accounted for in synchronize_irq():

https://elixir.bootlin.com/linux/v5.9/source/kernel/irq/manage.c#L134


> > If it doesn't work like I think it does, I'd be OK with peeking in the
> > IRQ status register, but we shouldn't _skip_ the logic IMO.  As long
> > as we believe that an interrupt could happen in the future we
> > shouldn't return from handle_fifo_timeout().  It's impossible to
> > reason about how future transfers would work if the pending interrupt
> > from the previous transfer could fire at any point.
>
> Right. I just meant skip the timeout handling logic. We'd have to go
> back to the timeout and keep waiting until the irq handler can run and
> complete the completion variable.
>
> I forgot that this is half handled in the spi core though. Peeking at
> m_irq doesn't look very easy to implement. It certainly seems like this
> means the timeout handler is busted and the diagram earlier could
> indicate that spi core is driving this logic from
> spi_transfer_one_message().

My assumption was that it was still OK (even if not perfect) to still
process it as a timeout.  I just want to really make sure a future
interrupt isn't going to show up.

If we want to try to do better, we can do timeout handling ourselves.
The SPI core allows for that.


> So why don't we check for cur_xfer being NULL in the rx/tx handling
> paths too and bail out there? Does the FIFO need to be cleared out in
> such a situation that spi core thinks a timeout happened but there's RX
> data according to m_irq? Do we need to read it all and throw it away? Or
> does the abort/cancel clear out the RX fifo?

I don't know for sure, but IMO it's safest to read anything that's in
the FIFO.  It's also important to adjust the watermark in the TX case.
The suggestions I provided in my original reply (#2 and #3) handle
this and are plenty simple.

As per my original reply, though, anything we do in the ISR doesn't
replace the changes we need to make to handle_fifo_timeout().  It is
very important that when handle_fifo_timeout() finishes that no future
interrupts for old transfers will fire.


-Doug

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Stephen Boyd]

Quoting Doug Anderson (2020-12-10 15:50:04)
> Hi,
> 
> On Thu, Dec 10, 2020 at 3:32 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Doug Anderson (2020-12-10 15:07:39)
> > > Hi,
> > >
> > > On Thu, Dec 10, 2020 at 2:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > right? It will only ensure that other irq handlers have completed, which
> > > > may be a problem, but not the only one.
> > > >
> > > > TL;DR: Peek at the irq status register in the timeout logic and skip it
> > > > if the irq is pending?
> > >
> > > I don't have tons of experience with synchronize_irq(), but the
> > > function comment seems to indicate that as long as the interrupt is
> > > pending synchronize_irq() will do what we want even if the CPU that
> > > should handle the interrupt is in an irqsoff section.  Digging a
> > > little bit I guess it relies upon the interrupt controller being able
> > > to read this state, but (hopefully) the GIC can?
> >
> > I didn't read synchronize_irq() more than the single line summary. I
> > thought it would only make sure other irq handlers have finished, which
> > is beside the point of some long section of code that has disabled irqs
> > on CPU0 with local_irq_disable(). And further more, presumably the irq
> > handler could be threaded, and then we could put a sufficiently large
> > msleep() at the start of geni_spi_isr() and see the same problem?
> 
> As I understand it synchronize_irq():
> 1. If the interrupt is not running but is pending at a hardware level,
> it'll wait.
> 2. If the interrupt is currently running it waits for it to finish.
> 
> That should handle all the cases you're talking about including
> waiting for the threaded IRQ handler.  There's an explicit comment
> about the threaded IRQ being accounted for in synchronize_irq():
> 
> https://elixir.bootlin.com/linux/v5.9/source/kernel/irq/manage.c#L134

Ok cool sounds like it would work then. Thanks for reading the code for
me! :)

> 
> 
> > > If it doesn't work like I think it does, I'd be OK with peeking in the
> > > IRQ status register, but we shouldn't _skip_ the logic IMO.  As long
> > > as we believe that an interrupt could happen in the future we
> > > shouldn't return from handle_fifo_timeout().  It's impossible to
> > > reason about how future transfers would work if the pending interrupt
> > > from the previous transfer could fire at any point.
> >
> > Right. I just meant skip the timeout handling logic. We'd have to go
> > back to the timeout and keep waiting until the irq handler can run and
> > complete the completion variable.
> >
> > I forgot that this is half handled in the spi core though. Peeking at
> > m_irq doesn't look very easy to implement. It certainly seems like this
> > means the timeout handler is busted and the diagram earlier could
> > indicate that spi core is driving this logic from
> > spi_transfer_one_message().
> 
> My assumption was that it was still OK (even if not perfect) to still
> process it as a timeout.  I just want to really make sure a future
> interrupt isn't going to show up.

I'm worried about the buffer disappearing if spi core calls handle_err()
but the geni_spi_isr() handler runs both an rx and a cancel/abort
routine. That doesn't seem to be the case though so it looks all fine.

> 
> If we want to try to do better, we can do timeout handling ourselves.
> The SPI core allows for that.
> 
> 
> > So why don't we check for cur_xfer being NULL in the rx/tx handling
> > paths too and bail out there? Does the FIFO need to be cleared out in
> > such a situation that spi core thinks a timeout happened but there's RX
> > data according to m_irq? Do we need to read it all and throw it away? Or
> > does the abort/cancel clear out the RX fifo?
> 
> I don't know for sure, but IMO it's safest to read anything that's in
> the FIFO.  It's also important to adjust the watermark in the TX case.
> The suggestions I provided in my original reply (#2 and #3) handle
> this and are plenty simple.
> 
> As per my original reply, though, anything we do in the ISR doesn't
> replace the changes we need to make to handle_fifo_timeout().  It is
> very important that when handle_fifo_timeout() finishes that no future
> interrupts for old transfers will fire.
> 

Alright. With a proper diagram in the commit text I think doing all of
the points, 1 through 3, would be good and required to leave the
hardware in a sane state for all the scenarios. Why do we need to call
synchronize_irq() at the start and end of handle_fifo_timeout() though?
Presumably having it at the start would make sure the long delayed irq
runs and handles any rx/tx by throwing it away. Sounds great, but having
it at the end leaves me confused. We want to make sure the cancel really
went through?  Don't we know that because the completion variable for
cancel succeeded?

I guess I'm not convinced that the hardware is so bad that it cancels
and aborts the sequencer, raises an irq for that, and then raises an irq
for the earlier rx/tx that the sequencer canceled out. Is that
happening? It's called a sequencer because presumably it runs a sequence
of operations like tx, rx, cs change, cancel, and abort. Hopefully it
doesn't run them out of order. If they run at the same time it's fine,
the irq handler will see all of them and throw away reads, etc.

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Doug Anderson]

Hi,

On Thu, Dec 10, 2020 at 4:51 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2020-12-10 15:50:04)
> > Hi,
> >
> > On Thu, Dec 10, 2020 at 3:32 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Doug Anderson (2020-12-10 15:07:39)
> > > > Hi,
> > > >
> > > > On Thu, Dec 10, 2020 at 2:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > right? It will only ensure that other irq handlers have completed, which
> > > > > may be a problem, but not the only one.
> > > > >
> > > > > TL;DR: Peek at the irq status register in the timeout logic and skip it
> > > > > if the irq is pending?
> > > >
> > > > I don't have tons of experience with synchronize_irq(), but the
> > > > function comment seems to indicate that as long as the interrupt is
> > > > pending synchronize_irq() will do what we want even if the CPU that
> > > > should handle the interrupt is in an irqsoff section.  Digging a
> > > > little bit I guess it relies upon the interrupt controller being able
> > > > to read this state, but (hopefully) the GIC can?
> > >
> > > I didn't read synchronize_irq() more than the single line summary. I
> > > thought it would only make sure other irq handlers have finished, which
> > > is beside the point of some long section of code that has disabled irqs
> > > on CPU0 with local_irq_disable(). And further more, presumably the irq
> > > handler could be threaded, and then we could put a sufficiently large
> > > msleep() at the start of geni_spi_isr() and see the same problem?
> >
> > As I understand it synchronize_irq():
> > 1. If the interrupt is not running but is pending at a hardware level,
> > it'll wait.
> > 2. If the interrupt is currently running it waits for it to finish.
> >
> > That should handle all the cases you're talking about including
> > waiting for the threaded IRQ handler.  There's an explicit comment
> > about the threaded IRQ being accounted for in synchronize_irq():
> >
> > https://elixir.bootlin.com/linux/v5.9/source/kernel/irq/manage.c#L134
>
> Ok cool sounds like it would work then. Thanks for reading the code for
> me! :)
>
> >
> >
> > > > If it doesn't work like I think it does, I'd be OK with peeking in the
> > > > IRQ status register, but we shouldn't _skip_ the logic IMO.  As long
> > > > as we believe that an interrupt could happen in the future we
> > > > shouldn't return from handle_fifo_timeout().  It's impossible to
> > > > reason about how future transfers would work if the pending interrupt
> > > > from the previous transfer could fire at any point.
> > >
> > > Right. I just meant skip the timeout handling logic. We'd have to go
> > > back to the timeout and keep waiting until the irq handler can run and
> > > complete the completion variable.
> > >
> > > I forgot that this is half handled in the spi core though. Peeking at
> > > m_irq doesn't look very easy to implement. It certainly seems like this
> > > means the timeout handler is busted and the diagram earlier could
> > > indicate that spi core is driving this logic from
> > > spi_transfer_one_message().
> >
> > My assumption was that it was still OK (even if not perfect) to still
> > process it as a timeout.  I just want to really make sure a future
> > interrupt isn't going to show up.
>
> I'm worried about the buffer disappearing if spi core calls handle_err()
> but the geni_spi_isr() handler runs both an rx and a cancel/abort
> routine. That doesn't seem to be the case though so it looks all fine.

It would be pretty racy if that was the case.  Until it calls
handle_timeout() we're still free to write to that buffer, right?


> > If we want to try to do better, we can do timeout handling ourselves.
> > The SPI core allows for that.
> >
> >
> > > So why don't we check for cur_xfer being NULL in the rx/tx handling
> > > paths too and bail out there? Does the FIFO need to be cleared out in
> > > such a situation that spi core thinks a timeout happened but there's RX
> > > data according to m_irq? Do we need to read it all and throw it away? Or
> > > does the abort/cancel clear out the RX fifo?
> >
> > I don't know for sure, but IMO it's safest to read anything that's in
> > the FIFO.  It's also important to adjust the watermark in the TX case.
> > The suggestions I provided in my original reply (#2 and #3) handle
> > this and are plenty simple.
> >
> > As per my original reply, though, anything we do in the ISR doesn't
> > replace the changes we need to make to handle_fifo_timeout().  It is
> > very important that when handle_fifo_timeout() finishes that no future
> > interrupts for old transfers will fire.
> >
>
> Alright. With a proper diagram in the commit text I think doing all of
> the points, 1 through 3, would be good and required to leave the
> hardware in a sane state for all the scenarios. Why do we need to call
> synchronize_irq() at the start and end of handle_fifo_timeout() though?
> Presumably having it at the start would make sure the long delayed irq
> runs and handles any rx/tx by throwing it away. Sounds great, but having
> it at the end leaves me confused. We want to make sure the cancel really
> went through?  Don't we know that because the completion variable for
> cancel succeeded?

I want it to handle the case where the "abort" command timed out!  :-)
 If the "abort" command timed out then it's still pending and we could
get an interrupt for it at some future point in time.


> I guess I'm not convinced that the hardware is so bad that it cancels
> and aborts the sequencer, raises an irq for that, and then raises an irq
> for the earlier rx/tx that the sequencer canceled out. Is that
> happening? It's called a sequencer because presumably it runs a sequence
> of operations like tx, rx, cs change, cancel, and abort. Hopefully it
> doesn't run them out of order. If they run at the same time it's fine,
> the irq handler will see all of them and throw away reads, etc.

Maybe answered by me explaining that I'm worried about the case where
"abort" times out (and thus the "done" from the abort is still
pending).

NOTE: I will also assert that if we send the "abort" then it seems
like it has a high likelihood of timing out.  Why do I say that?  In
order to even get to sending the "abort", it means:

a) The original transfer timed out

b) The "cancel" timed out.  As you can see, if the "cancel" doesn't
time out we don't even send the "abort"

...so two things timed out, one of which we _just_ sent.  The "abort"
feels like a last ditch effort.  Might as well try it, but things were
in pretty sorry shape to start with by the time we tried it.

-Doug

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Stephen Boyd]

Quoting Doug Anderson (2020-12-10 17:04:06)
> On Thu, Dec 10, 2020 at 4:51 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > I'm worried about the buffer disappearing if spi core calls handle_err()
> > but the geni_spi_isr() handler runs both an rx and a cancel/abort
> > routine. That doesn't seem to be the case though so it looks all fine.
> 
> It would be pretty racy if that was the case.  Until it calls
> handle_timeout() we're still free to write to that buffer, right?

Right I don't see any badness here.

> 
> 
> > > If we want to try to do better, we can do timeout handling ourselves.
> > > The SPI core allows for that.
> > >
> > >
> > > > So why don't we check for cur_xfer being NULL in the rx/tx handling
> > > > paths too and bail out there? Does the FIFO need to be cleared out in
> > > > such a situation that spi core thinks a timeout happened but there's RX
> > > > data according to m_irq? Do we need to read it all and throw it away? Or
> > > > does the abort/cancel clear out the RX fifo?
> > >
> > > I don't know for sure, but IMO it's safest to read anything that's in
> > > the FIFO.  It's also important to adjust the watermark in the TX case.
> > > The suggestions I provided in my original reply (#2 and #3) handle
> > > this and are plenty simple.
> > >
> > > As per my original reply, though, anything we do in the ISR doesn't
> > > replace the changes we need to make to handle_fifo_timeout().  It is
> > > very important that when handle_fifo_timeout() finishes that no future
> > > interrupts for old transfers will fire.
> > >
> >
> > Alright. With a proper diagram in the commit text I think doing all of
> > the points, 1 through 3, would be good and required to leave the
> > hardware in a sane state for all the scenarios. Why do we need to call
> > synchronize_irq() at the start and end of handle_fifo_timeout() though?
> > Presumably having it at the start would make sure the long delayed irq
> > runs and handles any rx/tx by throwing it away. Sounds great, but having
> > it at the end leaves me confused. We want to make sure the cancel really
> > went through?  Don't we know that because the completion variable for
> > cancel succeeded?
> 
> I want it to handle the case where the "abort" command timed out!  :-)
>  If the "abort" command timed out then it's still pending and we could
> get an interrupt for it at some future point in time.

Sure but who cares? We set a completion variable if abort comes in
later. We'll put a message in the log indicating that it "Failed" but
otherwise handle_fifo_timeout() can't return an error so we have to give
up eventually.

> 
> 
> > I guess I'm not convinced that the hardware is so bad that it cancels
> > and aborts the sequencer, raises an irq for that, and then raises an irq
> > for the earlier rx/tx that the sequencer canceled out. Is that
> > happening? It's called a sequencer because presumably it runs a sequence
> > of operations like tx, rx, cs change, cancel, and abort. Hopefully it
> > doesn't run them out of order. If they run at the same time it's fine,
> > the irq handler will see all of them and throw away reads, etc.
> 
> Maybe answered by me explaining that I'm worried about the case where
> "abort" times out (and thus the "done" from the abort is still
> pending).
> 
> NOTE: I will also assert that if we send the "abort" then it seems
> like it has a high likelihood of timing out.  Why do I say that?  In
> order to even get to sending the "abort", it means:
> 
> a) The original transfer timed out
> 
> b) The "cancel" timed out.  As you can see, if the "cancel" doesn't
> time out we don't even send the "abort"
> 
> ...so two things timed out, one of which we _just_ sent.  The "abort"
> feels like a last ditch effort.  Might as well try it, but things were
> in pretty sorry shape to start with by the time we tried it.
> 

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?

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Doug Anderson]

Hi,

On Thu, Dec 10, 2020 at 5:21 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> > > I guess I'm not convinced that the hardware is so bad that it cancels
> > > and aborts the sequencer, raises an irq for that, and then raises an irq
> > > for the earlier rx/tx that the sequencer canceled out. Is that
> > > happening? It's called a sequencer because presumably it runs a sequence
> > > of operations like tx, rx, cs change, cancel, and abort. Hopefully it
> > > doesn't run them out of order. If they run at the same time it's fine,
> > > the irq handler will see all of them and throw away reads, etc.
> >
> > Maybe answered by me explaining that I'm worried about the case where
> > "abort" times out (and thus the "done" from the abort is still
> > pending).
> >
> > NOTE: I will also assert that if we send the "abort" then it seems
> > like it has a high likelihood of timing out.  Why do I say that?  In
> > order to even get to sending the "abort", it means:
> >
> > a) The original transfer timed out
> >
> > b) The "cancel" timed out.  As you can see, if the "cancel" doesn't
> > time out we don't even send the "abort"
> >
> > ...so two things timed out, one of which we _just_ sent.  The "abort"
> > feels like a last ditch effort.  Might as well try it, but things were
> > in pretty sorry shape to start with by the time we tried it.
> >
>
> 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?

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.

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.

-Doug

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Stephen Boyd]

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

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

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?

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Doug Anderson]

Hi,

On Thu, Dec 10, 2020 at 5:39 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> 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?
> >
> > 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?

The interrupt handler sees a "DONE" interrupt.  If we've made it far
enough into setting up the next transfer that "cur_xfer" has been set
then it might do more, no?


> > 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.
> >
>
> 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?

I guess I'm asserting that if a full second passed (because we timed
out) and after that full second no interrupts are pending then the
interrupt will never come.  That seems a reasonable assumption to me.
It seems hard to believe it'd be stuck in a write buffer for a full
second?

-Doug

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Stephen Boyd]

Quoting Doug Anderson (2020-12-10 17:51:53)
> Hi,
> 
> On Thu, Dec 10, 2020 at 5:39 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > 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?
> > >
> > > 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?
> 
> The interrupt handler sees a "DONE" interrupt.  If we've made it far
> enough into setting up the next transfer that "cur_xfer" has been set
> then it might do more, no?

I thought it saw a cancel/abort EN bit?

        if (m_irq & M_CMD_CANCEL_EN)
                complete(&mas->cancel_done);
        if (m_irq & M_CMD_ABORT_EN)
                complete(&mas->abort_done)

and only a DONE bit if a transfer happened.

> 
> 
> > > 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.
> > >
> >
> > 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?
> 
> I guess I'm asserting that if a full second passed (because we timed
> out) and after that full second no interrupts are pending then the
> interrupt will never come.  That seems a reasonable assumption to me.
> It seems hard to believe it'd be stuck in a write buffer for a full
> second?
> 

Ok, so if we don't expect an irq to come in why are we calling
synchronize_irq()? I'm lost.

Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

[Doug Anderson]

Hi,

On Fri, Dec 11, 2020 at 5:32 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2020-12-10 17:51:53)
> > Hi,
> >
> > On Thu, Dec 10, 2020 at 5:39 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > 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?
> > > >
> > > > 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?
> >
> > The interrupt handler sees a "DONE" interrupt.  If we've made it far
> > enough into setting up the next transfer that "cur_xfer" has been set
> > then it might do more, no?
>
> I thought it saw a cancel/abort EN bit?
>
>         if (m_irq & M_CMD_CANCEL_EN)
>                 complete(&mas->cancel_done);
>         if (m_irq & M_CMD_ABORT_EN)
>                 complete(&mas->abort_done)
>
> and only a DONE bit if a transfer happened.

Ah, true.  The crazy thing is that since we do abort / cancel with
commands we get them together with "done".  That "done" could
potentially confuse the next transfer...  In theory we could ignore
DONE if we see ABORT / CANCEL, but I've now spent a bunch of time on
this and I think the best thing is to just make sure we won't start
the next transfer if any IRQs are pending.  I'll post patches...


> > > > 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.
> > > >
> > >
> > > 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?
> >
> > I guess I'm asserting that if a full second passed (because we timed
> > out) and after that full second no interrupts are pending then the
> > interrupt will never come.  That seems a reasonable assumption to me.
> > It seems hard to believe it'd be stuck in a write buffer for a full
> > second?
> >
>
> Ok, so if we don't expect an irq to come in why are we calling
> synchronize_irq()? I'm lost.

It turns out that synchronize_irq() doesn't do what I thought it did,
actually.  :(  Despite __synchronize_hardirq() talking about waiting
for "pending" interrupts, it actually passes in "IRQCHIP_STATE_ACTIVE"
and not "IRQCHIP_STATE_PENDING".  So much for that.

...but, if it did, I guess my point (which no longer matters) was:

a) If you wait a second but don't wait for pending interrupts to be
done, interrupts might still come later if the CPU servicing
interrupts was blocked.

b) If you don't wait a second but wait for pending interrupts to be
done, interrupts might still come later because maybe the transaction
wasn't finished first.

c) If you wait a second (enough for the transaction to finish) and
then wait for pending interrupts (to handle ISR being blocked) then
you're good.

---

So I got tired of all this conjecture and decided to write some code.
I reproduced the problem with some test code that let me call
local_irq_disable() for a set amount of time based on sysfs.

In terminal 1:
  while true; do ectool version > /dev/null; done

In terminal 2, disable interrupts on cpu0 for 2000 ms:
  taskset -c 0 echo 2000 > /sys/module/spi_geni_qcom/parameters/doug_test

Of course, I got the timeout and the NULL dereference.


Then I could poke at all the corner cases.  Posting patches for what I
think is the best solution...

-Doug