[dpdk-dev] [PATCH 0/7] enhancements for i40e

[dpdk-dev] [PATCH 0/7] enhancements for i40e

[Helin Zhang]

These patches are enhancements for i40e or relevant. In detail,
they include:
 * fix for getting correct RSS hash result in i40e RX functions.
 * support crc stripping hw offload in VF driver.
 * ignore the failure of updating default filter setting.
 * fix for updating the hash lookup table of PF RSS.
 * disable double vlan by default during initialization.
 * fix for copying wrong size of link info, and remove an
   useless function.
 * rework in testpmd to support displaying different size
   of RX descriptors.

Helin Zhang (7):
  i40e: fix for getting correct RSS hash result
  i40evf: support configuring crc stripping hw offload
  i40e: ignore the failure of updating default filter settings
  i40e: fix for updating the hash lookup table of PF RSS
  i40e: double vlan should be specifically disabled by default
  i40evf: fix for copying wrong size of link info, and remove an useless
    function
  app/testpmd: rework for displaying different size of RX descriptors

 app/test-pmd/config.c                | 77 ++++++++++++++++++++++++------------
 lib/librte_pmd_i40e/i40e_ethdev.c    | 20 ++++++----
 lib/librte_pmd_i40e/i40e_ethdev_vf.c | 32 +++++++--------
 lib/librte_pmd_i40e/i40e_rxtx.c      |  4 +-
 4 files changed, 82 insertions(+), 51 deletions(-)

-- 
1.8.1.4

[dpdk-dev] [PATCH 1/7] i40e: fix for getting correct RSS hash result

[Helin Zhang]

It wrongly gets the RSS hash result from the RX descriptor which
has been modified for receiving new packet. The fix is to get the
RSS hash result from the buffer which saves the RX descriptor.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Acked-by: Cunming Liang <cunming.liang@intel.com>
Acked-by: Jing Chen <jing.d.chen@intel.com>
---
 lib/librte_pmd_i40e/i40e_rxtx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c
index d802894..22f55b9 100644
--- a/lib/librte_pmd_i40e/i40e_rxtx.c
+++ b/lib/librte_pmd_i40e/i40e_rxtx.c
@@ -864,7 +864,7 @@ i40e_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 		rxm->ol_flags = pkt_flags;
 		if (pkt_flags & PKT_RX_RSS_HASH)
 			rxm->pkt.hash.rss =
-				rte_le_to_cpu_32(rxdp->wb.qword0.hi_dword.rss);
+				rte_le_to_cpu_32(rxd.wb.qword0.hi_dword.rss);
 
 		rx_pkts[nb_rx++] = rxm;
 	}
@@ -1017,7 +1017,7 @@ i40e_recv_scattered_pkts(void *rx_queue,
 		first_seg->ol_flags = pkt_flags;
 		if (pkt_flags & PKT_RX_RSS_HASH)
 			rxm->pkt.hash.rss =
-				rte_le_to_cpu_32(rxdp->wb.qword0.hi_dword.rss);
+				rte_le_to_cpu_32(rxd.wb.qword0.hi_dword.rss);
 
 		/* Prefetch data of first segment, if configured to do so. */
 		rte_prefetch0(first_seg->pkt.data);
-- 
1.8.1.4

[dpdk-dev] [PATCH 2/7] i40evf: support configuring crc stripping hw offload

[Helin Zhang]

In VF driver, crc stripping hw offload is enabled or not, according
to the configurations in config file.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Acked-by: Cunming Liang <cunming.liang@intel.com>
Acked-by: Jing Chen <jing.d.chen@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev_vf.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev_vf.c b/lib/librte_pmd_i40e/i40e_ethdev_vf.c
index 4851df9..de71407 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev_vf.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev_vf.c
@@ -1062,8 +1062,22 @@ static struct rte_driver rte_i40evf_driver = {
 PMD_REGISTER_DRIVER(rte_i40evf_driver);
 
 static int
-i40evf_dev_configure(__rte_unused struct rte_eth_dev *dev)
+i40evf_dev_configure(struct rte_eth_dev *dev)
 {
+	struct rte_eth_conf* conf = &dev->data->dev_conf;
+
+#ifdef RTE_LIBRTE_I40E_PF_DISABLE_STRIP_CRC
+	if (conf->rxmode.hw_strip_crc) {
+		conf->rxmode.hw_strip_crc = 0;
+		PMD_DRV_LOG(INFO, "VF can not enable hw CRC stripping\n");
+	}
+#else
+	if (!conf->rxmode.hw_strip_crc) {
+		conf->rxmode.hw_strip_crc = 1;
+		PMD_DRV_LOG(INFO, "VF can not disable hw CRC stripping\n");
+}
+#endif
+
 	return 0;
 }
 
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH 2/7] i40evf: support configuring crc stripping hw offload

[Thomas Monjalon]

Hi Helin,

2014-06-20 14:14, Helin Zhang:
> In VF driver, crc stripping hw offload is enabled or not, according
> to the configurations in config file.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> Acked-by: Cunming Liang <cunming.liang@intel.com>
> Acked-by: Jing Chen <jing.d.chen@intel.com>
[...]
>  static int
> -i40evf_dev_configure(__rte_unused struct rte_eth_dev *dev)
> +i40evf_dev_configure(struct rte_eth_dev *dev)
>  {
> +	struct rte_eth_conf* conf = &dev->data->dev_conf;
> +
> +#ifdef RTE_LIBRTE_I40E_PF_DISABLE_STRIP_CRC
> +	if (conf->rxmode.hw_strip_crc) {
> +		conf->rxmode.hw_strip_crc = 0;
> +		PMD_DRV_LOG(INFO, "VF can not enable hw CRC stripping\n");
> +	}
> +#else
> +	if (!conf->rxmode.hw_strip_crc) {
> +		conf->rxmode.hw_strip_crc = 1;
> +		PMD_DRV_LOG(INFO, "VF can not disable hw CRC stripping\n");
> +}
> +#endif
> +
>  	return 0;
>  }

Please, I don't understand why CRC stripping must be configured at build time.
I understand VF capability depends of the PF configuration, but we should be 
able to configure it at run time.

-- 
Thomas

Re: [dpdk-dev] [PATCH 2/7] i40evf: support configuring crc stripping hw offload

[Zhang, Helin]

-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
Sent: Friday, June 20, 2014 10:08 PM
To: Zhang, Helin
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 2/7] i40evf: support configuring crc stripping hw offload

Hi Helin,

2014-06-20 14:14, Helin Zhang:
> In VF driver, crc stripping hw offload is enabled or not, according to 
> the configurations in config file.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> Acked-by: Cunming Liang <cunming.liang@intel.com>
> Acked-by: Jing Chen <jing.d.chen@intel.com>
[...]
>  static int
> -i40evf_dev_configure(__rte_unused struct rte_eth_dev *dev)
> +i40evf_dev_configure(struct rte_eth_dev *dev)
>  {
> +	struct rte_eth_conf* conf = &dev->data->dev_conf;
> +
> +#ifdef RTE_LIBRTE_I40E_PF_DISABLE_STRIP_CRC
> +	if (conf->rxmode.hw_strip_crc) {
> +		conf->rxmode.hw_strip_crc = 0;
> +		PMD_DRV_LOG(INFO, "VF can not enable hw CRC stripping\n");
> +	}
> +#else
> +	if (!conf->rxmode.hw_strip_crc) {
> +		conf->rxmode.hw_strip_crc = 1;
> +		PMD_DRV_LOG(INFO, "VF can not disable hw CRC stripping\n"); } 
> +#endif
> +
>  	return 0;
>  }

Please, I don't understand why CRC stripping must be configured at build time.
I understand VF capability depends of the PF configuration, but we should be able to configure it at run time.

--
Thomas

----------------------------------------------------------------------------------------------------------------------------------------

Hi Thomas

This solution is the same as what we did in ixgbe. As you know most of VF functionalities are controlled by PF.
If the configuration is set for global, VF driver cannot ask for change, as the change will impact PF functionality.
If the configuration can be set for PF/VF function separately, VF driver can change it directly or ask PF to change according to the hardware implementation.

Let's skip the VF CRC change for i40e of 1.7.0-rc2 at this moment, and I will investigate more and send separate patches later for that.

Regards,
Helin

[dpdk-dev] [PATCH 3/7] i40e: ignore the failure of updating default filter settings

[Helin Zhang]

The failure of updating the default filter setting should be
ignored. The updating is to change the default vlan filter
behaviours configured by firmware to expected.
The failure happens on the firmware version of 4.2.2,
while doesn't happen on previous versions, as the default
settings of firmware changed.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Acked-by: Cunming Liang <cunming.liang@intel.com>
Acked-by: Jing Chen <jing.d.chen@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index a335242..102a206 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -2473,13 +2473,14 @@ i40e_vsi_setup(struct i40e_pf *pf,
 		(void)rte_memcpy(pf->dev_addr.addr_bytes, hw->mac.perm_addr,
 				ETH_ADDR_LEN);
 		ret = i40e_update_default_filter_setting(vsi);
-		if (ret != I40E_SUCCESS) {
-			PMD_DRV_LOG(ERR, "Failed to remove default "
-						"filter setting\n");
-			goto fail_msix_alloc;
-		}
-	}
-	else if (type == I40E_VSI_SRIOV) {
+		if (ret != I40E_SUCCESS)
+			PMD_DRV_LOG(ERR, "Failure of removing default filter "
+						"setting can be ignored\n");
+		/**
+		 * The failure of updating default filter setting
+		 * can be ignored
+		 */
+	} else if (type == I40E_VSI_SRIOV) {
 		memset(&ctxt, 0, sizeof(ctxt));
 		/**
 		 * For other VSI, the uplink_seid equals to uplink VSI's
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH 3/7] i40e: ignore the failure of updating default filter settings

[Thomas Monjalon]

2014-06-20 14:14, Helin Zhang:
> The failure of updating the default filter setting should be
> ignored. The updating is to change the default vlan filter
> behaviours configured by firmware to expected.
> The failure happens on the firmware version of 4.2.2,
> while doesn't happen on previous versions, as the default
> settings of firmware changed.
[...]
>  		ret = i40e_update_default_filter_setting(vsi);
> -		if (ret != I40E_SUCCESS) {
> -			PMD_DRV_LOG(ERR, "Failed to remove default "
> -						"filter setting\n");
> -			goto fail_msix_alloc;
> -		}
> -	}
> -	else if (type == I40E_VSI_SRIOV) {
> +		if (ret != I40E_SUCCESS)
> +			PMD_DRV_LOG(ERR, "Failure of removing default filter "
> +						"setting can be ignored\n");
> +		/**
> +		 * The failure of updating default filter setting
> +		 * can be ignored
> +		 */
> +	} else if (type == I40E_VSI_SRIOV) {

The log is not clear and the log message doesn't include firmware explanation. 
Please reword.

By the way, there is already a log message in the function:
	PMD_DRV_LOG(WARNING, "Failed to remove default [mac,vlan] config\n");
	http://dpdk.org/browse/dpdk/commit/?id=9d7d8513b587d32b8f66

Will we see these error messages each time we configure an i40e device?
I think it's strange to have a log message saying it can be ignored.
Can it be a real error in some cases?

-- 
Thomas

Re: [dpdk-dev] [PATCH 3/7] i40e: ignore the failure of updating default filter settings

[Zhang, Helin]

-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
Sent: Friday, June 20, 2014 10:16 PM
To: Zhang, Helin
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 3/7] i40e: ignore the failure of updating default filter settings

2014-06-20 14:14, Helin Zhang:
> The failure of updating the default filter setting should be ignored. 
> The updating is to change the default vlan filter behaviours 
> configured by firmware to expected.
> The failure happens on the firmware version of 4.2.2, while doesn't 
> happen on previous versions, as the default settings of firmware 
> changed.
[...]
>  		ret = i40e_update_default_filter_setting(vsi);
> -		if (ret != I40E_SUCCESS) {
> -			PMD_DRV_LOG(ERR, "Failed to remove default "
> -						"filter setting\n");
> -			goto fail_msix_alloc;
> -		}
> -	}
> -	else if (type == I40E_VSI_SRIOV) {
> +		if (ret != I40E_SUCCESS)
> +			PMD_DRV_LOG(ERR, "Failure of removing default filter "
> +						"setting can be ignored\n");
> +		/**
> +		 * The failure of updating default filter setting
> +		 * can be ignored
> +		 */
> +	} else if (type == I40E_VSI_SRIOV) {

The log is not clear and the log message doesn't include firmware explanation. 
Please reword.

By the way, there is already a log message in the function:
	PMD_DRV_LOG(WARNING, "Failed to remove default [mac,vlan] config\n");
	http://dpdk.org/browse/dpdk/commit/?id=9d7d8513b587d32b8f66

Will we see these error messages each time we configure an i40e device?
I think it's strange to have a log message saying it can be ignored.
Can it be a real error in some cases?

--
Thomas

----------------------------------------------------------------------------------------------------------------------------

Hi Thomas

For recently firmware 4.2.2, the removing default macvlan filter will always fail during initialization. It is not an error.
For old firmware versions, the firmwares load a default macvlan filter which has wrong configurations, it needs to remove the default one and reload a macvlan filter with correct configurations.

So the return value at that moment should be ignored. I will write more detailed annotations to describe the issue and why we need it, and delete logs to prevent confusing users.

Regards,
Helin

[dpdk-dev] [PATCH 4/7] i40e: fix for updating the hash lookup table of PF RSS

[Helin Zhang]

The bit shifting were written wrongly in '0x1 < j',
the correct one should be '0x1 << j'.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Acked-by: Cunming Liang <cunming.liang@intel.com>
Acked-by: Jing Chen <jing.d.chen@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index 102a206..f6beee6 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -1475,7 +1475,7 @@ i40e_dev_rss_reta_update(struct rte_eth_dev *dev,
 			l = I40E_READ_REG(hw, I40E_PFQF_HLUT(i >> 2));
 
 		for (j = 0, lut = 0; j < 4; j++) {
-			if (mask & (0x1 < j))
+			if (mask & (0x1 << j))
 				lut |= reta_conf->reta[i + j] << (8 * j);
 			else
 				lut |= l & (0xFF << (8 * j));
-- 
1.8.1.4

[dpdk-dev] [PATCH 5/7] i40e: double vlan should be specifically disabled by default

[Helin Zhang]

Double vlan should be specifically disabled by default during
port initialization which is expected.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Acked-by: Cunming Liang <cunming.liang@intel.com>
Acked-by: Jing Chen <jing.d.chen@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index f6beee6..a5fe39a 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -483,6 +483,9 @@ eth_i40e_dev_init(__rte_unused struct eth_driver *eth_drv,
 	}
 
 	vsi = pf->main_vsi;
+
+	/* Disable double vlan by default */
+	i40e_vsi_config_double_vlan(vsi, FALSE);
 	if (!vsi->max_macaddrs)
 		len = ETHER_ADDR_LEN;
 	else
-- 
1.8.1.4

[dpdk-dev] [PATCH 6/7] i40evf: fix for copying wrong size of link info, and remove an useless function

[Helin Zhang]

Delete the inline function which is not used at this moment.
Fix the bug of copying wrong size of link info in function of
i40evf_get_link_status().

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Acked-by: Cunming Liang <cunming.liang@intel.com>
Acked-by: Jing Chen <jing.d.chen@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev_vf.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev_vf.c b/lib/librte_pmd_i40e/i40e_ethdev_vf.c
index de71407..8989753 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev_vf.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev_vf.c
@@ -809,7 +809,7 @@ i40evf_get_link_status(struct rte_eth_dev *dev, struct rte_eth_link *link)
 	}
 
 	new_link = (struct rte_eth_link *)args.out_buffer;
-	(void)rte_memcpy(link, new_link, sizeof(link));
+	(void)rte_memcpy(link, new_link, sizeof(*link));
 
 	return 0;
 }
@@ -821,20 +821,6 @@ static struct rte_pci_id pci_id_i40evf_map[] = {
 };
 
 static inline int
-i40evf_dev_atomic_read_link_status(struct rte_eth_dev *dev,
-				   struct rte_eth_link *link)
-{
-	struct rte_eth_link *dst = link;
-	struct rte_eth_link *src = &(dev->data->dev_link);
-
-	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
-					*(uint64_t *)src) == 0)
-		return -1;
-
-	return 0;
-}
-
-static inline int
 i40evf_dev_atomic_write_link_status(struct rte_eth_dev *dev,
 				    struct rte_eth_link *link)
 {
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH 6/7] i40evf: fix for copying wrong size of link info, and remove an useless function

[Thomas Monjalon]

2014-06-20 14:14, Helin Zhang:
> Delete the inline function which is not used at this moment.
> Fix the bug of copying wrong size of link info in function of
> i40evf_get_link_status().

There are 2 patches here. Please split.

>  static inline int
> -i40evf_dev_atomic_read_link_status(struct rte_eth_dev *dev,
> -				   struct rte_eth_link *link)
> -{
> -	struct rte_eth_link *dst = link;
> -	struct rte_eth_link *src = &(dev->data->dev_link);
> -
> -	if (rte_atomic64_cmpset((uint64_t *)dst, *(uint64_t *)dst,
> -					*(uint64_t *)src) == 0)
> -		return -1;
> -
> -	return 0;
> -}

Not directly related to this patch, but it could be a good idea to start
using rte_eth_dev_atomic_read_link_status() instead of copying this function 
in each PMD.

-- 
Thomas

[dpdk-dev] [PATCH 7/7] app/testpmd: rework for displaying different size of RX descriptors

[Helin Zhang]

i40e supports 16 and 32 bytes RX descriptors which can be configured.
It needs to check the driver type and the configuration to determine
if 16 or 32 bytes RX descriptors is being used, for reading and
displaying the different sizes of RX descriptors.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Acked-by: Cunming Liang <cunming.liang@intel.com>
Acked-by: Jing Chen <jing.d.chen@intel.com>
---
 app/test-pmd/config.c | 77 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 25 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 0023ab2..506058b 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -581,53 +581,80 @@ union igb_ring_dword {
 	} words;
 };
 
-#ifndef RTE_LIBRTE_I40E_16BYTE_RX_DESC
-struct igb_ring_desc_32B {
+struct igb_ring_desc_32_bytes {
 	union igb_ring_dword lo_dword;
 	union igb_ring_dword hi_dword;
 	union igb_ring_dword resv1;
 	union igb_ring_dword resv2;
 };
-#endif
 
-struct igb_ring_desc {
+struct igb_ring_desc_16_bytes {
 	union igb_ring_dword lo_dword;
 	union igb_ring_dword hi_dword;
 };
 
 static void
-ring_rx_descriptor_display(const struct rte_memzone *ring_mz, uint16_t desc_id)
+ring_rxd_display_dword(union igb_ring_dword dword)
+{
+	printf("    0x%08X - 0x%08X\n", (unsigned)dword.words.lo,
+					(unsigned)dword.words.hi);
+}
+
+static void
+ring_rx_descriptor_display(const struct rte_memzone *ring_mz,
+			   uint8_t port_id,
+			   uint16_t desc_id)
 {
-#ifdef RTE_LIBRTE_I40E_16BYTE_RX_DESC
-	struct igb_ring_desc *ring;
-	struct igb_ring_desc rd;
+	struct rte_eth_dev_info dev_info;
 
-	ring = (struct igb_ring_desc *) ring_mz->addr;
+	memset(&dev_info, 0, sizeof(dev_info));
+	rte_eth_dev_info_get(port_id, &dev_info);
+
+	if (strstr(dev_info.driver_name, "i40e") != NULL) { /* i40e */
+#ifndef RTE_LIBRTE_I40E_16BYTE_RX_DESC
+		struct igb_ring_desc_32_bytes *ring =
+			(struct igb_ring_desc_32_bytes *)ring_mz->addr;
+
+		ring_rxd_display_dword(rte_le_to_cpu_64(\
+				ring[desc_id].lo_dword));
+		ring_rxd_display_dword(rte_le_to_cpu_64(\
+				ring[desc_id].hi_dword));
+		ring_rxd_display_dword(rte_le_to_cpu_64(\
+				ring[desc_id].resv1));
+		ring_rxd_display_dword(rte_le_to_cpu_64(\
+				ring[desc_id].resv2));
 #else
-	struct igb_ring_desc_32B *ring;
-	struct igb_ring_desc_32B rd;
+		struct igb_ring_desc_16_bytes *ring =
+			(struct igb_ring_desc_16_bytes *)ring_mz->addr;
 
-	ring = (struct igb_ring_desc_32B *) ring_mz->addr;
+		ring_rxd_display_dword(rte_le_to_cpu_64(\
+				ring[desc_id].lo_dword));
+		ring_rxd_display_dword(rte_le_to_cpu_64(\
+				ring[desc_id].hi_dword));
 #endif
-	rd.lo_dword = rte_le_to_cpu_64(ring[desc_id].lo_dword);
-	rd.hi_dword = rte_le_to_cpu_64(ring[desc_id].hi_dword);
-	printf("    0x%08X - 0x%08X / 0x%08X - 0x%08X\n",
-		(unsigned)rd.lo_dword.words.lo, (unsigned)rd.lo_dword.words.hi,
-		(unsigned)rd.hi_dword.words.lo, (unsigned)rd.hi_dword.words.hi);
+	} else { /* not i40e */
+		struct igb_ring_desc_16_bytes *ring =
+			(struct igb_ring_desc_16_bytes *)ring_mz->addr;
+
+		ring_rxd_display_dword(rte_le_to_cpu_64(\
+				ring[desc_id].lo_dword));
+		ring_rxd_display_dword(rte_le_to_cpu_64(\
+				ring[desc_id].hi_dword));
+	}
 }
 
 static void
 ring_tx_descriptor_display(const struct rte_memzone *ring_mz, uint16_t desc_id)
 {
-	struct igb_ring_desc *ring;
-	struct igb_ring_desc rd;
+	struct igb_ring_desc_16_bytes *ring;
+	struct igb_ring_desc_16_bytes txd;
 
-	ring = (struct igb_ring_desc *) ring_mz->addr;
-	rd.lo_dword = rte_le_to_cpu_64(ring[desc_id].lo_dword);
-	rd.hi_dword = rte_le_to_cpu_64(ring[desc_id].hi_dword);
+	ring = (struct igb_ring_desc_16_bytes *)ring_mz->addr;
+	txd.lo_dword = rte_le_to_cpu_64(ring[desc_id].lo_dword);
+	txd.hi_dword = rte_le_to_cpu_64(ring[desc_id].hi_dword);
 	printf("    0x%08X - 0x%08X / 0x%08X - 0x%08X\n",
-		(unsigned)rd.lo_dword.words.lo, (unsigned)rd.lo_dword.words.hi,
-		(unsigned)rd.hi_dword.words.lo, (unsigned)rd.hi_dword.words.hi);
+		(unsigned)txd.lo_dword.words.lo, (unsigned)txd.lo_dword.words.hi,
+		(unsigned)txd.hi_dword.words.lo, (unsigned)txd.hi_dword.words.hi);
 }
 
 void
@@ -644,7 +671,7 @@ rx_ring_desc_display(portid_t port_id, queueid_t rxq_id, uint16_t rxd_id)
 	rx_mz = ring_dma_zone_lookup("rx_ring", port_id, rxq_id);
 	if (rx_mz == NULL)
 		return;
-	ring_rx_descriptor_display(rx_mz, rxd_id);
+	ring_rx_descriptor_display(rx_mz, port_id, rxd_id);
 }
 
 void
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH 7/7] app/testpmd: rework for displaying different size of RX descriptors

[Thomas Monjalon]

2014-06-20 14:14, Helin Zhang:
> i40e supports 16 and 32 bytes RX descriptors which can be configured.
> It needs to check the driver type and the configuration to determine
> if 16 or 32 bytes RX descriptors is being used, for reading and
> displaying the different sizes of RX descriptors.
[...]
> +	if (strstr(dev_info.driver_name, "i40e") != NULL) { /* i40e */
> +#ifndef RTE_LIBRTE_I40E_16BYTE_RX_DESC

Do we need to handle i40e case if RTE_LIBRTE_I40E_16BYTE_RX_DESC is defined?

[simplified diff follows]
>  #else
> +		struct igb_ring_desc_16_bytes *ring =
> +			(struct igb_ring_desc_16_bytes *)ring_mz->addr;
>  
> +		ring_rxd_display_dword(rte_le_to_cpu_64(\
> +				ring[desc_id].lo_dword));
> +		ring_rxd_display_dword(rte_le_to_cpu_64(\
> +				ring[desc_id].hi_dword));
>  #endif
> +	} else { /* not i40e */
> +		struct igb_ring_desc_16_bytes *ring =
> +			(struct igb_ring_desc_16_bytes *)ring_mz->addr;
> +
> +		ring_rxd_display_dword(rte_le_to_cpu_64(\
> +				ring[desc_id].lo_dword));
> +		ring_rxd_display_dword(rte_le_to_cpu_64(\
> +				ring[desc_id].hi_dword));
> +	}

You could factorize these 2 cases.

-- 
Thomas

Re: [dpdk-dev] [PATCH 7/7] app/testpmd: rework for displaying different size of RX descriptors

[Zhang, Helin]

-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
Sent: Friday, June 20, 2014 10:35 PM
To: Zhang, Helin
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 7/7] app/testpmd: rework for displaying different size of RX descriptors

2014-06-20 14:14, Helin Zhang:
> i40e supports 16 and 32 bytes RX descriptors which can be configured.
> It needs to check the driver type and the configuration to determine 
> if 16 or 32 bytes RX descriptors is being used, for reading and 
> displaying the different sizes of RX descriptors.
[...]
> +	if (strstr(dev_info.driver_name, "i40e") != NULL) { /* i40e */ 
> +#ifndef RTE_LIBRTE_I40E_16BYTE_RX_DESC

Do we need to handle i40e case if RTE_LIBRTE_I40E_16BYTE_RX_DESC is defined?

[simplified diff follows]
>  #else
> +		struct igb_ring_desc_16_bytes *ring =
> +			(struct igb_ring_desc_16_bytes *)ring_mz->addr;
>  
> +		ring_rxd_display_dword(rte_le_to_cpu_64(\
> +				ring[desc_id].lo_dword));
> +		ring_rxd_display_dword(rte_le_to_cpu_64(\
> +				ring[desc_id].hi_dword));
>  #endif
> +	} else { /* not i40e */
> +		struct igb_ring_desc_16_bytes *ring =
> +			(struct igb_ring_desc_16_bytes *)ring_mz->addr;
> +
> +		ring_rxd_display_dword(rte_le_to_cpu_64(\
> +				ring[desc_id].lo_dword));
> +		ring_rxd_display_dword(rte_le_to_cpu_64(\
> +				ring[desc_id].hi_dword));
> +	}

You could factorize these 2 cases.

--
Thomas

----------------------------------------------------------------------------------------------------------------
Hi Thomas

As RX descriptor size can vary in i40e only, and for other PMD, RTE_LIBRTE_I40E_16BYTE_RX_DESC will never be defined, we need to handle all possibilities.
Yes, I can rework the changes and let it be more graceful. Thanks!

Regards,
Helin

Re: [dpdk-dev] [PATCH 0/7] enhancements for i40e

[Zhang, Helin]

-----Original Message-----
From: Zhang, Helin 
Sent: Friday, June 20, 2014 2:15 PM
To: dev@dpdk.org
Cc: Zhang, Helin
Subject: [PATCH 0/7] enhancements for i40e

These patches are enhancements for i40e or relevant. In detail, they include:
 * fix for getting correct RSS hash result in i40e RX functions.
 * support crc stripping hw offload in VF driver.
 * ignore the failure of updating default filter setting.
 * fix for updating the hash lookup table of PF RSS.
 * disable double vlan by default during initialization.
 * fix for copying wrong size of link info, and remove an
   useless function.
 * rework in testpmd to support displaying different size
   of RX descriptors.

Helin Zhang (7):
  i40e: fix for getting correct RSS hash result
  i40evf: support configuring crc stripping hw offload
  i40e: ignore the failure of updating default filter settings
  i40e: fix for updating the hash lookup table of PF RSS
  i40e: double vlan should be specifically disabled by default
  i40evf: fix for copying wrong size of link info, and remove an useless
    function
  app/testpmd: rework for displaying different size of RX descriptors

 app/test-pmd/config.c                | 77 ++++++++++++++++++++++++------------
 lib/librte_pmd_i40e/i40e_ethdev.c    | 20 ++++++----
 lib/librte_pmd_i40e/i40e_ethdev_vf.c | 32 +++++++--------
 lib/librte_pmd_i40e/i40e_rxtx.c      |  4 +-
 4 files changed, 82 insertions(+), 51 deletions(-)

--
1.8.1.4

-----------------------------------------------------------------------------------------------------------------------------------------------

Hi Thomas

Please help to merge these patches which is for 1.7.0-rc2!
It has important fix for i40e PMD running on the latest version of NIC firmware.
It also contains the rework you asked in testpmd.

Thank you very much!

Regards,
Helin