| Message ID | 20240701071416.8468-2-jiawenwu@trustnetic.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7c36711a2cd8059c2d24f5e5c1d76e8ea2d5613c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: txgbe: fix MSI and INTx interrupts | expand |
On Mon, Jul 01, 2024 at 03:14:13PM +0800, Jiawen Wu wrote: > When using MSI/INTx interrupts, wx->num_q_vectors is uninitialized. > Thus there will be kernel panic in wx_alloc_q_vectors() to allocate > queue vectors. > > Fixes: 3f703186113f ("net: libwx: Add irq flow functions") > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> > --- > drivers/net/ethernet/wangxun/libwx/wx_lib.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c > index 68bde91b67a0..f53776877f71 100644 > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c > @@ -1686,6 +1686,7 @@ static int wx_set_interrupt_capability(struct wx *wx) > } > > pdev->irq = pci_irq_vector(pdev, 0); > + wx->num_q_vectors = 1; I would suggest improving readability of that logic. TBH, initially it wasn't obvious to me why you assign 1 to num_q_vectors (instead of nvecs variable). Maybe you just want to exit with an error when nvecs != 1 and avoid some nesting. I think that should make that logic easier to read. For example: /* minmum one for queue, one for misc*/ nvecs = 1; nvecs = pci_alloc_irq_vectors(pdev, nvecs, nvecs, PCI_IRQ_MSI | PCI_IRQ_INTX); if (nvecs != 1) { wx_err(wx, "Failed to allocate MSI/INTx interrupts. Error: %d\n", nvecs); return nvecs; } if (pdev->msi_enabled) wx_err(wx, "Fallback to MSI.\n"); else wx_err(wx, "Fallback to INTx.\n"); pdev->irq = pci_irq_vector(pdev, 0); wx->num_q_vectors = 1; (Please consider it as a suggestion only). > > return 0; > } > -- > 2.27.0 > > Thanks, Michal
On Tue, Jul 2, 2024 11:22 PM, Michal Kubiak wrote: > On Mon, Jul 01, 2024 at 03:14:13PM +0800, Jiawen Wu wrote: > > When using MSI/INTx interrupts, wx->num_q_vectors is uninitialized. > > Thus there will be kernel panic in wx_alloc_q_vectors() to allocate > > queue vectors. > > > > Fixes: 3f703186113f ("net: libwx: Add irq flow functions") > > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> > > --- > > drivers/net/ethernet/wangxun/libwx/wx_lib.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c > > index 68bde91b67a0..f53776877f71 100644 > > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c > > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c > > @@ -1686,6 +1686,7 @@ static int wx_set_interrupt_capability(struct wx *wx) > > } > > > > pdev->irq = pci_irq_vector(pdev, 0); > > + wx->num_q_vectors = 1; > > I would suggest improving readability of that logic. TBH, initially it wasn't > obvious to me why you assign 1 to num_q_vectors (instead of nvecs variable). > Maybe you just want to exit with an error when nvecs != 1 and avoid some nesting. > I think that should make that logic easier to read. For example: > > /* minmum one for queue, one for misc*/ > nvecs = 1; > nvecs = pci_alloc_irq_vectors(pdev, nvecs, > nvecs, PCI_IRQ_MSI | PCI_IRQ_INTX); > if (nvecs != 1) { > wx_err(wx, "Failed to allocate MSI/INTx interrupts. Error: %d\n", nvecs); > return nvecs; > } > > if (pdev->msi_enabled) > wx_err(wx, "Fallback to MSI.\n"); > else > wx_err(wx, "Fallback to INTx.\n"); > > pdev->irq = pci_irq_vector(pdev, 0); > wx->num_q_vectors = 1; > > (Please consider it as a suggestion only). Thanks for the suggestion, it can be considered as a commit for code optimization.
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c index 68bde91b67a0..f53776877f71 100644 --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c @@ -1686,6 +1686,7 @@ static int wx_set_interrupt_capability(struct wx *wx) } pdev->irq = pci_irq_vector(pdev, 0); + wx->num_q_vectors = 1; return 0; }
When using MSI/INTx interrupts, wx->num_q_vectors is uninitialized. Thus there will be kernel panic in wx_alloc_q_vectors() to allocate queue vectors. Fixes: 3f703186113f ("net: libwx: Add irq flow functions") Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> --- drivers/net/ethernet/wangxun/libwx/wx_lib.c | 1 + 1 file changed, 1 insertion(+)