[dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Wenzhuo Lu]

It'not appropriate to call rte_eth_dev_info_get in PMD,
as rte_eth_dev_info_get need to get info from PMD.
Remove rte_eth_dev_info_get from PMD code and get the
info directly.

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
 1 file changed, 68 insertions(+), 76 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 64ce55a..f14a68b 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	int rar_entry;
 	uint8_t *new_mac = (uint8_t *)(mac_addr);
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
@@ -4902,17 +4902,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	struct ixgbe_mac_info *mac;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -4932,17 +4932,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	struct ixgbe_mac_info *mac;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -4961,17 +4961,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	uint32_t ctrl;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (vlan_id > ETHER_MAX_VLAN_ID)
@@ -4997,14 +4997,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	uint32_t ctrl;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
 	if (on > 1)
@@ -5031,14 +5029,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	int i;
 	int num_queues = (int)(IXGBE_QDE_IDX_MASK >> IXGBE_QDE_IDX_SHIFT);
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
 	if (on > 1)
@@ -5061,18 +5057,18 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	uint32_t reg_value;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
 	/* only support VF's 0 to 63 */
-	if ((vf >= dev_info.max_vfs) || (vf > 63))
+	if ((vf >= pci_dev->max_vfs) || (vf > 63))
 		return -EINVAL;
 
 	if (on > 1)
@@ -5094,19 +5090,21 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 rte_pmd_ixgbe_set_vf_vlan_stripq(uint8_t port, uint16_t vf, uint8_t on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
+	struct ixgbe_hw *hw;
 	uint16_t queues_per_pool;
 	uint32_t q;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
+	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5122,8 +5120,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	 * first 124 queues 0-123 will be allocated to VF's and only
 	 * the last 4 queues 123-127 will be assigned to the PF.
 	 */
-
-	queues_per_pool = dev_info.vmdq_queue_num / dev_info.max_vmdq_pools;
+	if (hw->mac.type == ixgbe_mac_82598EB)
+		queues_per_pool = (uint16_t)hw->mac.max_rx_queues /
+				  ETH_16_POOLS;
+	else
+		queues_per_pool = (uint16_t)hw->mac.max_rx_queues /
+				  ETH_64_POOLS;
 
 	for (q = 0; q < queues_per_pool; q++)
 		(*dev->dev_ops->vlan_strip_queue_set)(dev,
@@ -5136,19 +5138,19 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 {
 	int val = 0;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 	struct ixgbe_hw *hw;
 	uint32_t vmolr;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5181,7 +5183,7 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 rte_pmd_ixgbe_set_vf_rx(uint8_t port, uint16_t vf, uint8_t on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 	uint32_t reg, addr;
 	uint32_t val;
 	const uint8_t bit1 = 0x1;
@@ -5190,12 +5192,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5231,7 +5233,7 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 rte_pmd_ixgbe_set_vf_tx(uint8_t port, uint16_t vf, uint8_t on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 	uint32_t reg, addr;
 	uint32_t val;
 	const uint8_t bit1 = 0x1;
@@ -5241,12 +5243,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5282,7 +5284,6 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 			uint64_t vf_mask, uint8_t vlan_on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	int ret = 0;
 	uint16_t vf_idx;
 	struct ixgbe_hw *hw;
@@ -5290,9 +5291,8 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
 	if ((vlan > ETHER_MAX_VLAN_ID) || (vf_mask == 0))
@@ -5318,7 +5318,6 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
 	uint16_t tx_rate, uint64_t q_msk)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	struct ixgbe_hw *hw;
 	struct ixgbe_vf_info *vfinfo;
 	struct rte_eth_link link;
@@ -5332,13 +5331,13 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 	rte_eth_link_get_nowait(port, &link);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (tx_rate > link.link_speed)
@@ -5347,7 +5346,6 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
 	if (q_msk == 0)
 		return 0;
 
-	pci_dev = IXGBE_DEV_TO_PCI(dev);
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	vfinfo = *(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private));
 	nb_q_per_pool = RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool;
@@ -8227,16 +8225,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	/* Stop the data paths */
@@ -8311,16 +8308,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	/* Stop the data paths */
@@ -8376,16 +8372,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	ctrl = mac[0] | (mac[1] << 8) | (mac[2] << 16) | (mac[3] << 24);
@@ -8402,16 +8397,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	ctrl = mac[0] | (mac[1] << 8) | (mac[2] << 16) | (mac[3] << 24);
@@ -8430,16 +8424,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl, i;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	if (idx != 0 && idx != 1)
@@ -8487,16 +8480,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl, i;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (is_ixgbe_pmd(dev->data->drv_name))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	if (idx != 0 && idx != 1)
-- 
1.9.3

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Tiwei Bie]

On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
> It'not appropriate to call rte_eth_dev_info_get in PMD,
> as rte_eth_dev_info_get need to get info from PMD.
> Remove rte_eth_dev_info_get from PMD code and get the
> info directly.
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
>  1 file changed, 68 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 64ce55a..f14a68b 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>  	int rar_entry;
>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
>  	struct rte_eth_dev *dev;
> -	struct rte_eth_dev_info dev_info;
> +	struct rte_pci_device *pci_dev;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>  
>  	dev = &rte_eth_devices[port];
> -	rte_eth_dev_info_get(port, &dev_info);
> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
>  
> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> +	if (is_ixgbe_pmd(dev->data->drv_name))
>  		return -ENOTSUP;
>  

The return value of is_ixgbe_pmd() is not boolean (actually I think it
should be based on its name). If we omit the comparison with 0, the code
looks weird. It looks like it'll return -ENOTSUP if the port's driver
is ixgbe PMD.

Best regards,
Tiwei Bie

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Lu, Wenzhuo]

Hi Tiwei,

> -----Original Message-----
> From: Bie, Tiwei
> Sent: Wednesday, January 25, 2017 11:17 AM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> 
> On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
> > It'not appropriate to call rte_eth_dev_info_get in PMD, as
> > rte_eth_dev_info_get need to get info from PMD.
> > Remove rte_eth_dev_info_get from PMD code and get the info directly.
> >
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_ethdev.c | 144
> > ++++++++++++++++++---------------------
> >  1 file changed, 68 insertions(+), 76 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 64ce55a..f14a68b 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -4401,17 +4401,17 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> >  	int rar_entry;
> >  	uint8_t *new_mac = (uint8_t *)(mac_addr);
> >  	struct rte_eth_dev *dev;
> > -	struct rte_eth_dev_info dev_info;
> > +	struct rte_pci_device *pci_dev;
> >
> >  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> >
> >  	dev = &rte_eth_devices[port];
> > -	rte_eth_dev_info_get(port, &dev_info);
> > +	pci_dev = IXGBE_DEV_TO_PCI(dev);
> >
> > -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> > +	if (is_ixgbe_pmd(dev->data->drv_name))
> >  		return -ENOTSUP;
> >
> 
> The return value of is_ixgbe_pmd() is not boolean (actually I think it should be
> based on its name). If we omit the comparison with 0, the code looks weird. It
> looks like it'll return -ENOTSUP if the port's driver is ixgbe PMD.
Yes, it’s weird. But what makes it weird is not the missing comparison but the function name.
Better changing it to ixgbe_pmd_check. How about it?

> 
> Best regards,
> Tiwei Bie

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Tiwei Bie]

On Wed, Jan 25, 2017 at 01:13:32PM +0800, Lu, Wenzhuo wrote:
> Hi Tiwei,
> 
> > -----Original Message-----
> > From: Bie, Tiwei
> > Sent: Wednesday, January 25, 2017 11:17 AM
> > To: Lu, Wenzhuo
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> > 
> > On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
> > > It'not appropriate to call rte_eth_dev_info_get in PMD, as
> > > rte_eth_dev_info_get need to get info from PMD.
> > > Remove rte_eth_dev_info_get from PMD code and get the info directly.
> > >
> > > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > > ---
> > >  drivers/net/ixgbe/ixgbe_ethdev.c | 144
> > > ++++++++++++++++++---------------------
> > >  1 file changed, 68 insertions(+), 76 deletions(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > index 64ce55a..f14a68b 100644
> > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > @@ -4401,17 +4401,17 @@ static int
> > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > >  	int rar_entry;
> > >  	uint8_t *new_mac = (uint8_t *)(mac_addr);
> > >  	struct rte_eth_dev *dev;
> > > -	struct rte_eth_dev_info dev_info;
> > > +	struct rte_pci_device *pci_dev;
> > >
> > >  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> > >
> > >  	dev = &rte_eth_devices[port];
> > > -	rte_eth_dev_info_get(port, &dev_info);
> > > +	pci_dev = IXGBE_DEV_TO_PCI(dev);
> > >
> > > -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> > > +	if (is_ixgbe_pmd(dev->data->drv_name))
> > >  		return -ENOTSUP;
> > >
> > 
> > The return value of is_ixgbe_pmd() is not boolean (actually I think it should be
> > based on its name). If we omit the comparison with 0, the code looks weird. It
> > looks like it'll return -ENOTSUP if the port's driver is ixgbe PMD.
> 
> Yes, it’s weird. But what makes it weird is not the missing comparison but the function name.
> Better changing it to ixgbe_pmd_check. How about it?
> 

Yeah, I also prefer to change the helper function itself. But I'm not
good at the naming. I'd like to hear others' opinion. :-)

Best regards,
Tiwei Bie

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Ferruh Yigit]

On 1/25/2017 5:24 AM, Tiwei Bie wrote:
> On Wed, Jan 25, 2017 at 01:13:32PM +0800, Lu, Wenzhuo wrote:
>> Hi Tiwei,
>>
>>> -----Original Message-----
>>> From: Bie, Tiwei
>>> Sent: Wednesday, January 25, 2017 11:17 AM
>>> To: Lu, Wenzhuo
>>> Cc: dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>>
>>> On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
>>>> It'not appropriate to call rte_eth_dev_info_get in PMD, as
>>>> rte_eth_dev_info_get need to get info from PMD.
>>>> Remove rte_eth_dev_info_get from PMD code and get the info directly.
>>>>
>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>> ---
>>>>  drivers/net/ixgbe/ixgbe_ethdev.c | 144
>>>> ++++++++++++++++++---------------------
>>>>  1 file changed, 68 insertions(+), 76 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> index 64ce55a..f14a68b 100644
>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> @@ -4401,17 +4401,17 @@ static int
>>> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>>>  	int rar_entry;
>>>>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
>>>>  	struct rte_eth_dev *dev;
>>>> -	struct rte_eth_dev_info dev_info;
>>>> +	struct rte_pci_device *pci_dev;
>>>>
>>>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>>>
>>>>  	dev = &rte_eth_devices[port];
>>>> -	rte_eth_dev_info_get(port, &dev_info);
>>>> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
>>>>
>>>> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
>>>> +	if (is_ixgbe_pmd(dev->data->drv_name))
>>>>  		return -ENOTSUP;
>>>>
>>>
>>> The return value of is_ixgbe_pmd() is not boolean (actually I think it should be
>>> based on its name). If we omit the comparison with 0, the code looks weird. It
>>> looks like it'll return -ENOTSUP if the port's driver is ixgbe PMD.
>>
>> Yes, it’s weird. But what makes it weird is not the missing comparison but the function name.
>> Better changing it to ixgbe_pmd_check. How about it?
>>
> 
> Yeah, I also prefer to change the helper function itself. But I'm not
> good at the naming. I'd like to hear others' opinion. :-)

Agree that it looks wrong without 0 comparison.

Helper function is checking if the given port is an ixgbe port or not,
unfortunately you need to this for PMD specific APIs.
So What about is_device_supported(),

I agree it is better if it returns bool,
and I also think it is better if it gets the rte_eth_dev as input
parameter, validating port based on name is API internal knowledge.

Also instead of name comparison against fixed string, it can be
eth_dev->driver->pci_drv.name against driver->name. This makes function
more generic, and perhaps this helper function can be moved into ethdev
layer, later. For this function needs to get both eth_dev and rte_driver
as argument.


> 
> Best regards,
> Tiwei Bie
> 

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Ananyev, Konstantin]

Hi Wenzhuo,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> Sent: Wednesday, January 25, 2017 2:39 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> 
> It'not appropriate to call rte_eth_dev_info_get in PMD,
> as rte_eth_dev_info_get need to get info from PMD.
> Remove rte_eth_dev_info_get from PMD code and get the
> info directly.
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
>  1 file changed, 68 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 64ce55a..f14a68b 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>  	int rar_entry;
>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
>  	struct rte_eth_dev *dev;
> -	struct rte_eth_dev_info dev_info;
> +	struct rte_pci_device *pci_dev;
> 
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> 
>  	dev = &rte_eth_devices[port];
> -	rte_eth_dev_info_get(port, &dev_info);
> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
> 
> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> +	if (is_ixgbe_pmd(dev->data->drv_name))
>  		return -ENOTSUP;

I wonder why do we need now that it is really an ixgbe device all over the place?
Konstantin

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Ferruh Yigit]

On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
> Hi Wenzhuo,
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
>> Sent: Wednesday, January 25, 2017 2:39 AM
>> To: dev@dpdk.org
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>
>> It'not appropriate to call rte_eth_dev_info_get in PMD,
>> as rte_eth_dev_info_get need to get info from PMD.
>> Remove rte_eth_dev_info_get from PMD code and get the
>> info directly.
>>
>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>> ---
>>  drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
>>  1 file changed, 68 insertions(+), 76 deletions(-)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index 64ce55a..f14a68b 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>  	int rar_entry;
>>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
>>  	struct rte_eth_dev *dev;
>> -	struct rte_eth_dev_info dev_info;
>> +	struct rte_pci_device *pci_dev;
>>
>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>
>>  	dev = &rte_eth_devices[port];
>> -	rte_eth_dev_info_get(port, &dev_info);
>> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
>>
>> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
>> +	if (is_ixgbe_pmd(dev->data->drv_name))
>>  		return -ENOTSUP;
> 
> I wonder why do we need now that it is really an ixgbe device all over the place?

This device specific API, so it is missing merits of abstraction layer,
application can these APIs with any port_id, API should be protected for it.

> Konstantin
> 

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Ananyev, Konstantin]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, February 1, 2017 5:40 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> 
> On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
> > Hi Wenzhuo,
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> >> Sent: Wednesday, January 25, 2017 2:39 AM
> >> To: dev@dpdk.org
> >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> >> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> >>
> >> It'not appropriate to call rte_eth_dev_info_get in PMD,
> >> as rte_eth_dev_info_get need to get info from PMD.
> >> Remove rte_eth_dev_info_get from PMD code and get the
> >> info directly.
> >>
> >> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >> ---
> >>  drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
> >>  1 file changed, 68 insertions(+), 76 deletions(-)
> >>
> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> index 64ce55a..f14a68b 100644
> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> @@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> >>  	int rar_entry;
> >>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
> >>  	struct rte_eth_dev *dev;
> >> -	struct rte_eth_dev_info dev_info;
> >> +	struct rte_pci_device *pci_dev;
> >>
> >>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> >>
> >>  	dev = &rte_eth_devices[port];
> >> -	rte_eth_dev_info_get(port, &dev_info);
> >> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
> >>
> >> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> >> +	if (is_ixgbe_pmd(dev->data->drv_name))
> >>  		return -ENOTSUP;
> >
> > I wonder why do we need now that it is really an ixgbe device all over the place?
> 
> This device specific API, so it is missing merits of abstraction layer,
> application can these APIs with any port_id, API should be protected for it.

Ah ok, my bad - didn't realize from the patch that it affects only device specific API :)
Would It be too much hassle to move these functions into a separate file (rte_ixgbe_pmd.c or so)?
Konstantin

> 
> > Konstantin
> >

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Ferruh Yigit]

On 2/1/2017 6:10 PM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, February 1, 2017 5:40 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>
>> On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
>>> Hi Wenzhuo,
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
>>>> Sent: Wednesday, January 25, 2017 2:39 AM
>>>> To: dev@dpdk.org
>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>>>> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>>>
>>>> It'not appropriate to call rte_eth_dev_info_get in PMD,
>>>> as rte_eth_dev_info_get need to get info from PMD.
>>>> Remove rte_eth_dev_info_get from PMD code and get the
>>>> info directly.
>>>>
>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>> ---
>>>>  drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
>>>>  1 file changed, 68 insertions(+), 76 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> index 64ce55a..f14a68b 100644
>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> @@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>>>  	int rar_entry;
>>>>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
>>>>  	struct rte_eth_dev *dev;
>>>> -	struct rte_eth_dev_info dev_info;
>>>> +	struct rte_pci_device *pci_dev;
>>>>
>>>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>>>
>>>>  	dev = &rte_eth_devices[port];
>>>> -	rte_eth_dev_info_get(port, &dev_info);
>>>> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
>>>>
>>>> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
>>>> +	if (is_ixgbe_pmd(dev->data->drv_name))
>>>>  		return -ENOTSUP;
>>>
>>> I wonder why do we need now that it is really an ixgbe device all over the place?
>>
>> This device specific API, so it is missing merits of abstraction layer,
>> application can these APIs with any port_id, API should be protected for it.
> 
> Ah ok, my bad - didn't realize from the patch that it affects only device specific API :)
> Would It be too much hassle to move these functions into a separate file (rte_ixgbe_pmd.c or so)?

Not sure about the effort it requires, but I second that.

> Konstantin
> 
>>
>>> Konstantin
>>>
> 

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Lu, Wenzhuo]

Hi Ferruh,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, January 30, 2017 8:16 PM
> To: Bie, Tiwei; Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> 
> On 1/25/2017 5:24 AM, Tiwei Bie wrote:
> > On Wed, Jan 25, 2017 at 01:13:32PM +0800, Lu, Wenzhuo wrote:
> >> Hi Tiwei,
> >>
> >>> -----Original Message-----
> >>> From: Bie, Tiwei
> >>> Sent: Wednesday, January 25, 2017 11:17 AM
> >>> To: Lu, Wenzhuo
> >>> Cc: dev@dpdk.org
> >>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
> >>> rte_eth_dev_info_get
> >>>
> >>> On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
> >>>> It'not appropriate to call rte_eth_dev_info_get in PMD, as
> >>>> rte_eth_dev_info_get need to get info from PMD.
> >>>> Remove rte_eth_dev_info_get from PMD code and get the info directly.
> >>>>
> >>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >>>> ---
> >>>>  drivers/net/ixgbe/ixgbe_ethdev.c | 144
> >>>> ++++++++++++++++++---------------------
> >>>>  1 file changed, 68 insertions(+), 76 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> index 64ce55a..f14a68b 100644
> >>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> @@ -4401,17 +4401,17 @@ static int
> >>> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> >>>>  	int rar_entry;
> >>>>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
> >>>>  	struct rte_eth_dev *dev;
> >>>> -	struct rte_eth_dev_info dev_info;
> >>>> +	struct rte_pci_device *pci_dev;
> >>>>
> >>>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> >>>>
> >>>>  	dev = &rte_eth_devices[port];
> >>>> -	rte_eth_dev_info_get(port, &dev_info);
> >>>> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
> >>>>
> >>>> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> >>>> +	if (is_ixgbe_pmd(dev->data->drv_name))
> >>>>  		return -ENOTSUP;
> >>>>
> >>>
> >>> The return value of is_ixgbe_pmd() is not boolean (actually I think
> >>> it should be based on its name). If we omit the comparison with 0,
> >>> the code looks weird. It looks like it'll return -ENOTSUP if the port's driver
> is ixgbe PMD.
> >>
> >> Yes, it’s weird. But what makes it weird is not the missing comparison but
> the function name.
> >> Better changing it to ixgbe_pmd_check. How about it?
> >>
> >
> > Yeah, I also prefer to change the helper function itself. But I'm not
> > good at the naming. I'd like to hear others' opinion. :-)
> 
> Agree that it looks wrong without 0 comparison.
> 
> Helper function is checking if the given port is an ixgbe port or not,
> unfortunately you need to this for PMD specific APIs.
> So What about is_device_supported(),
> 
> I agree it is better if it returns bool, and I also think it is better if it gets the
> rte_eth_dev as input parameter, validating port based on name is API internal
> knowledge.
> 
> Also instead of name comparison against fixed string, it can be eth_dev-
> >driver->pci_drv.name against driver->name. This makes function more
Thanks for your suggestion. But I don’t get your point here. 
For a specific device, should not the eth_dev->driver->pci_drv.name and the driver->name be the same?


> generic, and perhaps this helper function can be moved into ethdev layer,
> later. For this function needs to get both eth_dev and rte_driver as argument.
> 
> 
> >
> > Best regards,
> > Tiwei Bie
> >

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Iremonger, Bernard]

Hi Konstantin,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> Sent: Wednesday, February 1, 2017 6:10 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> 
> 
> 
> > -----Original Message-----
> > From: Yigit, Ferruh
> > Sent: Wednesday, February 1, 2017 5:40 PM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > rte_eth_dev_info_get
> >
> > On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
> > > Hi Wenzhuo,
> > >
> > >> -----Original Message-----
> > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> > >> Sent: Wednesday, January 25, 2017 2:39 AM
> > >> To: dev@dpdk.org
> > >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > >> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > >> rte_eth_dev_info_get
> > >>
> > >> It'not appropriate to call rte_eth_dev_info_get in PMD, as
> > >> rte_eth_dev_info_get need to get info from PMD.
> > >> Remove rte_eth_dev_info_get from PMD code and get the info
> > >> directly.
> > >>
> > >> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > >> ---
> > >>  drivers/net/ixgbe/ixgbe_ethdev.c | 144
> > >> ++++++++++++++++++---------------------
> > >>  1 file changed, 68 insertions(+), 76 deletions(-)
> > >>
> > >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> index 64ce55a..f14a68b 100644
> > >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> @@ -4401,17 +4401,17 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > >>  	int rar_entry;
> > >>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
> > >>  	struct rte_eth_dev *dev;
> > >> -	struct rte_eth_dev_info dev_info;
> > >> +	struct rte_pci_device *pci_dev;
> > >>
> > >>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> > >>
> > >>  	dev = &rte_eth_devices[port];
> > >> -	rte_eth_dev_info_get(port, &dev_info);
> > >> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
> > >>
> > >> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> > >> +	if (is_ixgbe_pmd(dev->data->drv_name))
> > >>  		return -ENOTSUP;
> > >
> > > I wonder why do we need now that it is really an ixgbe device all over the
> place?
> >
> > This device specific API, so it is missing merits of abstraction
> > layer, application can these APIs with any port_id, API should be protected
> for it.
> 
> Ah ok, my bad - didn't realize from the patch that it affects only device
> specific API :) Would It be too much hassle to move these functions into a
> separate file (rte_ixgbe_pmd.c or so)?
> Konstantin
> 
> >
> > > Konstantin
> > >
All the device specific API functions are prefixed with rte_pmd_ixgbe so I don't think a separate file is necessary.

Regards,

Bernard.

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Ananyev, Konstantin]

Hi Bernard,

> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Friday, February 3, 2017 9:21 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> 
> Hi Konstantin,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > Konstantin
> > Sent: Wednesday, February 1, 2017 6:10 PM
> > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> >
> >
> >
> > > -----Original Message-----
> > > From: Yigit, Ferruh
> > > Sent: Wednesday, February 1, 2017 5:40 PM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu, Wenzhuo
> > > <wenzhuo.lu@intel.com>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > > rte_eth_dev_info_get
> > >
> > > On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
> > > > Hi Wenzhuo,
> > > >
> > > >> -----Original Message-----
> > > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> > > >> Sent: Wednesday, January 25, 2017 2:39 AM
> > > >> To: dev@dpdk.org
> > > >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > >> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > > >> rte_eth_dev_info_get
> > > >>
> > > >> It'not appropriate to call rte_eth_dev_info_get in PMD, as
> > > >> rte_eth_dev_info_get need to get info from PMD.
> > > >> Remove rte_eth_dev_info_get from PMD code and get the info
> > > >> directly.
> > > >>
> > > >> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > > >> ---
> > > >>  drivers/net/ixgbe/ixgbe_ethdev.c | 144
> > > >> ++++++++++++++++++---------------------
> > > >>  1 file changed, 68 insertions(+), 76 deletions(-)
> > > >>
> > > >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > >> index 64ce55a..f14a68b 100644
> > > >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > >> @@ -4401,17 +4401,17 @@ static int
> > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > > >>  	int rar_entry;
> > > >>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
> > > >>  	struct rte_eth_dev *dev;
> > > >> -	struct rte_eth_dev_info dev_info;
> > > >> +	struct rte_pci_device *pci_dev;
> > > >>
> > > >>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> > > >>
> > > >>  	dev = &rte_eth_devices[port];
> > > >> -	rte_eth_dev_info_get(port, &dev_info);
> > > >> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
> > > >>
> > > >> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> > > >> +	if (is_ixgbe_pmd(dev->data->drv_name))
> > > >>  		return -ENOTSUP;
> > > >
> > > > I wonder why do we need now that it is really an ixgbe device all over the
> > place?
> > >
> > > This device specific API, so it is missing merits of abstraction
> > > layer, application can these APIs with any port_id, API should be protected
> > for it.
> >
> > Ah ok, my bad - didn't realize from the patch that it affects only device
> > specific API :) Would It be too much hassle to move these functions into a
> > separate file (rte_ixgbe_pmd.c or so)?
> > Konstantin
> >
> > >
> > > > Konstantin
> > > >
> All the device specific API functions are prefixed with rte_pmd_ixgbe

That's ok.


> so I don't think a separate file is necessary.

So far I didn't say it is necessary.
Though I think it would be good to group these functions in a separate file
to help avoid confusion (as happened to me) and keep ixgbe_ethdev.c smaller and cleaner.
Again would be easier to maintain things in future, when more folks will come up with some extensions for it.
That's why I am asking:  would it be a lot of work to do?
It is probably worth doing it now, while we have this API relatively small.
Konstantin

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Iremonger, Bernard]

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, February 3, 2017 9:50 AM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> 
> Hi Bernard,
> 
> > -----Original Message-----
> > From: Iremonger, Bernard
> > Sent: Friday, February 3, 2017 9:21 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> > dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > rte_eth_dev_info_get
> >
> > Hi Konstantin,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > > Konstantin
> > > Sent: Wednesday, February 1, 2017 6:10 PM
> > > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo
> > > <wenzhuo.lu@intel.com>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > > rte_eth_dev_info_get
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Yigit, Ferruh
> > > > Sent: Wednesday, February 1, 2017 5:40 PM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu,
> > > > Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > > > rte_eth_dev_info_get
> > > >
> > > > On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
> > > > > Hi Wenzhuo,
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo
> Lu
> > > > >> Sent: Wednesday, January 25, 2017 2:39 AM
> > > > >> To: dev@dpdk.org
> > > > >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > > >> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > > > >> rte_eth_dev_info_get
> > > > >>
> > > > >> It'not appropriate to call rte_eth_dev_info_get in PMD, as
> > > > >> rte_eth_dev_info_get need to get info from PMD.
> > > > >> Remove rte_eth_dev_info_get from PMD code and get the info
> > > > >> directly.
> > > > >>
> > > > >> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > > > >> ---
> > > > >>  drivers/net/ixgbe/ixgbe_ethdev.c | 144
> > > > >> ++++++++++++++++++---------------------
> > > > >>  1 file changed, 68 insertions(+), 76 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > >> index 64ce55a..f14a68b 100644
> > > > >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > >> @@ -4401,17 +4401,17 @@ static int
> > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > > > >>  	int rar_entry;
> > > > >>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
> > > > >>  	struct rte_eth_dev *dev;
> > > > >> -	struct rte_eth_dev_info dev_info;
> > > > >> +	struct rte_pci_device *pci_dev;
> > > > >>
> > > > >>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> > > > >>
> > > > >>  	dev = &rte_eth_devices[port];
> > > > >> -	rte_eth_dev_info_get(port, &dev_info);
> > > > >> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
> > > > >>
> > > > >> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> > > > >> +	if (is_ixgbe_pmd(dev->data->drv_name))
> > > > >>  		return -ENOTSUP;
> > > > >
> > > > > I wonder why do we need now that it is really an ixgbe device
> > > > > all over the
> > > place?
> > > >
> > > > This device specific API, so it is missing merits of abstraction
> > > > layer, application can these APIs with any port_id, API should be
> > > > protected
> > > for it.
> > >
> > > Ah ok, my bad - didn't realize from the patch that it affects only
> > > device specific API :) Would It be too much hassle to move these
> > > functions into a separate file (rte_ixgbe_pmd.c or so)?
> > > Konstantin
> > >
> > > >
> > > > > Konstantin
> > > > >
> > All the device specific API functions are prefixed with rte_pmd_ixgbe
> 
> That's ok.
> 
> 
> > so I don't think a separate file is necessary.
> 
> So far I didn't say it is necessary.
> Though I think it would be good to group these functions in a separate file to
> help avoid confusion (as happened to me) and keep ixgbe_ethdev.c smaller
> and cleaner.
> Again would be easier to maintain things in future, when more folks will
> come up with some extensions for it.
> That's why I am asking:  would it be a lot of work to do?
> It is probably worth doing it now, while we have this API relatively small.
> Konstantin
> 
I would need to investigate what is involved in moving the rte_pmd_ixgbe_* functions to a separate file.
They may be using some of the static functions and data in the ixgbe_ethdev.c  file which could be a problem.
The rte_pmd_ixgbe_* functions are considered an "interim solution" until a "generic solution" is agreed.
It might be best to postpone this work until the "generic solution" is agreed.

Regards,

Bernard.

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Ferruh Yigit]

On 2/3/2017 6:50 AM, Lu, Wenzhuo wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Monday, January 30, 2017 8:16 PM
>> To: Bie, Tiwei; Lu, Wenzhuo
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>
>> On 1/25/2017 5:24 AM, Tiwei Bie wrote:
>>> On Wed, Jan 25, 2017 at 01:13:32PM +0800, Lu, Wenzhuo wrote:
>>>> Hi Tiwei,
>>>>
>>>>> -----Original Message-----
>>>>> From: Bie, Tiwei
>>>>> Sent: Wednesday, January 25, 2017 11:17 AM
>>>>> To: Lu, Wenzhuo
>>>>> Cc: dev@dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>>>> rte_eth_dev_info_get
>>>>>
>>>>> On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
>>>>>> It'not appropriate to call rte_eth_dev_info_get in PMD, as
>>>>>> rte_eth_dev_info_get need to get info from PMD.
>>>>>> Remove rte_eth_dev_info_get from PMD code and get the info directly.
>>>>>>
>>>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>>>> ---
>>>>>>  drivers/net/ixgbe/ixgbe_ethdev.c | 144
>>>>>> ++++++++++++++++++---------------------
>>>>>>  1 file changed, 68 insertions(+), 76 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>> index 64ce55a..f14a68b 100644
>>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>> @@ -4401,17 +4401,17 @@ static int
>>>>> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>>>>>  	int rar_entry;
>>>>>>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
>>>>>>  	struct rte_eth_dev *dev;
>>>>>> -	struct rte_eth_dev_info dev_info;
>>>>>> +	struct rte_pci_device *pci_dev;
>>>>>>
>>>>>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>>>>>
>>>>>>  	dev = &rte_eth_devices[port];
>>>>>> -	rte_eth_dev_info_get(port, &dev_info);
>>>>>> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
>>>>>>
>>>>>> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
>>>>>> +	if (is_ixgbe_pmd(dev->data->drv_name))
>>>>>>  		return -ENOTSUP;
>>>>>>
>>>>>
>>>>> The return value of is_ixgbe_pmd() is not boolean (actually I think
>>>>> it should be based on its name). If we omit the comparison with 0,
>>>>> the code looks weird. It looks like it'll return -ENOTSUP if the port's driver
>> is ixgbe PMD.
>>>>
>>>> Yes, it’s weird. But what makes it weird is not the missing comparison but
>> the function name.
>>>> Better changing it to ixgbe_pmd_check. How about it?
>>>>
>>>
>>> Yeah, I also prefer to change the helper function itself. But I'm not
>>> good at the naming. I'd like to hear others' opinion. :-)
>>
>> Agree that it looks wrong without 0 comparison.
>>
>> Helper function is checking if the given port is an ixgbe port or not,
>> unfortunately you need to this for PMD specific APIs.
>> So What about is_device_supported(),
>>
>> I agree it is better if it returns bool, and I also think it is better if it gets the
>> rte_eth_dev as input parameter, validating port based on name is API internal
>> knowledge.
>>
>> Also instead of name comparison against fixed string, it can be eth_dev-
>>> driver->pci_drv.name against driver->name. This makes function more
> Thanks for your suggestion. But I don’t get your point here. 
> For a specific device, should not the eth_dev->driver->pci_drv.name and the driver->name be the same?

Yes they are same.

But there is a intention to unlink "eth_drv->pci_drv", to better support
non pci devices,

so instead of a PMD directly accessing name through this link, I believe
it is better to use rte_drier->name, which is more generic.

> 
> 
>> generic, and perhaps this helper function can be moved into ethdev layer,
>> later. For this function needs to get both eth_dev and rte_driver as argument.
>>
>>
>>>
>>> Best regards,
>>> Tiwei Bie
>>>
> 

Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

[Ferruh Yigit]

On 2/3/2017 10:02 AM, Iremonger, Bernard wrote:
> Hi Konstantin,
> 
>> -----Original Message-----
>> From: Ananyev, Konstantin
>> Sent: Friday, February 3, 2017 9:50 AM
>> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>> dev@dpdk.org
>> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>
>> Hi Bernard,
>>
>>> -----Original Message-----
>>> From: Iremonger, Bernard
>>> Sent: Friday, February 3, 2017 9:21 AM
>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh
>>> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>>> dev@dpdk.org
>>> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>> rte_eth_dev_info_get
>>>
>>> Hi Konstantin,
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
>>>> Konstantin
>>>> Sent: Wednesday, February 1, 2017 6:10 PM
>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo
>>>> <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>>> rte_eth_dev_info_get
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Yigit, Ferruh
>>>>> Sent: Wednesday, February 1, 2017 5:40 PM
>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu,
>>>>> Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>>>> rte_eth_dev_info_get
>>>>>
>>>>> On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
>>>>>> Hi Wenzhuo,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo
>> Lu
>>>>>>> Sent: Wednesday, January 25, 2017 2:39 AM
>>>>>>> To: dev@dpdk.org
>>>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>>>>>>> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>>>>>> rte_eth_dev_info_get
>>>>>>>
>>>>>>> It'not appropriate to call rte_eth_dev_info_get in PMD, as
>>>>>>> rte_eth_dev_info_get need to get info from PMD.
>>>>>>> Remove rte_eth_dev_info_get from PMD code and get the info
>>>>>>> directly.
>>>>>>>
>>>>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>>>>> ---
>>>>>>>  drivers/net/ixgbe/ixgbe_ethdev.c | 144
>>>>>>> ++++++++++++++++++---------------------
>>>>>>>  1 file changed, 68 insertions(+), 76 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> index 64ce55a..f14a68b 100644
>>>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> @@ -4401,17 +4401,17 @@ static int
>>>> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>>>>>>  	int rar_entry;
>>>>>>>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
>>>>>>>  	struct rte_eth_dev *dev;
>>>>>>> -	struct rte_eth_dev_info dev_info;
>>>>>>> +	struct rte_pci_device *pci_dev;
>>>>>>>
>>>>>>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>>>>>>
>>>>>>>  	dev = &rte_eth_devices[port];
>>>>>>> -	rte_eth_dev_info_get(port, &dev_info);
>>>>>>> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
>>>>>>>
>>>>>>> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
>>>>>>> +	if (is_ixgbe_pmd(dev->data->drv_name))
>>>>>>>  		return -ENOTSUP;
>>>>>>
>>>>>> I wonder why do we need now that it is really an ixgbe device
>>>>>> all over the
>>>> place?
>>>>>
>>>>> This device specific API, so it is missing merits of abstraction
>>>>> layer, application can these APIs with any port_id, API should be
>>>>> protected
>>>> for it.
>>>>
>>>> Ah ok, my bad - didn't realize from the patch that it affects only
>>>> device specific API :) Would It be too much hassle to move these
>>>> functions into a separate file (rte_ixgbe_pmd.c or so)?
>>>> Konstantin
>>>>
>>>>>
>>>>>> Konstantin
>>>>>>
>>> All the device specific API functions are prefixed with rte_pmd_ixgbe
>>
>> That's ok.
>>
>>
>>> so I don't think a separate file is necessary.
>>
>> So far I didn't say it is necessary.
>> Though I think it would be good to group these functions in a separate file to
>> help avoid confusion (as happened to me) and keep ixgbe_ethdev.c smaller
>> and cleaner.
>> Again would be easier to maintain things in future, when more folks will
>> come up with some extensions for it.
>> That's why I am asking:  would it be a lot of work to do?
>> It is probably worth doing it now, while we have this API relatively small.
>> Konstantin
>>
> I would need to investigate what is involved in moving the rte_pmd_ixgbe_* functions to a separate file.
> They may be using some of the static functions and data in the ixgbe_ethdev.c  file which could be a problem.
> The rte_pmd_ixgbe_* functions are considered an "interim solution" until a "generic solution" is agreed.
> It might be best to postpone this work until the "generic solution" is agreed.

Still I believe it is good idea to move these functions into a separate
file, but not for this release J.

For PMD specific APIs, even we keep using direct API call method or
benefit from abstraction layer, means of using ioctl perhaps, these APIs
will be implemented in the PMD. And logically grouping them is good idea.
Briefly, I don't think we need to wait until "generic solution" agreed
for this.

But, of course an investigation needs to be done on required effort, and
decide based on that.

Thanks,
ferruh

> 
> Regards,
> 
> Bernard.
> 

[dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get

[Wenzhuo Lu]

It's not appropriate to call rte_eth_dev_info_get in PMD,
as rte_eth_dev_info_get need to get info from PMD.
Remove rte_eth_dev_info_get from PMD code and get the
info directly.

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
v2:
- change is_ixgbe_pmd to is_device_supported to make it more generic.

 drivers/net/ixgbe/ixgbe_ethdev.c | 160 ++++++++++++++++++---------------------
 1 file changed, 75 insertions(+), 85 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 5b625a3..2f8d43a 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -253,7 +253,7 @@ static void ixgbe_add_rar(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 static void ixgbe_set_default_mac_addr(struct rte_eth_dev *dev,
 					   struct ether_addr *mac_addr);
 static void ixgbe_dcb_init(struct ixgbe_hw *hw, struct ixgbe_dcb_config *dcb_config);
-static int is_ixgbe_pmd(const char *driver_name);
+static int is_device_supported(struct rte_eth_dev *dev, struct eth_driver *drv);
 
 /* For Virtual Function support */
 static int eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev);
@@ -4380,16 +4380,14 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	ixgbe_add_rar(dev, addr, 0, 0);
 }
 
-static int
-is_ixgbe_pmd(const char *driver_name)
+static bool
+is_device_supported(struct rte_eth_dev *dev, struct eth_driver *drv)
 {
-	if (!strstr(driver_name, "ixgbe"))
-		return -ENOTSUP;
+	if (strcmp(dev->driver->pci_drv.driver.name,
+		   drv->pci_drv.driver.name))
+		return FALSE;
 
-	if (strstr(driver_name, "ixgbe_vf"))
-		return -ENOTSUP;
-
-	return 0;
+	return TRUE;
 }
 
 int
@@ -4401,17 +4399,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	int rar_entry;
 	uint8_t *new_mac = (uint8_t *)(mac_addr);
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
@@ -4902,17 +4900,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	struct ixgbe_mac_info *mac;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -4932,17 +4930,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	struct ixgbe_mac_info *mac;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -4961,17 +4959,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	uint32_t ctrl;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (vlan_id > ETHER_MAX_VLAN_ID)
@@ -4997,14 +4995,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	uint32_t ctrl;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
 	if (on > 1)
@@ -5031,14 +5027,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	int i;
 	int num_queues = (int)(IXGBE_QDE_IDX_MASK >> IXGBE_QDE_IDX_SHIFT);
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
 	if (on > 1)
@@ -5061,18 +5055,18 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	uint32_t reg_value;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
 	/* only support VF's 0 to 63 */
-	if ((vf >= dev_info.max_vfs) || (vf > 63))
+	if ((vf >= pci_dev->max_vfs) || (vf > 63))
 		return -EINVAL;
 
 	if (on > 1)
@@ -5094,19 +5088,21 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 rte_pmd_ixgbe_set_vf_vlan_stripq(uint8_t port, uint16_t vf, uint8_t on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
+	struct ixgbe_hw *hw;
 	uint16_t queues_per_pool;
 	uint32_t q;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
+	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5122,8 +5118,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	 * first 124 queues 0-123 will be allocated to VF's and only
 	 * the last 4 queues 123-127 will be assigned to the PF.
 	 */
-
-	queues_per_pool = dev_info.vmdq_queue_num / dev_info.max_vmdq_pools;
+	if (hw->mac.type == ixgbe_mac_82598EB)
+		queues_per_pool = (uint16_t)hw->mac.max_rx_queues /
+				  ETH_16_POOLS;
+	else
+		queues_per_pool = (uint16_t)hw->mac.max_rx_queues /
+				  ETH_64_POOLS;
 
 	for (q = 0; q < queues_per_pool; q++)
 		(*dev->dev_ops->vlan_strip_queue_set)(dev,
@@ -5136,19 +5136,19 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 {
 	int val = 0;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 	struct ixgbe_hw *hw;
 	uint32_t vmolr;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5181,7 +5181,7 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 rte_pmd_ixgbe_set_vf_rx(uint8_t port, uint16_t vf, uint8_t on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 	uint32_t reg, addr;
 	uint32_t val;
 	const uint8_t bit1 = 0x1;
@@ -5190,12 +5190,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5231,7 +5231,7 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 rte_pmd_ixgbe_set_vf_tx(uint8_t port, uint16_t vf, uint8_t on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 	uint32_t reg, addr;
 	uint32_t val;
 	const uint8_t bit1 = 0x1;
@@ -5241,12 +5241,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5282,7 +5282,6 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 			uint64_t vf_mask, uint8_t vlan_on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	int ret = 0;
 	uint16_t vf_idx;
 	struct ixgbe_hw *hw;
@@ -5290,9 +5289,8 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
 	if ((vlan > ETHER_MAX_VLAN_ID) || (vf_mask == 0))
@@ -5318,7 +5316,6 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
 	uint16_t tx_rate, uint64_t q_msk)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	struct ixgbe_hw *hw;
 	struct ixgbe_vf_info *vfinfo;
 	struct rte_eth_link link;
@@ -5332,13 +5329,13 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 	rte_eth_link_get_nowait(port, &link);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (tx_rate > link.link_speed)
@@ -5347,7 +5344,6 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
 	if (q_msk == 0)
 		return 0;
 
-	pci_dev = IXGBE_DEV_TO_PCI(dev);
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	vfinfo = *(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private));
 	nb_q_per_pool = RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool;
@@ -8227,16 +8223,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	/* Stop the data paths */
@@ -8311,16 +8306,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	/* Stop the data paths */
@@ -8376,16 +8370,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	ctrl = mac[0] | (mac[1] << 8) | (mac[2] << 16) | (mac[3] << 24);
@@ -8402,16 +8395,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	ctrl = mac[0] | (mac[1] << 8) | (mac[2] << 16) | (mac[3] << 24);
@@ -8430,16 +8422,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl, i;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	if (idx != 0 && idx != 1)
@@ -8487,16 +8478,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl, i;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	if (idx != 0 && idx != 1)
-- 
1.9.3

Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get

[Tiwei Bie]

On Mon, Feb 06, 2017 at 10:09:32AM +0800, Wenzhuo Lu wrote:
[...]
>  static void ixgbe_dcb_init(struct ixgbe_hw *hw, struct ixgbe_dcb_config *dcb_config);
> -static int is_ixgbe_pmd(const char *driver_name);
> +static int is_device_supported(struct rte_eth_dev *dev, struct eth_driver *drv);
>  

Should be:
static bool is_device_supported(struct rte_eth_dev *dev, struct eth_driver *drv);

>  /* For Virtual Function support */
>  static int eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev);
> @@ -4380,16 +4380,14 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>  	ixgbe_add_rar(dev, addr, 0, 0);
>  }
>  
> -static int
> -is_ixgbe_pmd(const char *driver_name)
> +static bool
> +is_device_supported(struct rte_eth_dev *dev, struct eth_driver *drv)
>  {
> -	if (!strstr(driver_name, "ixgbe"))
> -		return -ENOTSUP;
> +	if (strcmp(dev->driver->pci_drv.driver.name,
> +		   drv->pci_drv.driver.name))
> +		return FALSE;
>  

It would be better to use `false' instead of `FALSE'.

Best regards,
Tiwei Bie

Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get

[Lu, Wenzhuo]

Hi Tiwei,

> -----Original Message-----
> From: Bie, Tiwei
> Sent: Monday, February 6, 2017 10:31 AM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get
> 
> On Mon, Feb 06, 2017 at 10:09:32AM +0800, Wenzhuo Lu wrote:
> [...]
> >  static void ixgbe_dcb_init(struct ixgbe_hw *hw, struct
> > ixgbe_dcb_config *dcb_config); -static int is_ixgbe_pmd(const char
> > *driver_name);
> > +static int is_device_supported(struct rte_eth_dev *dev, struct
> > +eth_driver *drv);
> >
> 
> Should be:
> static bool is_device_supported(struct rte_eth_dev *dev, struct eth_driver
> *drv);
O, forget to change it. Thanks.

> 
> >  /* For Virtual Function support */
> >  static int eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev); @@
> > -4380,16 +4380,14 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> >  	ixgbe_add_rar(dev, addr, 0, 0);
> >  }
> >
> > -static int
> > -is_ixgbe_pmd(const char *driver_name)
> > +static bool
> > +is_device_supported(struct rte_eth_dev *dev, struct eth_driver *drv)
> >  {
> > -	if (!strstr(driver_name, "ixgbe"))
> > -		return -ENOTSUP;
> > +	if (strcmp(dev->driver->pci_drv.driver.name,
> > +		   drv->pci_drv.driver.name))
> > +		return FALSE;
> >
> 
> It would be better to use `false' instead of `FALSE'.
I see both 'false' and 'FALSE' are defined and used. Is there any reason that 'false' is better?

> 
> Best regards,
> Tiwei Bie

Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get

[Tiwei Bie]

On Mon, Feb 06, 2017 at 10:41:28AM +0800, Lu, Wenzhuo wrote:
> Hi Tiwei,
> 
> > -----Original Message-----
> > From: Bie, Tiwei
> > Sent: Monday, February 6, 2017 10:31 AM
> > To: Lu, Wenzhuo
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get
> > 
> > On Mon, Feb 06, 2017 at 10:09:32AM +0800, Wenzhuo Lu wrote:
> > [...]
> > >  static void ixgbe_dcb_init(struct ixgbe_hw *hw, struct
> > > ixgbe_dcb_config *dcb_config); -static int is_ixgbe_pmd(const char
> > > *driver_name);
> > > +static int is_device_supported(struct rte_eth_dev *dev, struct
> > > +eth_driver *drv);
> > >
> > 
> > Should be:
> > static bool is_device_supported(struct rte_eth_dev *dev, struct eth_driver
> > *drv);
> O, forget to change it. Thanks.
> 
> > 
> > >  /* For Virtual Function support */
> > >  static int eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev); @@
> > > -4380,16 +4380,14 @@ static int
> > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > >  	ixgbe_add_rar(dev, addr, 0, 0);
> > >  }
> > >
> > > -static int
> > > -is_ixgbe_pmd(const char *driver_name)
> > > +static bool
> > > +is_device_supported(struct rte_eth_dev *dev, struct eth_driver *drv)
> > >  {
> > > -	if (!strstr(driver_name, "ixgbe"))
> > > -		return -ENOTSUP;
> > > +	if (strcmp(dev->driver->pci_drv.driver.name,
> > > +		   drv->pci_drv.driver.name))
> > > +		return FALSE;
> > >
> > 
> > It would be better to use `false' instead of `FALSE'.
> I see both 'false' and 'FALSE' are defined and used. Is there any reason that 'false' is better?
> 

I think `true' and `false' are standard keywords defined and
reserved by C. So I think it would be better to use them if
the return type is `bool'.

Best regards,
Tiwei Bie

Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get

[Lu, Wenzhuo]

Hi Tiwei,


> -----Original Message-----
> From: Bie, Tiwei
> Sent: Monday, February 6, 2017 10:51 AM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get
> 
> On Mon, Feb 06, 2017 at 10:41:28AM +0800, Lu, Wenzhuo wrote:
> > Hi Tiwei,
> >
> > > -----Original Message-----
> > > From: Bie, Tiwei
> > > Sent: Monday, February 6, 2017 10:31 AM
> > > To: Lu, Wenzhuo
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > rte_eth_dev_info_get
> > >
> > > On Mon, Feb 06, 2017 at 10:09:32AM +0800, Wenzhuo Lu wrote:
> > > [...]
> > > >  static void ixgbe_dcb_init(struct ixgbe_hw *hw, struct
> > > > ixgbe_dcb_config *dcb_config); -static int is_ixgbe_pmd(const char
> > > > *driver_name);
> > > > +static int is_device_supported(struct rte_eth_dev *dev, struct
> > > > +eth_driver *drv);
> > > >
> > >
> > > Should be:
> > > static bool is_device_supported(struct rte_eth_dev *dev, struct
> > > eth_driver *drv);
> > O, forget to change it. Thanks.
> >
> > >
> > > >  /* For Virtual Function support */  static int
> > > > eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev); @@
> > > > -4380,16 +4380,14 @@ static int
> > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > > >  	ixgbe_add_rar(dev, addr, 0, 0);
> > > >  }
> > > >
> > > > -static int
> > > > -is_ixgbe_pmd(const char *driver_name)
> > > > +static bool
> > > > +is_device_supported(struct rte_eth_dev *dev, struct eth_driver
> > > > +*drv)
> > > >  {
> > > > -	if (!strstr(driver_name, "ixgbe"))
> > > > -		return -ENOTSUP;
> > > > +	if (strcmp(dev->driver->pci_drv.driver.name,
> > > > +		   drv->pci_drv.driver.name))
> > > > +		return FALSE;
> > > >
> > >
> > > It would be better to use `false' instead of `FALSE'.
> > I see both 'false' and 'FALSE' are defined and used. Is there any reason that
> 'false' is better?
> >
> 
> I think `true' and `false' are standard keywords defined and reserved by C. So I
> think it would be better to use them if the return type is `bool'.
O, there's no 'bool' in C. You have to define it. The same for 'false' and 'true'.

> 
> Best regards,
> Tiwei Bie

Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get

[Tiwei Bie]

On Mon, Feb 06, 2017 at 10:59:42AM +0800, Lu, Wenzhuo wrote:
> Hi Tiwei,
> 
> 
> > -----Original Message-----
> > From: Bie, Tiwei
> > Sent: Monday, February 6, 2017 10:51 AM
> > To: Lu, Wenzhuo
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get
> > 
> > On Mon, Feb 06, 2017 at 10:41:28AM +0800, Lu, Wenzhuo wrote:
> > > Hi Tiwei,
> > >
> > > > -----Original Message-----
> > > > From: Bie, Tiwei
> > > > Sent: Monday, February 6, 2017 10:31 AM
> > > > To: Lu, Wenzhuo
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > > rte_eth_dev_info_get
> > > >
> > > > On Mon, Feb 06, 2017 at 10:09:32AM +0800, Wenzhuo Lu wrote:
> > > > [...]
> > > > >  static void ixgbe_dcb_init(struct ixgbe_hw *hw, struct
> > > > > ixgbe_dcb_config *dcb_config); -static int is_ixgbe_pmd(const char
> > > > > *driver_name);
> > > > > +static int is_device_supported(struct rte_eth_dev *dev, struct
> > > > > +eth_driver *drv);
> > > > >
> > > >
> > > > Should be:
> > > > static bool is_device_supported(struct rte_eth_dev *dev, struct
> > > > eth_driver *drv);
> > > O, forget to change it. Thanks.
> > >
> > > >
> > > > >  /* For Virtual Function support */  static int
> > > > > eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev); @@
> > > > > -4380,16 +4380,14 @@ static int
> > > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > > > >  	ixgbe_add_rar(dev, addr, 0, 0);
> > > > >  }
> > > > >
> > > > > -static int
> > > > > -is_ixgbe_pmd(const char *driver_name)
> > > > > +static bool
> > > > > +is_device_supported(struct rte_eth_dev *dev, struct eth_driver
> > > > > +*drv)
> > > > >  {
> > > > > -	if (!strstr(driver_name, "ixgbe"))
> > > > > -		return -ENOTSUP;
> > > > > +	if (strcmp(dev->driver->pci_drv.driver.name,
> > > > > +		   drv->pci_drv.driver.name))
> > > > > +		return FALSE;
> > > > >
> > > >
> > > > It would be better to use `false' instead of `FALSE'.
> > > I see both 'false' and 'FALSE' are defined and used. Is there any reason that
> > 'false' is better?
> > >
> > 
> > I think `true' and `false' are standard keywords defined and reserved by C. So I
> > think it would be better to use them if the return type is `bool'.
> O, there's no 'bool' in C. You have to define it. The same for 'false' and 'true'.
> 

The `bool', `true' and `false' are all standard keywords defined and
reserved by C, although the stdbool.h is not used in ixgbe.

C adds this support by introducing a new header stdbool.h:

#ifndef __bool_true_false_are_defined
#define __bool_true_false_are_defined   1

#ifndef __cplusplus

#define false   0
#define true    1

#define bool    _Bool
#if __STDC_VERSION__ < 199901L && __GNUC__ < 3 && !defined(__INTEL_COMPILER)
typedef int     _Bool;
#endif

#endif /* !__cplusplus */
#endif /* __bool_true_false_are_defined */

Best regards,
Tiwei Bie

Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get

[Lu, Wenzhuo]

Hi Tiwei,

> -----Original Message-----
> From: Bie, Tiwei
> Sent: Monday, February 6, 2017 11:08 AM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get
> 
> On Mon, Feb 06, 2017 at 10:59:42AM +0800, Lu, Wenzhuo wrote:
> > Hi Tiwei,
> >
> >
> > > -----Original Message-----
> > > From: Bie, Tiwei
> > > Sent: Monday, February 6, 2017 10:51 AM
> > > To: Lu, Wenzhuo
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > rte_eth_dev_info_get
> > >
> > > On Mon, Feb 06, 2017 at 10:41:28AM +0800, Lu, Wenzhuo wrote:
> > > > Hi Tiwei,
> > > >
> > > > > -----Original Message-----
> > > > > From: Bie, Tiwei
> > > > > Sent: Monday, February 6, 2017 10:31 AM
> > > > > To: Lu, Wenzhuo
> > > > > Cc: dev@dpdk.org
> > > > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > > > rte_eth_dev_info_get
> > > > >
> > > > > On Mon, Feb 06, 2017 at 10:09:32AM +0800, Wenzhuo Lu wrote:
> > > > > [...]
> > > > > >  static void ixgbe_dcb_init(struct ixgbe_hw *hw, struct
> > > > > > ixgbe_dcb_config *dcb_config); -static int is_ixgbe_pmd(const
> > > > > > char *driver_name);
> > > > > > +static int is_device_supported(struct rte_eth_dev *dev,
> > > > > > +struct eth_driver *drv);
> > > > > >
> > > > >
> > > > > Should be:
> > > > > static bool is_device_supported(struct rte_eth_dev *dev, struct
> > > > > eth_driver *drv);
> > > > O, forget to change it. Thanks.
> > > >
> > > > >
> > > > > >  /* For Virtual Function support */  static int
> > > > > > eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev); @@
> > > > > > -4380,16 +4380,14 @@ static int
> > > > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev
> > > > > *dev,
> > > > > >  	ixgbe_add_rar(dev, addr, 0, 0);  }
> > > > > >
> > > > > > -static int
> > > > > > -is_ixgbe_pmd(const char *driver_name)
> > > > > > +static bool
> > > > > > +is_device_supported(struct rte_eth_dev *dev, struct
> > > > > > +eth_driver
> > > > > > +*drv)
> > > > > >  {
> > > > > > -	if (!strstr(driver_name, "ixgbe"))
> > > > > > -		return -ENOTSUP;
> > > > > > +	if (strcmp(dev->driver->pci_drv.driver.name,
> > > > > > +		   drv->pci_drv.driver.name))
> > > > > > +		return FALSE;
> > > > > >
> > > > >
> > > > > It would be better to use `false' instead of `FALSE'.
> > > > I see both 'false' and 'FALSE' are defined and used. Is there any
> > > > reason that
> > > 'false' is better?
> > > >
> > >
> > > I think `true' and `false' are standard keywords defined and
> > > reserved by C. So I think it would be better to use them if the return type is
> `bool'.
> > O, there's no 'bool' in C. You have to define it. The same for 'false' and 'true'.
> >
> 
> The `bool', `true' and `false' are all standard keywords defined and reserved by
> C, although the stdbool.h is not used in ixgbe.
> 
> C adds this support by introducing a new header stdbool.h:
> 
> #ifndef __bool_true_false_are_defined
> #define __bool_true_false_are_defined   1
> 
> #ifndef __cplusplus
> 
> #define false   0
> #define true    1
> 
> #define bool    _Bool
> #if __STDC_VERSION__ < 199901L && __GNUC__ < 3
> && !defined(__INTEL_COMPILER)
> typedef int     _Bool;
> #endif
O, you're talking about C99. _Bool is a  keyword added by it. 'bool', 'true', 'false' are  not. That's why this header file have to define them.

> 
> #endif /* !__cplusplus */
> #endif /* __bool_true_false_are_defined */
> 
> Best regards,
> Tiwei Bie

Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get

[Tiwei Bie]

On Mon, Feb 06, 2017 at 11:45:41AM +0800, Lu, Wenzhuo wrote:
> Hi Tiwei,
> 
> > -----Original Message-----
> > From: Bie, Tiwei
> > Sent: Monday, February 6, 2017 11:08 AM
> > To: Lu, Wenzhuo
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get
> > 
> > On Mon, Feb 06, 2017 at 10:59:42AM +0800, Lu, Wenzhuo wrote:
> > > Hi Tiwei,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Bie, Tiwei
> > > > Sent: Monday, February 6, 2017 10:51 AM
> > > > To: Lu, Wenzhuo
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > > rte_eth_dev_info_get
> > > >
> > > > On Mon, Feb 06, 2017 at 10:41:28AM +0800, Lu, Wenzhuo wrote:
> > > > > Hi Tiwei,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Bie, Tiwei
> > > > > > Sent: Monday, February 6, 2017 10:31 AM
> > > > > > To: Lu, Wenzhuo
> > > > > > Cc: dev@dpdk.org
> > > > > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > > > > rte_eth_dev_info_get
> > > > > >
> > > > > > On Mon, Feb 06, 2017 at 10:09:32AM +0800, Wenzhuo Lu wrote:
> > > > > > [...]
> > > > > > >  static void ixgbe_dcb_init(struct ixgbe_hw *hw, struct
> > > > > > > ixgbe_dcb_config *dcb_config); -static int is_ixgbe_pmd(const
> > > > > > > char *driver_name);
> > > > > > > +static int is_device_supported(struct rte_eth_dev *dev,
> > > > > > > +struct eth_driver *drv);
> > > > > > >
> > > > > >
> > > > > > Should be:
> > > > > > static bool is_device_supported(struct rte_eth_dev *dev, struct
> > > > > > eth_driver *drv);
> > > > > O, forget to change it. Thanks.
> > > > >
> > > > > >
> > > > > > >  /* For Virtual Function support */  static int
> > > > > > > eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev); @@
> > > > > > > -4380,16 +4380,14 @@ static int
> > > > > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev
> > > > > > *dev,
> > > > > > >  	ixgbe_add_rar(dev, addr, 0, 0);  }
> > > > > > >
> > > > > > > -static int
> > > > > > > -is_ixgbe_pmd(const char *driver_name)
> > > > > > > +static bool
> > > > > > > +is_device_supported(struct rte_eth_dev *dev, struct
> > > > > > > +eth_driver
> > > > > > > +*drv)
> > > > > > >  {
> > > > > > > -	if (!strstr(driver_name, "ixgbe"))
> > > > > > > -		return -ENOTSUP;
> > > > > > > +	if (strcmp(dev->driver->pci_drv.driver.name,
> > > > > > > +		   drv->pci_drv.driver.name))
> > > > > > > +		return FALSE;
> > > > > > >
> > > > > >
> > > > > > It would be better to use `false' instead of `FALSE'.
> > > > > I see both 'false' and 'FALSE' are defined and used. Is there any
> > > > > reason that
> > > > 'false' is better?
> > > > >
> > > >
> > > > I think `true' and `false' are standard keywords defined and
> > > > reserved by C. So I think it would be better to use them if the return type is
> > `bool'.
> > > O, there's no 'bool' in C. You have to define it. The same for 'false' and 'true'.
> > >
> > 
> > The `bool', `true' and `false' are all standard keywords defined and reserved by
> > C, although the stdbool.h is not used in ixgbe.
> > 
> > C adds this support by introducing a new header stdbool.h:
> > 
> > #ifndef __bool_true_false_are_defined
> > #define __bool_true_false_are_defined   1
> > 
> > #ifndef __cplusplus
> > 
> > #define false   0
> > #define true    1
> > 
> > #define bool    _Bool
> > #if __STDC_VERSION__ < 199901L && __GNUC__ < 3
> > && !defined(__INTEL_COMPILER)
> > typedef int     _Bool;
> > #endif
> O, you're talking about C99. _Bool is a  keyword added by it. 'bool', 'true', 'false' are  not. That's why this header file have to define them.
> 

C99 added all those as keyword, although doesn't implement
all of them as the builtin type (e.g. int). All of them are
standard keywords defined by C99. The `bool', `true' and
`false' are defined in section 7.16 of the C99 spec [1] and
implemented as macros:

7.16 Boolean type and values <stdbool.h>

1 The header <stdbool.h> defines four macros.

2 The macro
          bool
expands to _Bool.

3 The remaining three macros are suitable for use in #if preprocessing directives. They are
          true
which expands to the integer constant 1,
          false
which expands to the integer constant 0, and
          __bool_true_false_are_defined
which expands to the integer constant 1.

4 Notwithstanding the provisions of 7.1.3, a program may undefine and perhaps then redefine the macros bool, true, and false.222)

Footnotes

222) See ''future library directions'' (7.26.7).

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

Best regards,
Tiwei Bie

Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get

[Lu, Wenzhuo]

> -----Original Message-----
> From: Bie, Tiwei
> Sent: Monday, February 6, 2017 12:57 PM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get
> 
> On Mon, Feb 06, 2017 at 11:45:41AM +0800, Lu, Wenzhuo wrote:
> > Hi Tiwei,
> >
> > > -----Original Message-----
> > > From: Bie, Tiwei
> > > Sent: Monday, February 6, 2017 11:08 AM
> > > To: Lu, Wenzhuo
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > rte_eth_dev_info_get
> > >
> > > On Mon, Feb 06, 2017 at 10:59:42AM +0800, Lu, Wenzhuo wrote:
> > > > Hi Tiwei,
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Bie, Tiwei
> > > > > Sent: Monday, February 6, 2017 10:51 AM
> > > > > To: Lu, Wenzhuo
> > > > > Cc: dev@dpdk.org
> > > > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > > > rte_eth_dev_info_get
> > > > >
> > > > > On Mon, Feb 06, 2017 at 10:41:28AM +0800, Lu, Wenzhuo wrote:
> > > > > > Hi Tiwei,
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Bie, Tiwei
> > > > > > > Sent: Monday, February 6, 2017 10:31 AM
> > > > > > > To: Lu, Wenzhuo
> > > > > > > Cc: dev@dpdk.org
> > > > > > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > > > > > rte_eth_dev_info_get
> > > > > > >
> > > > > > > On Mon, Feb 06, 2017 at 10:09:32AM +0800, Wenzhuo Lu wrote:
> > > > > > > [...]
> > > > > > > >  static void ixgbe_dcb_init(struct ixgbe_hw *hw, struct
> > > > > > > > ixgbe_dcb_config *dcb_config); -static int
> > > > > > > > is_ixgbe_pmd(const char *driver_name);
> > > > > > > > +static int is_device_supported(struct rte_eth_dev *dev,
> > > > > > > > +struct eth_driver *drv);
> > > > > > > >
> > > > > > >
> > > > > > > Should be:
> > > > > > > static bool is_device_supported(struct rte_eth_dev *dev,
> > > > > > > struct eth_driver *drv);
> > > > > > O, forget to change it. Thanks.
> > > > > >
> > > > > > >
> > > > > > > >  /* For Virtual Function support */  static int
> > > > > > > > eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev); @@
> > > > > > > > -4380,16 +4380,14 @@ static int
> > > > > > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev
> > > > > > > *dev,
> > > > > > > >  	ixgbe_add_rar(dev, addr, 0, 0);  }
> > > > > > > >
> > > > > > > > -static int
> > > > > > > > -is_ixgbe_pmd(const char *driver_name)
> > > > > > > > +static bool
> > > > > > > > +is_device_supported(struct rte_eth_dev *dev, struct
> > > > > > > > +eth_driver
> > > > > > > > +*drv)
> > > > > > > >  {
> > > > > > > > -	if (!strstr(driver_name, "ixgbe"))
> > > > > > > > -		return -ENOTSUP;
> > > > > > > > +	if (strcmp(dev->driver->pci_drv.driver.name,
> > > > > > > > +		   drv->pci_drv.driver.name))
> > > > > > > > +		return FALSE;
> > > > > > > >
> > > > > > >
> > > > > > > It would be better to use `false' instead of `FALSE'.
> > > > > > I see both 'false' and 'FALSE' are defined and used. Is there
> > > > > > any reason that
> > > > > 'false' is better?
> > > > > >
> > > > >
> > > > > I think `true' and `false' are standard keywords defined and
> > > > > reserved by C. So I think it would be better to use them if the
> > > > > return type is
> > > `bool'.
> > > > O, there's no 'bool' in C. You have to define it. The same for 'false' and
> 'true'.
> > > >
> > >
> > > The `bool', `true' and `false' are all standard keywords defined and
> > > reserved by C, although the stdbool.h is not used in ixgbe.
> > >
> > > C adds this support by introducing a new header stdbool.h:
> > >
> > > #ifndef __bool_true_false_are_defined
> > > #define __bool_true_false_are_defined   1
> > >
> > > #ifndef __cplusplus
> > >
> > > #define false   0
> > > #define true    1
> > >
> > > #define bool    _Bool
> > > #if __STDC_VERSION__ < 199901L && __GNUC__ < 3 &&
> > > !defined(__INTEL_COMPILER)
> > > typedef int     _Bool;
> > > #endif
> > O, you're talking about C99. _Bool is a  keyword added by it. 'bool', 'true',
> 'false' are  not. That's why this header file have to define them.
> >
> 
> C99 added all those as keyword, although doesn't implement all of them as
> the builtin type (e.g. int). All of them are standard keywords defined by C99.
> The `bool', `true' and `false' are defined in section 7.16 of the C99 spec [1] and
> implemented as macros:
> 
> 7.16 Boolean type and values <stdbool.h>
> 
> 1 The header <stdbool.h> defines four macros.
> 
> 2 The macro
>           bool
> expands to _Bool.
> 
> 3 The remaining three macros are suitable for use in #if preprocessing
> directives. They are
>           true
> which expands to the integer constant 1,
>           false
> which expands to the integer constant 0, and
>           __bool_true_false_are_defined
> which expands to the integer constant 1.
> 
> 4 Notwithstanding the provisions of 7.1.3, a program may undefine and
> perhaps then redefine the macros bool, true, and false.222)
> 
> Footnotes
> 
> 222) See ''future library directions'' (7.26.7).
> 
> [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
O, I see the divergence. It's about the term 'keyword'. I only count '6.4.1 Keywords'. 
Anyway, as both 'false'/'true' and 'FALSE'/'TRUE' are defined. I don’t know why we cannot use any of them. If 'FALSE'/'TRUE' is not preferred, better create a new patch to clean them up.

> 
> Best regards,
> Tiwei Bie

Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get

[Tiwei Bie]

On Mon, Feb 06, 2017 at 01:17:30PM +0800, Lu, Wenzhuo wrote:
> > -----Original Message-----
> > From: Bie, Tiwei
> > Sent: Monday, February 6, 2017 12:57 PM
> > To: Lu, Wenzhuo
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up rte_eth_dev_info_get
> > 
> > On Mon, Feb 06, 2017 at 11:45:41AM +0800, Lu, Wenzhuo wrote:
> > > Hi Tiwei,
> > >
> > > > -----Original Message-----
> > > > From: Bie, Tiwei
> > > > Sent: Monday, February 6, 2017 11:08 AM
> > > > To: Lu, Wenzhuo
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > > rte_eth_dev_info_get
> > > >
> > > > On Mon, Feb 06, 2017 at 10:59:42AM +0800, Lu, Wenzhuo wrote:
> > > > > Hi Tiwei,
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Bie, Tiwei
> > > > > > Sent: Monday, February 6, 2017 10:51 AM
> > > > > > To: Lu, Wenzhuo
> > > > > > Cc: dev@dpdk.org
> > > > > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > > > > rte_eth_dev_info_get
> > > > > >
> > > > > > On Mon, Feb 06, 2017 at 10:41:28AM +0800, Lu, Wenzhuo wrote:
> > > > > > > Hi Tiwei,
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Bie, Tiwei
> > > > > > > > Sent: Monday, February 6, 2017 10:31 AM
> > > > > > > > To: Lu, Wenzhuo
> > > > > > > > Cc: dev@dpdk.org
> > > > > > > > Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: clean up
> > > > > > > > rte_eth_dev_info_get
> > > > > > > >
> > > > > > > > On Mon, Feb 06, 2017 at 10:09:32AM +0800, Wenzhuo Lu wrote:
> > > > > > > > [...]
> > > > > > > > >  static void ixgbe_dcb_init(struct ixgbe_hw *hw, struct
> > > > > > > > > ixgbe_dcb_config *dcb_config); -static int
> > > > > > > > > is_ixgbe_pmd(const char *driver_name);
> > > > > > > > > +static int is_device_supported(struct rte_eth_dev *dev,
> > > > > > > > > +struct eth_driver *drv);
> > > > > > > > >
> > > > > > > >
> > > > > > > > Should be:
> > > > > > > > static bool is_device_supported(struct rte_eth_dev *dev,
> > > > > > > > struct eth_driver *drv);
> > > > > > > O, forget to change it. Thanks.
> > > > > > >
> > > > > > > >
> > > > > > > > >  /* For Virtual Function support */  static int
> > > > > > > > > eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev); @@
> > > > > > > > > -4380,16 +4380,14 @@ static int
> > > > > > > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev
> > > > > > > > *dev,
> > > > > > > > >  	ixgbe_add_rar(dev, addr, 0, 0);  }
> > > > > > > > >
> > > > > > > > > -static int
> > > > > > > > > -is_ixgbe_pmd(const char *driver_name)
> > > > > > > > > +static bool
> > > > > > > > > +is_device_supported(struct rte_eth_dev *dev, struct
> > > > > > > > > +eth_driver
> > > > > > > > > +*drv)
> > > > > > > > >  {
> > > > > > > > > -	if (!strstr(driver_name, "ixgbe"))
> > > > > > > > > -		return -ENOTSUP;
> > > > > > > > > +	if (strcmp(dev->driver->pci_drv.driver.name,
> > > > > > > > > +		   drv->pci_drv.driver.name))
> > > > > > > > > +		return FALSE;
> > > > > > > > >
> > > > > > > >
> > > > > > > > It would be better to use `false' instead of `FALSE'.
> > > > > > > I see both 'false' and 'FALSE' are defined and used. Is there
> > > > > > > any reason that
> > > > > > 'false' is better?
> > > > > > >
> > > > > >
> > > > > > I think `true' and `false' are standard keywords defined and
> > > > > > reserved by C. So I think it would be better to use them if the
> > > > > > return type is
> > > > `bool'.
> > > > > O, there's no 'bool' in C. You have to define it. The same for 'false' and
> > 'true'.
> > > > >
> > > >
> > > > The `bool', `true' and `false' are all standard keywords defined and
> > > > reserved by C, although the stdbool.h is not used in ixgbe.
> > > >
> > > > C adds this support by introducing a new header stdbool.h:
> > > >
> > > > #ifndef __bool_true_false_are_defined
> > > > #define __bool_true_false_are_defined   1
> > > >
> > > > #ifndef __cplusplus
> > > >
> > > > #define false   0
> > > > #define true    1
> > > >
> > > > #define bool    _Bool
> > > > #if __STDC_VERSION__ < 199901L && __GNUC__ < 3 &&
> > > > !defined(__INTEL_COMPILER)
> > > > typedef int     _Bool;
> > > > #endif
> > > O, you're talking about C99. _Bool is a  keyword added by it. 'bool', 'true',
> > 'false' are  not. That's why this header file have to define them.
> > >
> > 
> > C99 added all those as keyword, although doesn't implement all of them as
> > the builtin type (e.g. int). All of them are standard keywords defined by C99.
> > The `bool', `true' and `false' are defined in section 7.16 of the C99 spec [1] and
> > implemented as macros:
> > 
> > 7.16 Boolean type and values <stdbool.h>
> > 
> > 1 The header <stdbool.h> defines four macros.
> > 
> > 2 The macro
> >           bool
> > expands to _Bool.
> > 
> > 3 The remaining three macros are suitable for use in #if preprocessing
> > directives. They are
> >           true
> > which expands to the integer constant 1,
> >           false
> > which expands to the integer constant 0, and
> >           __bool_true_false_are_defined
> > which expands to the integer constant 1.
> > 
> > 4 Notwithstanding the provisions of 7.1.3, a program may undefine and
> > perhaps then redefine the macros bool, true, and false.222)
> > 
> > Footnotes
> > 
> > 222) See ''future library directions'' (7.26.7).
> > 
> > [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
> O, I see the divergence. It's about the term 'keyword'. I only count '6.4.1 Keywords'. 
> Anyway, as both 'false'/'true' and 'FALSE'/'TRUE' are defined. I don’t know why we cannot use any of them. If 'FALSE'/'TRUE' is not preferred, better create a new patch to clean them up.
> 

I didn't say we cannot use FALSE/TRUE, I just suggested that false/true
would be better. :-)

I think the reason why introduce _Bool as builtin keyword and implement
others as macros is to provide applications the ability to redefine them
for compatibility issues. I think new code should follow the spec if
possible. :-)

Best regards,
Tiwei Bie

[dpdk-dev] [PATCH v3] net/ixgbe: clean up rte_eth_dev_info_get

[Wenzhuo Lu]

It's not appropriate to call rte_eth_dev_info_get in PMD,
as rte_eth_dev_info_get need to get info from PMD.
Remove rte_eth_dev_info_get from PMD code and get the
info directly.

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
v2:
- change is_ixgbe_pmd to is_device_supported to make it more generic.
v3:
- minor change.

 drivers/net/ixgbe/ixgbe_ethdev.c | 161 ++++++++++++++++++---------------------
 1 file changed, 76 insertions(+), 85 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 5b625a3..e565ae3 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -253,7 +253,8 @@ static void ixgbe_add_rar(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 static void ixgbe_set_default_mac_addr(struct rte_eth_dev *dev,
 					   struct ether_addr *mac_addr);
 static void ixgbe_dcb_init(struct ixgbe_hw *hw, struct ixgbe_dcb_config *dcb_config);
-static int is_ixgbe_pmd(const char *driver_name);
+static bool is_device_supported(struct rte_eth_dev *dev,
+				struct eth_driver *drv);
 
 /* For Virtual Function support */
 static int eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev);
@@ -4380,16 +4381,14 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	ixgbe_add_rar(dev, addr, 0, 0);
 }
 
-static int
-is_ixgbe_pmd(const char *driver_name)
+static bool
+is_device_supported(struct rte_eth_dev *dev, struct eth_driver *drv)
 {
-	if (!strstr(driver_name, "ixgbe"))
-		return -ENOTSUP;
+	if (strcmp(dev->driver->pci_drv.driver.name,
+		   drv->pci_drv.driver.name))
+		return false;
 
-	if (strstr(driver_name, "ixgbe_vf"))
-		return -ENOTSUP;
-
-	return 0;
+	return true;
 }
 
 int
@@ -4401,17 +4400,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	int rar_entry;
 	uint8_t *new_mac = (uint8_t *)(mac_addr);
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
@@ -4902,17 +4901,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	struct ixgbe_mac_info *mac;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -4932,17 +4931,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	struct ixgbe_mac_info *mac;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -4961,17 +4960,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	uint32_t ctrl;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (vlan_id > ETHER_MAX_VLAN_ID)
@@ -4997,14 +4996,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	uint32_t ctrl;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
 	if (on > 1)
@@ -5031,14 +5028,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	int i;
 	int num_queues = (int)(IXGBE_QDE_IDX_MASK >> IXGBE_QDE_IDX_SHIFT);
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
 	if (on > 1)
@@ -5061,18 +5056,18 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	struct ixgbe_hw *hw;
 	uint32_t reg_value;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
 	/* only support VF's 0 to 63 */
-	if ((vf >= dev_info.max_vfs) || (vf > 63))
+	if ((vf >= pci_dev->max_vfs) || (vf > 63))
 		return -EINVAL;
 
 	if (on > 1)
@@ -5094,19 +5089,21 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 rte_pmd_ixgbe_set_vf_vlan_stripq(uint8_t port, uint16_t vf, uint8_t on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
+	struct ixgbe_hw *hw;
 	uint16_t queues_per_pool;
 	uint32_t q;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
+	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5122,8 +5119,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	 * first 124 queues 0-123 will be allocated to VF's and only
 	 * the last 4 queues 123-127 will be assigned to the PF.
 	 */
-
-	queues_per_pool = dev_info.vmdq_queue_num / dev_info.max_vmdq_pools;
+	if (hw->mac.type == ixgbe_mac_82598EB)
+		queues_per_pool = (uint16_t)hw->mac.max_rx_queues /
+				  ETH_16_POOLS;
+	else
+		queues_per_pool = (uint16_t)hw->mac.max_rx_queues /
+				  ETH_64_POOLS;
 
 	for (q = 0; q < queues_per_pool; q++)
 		(*dev->dev_ops->vlan_strip_queue_set)(dev,
@@ -5136,19 +5137,19 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 {
 	int val = 0;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 	struct ixgbe_hw *hw;
 	uint32_t vmolr;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5181,7 +5182,7 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 rte_pmd_ixgbe_set_vf_rx(uint8_t port, uint16_t vf, uint8_t on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 	uint32_t reg, addr;
 	uint32_t val;
 	const uint8_t bit1 = 0x1;
@@ -5190,12 +5191,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5231,7 +5232,7 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 rte_pmd_ixgbe_set_vf_tx(uint8_t port, uint16_t vf, uint8_t on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
+	struct rte_pci_device *pci_dev;
 	uint32_t reg, addr;
 	uint32_t val;
 	const uint8_t bit1 = 0x1;
@@ -5241,12 +5242,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (on > 1)
@@ -5282,7 +5283,6 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 			uint64_t vf_mask, uint8_t vlan_on)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	int ret = 0;
 	uint16_t vf_idx;
 	struct ixgbe_hw *hw;
@@ -5290,9 +5290,8 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
 	if ((vlan > ETHER_MAX_VLAN_ID) || (vf_mask == 0))
@@ -5318,7 +5317,6 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
 	uint16_t tx_rate, uint64_t q_msk)
 {
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	struct ixgbe_hw *hw;
 	struct ixgbe_vf_info *vfinfo;
 	struct rte_eth_link link;
@@ -5332,13 +5330,13 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
-	rte_eth_dev_info_get(port, &dev_info);
+	pci_dev = IXGBE_DEV_TO_PCI(dev);
 	rte_eth_link_get_nowait(port, &link);
 
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	if (vf >= dev_info.max_vfs)
+	if (vf >= pci_dev->max_vfs)
 		return -EINVAL;
 
 	if (tx_rate > link.link_speed)
@@ -5347,7 +5345,6 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
 	if (q_msk == 0)
 		return 0;
 
-	pci_dev = IXGBE_DEV_TO_PCI(dev);
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	vfinfo = *(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private));
 	nb_q_per_pool = RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool;
@@ -8227,16 +8224,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	/* Stop the data paths */
@@ -8311,16 +8307,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	/* Stop the data paths */
@@ -8376,16 +8371,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	ctrl = mac[0] | (mac[1] << 8) | (mac[2] << 16) | (mac[3] << 24);
@@ -8402,16 +8396,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	ctrl = mac[0] | (mac[1] << 8) | (mac[2] << 16) | (mac[3] << 24);
@@ -8430,16 +8423,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl, i;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	if (idx != 0 && idx != 1)
@@ -8487,16 +8479,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
 {
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	struct rte_eth_dev_info dev_info;
 	uint32_t ctrl, i;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-	rte_eth_dev_info_get(port, &dev_info);
-	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+	dev = &rte_eth_devices[port];
+
+	if (!is_device_supported(dev, &rte_ixgbe_pmd))
 		return -ENOTSUP;
 
-	dev = &rte_eth_devices[port];
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	if (idx != 0 && idx != 1)
-- 
1.9.3

Re: [dpdk-dev] [PATCH v3] net/ixgbe: clean up rte_eth_dev_info_get

[Ferruh Yigit]

On 2/7/2017 6:33 AM, Wenzhuo Lu wrote:
> It's not appropriate to call rte_eth_dev_info_get in PMD,
> as rte_eth_dev_info_get need to get info from PMD.
> Remove rte_eth_dev_info_get from PMD code and get the
> info directly.
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH v3] net/ixgbe: clean up rte_eth_dev_info_get

[Ferruh Yigit]

On 2/7/2017 2:08 PM, Ferruh Yigit wrote:
> On 2/7/2017 6:33 AM, Wenzhuo Lu wrote:
>> It's not appropriate to call rte_eth_dev_info_get in PMD,
>> as rte_eth_dev_info_get need to get info from PMD.
>> Remove rte_eth_dev_info_get from PMD code and get the
>> info directly.
>>
>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> 
> Applied to dpdk-next-net/master, thanks.

Hi Jingjing,

Similar work is also required for i40e.

Thanks,
ferruh

Re: [dpdk-dev] [PATCH v3] net/ixgbe: clean up rte_eth_dev_info_get

[Lu, Wenzhuo]

Hi Ferruh, Jingjing,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, February 7, 2017 10:10 PM
> To: dev@dpdk.org; Wu, Jingjing
> Cc: Lu, Wenzhuo; Iremonger, Bernard
> Subject: Re: [dpdk-dev] [PATCH v3] net/ixgbe: clean up rte_eth_dev_info_get
> 
> On 2/7/2017 2:08 PM, Ferruh Yigit wrote:
> > On 2/7/2017 6:33 AM, Wenzhuo Lu wrote:
> >> It's not appropriate to call rte_eth_dev_info_get in PMD, as
> >> rte_eth_dev_info_get need to get info from PMD.
> >> Remove rte_eth_dev_info_get from PMD code and get the info directly.
> >>
> >> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >
> > Applied to dpdk-next-net/master, thanks.
> 
> Hi Jingjing,
> 
> Similar work is also required for i40e.
Let me do it. I plan to change the i40e code after ixgbe patch is accepted, so I need not rework both the i40e and ixgbe patches at the same time :)

> 
> Thanks,
> ferruh
> 
>