Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1564487414-9615-1-git-send-email-yhchuang@realtek.com>
 <20190730195703.GA224792@google.com> <F7CD281DE3E379468C6D07993EA72F84D18855BB@RTITMBSVM04.realtek.com.tw>
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D18855BB@RTITMBSVM04.realtek.com.tw>
From:   Brian Norris <briannorris@chromium.org>
Date:   Thu, 8 Aug 2019 09:51:24 -0700
Message-ID: <CA+ASDXPFVHaXM_5VtWNeTW8mPSZi6EX9JYoJRn4RTygA6iKQpg@mail.gmail.com>
Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "jano.vesely@gmail.com" <jano.vesely@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Thu, Aug 1, 2019 at 2:21 AM Tony Chuang <yhchuang@realtek.com> wrote:
> > Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt
> > On Tue, Jul 30, 2019 at 07:50:14PM +0800, yhchuang@realtek.com wrote:
> > > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > > @@ -874,6 +878,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq,
> > void *dev)
> > >     if (!rtwpci->irq_enabled)
> > >             goto out;
> > >
> > > +   rtw_pci_disable_interrupt(rtwdev, rtwpci);
> >
> > Why exactly do you have to mask interrupts during the ISR? Is there a
> > race in rtw_pci_irq_recognized() or something?
>
>
> I think there is a race between SW and HW, if we do not stop the
> IRQ first, write 1 clear will make the interrupt to be lost.

This doesn't need to slow down this patch (I think v2 is fine), but I
still don't quite understand. Before this addition, the sequence is:
(a) read out your IRQ status
(b) ack the un-masked IRQs you see
(c) operate on those IRQs

Even if a new IRQ comes in the middle of (b), shouldn't it be
sufficient to move on to (c), where you're still prepared to handle
that IRQ?

Or if the IRQ comes after (b), you won't ACK it, and you should
immediately get a new IRQ after you return?

I guess that's assuming that these registers are Write 1 to Clear. But
if so, that means rtw_pci_irq_recognized() is effectively atomic, no?

Also, somewhat unrelated: but why do you unmask HIMR1, when you're not
actually handling any of its IRQ bits?

Brian

> >
> > >     rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status);
> > >
> > >     if (irq_status[0] & IMR_MGNTDOK)