[PATCH net v2 0/2] net: txgbe: fix MSI and INTx interrupts

[PATCH net v2 0/2] net: txgbe: fix MSI and INTx interrupts

[Jiawen Wu]

Fix MSI and INTx interrupts for txgbe driver.

changes in v2:
- Split into two commits.
- Detail commit description.

v1: https://lore.kernel.org/all/20240621080951.14368-1-jiawenwu@trustnetic.com

Jiawen Wu (2):
  net: txgbe: remove separate irq request for MSI and INTx
  net/txgbe: add extra handle for MSI/INTx into thread irq handle

 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(-)

-- 
2.27.0

[PATCH net v2 1/2] net: txgbe: remove separate irq request for MSI and INTx

[Jiawen Wu]

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.

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   |  3 +-
 .../net/ethernet/wangxun/txgbe/txgbe_irq.c    | 78 ++-----------------
 .../net/ethernet/wangxun/txgbe/txgbe_irq.h    |  2 +-
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  2 +-
 4 files changed, 10 insertions(+), 75 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index 68bde91b67a0..99f55a3573c8 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -1996,7 +1996,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;
 	}
 
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
index b3e3605d1edb..ac789ec0091a 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);
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;
 
-- 
2.27.0

[PATCH net v2 2/2] net/txgbe: add extra handle for MSI/INTx into thread irq handle

[Jiawen Wu]

Rename original txgbe_misc_irq_handle() to txgbe_misc_irq_thread_fn()
since it is the handle thread to wake up. And add the primary handler
to deal the case of MSI/INTx, because there is a schedule NAPI poll.

Moreover, do not free isb resources in .ndo_stop, to avoid reading
memory by a null pointer.

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   | 10 ++++-
 .../net/ethernet/wangxun/txgbe/txgbe_irq.c    | 44 ++++++++++++++++---
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  1 +
 3 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index 99f55a3573c8..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;
 }
@@ -2027,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,
@@ -2050,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;
@@ -2386,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 ac789ec0091a..15e0fef02aac 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
@@ -111,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;
@@ -157,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;
 
@@ -170,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_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index 76b5672c0a17..92c1fae826d0 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -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));
-- 
2.27.0

Re: [PATCH net v2 1/2] net: txgbe: remove separate irq request for MSI and INTx

[Jakub Kicinski]

On Wed, 26 Jun 2024 14:07:02 +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.

Do you have any users who need INTx support? Maybe you could drop
the support and simplify the code?

> 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   |  3 +-
>  .../net/ethernet/wangxun/txgbe/txgbe_irq.c    | 78 ++-----------------
>  .../net/ethernet/wangxun/txgbe/txgbe_irq.h    |  2 +-
>  .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  2 +-
>  4 files changed, 10 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index 68bde91b67a0..99f55a3573c8 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> @@ -1996,7 +1996,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);

It seems strange to match on type to decide whether to free an IRQ.
Isn't there or shouldn't there be some IRQ related flag informing
the library how to manage the IRQs?

Re: [PATCH net v2 2/2] net/txgbe: add extra handle for MSI/INTx into thread irq handle

[Jakub Kicinski]

On Wed, 26 Jun 2024 14:07:03 +0800 Jiawen Wu wrote:
> Moreover, do not free isb resources in .ndo_stop, to avoid reading
> memory by a null pointer.

Please provide more detail on the sequence of events leading to the
null-defer.

>  	pdev->irq = pci_irq_vector(pdev, 0);
> +	wx->num_q_vectors = 1;

this doesn't seem obviously related

>  
>  	return 0;
>  }
> @@ -2027,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,
> @@ -2050,6 +2054,9 @@ void wx_free_isb_resources(struct wx *wx)
>  {
>  	struct pci_dev *pdev = wx->pdev;
>  
> +	if (!wx->isb_mem)
> +		return;
> +

And neither does this. Why do you need to make these function
idempotent?
-- 
pw-bot: cr

RE: [PATCH net v2 1/2] net: txgbe: remove separate irq request for MSI and INTx

[Jiawen Wu]

On Fri, Jun 28, 2024 7:42 AM, Jakub Kicinski wrote:
> On Wed, 26 Jun 2024 14:07:02 +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.
> 
> Do you have any users who need INTx support? Maybe you could drop
> the support and simplify the code?

Yes, some domestic platforms use it.

> 
> > 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   |  3 +-
> >  .../net/ethernet/wangxun/txgbe/txgbe_irq.c    | 78 ++-----------------
> >  .../net/ethernet/wangxun/txgbe/txgbe_irq.h    |  2 +-
> >  .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  2 +-
> >  4 files changed, 10 insertions(+), 75 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > index 68bde91b67a0..99f55a3573c8 100644
> > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > @@ -1996,7 +1996,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);
> 
> It seems strange to match on type to decide whether to free an IRQ.
> Isn't there or shouldn't there be some IRQ related flag informing
> the library how to manage the IRQs?

My intention is not to change the IRQ structure of ngbe driver. So it
simply match the mac type. I would consider use a flag.

RE: [PATCH net v2 2/2] net/txgbe: add extra handle for MSI/INTx into thread irq handle

[Jiawen Wu]

On Fri, Jun 28, 2024 7:44 AM, Jakub Kicinski wrote:
> On Wed, 26 Jun 2024 14:07:03 +0800 Jiawen Wu wrote:
> > Moreover, do not free isb resources in .ndo_stop, to avoid reading
> > memory by a null pointer.
> 
> Please provide more detail on the sequence of events leading to the
> null-defer.
> 
> >  	pdev->irq = pci_irq_vector(pdev, 0);
> > +	wx->num_q_vectors = 1;
> 
> this doesn't seem obviously related

Umm, this is related another fix for MSI/INTx. I'll split it into a separate
commit.

> 
> >
> >  	return 0;
> >  }
> > @@ -2027,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,
> > @@ -2050,6 +2054,9 @@ void wx_free_isb_resources(struct wx *wx)
> >  {
> >  	struct pci_dev *pdev = wx->pdev;
> >
> > +	if (!wx->isb_mem)
> > +		return;
> > +
> 
> And neither does this. Why do you need to make these function
> idempotent?

This is also to implement txgbe changes without changing the flow of ngbe.