| Message ID | 1565166487-22048-1-git-send-email-yhchuang@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] rtw88: pci: enable MSI interrupt | expand |
> + rtw_pci_disable_interrupt(rtwdev, rtwpci); I checked the discussion on the v1 patch thread but I still don't follow this. You're worried about the case where we're inside the interrupt handler and: 1. We read the interrupt status to note what needs to be done 2. <another interrupt arrives here, requiring other work to be done> 3. We clear the interrupt status bits 4. We proceed to handle the interrupt but missing any work requested by the interrupt in step 2. Is that right? I'm not an expert here, but I don't think this is something that drivers have to worry about. Surely the interrupt controller can be expected to have a mechanism to "queue up" any interrupt that arrives while an interrupt is being handled? Otherwise handling of all types of edge-triggered interrupts (not just MSI) would be overly painful across the board. See e.g. https://patchwork.kernel.org/patch/3333681/ as a reference for what correct interrupt controller behaviour should look like. > + ret = pci_enable_msi(pdev); pci_enable_msi() is "deprecated, don't use" Thanks Daniel
On Fri, Aug 23, 2019 at 2:37 AM Daniel Drake <drake@endlessm.com> wrote: > > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); > > I checked the discussion on the v1 patch thread but I still don't follow > this. > > You're worried about the case where we're inside the interrupt handler and: > 1. We read the interrupt status to note what needs to be done > 2. <another interrupt arrives here, requiring other work to be done> > 3. We clear the interrupt status bits > 4. We proceed to handle the interrupt but missing any work requested by > the interrupt in step 2. > > Is that right? > > I'm not an expert here, but I don't think this is something that drivers > have to worry about. Surely the interrupt controller can be expected to > have a mechanism to "queue up" any interrupt that arrives while an > interrupt is being handled? Otherwise handling of all types of > edge-triggered interrupts (not just MSI) would be overly painful across the > board. That's my understanding as well. entering the interrupt vector clears the IFLAG, so any interrupt will wait until the IFLAG is restored, or delivered to a different CPU. wouldn't it be safer to enable interrupts only _after_ registering the handler in the "rtw_pci_request_irq" function? regards, Jan > > See e.g. https://patchwork.kernel.org/patch/3333681/ as a reference for > what correct interrupt controller behaviour should look like. > > > + ret = pci_enable_msi(pdev); > > pci_enable_msi() is "deprecated, don't use" > > Thanks > Daniel >
Hi Daniel and Ján, > From: Ján Veselý > Sent: Friday, August 23, 2019 3:22 PM > On Fri, Aug 23, 2019 at 2:37 AM Daniel Drake <drake@endlessm.com> wrote: > > > > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); > > > > I checked the discussion on the v1 patch thread but I still don't follow > > this. > > > > You're worried about the case where we're inside the interrupt handler and: > > 1. We read the interrupt status to note what needs to be done > > 2. <another interrupt arrives here, requiring other work to be done> > > 3. We clear the interrupt status bits > > 4. We proceed to handle the interrupt but missing any work requested by > > the interrupt in step 2. > > > > Is that right? > > > > I'm not an expert here, but I don't think this is something that drivers > > have to worry about. Surely the interrupt controller can be expected to > > have a mechanism to "queue up" any interrupt that arrives while an > > interrupt is being handled? Otherwise handling of all types of > > edge-triggered interrupts (not just MSI) would be overly painful across the > > board. > > That's my understanding as well. > entering the interrupt vector clears the IFLAG, so any interrupt will > wait until the IFLAG is restored, or delivered to a different CPU. > wouldn't it be safer to enable interrupts only _after_ registering the > handler in the "rtw_pci_request_irq" function? > > regards, > Jan Yes that's not something drivers need to care about. But I think it is 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. > > > > > > See e.g. https://patchwork.kernel.org/patch/3333681/ as a reference for > > what correct interrupt controller behaviour should look like. > > > > > + ret = pci_enable_msi(pdev); > > > > pci_enable_msi() is "deprecated, don't use" 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. Yan-Hsuan
Tony Chuang <yhchuang@realtek.com> 於 2019年8月27日 週二 下午8:12寫道： > > Hi Daniel and Ján, > > > > From: Ján Veselý > Sent: Friday, August 23, 2019 3:22 PM > > On Fri, Aug 23, 2019 at 2:37 AM Daniel Drake <drake@endlessm.com> wrote: > > > > > > > + rtw_pci_disable_interrupt(rtwdev, rtwpci); > > > > > > I checked the discussion on the v1 patch thread but I still don't follow > > > this. > > > > > > You're worried about the case where we're inside the interrupt handler and: > > > 1. We read the interrupt status to note what needs to be done > > > 2. <another interrupt arrives here, requiring other work to be done> > > > 3. We clear the interrupt status bits > > > 4. We proceed to handle the interrupt but missing any work requested by > > > the interrupt in step 2. > > > > > > Is that right? > > > > > > I'm not an expert here, but I don't think this is something that drivers > > > have to worry about. Surely the interrupt controller can be expected to > > > have a mechanism to "queue up" any interrupt that arrives while an > > > interrupt is being handled? Otherwise handling of all types of > > > edge-triggered interrupts (not just MSI) would be overly painful across the > > > board. > > > > That's my understanding as well. > > entering the interrupt vector clears the IFLAG, so any interrupt will > > wait until the IFLAG is restored, or delivered to a different CPU. > > wouldn't it be safer to enable interrupts only _after_ registering the > > handler in the "rtw_pci_request_irq" function? > > > > regards, > > Jan > > > Yes that's not something drivers need to care about. But I think it is > 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. > > > > > > > > > > > > See e.g. https://patchwork.kernel.org/patch/3333681/ as a reference for > > > what correct interrupt controller behaviour should look like. > > > > > > > + ret = pci_enable_msi(pdev); > > > > > > pci_enable_msi() is "deprecated, don't use" > > 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. According to the kernel documentation "The MSI Driver Guide HOWTO", pci_alloc_irq_vectors, pci_irq_vector and pci_free_irq_vectors are the functions. https://elixir.bootlin.com/linux/v5.3-rc6/source/Documentation/PCI/msi-howto.rst Here is an example in r8169 module. https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/net/ethernet/realtek/r8169_main.c#L6603 Jian-Hong Pan
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
Hi, A few scattered comments below. > From: Daniel Drake [mailto:drake@endlessm.com] > > 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? 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
On Mon, Sep 2, 2019 at 11:02 AM Tony Chuang <yhchuang@realtek.com> wrote: > 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. OK, thanks for checking & explaining. It's a bit hard to put in words as we have seen, but it would be good if you could try to summarise this unusual design in the commit message or in a code comment. Thanks! Daniel
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index 00ef229..c8c7951 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.c +++ b/drivers/net/wireless/realtek/rtw88/pci.c @@ -11,6 +11,10 @@ #include "fw.h" #include "debug.h" +static bool rtw_disable_msi; +module_param_named(disable_msi, rtw_disable_msi, bool, 0644); +MODULE_PARM_DESC(disable_msi, "Set Y to disable MSI interrupt support"); + static u32 rtw_pci_tx_queue_idx_addr[] = { [RTW_TX_QUEUE_BK] = RTK_PCI_TXBD_IDX_BKQ, [RTW_TX_QUEUE_BE] = RTK_PCI_TXBD_IDX_BEQ, @@ -872,6 +876,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, void *dev) if (!rtwpci->irq_enabled) goto out; + rtw_pci_disable_interrupt(rtwdev, rtwpci); rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status); if (irq_status[0] & IMR_MGNTDOK) @@ -891,6 +896,8 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, void *dev) if (irq_status[0] & IMR_ROK) rtw_pci_rx_isr(rtwdev, rtwpci, RTW_RX_QUEUE_MPDU); + rtw_pci_enable_interrupt(rtwdev, rtwpci); + out: spin_unlock(&rtwpci->irq_lock); @@ -1098,6 +1105,46 @@ static struct rtw_hci_ops rtw_pci_ops = { .write_data_h2c = rtw_pci_write_data_h2c, }; +static int rtw_pci_request_irq(struct rtw_dev *rtwdev, struct pci_dev *pdev) +{ + struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv; + int ret; + + if (!rtw_disable_msi) { + ret = pci_enable_msi(pdev); + if (ret) { + rtw_warn(rtwdev, "failed to enable msi %d, using legacy irq\n", + ret); + } else { + rtw_warn(rtwdev, "pci msi enabled\n"); + rtwpci->msi_enabled = true; + } + } + + ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler, IRQF_SHARED, + KBUILD_MODNAME, rtwdev); + if (ret) { + rtw_err(rtwdev, "failed to request irq %d\n", ret); + if (rtwpci->msi_enabled) { + pci_disable_msi(pdev); + rtwpci->msi_enabled = false; + } + } + + return ret; +} + +static void rtw_pci_free_irq(struct rtw_dev *rtwdev, struct pci_dev *pdev) +{ + struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv; + + free_irq(pdev->irq, rtwdev); + if (rtwpci->msi_enabled) { + pci_disable_msi(pdev); + rtwpci->msi_enabled = false; + } +} + static int rtw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -1152,8 +1199,7 @@ static int rtw_pci_probe(struct pci_dev *pdev, goto err_destroy_pci; } - ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler, - IRQF_SHARED, KBUILD_MODNAME, rtwdev); + ret = rtw_pci_request_irq(rtwdev, pdev); if (ret) { ieee80211_unregister_hw(hw); goto err_destroy_pci; @@ -1192,7 +1238,7 @@ static void rtw_pci_remove(struct pci_dev *pdev) rtw_pci_disable_interrupt(rtwdev, rtwpci); rtw_pci_destroy(rtwdev, pdev); rtw_pci_declaim(rtwdev, pdev); - free_irq(rtwpci->pdev->irq, rtwdev); + rtw_pci_free_irq(rtwdev, pdev); rtw_core_deinit(rtwdev); ieee80211_free_hw(hw); } diff --git a/drivers/net/wireless/realtek/rtw88/pci.h b/drivers/net/wireless/realtek/rtw88/pci.h index 87824a4..a8e369c 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.h +++ b/drivers/net/wireless/realtek/rtw88/pci.h @@ -186,6 +186,7 @@ struct rtw_pci { spinlock_t irq_lock; u32 irq_mask[4]; bool irq_enabled; + bool msi_enabled; u16 rx_tag; struct rtw_pci_tx_ring tx_rings[RTK_MAX_TX_QUEUE_NUM];