On Tue, Aug 27, 2019 at 8:11 PM Tony Chuang <yhchuang@xxxxxxxxxxx> 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
