On 2/3/2017 6:50 AM, Lu, Wenzhuo wrote:

Hi Ferruh,

quoted

-----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:

quoted

On Wed, Jan 25, 2017 at 01:13:32PM +0800, Lu, Wenzhuo wrote:

quoted

Hi Tiwei,

quoted

-----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:

quoted

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 <redacted>
---
 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,

quoted

 	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.

quoted

quoted

Yes, it’s weird. But what makes it weird is not the missing comparison but

the function name.

quoted

quoted

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-

quoted

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.

quoted

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.

quoted

Best regards,
Tiwei Bie