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;
Content-Type: text/plain;
        charset=utf-8;
        delsp=yes;
        format=flowed
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH] rtw88: pci: enable MSI interrupt
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
In-Reply-To: <CA+ASDXPFVHaXM_5VtWNeTW8mPSZi6EX9JYoJRn4RTygA6iKQpg@mail.gmail.com>
Date:   Tue, 20 Aug 2019 23:21:41 +0800
Cc:     Tony Chuang <yhchuang@realtek.com>,
        "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-Transfer-Encoding: 8bit
Message-Id: <1F1A57A7-3240-4595-B4A9-EF28A54B77E0@canonical.com>
References: <1564487414-9615-1-git-send-email-yhchuang@realtek.com>
 <20190730195703.GA224792@google.com>
 <F7CD281DE3E379468C6D07993EA72F84D18855BB@RTITMBSVM04.realtek.com.tw>
 <CA+ASDXPFVHaXM_5VtWNeTW8mPSZi6EX9JYoJRn4RTygA6iKQpg@mail.gmail.com>
To:     Brian Norris <briannorris@chromium.org>
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

at 00:51, Brian Norris <briannorris@chromium.org> wrote:

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

According to user feedback [1], this patch makes rtw88 work.
I’ll make it merged into Ubuntu’s kernel for now, hopefully this will be in  
upstream version soon.

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1838133/comments/48

Kai-Heng

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