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: <1565166487-22048-1-git-send-email-yhchuang@realtek.com>
 <20190823063728.14117-1-drake@endlessm.com> <CA+K+NcSYKEkdx5ux6iwUs7pMidObZBrg9yDcP1zT73DcccpDPQ@mail.gmail.com>
 <F7CD281DE3E379468C6D07993EA72F84D18AE7C8@RTITMBSVM04.realtek.com.tw>
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D18AE7C8@RTITMBSVM04.realtek.com.tw>
From:   Daniel Drake <drake@endlessm.com>
Date:   Wed, 28 Aug 2019 15:04:29 +0800
Message-ID: <CAD8Lp46=Adn3GWAkHspdqBHVjz6sekET9nzNOCnW7sCFGtoeDQ@mail.gmail.com>
Subject: Re: [PATCH v2] rtw88: pci: enable MSI interrupt
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     =?UTF-8?B?SsOhbiBWZXNlbMO9?= <jano.vesely@gmail.com>,
        "briannorris@chromium.org" <briannorris@chromium.org>,
        "gojun077@gmail.com" <gojun077@gmail.com>,
        "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        "linux@endlessm.com" <linux@endlessm.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Tue, Aug 27, 2019 at 8:11 PM Tony Chuang <yhchuang@realtek.com> wrote:
> Because there's a race condition between SW/HW when clearing the ISR.
> If interrupt comes after reading ISR and before write-1-clear, the interrupt
> controller would have interrupt status raised, and never issue interrupt
> signal to host when other new interrupts status are raised.
>
> To avoid this, driver requires to protect the ISR write-1-clear process by
> disabling the IMR.

Just to be clear with an example of two interrupts in quick
succession, first ROK then MGNTDOK. I would expect the hardware to
behave as follows:

1. Interrupts are enabled in HIMR reg. MSI is in use.
--- Hardware needs to flag RX condition. It sets IMR_ROK flag in HISR,
then raises a MSI interrupt.
--- Interrupt controller receives this and begins handling it:
2. rtw88 interrupt handler begins execution
3. rtw_pci_irq_recognized()  reads HISR values. ROK is the only bit set.
--- Hardware needs to flag MGNTDOK condition, so it sets the MGNTDOK
bit in HISR and raises another interrupt.
--- Interrupt controller queues this interrupt since it's in the
middle of handling the original ROK interrupt.
4. Back in rtw_pci_irq_recognized(), the HISR values from earlier are
written back to clear them. In this case just the ROK bit is written
back to unset the interrupt. MGNTDOK interrupt is left pending (not
yet noticed by the driver)
5. Interrupt handler continues & finishes handling the RX event.
6. With the first interrupt done, interrupt controller handles the
queued interrupt and causes rtw88 interrupt handler to execute again
7. rtw88 interrupt handler handles and MGNTDOK interrupt.


But are you saying it does not do this, and the behaviour (without
your change) is instead:

1. Interrupts are enabled in HIMR reg. MSI is in use.
--- Hardware needs to flag RX condition. It sets IMR_ROK flag in HISR,
then raises a MSI interrupt.
--- Interrupt controller receives this and begins handling it:
2. rtw88 interrupt handler begins execution
3. rtw_pci_irq_recognized()  reads HISR values. ROK is the only bit set.
--- Hardware needs to flag MGNTDOK condition, so it sets the MGNTDOK
bit in HISR. However it does NOT raise an interrupt since other bits
in HISR were still set at this time.
4. Back in rtw_pci_irq_recognized(), the HISR values from earlier are
written back. In this case just the ROK bit is written back to unset
the interrupt. MGNTDOK interrupt is left pending (not yet noticed by
the driver)
5. Interrupt handler continues & finishes handling the RX event.
6. MGNTDOK interrupt is left pending, and no more interrupts will be
generated by the hardware because even if it sets more HISR bits, the
MGNTDOK bit was already set so it doesn't raise more interrupts.

i.e. you're effectively saying that the hardware will *only* generate
an interrupt when *all* HISR bits were zero immediately before the new
interrupt HISR bit is set?


And then with your change applied it would look like this:

1. Interrupts are enabled in HIMR reg. MSI is in use.
--- Hardware needs to flag RX condition. It sets IMR_ROK flag in HISR,
then raises a MSI interrupt.
--- Interrupt controller receives this and begins handling it:
2. rtw88 interrupt handler begins execution
3. Interrupts are disabled in HIMR reg.
4. rtw_pci_irq_recognized()  reads HISR values. ROK is the only bit set.
--- Hardware needs to flag MGNTDOK condition, however because
interrupts are disabled in HIMR, it simply queues this condition
internally, without affecting HISR values, without generating another
interrupt.
4. Back in rtw_pci_irq_recognized(), the HISR values from earlier are
written back. In this case just the ROK bit is written back to unset
the interrupt. HISR is now value 0.
5. Interrupt handler handles the RX event.
6. Interrupt handler re-enables interrupts in HIMR just before finishing.
--- In response, hardware checks its internal queue and realises that
a MGNTDOK condition is pending. It sets the MGNTDOK bit in HISR and
raises a new interrupt.

Is that right? It seems like strange behaviour on the hardware side.

> Do you mean I should remove this?
> But I cannot find another proper way to enable the MSI.
> Maybe pci_alloc_irq_vectors() could work but I am not sure if
> It is recommended.

Yes, I think you should switch to pci_alloc_irq_vectors() which is the
recommended, documented way.

Thanks,
Daniel