| Message ID | 20240701071416.8468-3-jiawenwu@trustnetic.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bd07a98178462e7a02ed2bf7dec90a00944c1da5 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: txgbe: fix MSI and INTx interrupts | expand |
On Mon, Jul 01, 2024 at 03:14:14PM +0800, Jiawen Wu wrote: > When using MSI or INTx interrupts, request_irq() for pdev->irq will > conflict with request_threaded_irq() for txgbe->misc.irq, to cause > system crash. So remove txgbe_request_irq() for MSI/INTx case, and > rename txgbe_request_msix_irqs() since it only request for queue irqs. > Could you please provide any test scenario how the crash can be reproduced and/or add an example crash log? Also, could you describe how request_irq() conflicts with request_threaded_irq() in this case? > Add wx->misc_irq_domain to determine whether the driver creates an IRQ > domain and threaded request the IRQs. > > Fixes: aefd013624a1 ("net: txgbe: use irq_domain for interrupt controller") > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> > --- > drivers/net/ethernet/wangxun/libwx/wx_hw.c | 1 + > drivers/net/ethernet/wangxun/libwx/wx_lib.c | 5 +- > drivers/net/ethernet/wangxun/libwx/wx_type.h | 1 + > .../net/ethernet/wangxun/txgbe/txgbe_irq.c | 80 ++----------------- > .../net/ethernet/wangxun/txgbe/txgbe_irq.h | 2 +- > .../net/ethernet/wangxun/txgbe/txgbe_main.c | 2 +- > 6 files changed, 15 insertions(+), 76 deletions(-) > > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c > index 7c4b6881a93f..d1b682ce9c6d 100644 > --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c > +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c > @@ -1959,6 +1959,7 @@ int wx_sw_init(struct wx *wx) > } > > bitmap_zero(wx->state, WX_STATE_NBITS); > + wx->misc_irq_domain = false; > > return 0; > } > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c > index f53776877f71..e1f514b21090 100644 > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c > @@ -1997,7 +1997,8 @@ void wx_free_irq(struct wx *wx) > int vector; > > if (!(pdev->msix_enabled)) { > - free_irq(pdev->irq, wx); > + if (!wx->misc_irq_domain) > + free_irq(pdev->irq, wx); Does it mean "pdev->irq" will never be freed if you set misc_irq_domain to "true"? It seems you set it to true always during the initializaion, in "txgbe_setup_misc_irq()"? > return; > } > > @@ -2012,7 +2013,7 @@ void wx_free_irq(struct wx *wx) > free_irq(entry->vector, q_vector); > } > > - if (wx->mac.type == wx_mac_em) > + if (!wx->misc_irq_domain) > free_irq(wx->msix_entry->vector, wx); > } > EXPORT_SYMBOL(wx_free_irq); > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h > index 5aaf7b1fa2db..0df7f5712b6f 100644 > --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h > +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h > @@ -1058,6 +1058,7 @@ struct wx { > dma_addr_t isb_dma; > u32 *isb_mem; > u32 isb_tag[WX_ISB_MAX]; > + bool misc_irq_domain; I don't think that introducing a kind of global variable to determine the call context is a good idea. Also, it seems that member is always true after the "probe" context is completed, isn't it? > [...] > @@ -256,6 +190,8 @@ int txgbe_setup_misc_irq(struct txgbe *txgbe) > if (err) > goto free_gpio_irq; > > + wx->misc_irq_domain = true; > + > return 0; > Is there any chance that member will be set back to "false" after the initialization is completed? Thanks, Michal
On Wed, Jul 03, 2024 at 03:33:24PM +0200, Michal Kubiak wrote: > > if (!(pdev->msix_enabled)) { > > - free_irq(pdev->irq, wx); > > + if (!wx->misc_irq_domain) > > + free_irq(pdev->irq, wx); > > Does it mean "pdev->irq" will never be freed if you set misc_irq_domain > to "true"? It seems you set it to true always during the initializaion, > in "txgbe_setup_misc_irq()"? [...] > > > I don't think that introducing a kind of global variable to determine the call > context is a good idea. > Also, it seems that member is always true after the "probe" context is > completed, isn't it? > [...] > > Is there any chance that member will be set back to "false" after the > initialization is completed? > > > Thanks, > Michal > OK, I think I understood that change better. You probably just want to distinguish the calling your library function between "ngbe" and "txgbe" drivers. In such a case using a structure member in your board structure seems to be justified. If so, please ignore my questions. Anyway, I didn't notice that the patch is already applied to the tree before I sent my comment. Apologies for that. Thanks, Michal
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c index 7c4b6881a93f..d1b682ce9c6d 100644 --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c @@ -1959,6 +1959,7 @@ int wx_sw_init(struct wx *wx) } bitmap_zero(wx->state, WX_STATE_NBITS); + wx->misc_irq_domain = false; return 0; } diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c index f53776877f71..e1f514b21090 100644 --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c @@ -1997,7 +1997,8 @@ void wx_free_irq(struct wx *wx) int vector; if (!(pdev->msix_enabled)) { - free_irq(pdev->irq, wx); + if (!wx->misc_irq_domain) + free_irq(pdev->irq, wx); return; } @@ -2012,7 +2013,7 @@ void wx_free_irq(struct wx *wx) free_irq(entry->vector, q_vector); } - if (wx->mac.type == wx_mac_em) + if (!wx->misc_irq_domain) free_irq(wx->msix_entry->vector, wx); } EXPORT_SYMBOL(wx_free_irq); diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h index 5aaf7b1fa2db..0df7f5712b6f 100644 --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h @@ -1058,6 +1058,7 @@ struct wx { dma_addr_t isb_dma; u32 *isb_mem; u32 isb_tag[WX_ISB_MAX]; + bool misc_irq_domain; #define WX_MAX_RETA_ENTRIES 128 #define WX_RSS_INDIR_TBL_MAX 64 diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c index b3e3605d1edb..1490fd6ddbdf 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c @@ -27,57 +27,19 @@ void txgbe_irq_enable(struct wx *wx, bool queues) } /** - * txgbe_intr - msi/legacy mode Interrupt Handler - * @irq: interrupt number - * @data: pointer to a network interface device structure - **/ -static irqreturn_t txgbe_intr(int __always_unused irq, void *data) -{ - struct wx_q_vector *q_vector; - struct wx *wx = data; - struct pci_dev *pdev; - u32 eicr; - - q_vector = wx->q_vector[0]; - pdev = wx->pdev; - - eicr = wx_misc_isb(wx, WX_ISB_VEC0); - if (!eicr) { - /* shared interrupt alert! - * the interrupt that we masked before the ICR read. - */ - if (netif_running(wx->netdev)) - txgbe_irq_enable(wx, true); - return IRQ_NONE; /* Not our interrupt */ - } - wx->isb_mem[WX_ISB_VEC0] = 0; - if (!(pdev->msi_enabled)) - wr32(wx, WX_PX_INTA, 1); - - wx->isb_mem[WX_ISB_MISC] = 0; - /* would disable interrupts here but it is auto disabled */ - napi_schedule_irqoff(&q_vector->napi); - - /* re-enable link(maybe) and non-queue interrupts, no flush. - * txgbe_poll will re-enable the queue interrupts - */ - if (netif_running(wx->netdev)) - txgbe_irq_enable(wx, false); - - return IRQ_HANDLED; -} - -/** - * txgbe_request_msix_irqs - Initialize MSI-X interrupts + * txgbe_request_queue_irqs - Initialize MSI-X queue interrupts * @wx: board private structure * - * Allocate MSI-X vectors and request interrupts from the kernel. + * Allocate MSI-X queue vectors and request interrupts from the kernel. **/ -static int txgbe_request_msix_irqs(struct wx *wx) +int txgbe_request_queue_irqs(struct wx *wx) { struct net_device *netdev = wx->netdev; int vector, err; + if (!wx->pdev->msix_enabled) + return 0; + for (vector = 0; vector < wx->num_q_vectors; vector++) { struct wx_q_vector *q_vector = wx->q_vector[vector]; struct msix_entry *entry = &wx->msix_q_entries[vector]; @@ -110,34 +72,6 @@ static int txgbe_request_msix_irqs(struct wx *wx) return err; } -/** - * txgbe_request_irq - initialize interrupts - * @wx: board private structure - * - * Attempt to configure interrupts using the best available - * capabilities of the hardware and kernel. - **/ -int txgbe_request_irq(struct wx *wx) -{ - struct net_device *netdev = wx->netdev; - struct pci_dev *pdev = wx->pdev; - int err; - - if (pdev->msix_enabled) - err = txgbe_request_msix_irqs(wx); - else if (pdev->msi_enabled) - err = request_irq(wx->pdev->irq, &txgbe_intr, 0, - netdev->name, wx); - else - err = request_irq(wx->pdev->irq, &txgbe_intr, IRQF_SHARED, - netdev->name, wx); - - if (err) - wx_err(wx, "request_irq failed, Error %d\n", err); - - return err; -} - static int txgbe_request_gpio_irq(struct txgbe *txgbe) { txgbe->gpio_irq = irq_find_mapping(txgbe->misc.domain, TXGBE_IRQ_GPIO); @@ -256,6 +190,8 @@ int txgbe_setup_misc_irq(struct txgbe *txgbe) if (err) goto free_gpio_irq; + wx->misc_irq_domain = true; + return 0; free_gpio_irq: diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.h index b77945e7a0f2..e6285b94625e 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.h +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.h @@ -2,6 +2,6 @@ /* Copyright (c) 2015 - 2024 Beijing WangXun Technology Co., Ltd. */ void txgbe_irq_enable(struct wx *wx, bool queues); -int txgbe_request_irq(struct wx *wx); +int txgbe_request_queue_irqs(struct wx *wx); void txgbe_free_misc_irq(struct txgbe *txgbe); int txgbe_setup_misc_irq(struct txgbe *txgbe); diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c index 8c7a74981b90..76b5672c0a17 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c @@ -294,7 +294,7 @@ static int txgbe_open(struct net_device *netdev) wx_configure(wx); - err = txgbe_request_irq(wx); + err = txgbe_request_queue_irqs(wx); if (err) goto err_free_isb;
When using MSI or INTx interrupts, request_irq() for pdev->irq will conflict with request_threaded_irq() for txgbe->misc.irq, to cause system crash. So remove txgbe_request_irq() for MSI/INTx case, and rename txgbe_request_msix_irqs() since it only request for queue irqs. Add wx->misc_irq_domain to determine whether the driver creates an IRQ domain and threaded request the IRQs. Fixes: aefd013624a1 ("net: txgbe: use irq_domain for interrupt controller") Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> --- drivers/net/ethernet/wangxun/libwx/wx_hw.c | 1 + drivers/net/ethernet/wangxun/libwx/wx_lib.c | 5 +- drivers/net/ethernet/wangxun/libwx/wx_type.h | 1 + .../net/ethernet/wangxun/txgbe/txgbe_irq.c | 80 ++----------------- .../net/ethernet/wangxun/txgbe/txgbe_irq.h | 2 +- .../net/ethernet/wangxun/txgbe/txgbe_main.c | 2 +- 6 files changed, 15 insertions(+), 76 deletions(-)