Re: [dpdk-dev] [PATCH v2] net/bonding: support bifurcated driver in eal cli using --vdev

[Declan Doherty <declan.doherty@intel.com>]

On 04/07/2017 12:57 PM, Gowrishankar wrote:
> From: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
>
> At present, creating bonding devices using --vdev is broken for PMD like
> mlx5 as it is neither UIO nor VFIO based and hence PMD driver is unknown
> to find_port_id_by_pci_addr(), as below.
>
> testpmd <EAL args> --vdev 'net_bonding0,mode=1,slave=<PCI>,socket_id=0'
>
> PMD: bond_ethdev_parse_slave_port_kvarg(150) - Invalid slave port value
>  (<PCI ID>) specified
> EAL: Failed to parse slave ports for bonded device net_bonding0
>
> This patch fixes parsing PCI ID from bonding device params by verifying
> it in RTE PCI bus, rather than checking dev->kdrv.
>
> Changes:
>   v2 - revisit fix by iterating rte_pci_bus
>
> Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
> ---
...
>

Hey Gowrishankar,

I was having a look at this patch and there is the following checkpatch 
error.

_coding style issues_


WARNING:AVOID_EXTERNS: externs should be avoided in .c files
#48: FILE: drivers/net/bonding/rte_eth_bond_args.c:43:
+extern struct rte_pci_bus rte_pci_bus;


Looking at bit closer at the issue I think there is a simpler solution, 
the bonding driver really shouldn't be parsing the PCI bus directly, and 
since PCI devices use the PCI DBF as their name we can simply replace 
the all the scanning code with a simple call to 
rte_eth_dev_get_port_by_name API.

Can you try the following changes which removes any dependency on 
parsing the PCI bus tree directly, which works fine for me with UIO 
devices. If this works I'll send this fix to the list.

Thanks
Declan


---
  drivers/net/bonding/rte_eth_bond_args.c | 109 
++++----------------------------
  1 file changed, 14 insertions(+), 95 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c 
b/drivers/net/bonding/rte_eth_bond_args.c
index ffab6e0..9fb98c6 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -40,7 +40,6 @@
  #include "rte_eth_bond.h"
  #include "rte_eth_bond_private.h"

-extern struct rte_pci_bus rte_pci_bus;

  const char *pmd_bond_init_valid_arguments[] = {
  	PMD_BOND_SLAVE_PORT_KVARG,
@@ -53,104 +52,22 @@ const char *pmd_bond_init_valid_arguments[] = {
  	NULL
  };

-static inline int
-find_port_id_by_pci_addr(const struct rte_pci_addr *pci_addr)
-{
-	struct rte_pci_device *pci_dev;
-	struct rte_pci_addr *eth_pci_addr;
-	unsigned i;
-
-	for (i = 0; i < rte_eth_dev_count(); i++) {
-		pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
-		eth_pci_addr = &pci_dev->addr;
-
-		if (pci_addr->bus == eth_pci_addr->bus &&
-			pci_addr->devid == eth_pci_addr->devid &&
-			pci_addr->domain == eth_pci_addr->domain &&
-			pci_addr->function == eth_pci_addr->function)
-			return i;
-	}
-	return -1;
-}
-
-static inline int
-find_port_id_by_dev_name(const char *name)
-{
-	unsigned i;
-
-	for (i = 0; i < rte_eth_dev_count(); i++) {
-		if (rte_eth_devices[i].data == NULL)
-			continue;
-
-		if (strcmp(rte_eth_devices[i].device->name, name) == 0)
-			return i;
-	}
-	return -1;
-}
-
-/**
- * Parses a port identifier string to a port id by pci address, then by 
name,
- * and finally port id.
- */
-static inline int
-parse_port_id(const char *port_str)
-{
-	struct rte_pci_device *dev;
-	struct rte_pci_addr dev_addr;
-	int port_id = -1;
-
-	/* try parsing as pci address, physical devices */
-	if (eal_parse_pci_DomBDF(port_str, &dev_addr) == 0) {
-		FOREACH_DEVICE_ON_PCIBUS(dev) {
-			if (rte_eal_compare_pci_addr(&dev->addr, &dev_addr))
-				continue;
-
-			port_id = find_port_id_by_pci_addr(&dev_addr);
-		}
-		if (port_id < 0)
-			return -1;
-	} else {
-		/* try parsing as device name, virtual devices */
-		port_id = find_port_id_by_dev_name(port_str);
-		if (port_id < 0) {
-			char *end;
-			errno = 0;
-
-			/* try parsing as port id */
-			port_id = strtol(port_str, &end, 10);
-			if (*end != 0 || errno != 0)
-				return -1;
-		}
-	}
-
-	if (port_id < 0 || port_id > RTE_MAX_ETHPORTS) {
-		RTE_BOND_LOG(ERR, "Slave port specified (%s) outside expected range",
-				port_str);
-		return -1;
-	}
-	return port_id;
-}
-
  int
-bond_ethdev_parse_slave_port_kvarg(const char *key,
+bond_ethdev_parse_slave_port_kvarg(const char *key __rte_unused,
  		const char *value, void *extra_args)
  {
-	struct bond_ethdev_slave_ports *slave_ports;
+	struct bond_ethdev_slave_ports *slaves;
+	uint8_t pid;

  	if (value == NULL || extra_args == NULL)
  		return -1;

-	slave_ports = extra_args;
+	if (rte_eth_dev_get_port_by_name(value, &pid) < 0)
+		return -1;
+
+	slaves = extra_args;
+	slaves->slaves[slaves->slave_count++] = pid;

-	if (strcmp(key, PMD_BOND_SLAVE_PORT_KVARG) == 0) {
-		int port_id = parse_port_id(value);
-		if (port_id < 0) {
-			RTE_BOND_LOG(ERR, "Invalid slave port value (%s) specified", value);
-			return -1;
-		} else
-			slave_ports->slaves[slave_ports->slave_count++] =
-					(uint8_t)port_id;
-	}
  	return 0;
  }

@@ -214,20 +131,22 @@ int
  bond_ethdev_parse_primary_slave_port_id_kvarg(const char *key 
__rte_unused,
  		const char *value, void *extra_args)
  {
-	int primary_slave_port_id;
+	uint8_t pid, *primary_pid;

  	if (value == NULL || extra_args == NULL)
  		return -1;

-	primary_slave_port_id = parse_port_id(value);
-	if (primary_slave_port_id < 0)
+	if (rte_eth_dev_get_port_by_name(value, &pid) < 0)
  		return -1;

-	*(uint8_t *)extra_args = (uint8_t)primary_slave_port_id;
+	primary_pid = (uint8_t *)extra_args;
+	*primary_pid = pid;

  	return 0;
  }

+
+
  int
  bond_ethdev_parse_balance_xmit_policy_kvarg(const char *key __rte_unused,
  		const char *value, void *extra_args)
-- 
2.9.4