[dpdk-dev] [PATCH 0/3] fix of lsc interrupt in i40e PF

[dpdk-dev] [PATCH 0/3] fix of lsc interrupt in i40e PF

[Helin Zhang]

The patches include the fix for link status change interrupt
in i40e PF, and code style fixes.

Helin Zhang (3):
  i40e: renaming some local variables
  i40e: rework of PF interrupt cause enable flags processing
  i40e: fix of interrupt based link status change

 lib/librte_pmd_i40e/i40e_ethdev.c | 174 ++++++++++++++++++++++++++------------
 1 file changed, 122 insertions(+), 52 deletions(-)

-- 
1.8.1.4

[dpdk-dev] [PATCH 1/3] i40e: renaming some local variables

[Helin Zhang]

To be more straightforward, two local variables in interrupt
handler are renamed.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Reviewed-by: Jing Chen <jing.d.chen@intel.com>
Reviewed-by: Jijiang Liu <jijiang.liu@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index 4e65ca4..003b084 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -3391,53 +3391,53 @@ i40e_dev_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
 {
 	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	uint32_t cause, enable;
+	uint32_t icr0, icr0_ena;
 
 	i40e_pf_disable_irq0(hw);
 
-	cause = I40E_READ_REG(hw, I40E_PFINT_ICR0);
-	enable = I40E_READ_REG(hw, I40E_PFINT_ICR0_ENA);
+	icr0 = I40E_READ_REG(hw, I40E_PFINT_ICR0);
+	icr0_ena = I40E_READ_REG(hw, I40E_PFINT_ICR0_ENA);
 
 	/* Shared IRQ case, return */
-	if (!(cause & I40E_PFINT_ICR0_INTEVENT_MASK)) {
+	if (!(icr0 & I40E_PFINT_ICR0_INTEVENT_MASK)) {
 		PMD_DRV_LOG(INFO, "Port%d INT0:share IRQ case, "
 			"no INT event to process\n", hw->pf_id);
 		goto done;
 	}
 
-	if (cause & I40E_PFINT_ICR0_LINK_STAT_CHANGE_MASK) {
+	if (icr0 & I40E_PFINT_ICR0_LINK_STAT_CHANGE_MASK) {
 		PMD_DRV_LOG(INFO, "INT:Link status changed\n");
 		i40e_dev_link_update(dev, 0);
 	}
 
-	if (cause & I40E_PFINT_ICR0_ECC_ERR_MASK)
+	if (icr0 & I40E_PFINT_ICR0_ECC_ERR_MASK)
 		PMD_DRV_LOG(INFO, "INT:Unrecoverable ECC Error\n");
 
-	if (cause & I40E_PFINT_ICR0_MAL_DETECT_MASK)
+	if (icr0 & I40E_PFINT_ICR0_MAL_DETECT_MASK)
 		PMD_DRV_LOG(INFO, "INT:Malicious programming detected\n");
 
-	if (cause & I40E_PFINT_ICR0_GRST_MASK)
+	if (icr0 & I40E_PFINT_ICR0_GRST_MASK)
 		PMD_DRV_LOG(INFO, "INT:Global Resets Requested\n");
 
-	if (cause & I40E_PFINT_ICR0_PCI_EXCEPTION_MASK)
+	if (icr0 & I40E_PFINT_ICR0_PCI_EXCEPTION_MASK)
 		PMD_DRV_LOG(INFO, "INT:PCI EXCEPTION occured\n");
 
-	if (cause & I40E_PFINT_ICR0_HMC_ERR_MASK)
+	if (icr0 & I40E_PFINT_ICR0_HMC_ERR_MASK)
 		PMD_DRV_LOG(INFO, "INT:HMC error occured\n");
 
 	/* Add processing func to deal with VF reset vent */
-	if (cause & I40E_PFINT_ICR0_VFLR_MASK) {
+	if (icr0 & I40E_PFINT_ICR0_VFLR_MASK) {
 		PMD_DRV_LOG(INFO, "INT:VF reset detected\n");
 		i40e_dev_handle_vfr_event(dev);
 	}
 	/* Find admin queue event */
-	if (cause & I40E_PFINT_ICR0_ADMINQ_MASK) {
+	if (icr0 & I40E_PFINT_ICR0_ADMINQ_MASK) {
 		PMD_DRV_LOG(INFO, "INT:ADMINQ event\n");
 		i40e_dev_handle_aq_msg(dev);
 	}
 
 done:
-	I40E_WRITE_REG(hw, I40E_PFINT_ICR0_ENA, enable);
+	I40E_WRITE_REG(hw, I40E_PFINT_ICR0_ENA, icr0_ena);
 	/* Re-enable interrupt from device side */
 	i40e_pf_enable_irq0(hw);
 	/* Re-enable interrupt from host side */
-- 
1.8.1.4

[dpdk-dev] [PATCH 2/3] i40e: rework of PF interrupt cause enable flags processing

[Helin Zhang]

To get the code cleaner and more straightforward, a macro
is defined for all interrupt cause enable flags. Two more
causes are enabled, and all the interrupt causes for
reporting any errors are compiled conditionally, as they
are for debug only.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Reviewed-by: Jing Chen <jing.d.chen@intel.com>
Reviewed-by: Jijiang Liu <jijiang.liu@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 72 +++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index 003b084..6df41ea 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -117,6 +117,19 @@
 	(1ULL << I40E_FILTER_PCTYPE_FCOE_OTHER) | \
 	(1ULL << I40E_FILTER_PCTYPE_L2_PAYLOAD))
 
+/* Mask of PF interrupt causes */
+#define I40E_PFINT_ICR0_ENA_MASK ( \
+		I40E_PFINT_ICR0_ENA_ECC_ERR_MASK | \
+		I40E_PFINT_ICR0_ENA_MAL_DETECT_MASK | \
+		I40E_PFINT_ICR0_ENA_GRST_MASK | \
+		I40E_PFINT_ICR0_ENA_PCI_EXCEPTION_MASK | \
+		I40E_PFINT_ICR0_ENA_STORM_DETECT_MASK | \
+		I40E_PFINT_ICR0_ENA_LINK_STAT_CHANGE_MASK | \
+		I40E_PFINT_ICR0_ENA_HMC_ERR_MASK | \
+		I40E_PFINT_ICR0_ENA_PE_CRITERR_MASK | \
+		I40E_PFINT_ICR0_ENA_VFLR_MASK | \
+		I40E_PFINT_ICR0_ENA_ADMINQ_MASK)
+
 static int eth_i40e_dev_init(\
 			__attribute__((unused)) struct eth_driver *eth_drv,
 			struct rte_eth_dev *eth_dev);
@@ -3261,24 +3274,9 @@ i40e_pf_enable_irq0(struct i40e_hw *hw)
 static void
 i40e_pf_config_irq0(struct i40e_hw *hw)
 {
-	uint32_t enable;
-
 	/* read pending request and disable first */
 	i40e_pf_disable_irq0(hw);
-	/**
-	 * Enable all interrupt error options to detect possible errors,
-	 * other informative int are ignored
-	 */
-	enable = I40E_PFINT_ICR0_ENA_ECC_ERR_MASK |
-	         I40E_PFINT_ICR0_ENA_MAL_DETECT_MASK |
-	         I40E_PFINT_ICR0_ENA_GRST_MASK |
-	         I40E_PFINT_ICR0_ENA_PCI_EXCEPTION_MASK |
-	         I40E_PFINT_ICR0_ENA_LINK_STAT_CHANGE_MASK |
-	         I40E_PFINT_ICR0_ENA_HMC_ERR_MASK |
-	         I40E_PFINT_ICR0_ENA_VFLR_MASK |
-	         I40E_PFINT_ICR0_ENA_ADMINQ_MASK;
-
-	I40E_WRITE_REG(hw, I40E_PFINT_ICR0_ENA, enable);
+	I40E_WRITE_REG(hw, I40E_PFINT_ICR0_ENA, I40E_PFINT_ICR0_ENA_MASK);
 	I40E_WRITE_REG(hw, I40E_PFINT_STAT_CTL0,
 		I40E_PFINT_STAT_CTL0_OTHER_ITR_INDX_MASK);
 
@@ -3398,44 +3396,44 @@ i40e_dev_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
 	icr0 = I40E_READ_REG(hw, I40E_PFINT_ICR0);
 	icr0_ena = I40E_READ_REG(hw, I40E_PFINT_ICR0_ENA);
 
-	/* Shared IRQ case, return */
+	/* No interrupt event indicated */
 	if (!(icr0 & I40E_PFINT_ICR0_INTEVENT_MASK)) {
-		PMD_DRV_LOG(INFO, "Port%d INT0:share IRQ case, "
-			"no INT event to process\n", hw->pf_id);
+		PMD_DRV_LOG(INFO, "No interrupt event\n");
 		goto done;
 	}
 
-	if (icr0 & I40E_PFINT_ICR0_LINK_STAT_CHANGE_MASK) {
-		PMD_DRV_LOG(INFO, "INT:Link status changed\n");
-		i40e_dev_link_update(dev, 0);
-	}
-
+#ifdef RTE_LIBRTE_I40E_DEBUG_DRIVER
 	if (icr0 & I40E_PFINT_ICR0_ECC_ERR_MASK)
-		PMD_DRV_LOG(INFO, "INT:Unrecoverable ECC Error\n");
-
+		PMD_DRV_LOG(ERR, "ICR0: unrecoverable ECC error\n");
 	if (icr0 & I40E_PFINT_ICR0_MAL_DETECT_MASK)
-		PMD_DRV_LOG(INFO, "INT:Malicious programming detected\n");
-
+		PMD_DRV_LOG(ERR, "ICR0: malicious programming detected\n");
 	if (icr0 & I40E_PFINT_ICR0_GRST_MASK)
-		PMD_DRV_LOG(INFO, "INT:Global Resets Requested\n");
-
+		PMD_DRV_LOG(INFO, "ICR0: global reset requested\n");
 	if (icr0 & I40E_PFINT_ICR0_PCI_EXCEPTION_MASK)
-		PMD_DRV_LOG(INFO, "INT:PCI EXCEPTION occured\n");
-
+		PMD_DRV_LOG(INFO, "ICR0: PCI exception\n activated\n");
+	if (icr0 & I40E_PFINT_ICR0_STORM_DETECT_MASK)
+		PMD_DRV_LOG(INFO, "ICR0: a change in the storm control "
+								"state\n");
 	if (icr0 & I40E_PFINT_ICR0_HMC_ERR_MASK)
-		PMD_DRV_LOG(INFO, "INT:HMC error occured\n");
+		PMD_DRV_LOG(ERR, "ICR0: HMC error\n");
+	if (icr0 & I40E_PFINT_ICR0_PE_CRITERR_MASK)
+		PMD_DRV_LOG(ERR, "ICR0: protocol engine critical error\n");
+#endif /* RTE_LIBRTE_I40E_DEBUG_DRIVER */
 
-	/* Add processing func to deal with VF reset vent */
 	if (icr0 & I40E_PFINT_ICR0_VFLR_MASK) {
-		PMD_DRV_LOG(INFO, "INT:VF reset detected\n");
+		PMD_DRV_LOG(INFO, "ICR0: VF reset detected\n");
 		i40e_dev_handle_vfr_event(dev);
 	}
-	/* Find admin queue event */
 	if (icr0 & I40E_PFINT_ICR0_ADMINQ_MASK) {
-		PMD_DRV_LOG(INFO, "INT:ADMINQ event\n");
+		PMD_DRV_LOG(INFO, "ICR0: adminq event\n");
 		i40e_dev_handle_aq_msg(dev);
 	}
 
+	if (icr0 & I40E_PFINT_ICR0_LINK_STAT_CHANGE_MASK) {
+		PMD_DRV_LOG(INFO, "INT:Link status changed\n");
+		i40e_dev_link_update(dev, 0);
+	}
+
 done:
 	I40E_WRITE_REG(hw, I40E_PFINT_ICR0_ENA, icr0_ena);
 	/* Re-enable interrupt from device side */
-- 
1.8.1.4

[dpdk-dev] [PATCH 3/3] i40e: fix of interrupt based link status change

[Helin Zhang]

As driver flag of 'RTE_PCI_DRV_INTR_LSC' was introduced
recently, it must be added in i40e. One second waiting
is needed for link up event, to wait the hardware into a
stable state, so the interrupt could be processed in two
parts. In addition, it should finally call the callback
function registered by application.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Reviewed-by: Jing Chen <jing.d.chen@intel.com>
Reviewed-by: Jijiang Liu <jijiang.liu@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 88 +++++++++++++++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 8 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index 6df41ea..e223d3a 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -47,6 +47,7 @@
 #include <rte_memzone.h>
 #include <rte_malloc.h>
 #include <rte_memcpy.h>
+#include <rte_alarm.h>
 #include <rte_dev.h>
 
 #include "i40e_logs.h"
@@ -275,7 +276,7 @@ static struct eth_driver rte_i40e_pmd = {
 	{
 		.name = "rte_i40e_pmd",
 		.id_table = pci_id_i40e_map,
-		.drv_flags = RTE_PCI_DRV_NEED_MAPPING,
+		.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
 	},
 	.eth_dev_init = eth_i40e_dev_init,
 	.dev_private_size = sizeof(struct i40e_adapter),
@@ -3371,6 +3372,58 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev)
 	rte_free(info.msg_buf);
 }
 
+/*
+ * Interrupt handler is registered as the alarm callback for handling LSC
+ * interrupt in a definite of time, in order to wait the NIC into a stable
+ * state. Currently it waits 1 sec in i40e for the link up interrupt, and
+ * no need for link down interrupt.
+ */
+static void
+i40e_dev_interrupt_delayed_handler(void *param)
+{
+	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t icr0;
+
+	/* read interrupt causes again */
+	icr0 = I40E_READ_REG(hw, I40E_PFINT_ICR0);
+
+#ifdef RTE_LIBRTE_I40E_DEBUG_DRIVER
+	if (icr0 & I40E_PFINT_ICR0_ECC_ERR_MASK)
+		PMD_DRV_LOG(ERR, "ICR0: unrecoverable ECC error\n");
+	if (icr0 & I40E_PFINT_ICR0_MAL_DETECT_MASK)
+		PMD_DRV_LOG(ERR, "ICR0: malicious programming detected\n");
+	if (icr0 & I40E_PFINT_ICR0_GRST_MASK)
+		PMD_DRV_LOG(INFO, "ICR0: global reset requested\n");
+	if (icr0 & I40E_PFINT_ICR0_PCI_EXCEPTION_MASK)
+		PMD_DRV_LOG(INFO, "ICR0: PCI exception\n activated\n");
+	if (icr0 & I40E_PFINT_ICR0_STORM_DETECT_MASK)
+		PMD_DRV_LOG(INFO, "ICR0: a change in the storm control "
+								"state\n");
+	if (icr0 & I40E_PFINT_ICR0_HMC_ERR_MASK)
+		PMD_DRV_LOG(ERR, "ICR0: HMC error\n");
+	if (icr0 & I40E_PFINT_ICR0_PE_CRITERR_MASK)
+		PMD_DRV_LOG(ERR, "ICR0: protocol engine critical error\n");
+#endif /* RTE_LIBRTE_I40E_DEBUG_DRIVER */
+
+	if (icr0 & I40E_PFINT_ICR0_VFLR_MASK) {
+		PMD_DRV_LOG(INFO, "INT:VF reset detected\n");
+		i40e_dev_handle_vfr_event(dev);
+	}
+	if (icr0 & I40E_PFINT_ICR0_ADMINQ_MASK) {
+		PMD_DRV_LOG(INFO, "INT:ADMINQ event\n");
+		i40e_dev_handle_aq_msg(dev);
+	}
+
+	/* handle the link up interrupt in an alarm callback */
+	i40e_dev_link_update(dev, 0);
+	_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC);
+
+	I40E_WRITE_REG(hw, I40E_PFINT_ICR0_ENA, I40E_PFINT_ICR0_ENA_MASK);
+	i40e_pf_enable_irq0(hw);
+	rte_intr_enable(&(dev->pci_dev->intr_handle));
+}
+
 /**
  * Interrupt handler triggered by NIC  for handling
  * specific interrupt.
@@ -3389,19 +3442,20 @@ i40e_dev_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
 {
 	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	uint32_t icr0, icr0_ena;
+	uint32_t icr0;
 
+	/* Disable interrupt */
 	i40e_pf_disable_irq0(hw);
+	I40E_WRITE_REG(hw, I40E_PFINT_ICR0_ENA, 0);
 
+	/* read out interrupt causes */
 	icr0 = I40E_READ_REG(hw, I40E_PFINT_ICR0);
-	icr0_ena = I40E_READ_REG(hw, I40E_PFINT_ICR0_ENA);
 
 	/* No interrupt event indicated */
 	if (!(icr0 & I40E_PFINT_ICR0_INTEVENT_MASK)) {
 		PMD_DRV_LOG(INFO, "No interrupt event\n");
 		goto done;
 	}
-
 #ifdef RTE_LIBRTE_I40E_DEBUG_DRIVER
 	if (icr0 & I40E_PFINT_ICR0_ECC_ERR_MASK)
 		PMD_DRV_LOG(ERR, "ICR0: unrecoverable ECC error\n");
@@ -3429,16 +3483,34 @@ i40e_dev_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
 		i40e_dev_handle_aq_msg(dev);
 	}
 
+	/* Link Status Change interrupt */
 	if (icr0 & I40E_PFINT_ICR0_LINK_STAT_CHANGE_MASK) {
-		PMD_DRV_LOG(INFO, "INT:Link status changed\n");
+#define I40E_US_PER_SECOND 1000000
+		struct rte_eth_link link;
+
+		PMD_DRV_LOG(INFO, "ICR0: link status changed\n");
+		memset(&link, 0, sizeof(link));
+		rte_i40e_dev_atomic_read_link_status(dev, &link);
 		i40e_dev_link_update(dev, 0);
+
+		/*
+		 * For link up interrupt, it needs to wait 1 second to let the
+		 * hardware be a stable state. Otherwise several consecutive
+		 * interrupts can be observed.
+		 * For link down interrupt, no need to wait.
+		 */
+		if (!link.link_status && rte_eal_alarm_set(I40E_US_PER_SECOND,
+			i40e_dev_interrupt_delayed_handler, (void *)dev) >= 0)
+			return;
+		else
+			_rte_eth_dev_callback_process(dev,
+				RTE_ETH_EVENT_INTR_LSC);
 	}
 
 done:
-	I40E_WRITE_REG(hw, I40E_PFINT_ICR0_ENA, icr0_ena);
-	/* Re-enable interrupt from device side */
+	/* Enable interrupt */
+	I40E_WRITE_REG(hw, I40E_PFINT_ICR0_ENA, I40E_PFINT_ICR0_ENA_MASK);
 	i40e_pf_enable_irq0(hw);
-	/* Re-enable interrupt from host side */
 	rte_intr_enable(&(dev->pci_dev->intr_handle));
 }
 
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH 0/3] fix of lsc interrupt in i40e PF

[Cao, Min]

Tested-by: Min Cao <min.cao@intel.com>
This patch has been verified on FC20 with Eagle Fountain: 4*10G .
The i40e base driver update patch works well on FC20 with basic function.

The test environment detail information as the following:
HOST environment:
CPU: Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
OS: Linux 3.11.10
GCC: 4.8.3
NIC: Eagle Fountain: 4*10G 

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Helin Zhang
Sent: Wednesday, September 17, 2014 3:54 PM
To: dev@dpdk.org
Subject: [dpdk-dev] [PATCH 0/3] fix of lsc interrupt in i40e PF

The patches include the fix for link status change interrupt
in i40e PF, and code style fixes.

Helin Zhang (3):
  i40e: renaming some local variables
  i40e: rework of PF interrupt cause enable flags processing
  i40e: fix of interrupt based link status change

 lib/librte_pmd_i40e/i40e_ethdev.c | 174 ++++++++++++++++++++++++++------------
 1 file changed, 122 insertions(+), 52 deletions(-)

-- 
1.8.1.4

Re: [dpdk-dev] [PATCH 0/3] fix of lsc interrupt in i40e PF

[Chen, Jing D]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Helin Zhang
> Sent: Wednesday, September 17, 2014 3:54 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 0/3] fix of lsc interrupt in i40e PF
> 
> The patches include the fix for link status change interrupt
> in i40e PF, and code style fixes.
> 
> Helin Zhang (3):
>   i40e: renaming some local variables
>   i40e: rework of PF interrupt cause enable flags processing
>   i40e: fix of interrupt based link status change
> 
>  lib/librte_pmd_i40e/i40e_ethdev.c | 174 ++++++++++++++++++++++++++-
> -----------
>  1 file changed, 122 insertions(+), 52 deletions(-)
> 
> --
> 1.8.1.4

Acked-by: Jing Chen <jing.d.chen@intel.com>

Re: [dpdk-dev] [PATCH 0/3] fix of lsc interrupt in i40e PF

[Thomas Monjalon]

> > Helin Zhang (3):
> >   i40e: renaming some local variables
> >   i40e: rework of PF interrupt cause enable flags processing
> >   i40e: fix of interrupt based link status change
> 
> Acked-by: Jing Chen <jing.d.chen@intel.com>

Merge was needed because of \n removal in logs.
Applied

Thanks
-- 
Thomas