Re: [PATCH 2/2] spi: spi-geni-qcom: Really ensure the previous xfer is done before new one

[Doug Anderson]

Hi,

On Tue, Dec 15, 2020 at 2:25 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Doug Anderson (2020-12-15 09:25:51)
> > On Mon, Dec 14, 2020 at 6:57 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> > >
> > > Quoting Douglas Anderson (2020-12-14 16:30:19)
> > > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> > > > index 6f736e94e9f4..5ef2e9f38ac9 100644
> > > > --- a/drivers/spi/spi-geni-qcom.c
> > > > +++ b/drivers/spi/spi-geni-qcom.c
> >
> > > > +       spin_lock_irq(&mas->lock);
> > > > +       m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS);
> > > > +       m_irq_en = readl(se->base + SE_GENI_M_IRQ_EN);
> > > > +       spin_unlock_irq(&mas->lock);
> > > > +
> > > > +       if (m_irq & m_irq_en) {
> > >
> > > Is this really "busy" though? If we canceled something out then maybe
> > > the irq has fired but what if it's to tell us that we have some
> > > available space in the TX fifo? Does that really matter? It seems like
> > > if we have an RX irq when we're starting a transfer that might be bad
> > > too but we could forcibly clear that by acking it here and then setting
> > > the fifo word count that we're expecting for rx?
> > >
> > > Put another way, why isn't this driver looking at the TX and RX fifo
> > > status registers more than in one place?
> >
> > I'm not sure I understand all your concerns.  Can you clarify?  In
> > case it helps, I'll add a few thoughts here:
> >
> > 1. SPI is a controller clocked protocol and this is the driver for the
> > controller.  There is no way to get a RX IRQ unless we initiate it.
> >
> > 2. The code always takes care to make sure that when we're done with a
> > transfer that we disable the TX watermark.  This means we won't get
> > any more interrupts.
> >
> > The only time an interrupt could still be pending when we start a new
> > transfer is:
> >
> > a) If the interrupt handler is still running on another CPU.  In that
> > case it will have the spinlock and won't release it until it clears
> > the interrupts.
> >
> > b) If we had a timeout on the previous transfer and then got timeouts
> > sending the cancel and abort.
> >
> > In general when we're starting a new transfer we assume that we can
> > program the hardware willy-nilly.  If there's some chance something
> > else is happening (or our interrupt could go off) then it breaks that
> > whole model.
>
> Right. I thought this patch was making sure that the hardware wasn't in
> the process of doing something else when we setup the transfer. I'm
> saying that only checking the irq misses the fact that maybe the
> transfer hasn't completed yet or a pending irq hasn't come in yet, but
> the fifo status would tell us that the fifo is transferring something or
> receiving something. If an RX can't happen, then the code should clearly
> show that an RX irq isn't expected, and mask out that bit so it is
> ignored or explicitly check for it and call WARN_ON() if the bit is set.
>
> I'm wondering why we don't check the FIFO status and the irq bits to
> make sure that some previous cancelled operation isn't still pending
> either in the FIFO or as an irq. While this patch will fix the scenario
> where the irq is delayed but pending in the hardware it won't cover the
> case that the hardware itself is wedged, for example because the
> sequencer just decided to stop working entirely.

It also won't catch the case where the SoC decided that all GPIOs are
inverted and starts reporting highs for lows and lows for highs, nor
does it handle the case where the CPU suddenly switches to Big Endian
mode for no reason.  :-P

...by that, I mean I'm not trying to catch the case where the hardware
itself is behaving in a totally unexpected way.  I have seen no
instances where the hardware wedges nor where the sequencer stops
working and until I see them happen I'm not inclined to add code for
them.  Without seeing them actually happen I'm not really sure what
the right way to recover would be.  We've already tried "cancel" and
"abort" and then waited at least 1 second.  If you know of some sort
of magic "unwedge" then we should add it into handle_fifo_timeout().

However, super delayed interrupts due to software not servicing the
interrupt in time is something that really happens, if rarely.  Adding
code to account for that seems worth it and is easy to test...

Did I convince you?  ;-)

-Doug