On Thu, Aug 1, 2019 at 2:21 AM Tony Chuang <yhchuang@xxxxxxxxxxx> wrote: > > Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt > > On Tue, Jul 30, 2019 at 07:50:14PM +0800, yhchuang@xxxxxxxxxxx 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)
