Hi, A few scattered comments below. > From: Daniel Drake [mailto:drake@xxxxxxxxxxxx] > > 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? Yes, that's what I am saying about. I know it looks strange, but I think this is Realtek's design. And as far as I know this has been used for 9-10 years. > > > 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. This might be a little different here, I think the MGNTDOK flag is still set. But new interrupt will not be generated, until HIMR is re-enabled, and I think re-enabling the HIMR could trigger the hardware to check if any HISR bit is set, and then generate 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. > Indeed it is a little strange but it works like that, if I don't disable HIMR, interrupt could be lost and never gets into interrupt handler again. If so, the interrupt will be stopped, and TX/RX path will be freezed. > > Thanks, > Daniel > Thanks, Yan-Hsuan
