Date:   Tue, 15 Dec 2020 14:25:33 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Mark Brown <broonie@...nel.org>, msavaliy@....qualcomm.com,
        Akash Asthana <akashast@...eaurora.org>,
        Roja Rani Yarubandi <rojay@...eaurora.org>,
        Alok Chauhan <alokc@...eaurora.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-spi <linux-spi@...r.kernel.org>
Subject: Re: [PATCH 2/2] spi: spi-geni-qcom: Really ensure the previous xfer is done before new one

Quoting Doug Anderson (2020-12-15 09:25:51)
> On Mon, Dec 14, 2020 at 6:57 PM Stephen Boyd <swboyd@...omium.org> 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.

Powered by blists - more mailing lists