diff mbox series

[net] net: txgbe: fix MSI and INTx interrupts

Message ID 20240621080951.14368-1-jiawenwu@trustnetic.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] net: txgbe: fix MSI and INTx interrupts | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 859 this patch: 859
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 863 this patch: 863
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 863 this patch: 863
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 227 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 23 this patch: 21
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-25--09-00 (tests: 662)

Commit Message

Jiawen Wu June 21, 2024, 8:09 a.m. UTC
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.

Fixes: aefd013624a1 ("net: txgbe: use irq_domain for interrupt controller")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   |  13 +-
 .../net/ethernet/wangxun/txgbe/txgbe_irq.c    | 122 +++++++-----------
 .../net/ethernet/wangxun/txgbe/txgbe_irq.h    |   2 +-
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   |   3 +-
 4 files changed, 59 insertions(+), 81 deletions(-)

Comments

Paolo Abeni June 25, 2024, 11 a.m. UTC | #1
On Fri, 2024-06-21 at 16:09 +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.
> 
> Fixes: aefd013624a1 ("net: txgbe: use irq_domain for interrupt controller")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

I think you should add a longish description here.
This fix is fairly non trivial but the above contains no hint on how
and why the fix is supposed to address the issue.

Thanks,

Paolo
Przemek Kitszel June 25, 2024, 11:40 a.m. UTC | #2
On 6/21/24 10:09, 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.
> 
> Fixes: aefd013624a1 ("net: txgbe: use irq_domain for interrupt controller")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>   drivers/net/ethernet/wangxun/libwx/wx_lib.c   |  13 +-
>   .../net/ethernet/wangxun/txgbe/txgbe_irq.c    | 122 +++++++-----------
>   .../net/ethernet/wangxun/txgbe/txgbe_irq.h    |   2 +-
>   .../net/ethernet/wangxun/txgbe/txgbe_main.c   |   3 +-
>   4 files changed, 59 insertions(+), 81 deletions(-)
> 

Please split into two commits (by prepending one commit that will just
move/rename function/s) to avoid inflating the diff of the actual fix.
This will ease the review process.

[...]

> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
> index b3e3605d1edb..15e0fef02aac 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;
> -}
> -

[...]

> -
>   static int txgbe_request_gpio_irq(struct txgbe *txgbe)
>   {
>   	txgbe->gpio_irq = irq_find_mapping(txgbe->misc.domain, TXGBE_IRQ_GPIO);
> @@ -177,6 +111,36 @@ static const struct irq_domain_ops txgbe_misc_irq_domain_ops = {
>   };
>   
>   static irqreturn_t txgbe_misc_irq_handle(int irq, void *data)
> +{
> +	struct wx_q_vector *q_vector;
> +	struct txgbe *txgbe = data;
> +	struct wx *wx = txgbe->wx;
> +	u32 eicr;
> +
> +	if (wx->pdev->msix_enabled)
> +		return IRQ_WAKE_THREAD;
> +
> +	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 (!(wx->pdev->msi_enabled))
> +		wr32(wx, WX_PX_INTA, 1);
> +
> +	/* would disable interrupts here but it is auto disabled */
> +	q_vector = wx->q_vector[0];
> +	napi_schedule_irqoff(&q_vector->napi);
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +

[...]
Jiawen Wu June 26, 2024, 3:11 a.m. UTC | #3
On Tue, June 25, 2024 7:41 PM, Przemek Kitszel wrote:
> On 6/21/24 10:09, 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.
> >
> > Fixes: aefd013624a1 ("net: txgbe: use irq_domain for interrupt controller")
> > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > ---
> >   drivers/net/ethernet/wangxun/libwx/wx_lib.c   |  13 +-
> >   .../net/ethernet/wangxun/txgbe/txgbe_irq.c    | 122 +++++++-----------
> >   .../net/ethernet/wangxun/txgbe/txgbe_irq.h    |   2 +-
> >   .../net/ethernet/wangxun/txgbe/txgbe_main.c   |   3 +-
> >   4 files changed, 59 insertions(+), 81 deletions(-)
> >
> 
> Please split into two commits (by prepending one commit that will just
> move/rename function/s) to avoid inflating the diff of the actual fix.
> This will ease the review process.

I will split it into two commits to make it easier to review, but it may not
be just renaming the function in one commit.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index 68bde91b67a0..f098758893b3 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;
 }
@@ -1996,7 +1997,8 @@  void wx_free_irq(struct wx *wx)
 	int vector;
 
 	if (!(pdev->msix_enabled)) {
-		free_irq(pdev->irq, wx);
+		if (wx->mac.type == wx_mac_em)
+			free_irq(pdev->irq, wx);
 		return;
 	}
 
@@ -2026,6 +2028,9 @@  int wx_setup_isb_resources(struct wx *wx)
 {
 	struct pci_dev *pdev = wx->pdev;
 
+	if (wx->isb_mem)
+		return 0;
+
 	wx->isb_mem = dma_alloc_coherent(&pdev->dev,
 					 sizeof(u32) * 4,
 					 &wx->isb_dma,
@@ -2049,6 +2054,9 @@  void wx_free_isb_resources(struct wx *wx)
 {
 	struct pci_dev *pdev = wx->pdev;
 
+	if (!wx->isb_mem)
+		return;
+
 	dma_free_coherent(&pdev->dev, sizeof(u32) * 4,
 			  wx->isb_mem, wx->isb_dma);
 	wx->isb_mem = NULL;
@@ -2385,7 +2393,8 @@  static void wx_free_all_tx_resources(struct wx *wx)
 
 void wx_free_resources(struct wx *wx)
 {
-	wx_free_isb_resources(wx);
+	if (wx->mac.type == wx_mac_em)
+		wx_free_isb_resources(wx);
 	wx_free_all_rx_resources(wx);
 	wx_free_all_tx_resources(wx);
 }
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
index b3e3605d1edb..15e0fef02aac 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);
@@ -177,6 +111,36 @@  static const struct irq_domain_ops txgbe_misc_irq_domain_ops = {
 };
 
 static irqreturn_t txgbe_misc_irq_handle(int irq, void *data)
+{
+	struct wx_q_vector *q_vector;
+	struct txgbe *txgbe = data;
+	struct wx *wx = txgbe->wx;
+	u32 eicr;
+
+	if (wx->pdev->msix_enabled)
+		return IRQ_WAKE_THREAD;
+
+	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 (!(wx->pdev->msi_enabled))
+		wr32(wx, WX_PX_INTA, 1);
+
+	/* would disable interrupts here but it is auto disabled */
+	q_vector = wx->q_vector[0];
+	napi_schedule_irqoff(&q_vector->napi);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t txgbe_misc_irq_thread_fn(int irq, void *data)
 {
 	struct txgbe *txgbe = data;
 	struct wx *wx = txgbe->wx;
@@ -223,6 +187,7 @@  void txgbe_free_misc_irq(struct txgbe *txgbe)
 
 int txgbe_setup_misc_irq(struct txgbe *txgbe)
 {
+	unsigned long flags = IRQF_ONESHOT;
 	struct wx *wx = txgbe->wx;
 	int hwirq, err;
 
@@ -236,14 +201,17 @@  int txgbe_setup_misc_irq(struct txgbe *txgbe)
 		irq_create_mapping(txgbe->misc.domain, hwirq);
 
 	txgbe->misc.chip = txgbe_irq_chip;
-	if (wx->pdev->msix_enabled)
+	if (wx->pdev->msix_enabled) {
 		txgbe->misc.irq = wx->msix_entry->vector;
-	else
+	} else {
 		txgbe->misc.irq = wx->pdev->irq;
+		if (!wx->pdev->msi_enabled)
+			flags |= IRQF_SHARED;
+	}
 
-	err = request_threaded_irq(txgbe->misc.irq, NULL,
-				   txgbe_misc_irq_handle,
-				   IRQF_ONESHOT,
+	err = request_threaded_irq(txgbe->misc.irq, txgbe_misc_irq_handle,
+				   txgbe_misc_irq_thread_fn,
+				   flags,
 				   wx->netdev->name, txgbe);
 	if (err)
 		goto del_misc_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..92c1fae826d0 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;
 
@@ -729,6 +729,7 @@  static void txgbe_remove(struct pci_dev *pdev)
 
 	txgbe_remove_phy(txgbe);
 	txgbe_free_misc_irq(txgbe);
+	wx_free_isb_resources(wx);
 
 	pci_release_selected_regions(pdev,
 				     pci_select_bars(pdev, IORESOURCE_MEM));