[PATCH 0/2] Trivial bonding patches

[PATCH 0/2] Trivial bonding patches

[Stephen Hemminger]

From: Stephen Hemminger <shemming@brocade.com>

These are a couple things found while doing code inspection for
some other bonding related issues.

Stephen Hemminger (2):
  ethdev: make rte_eth_dev_is_valid_port public
  bonding: fix name and port validation

 drivers/net/bonding/rte_eth_bond_api.c     | 47 ++++++------------------------
 drivers/net/bonding/rte_eth_bond_pmd.c     |  1 -
 drivers/net/bonding/rte_eth_bond_private.h |  4 +--
 lib/librte_ether/rte_ethdev.c              |  2 +-
 lib/librte_ether/rte_ethdev.h              | 11 +++++++
 lib/librte_ether/rte_ether_version.map     |  1 +
 6 files changed, 24 insertions(+), 42 deletions(-)

-- 
2.1.4

[PATCH 1/2] ethdev: make rte_eth_dev_is_valid_port public

[Stephen Hemminger]

From: Stephen Hemminger <shemming@brocade.com>

The function rte_eth_dev_is_valid_port is good way to have all
drivers using same function and solves several hotplug related
bugs from drivers not checking attached flag.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_ether/rte_ethdev.c          |  2 +-
 lib/librte_ether/rte_ethdev.h          | 11 +++++++++++
 lib/librte_ether/rte_ether_version.map |  1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 5a94654..ae4d86d 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -401,7 +401,7 @@ rte_eth_driver_register(struct eth_driver *eth_drv)
 	rte_eal_pci_register(&eth_drv->pci_drv);
 }
 
-static int
+int
 rte_eth_dev_is_valid_port(uint8_t port_id)
 {
 	if (port_id >= RTE_MAX_ETHPORTS ||
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 16dbe00..b640e5b 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1853,6 +1853,17 @@ extern int rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
 extern int rte_eth_dev_socket_id(uint8_t port_id);
 
 /*
+ * Check if port_id of device is attached
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device
+ * @return
+ *   0 if port is out of range or not attached
+ *   1 if device is attached
+ */
+extern int rte_eth_dev_is_valid_port(uint8_t port_id);
+	
+/*
  * Allocate mbuf from mempool, setup the DMA physical address
  * and then start RX for specified queue of a port. It is used
  * when rx_deferred_start flag of the specified queue is true.
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index a2d25a6..8aa3501 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -63,6 +63,7 @@ DPDK_2.0 {
 	rte_eth_dev_set_vlan_offload;
 	rte_eth_dev_set_vlan_pvid;
 	rte_eth_dev_set_vlan_strip_on_queue;
+	rte_eth_dev_is_valid_port;
 	rte_eth_dev_socket_id;
 	rte_eth_dev_start;
 	rte_eth_dev_stop;
-- 
2.1.4

[PATCH 2/2] bonding: fix name and port validation

[Stephen Hemminger]

From: Stephen Hemminger <shemming@brocade.com>

Cleanup the code in bonding that checks ports.
  * Use standard rte_eth_dev_is_valid_port
  * Change name of driver string to avoid variable namespace conflicts
  * Get rid of unnecessary string comparison stuff. A simple pointer
    check is enough here.
  * Get rid of unnecessary assignment of driver_name, it is already
    done by common code.
  * Don't generate unnecessary log messages on error.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/bonding/rte_eth_bond_api.c     | 47 ++++++------------------------
 drivers/net/bonding/rte_eth_bond_pmd.c     |  1 -
 drivers/net/bonding/rte_eth_bond_private.h |  4 +--
 3 files changed, 11 insertions(+), 41 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index d4caa83..6d8597a 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -45,59 +45,30 @@
 #define DEFAULT_POLLING_INTERVAL_10_MS (10)
 
 int
-valid_bonded_ethdev(struct rte_eth_dev *eth_dev)
+valid_bonded_ethdev(const struct rte_eth_dev *eth_dev)
 {
-	size_t len;
-
 	/* Check valid pointer */
-	if (eth_dev->driver->pci_drv.name == NULL || driver_name == NULL)
-		return -1;
-
-	/* Check string lengths are equal */
-	len = strlen(driver_name);
-	if (strlen(eth_dev->driver->pci_drv.name) != len)
-		return -1;
-
-	/* Compare strings */
-	return strncmp(eth_dev->driver->pci_drv.name, driver_name, len);
-}
-
-int
-valid_port_id(uint8_t port_id)
-{
-	/* Verify that port id is valid */
-	int ethdev_count = rte_eth_dev_count();
-	if (port_id >= ethdev_count) {
-		RTE_BOND_LOG(ERR, "Port Id %d is greater than rte_eth_dev_count %d",
-				port_id, ethdev_count);
+	if (eth_dev->driver->pci_drv.name == NULL)
 		return -1;
-	}
 
-	return 0;
+	/* return 0 if driver name matches */
+	return eth_dev->driver->pci_drv.name != pmd_bond_driver_name;
 }
 
 int
 valid_bonded_port_id(uint8_t port_id)
 {
-	/* Verify that port id's are valid */
-	if (valid_port_id(port_id))
+	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
-	/* Verify that bonded_port_id refers to a bonded port */
-	if (valid_bonded_ethdev(&rte_eth_devices[port_id])) {
-		RTE_BOND_LOG(ERR, "Specified port Id %d is not a bonded eth_dev device",
-				port_id);
-		return -1;
-	}
-
-	return 0;
+	return valid_bonded_ethdev(&rte_eth_devices[port_id]);
 }
 
 int
 valid_slave_port_id(uint8_t port_id)
 {
 	/* Verify that port id's are valid */
-	if (valid_port_id(port_id))
+	if (!rte_eth_dev_is_valid_port(port_id))
 		return -1;
 
 	/* Verify that port_id refers to a non bonded port */
@@ -192,7 +163,7 @@ number_of_sockets(void)
 	return ++sockets;
 }
 
-const char *driver_name = "Link Bonding PMD";
+const char *pmd_bond_driver_name = "Link Bonding PMD";
 
 int
 rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
@@ -259,7 +230,7 @@ rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id)
 	}
 
 	pci_dev->numa_node = socket_id;
-	pci_drv->name = driver_name;
+	pci_drv->name = pmd_bond_driver_name;
 
 	eth_dev->driver = eth_drv;
 	eth_dev->data->dev_private = internals;
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 8bad2e1..034e60b 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1556,7 +1556,6 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 {
 	struct bond_dev_private *internals = dev->data->dev_private;
 
-	dev_info->driver_name = driver_name;
 	dev_info->max_mac_addrs = 1;
 
 	dev_info->max_rx_pktlen = (uint32_t)2048;
diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h
index 45e5c65..c531e87 100644
--- a/drivers/net/bonding/rte_eth_bond_private.h
+++ b/drivers/net/bonding/rte_eth_bond_private.h
@@ -62,7 +62,7 @@
 
 extern const char *pmd_bond_init_valid_arguments[];
 
-extern const char *driver_name;
+extern const char *pmd_bond_driver_name;
 
 /** Port Queue Mapping Structure */
 struct bond_rx_queue {
@@ -162,7 +162,7 @@ struct bond_dev_private {
 extern struct eth_dev_ops default_dev_ops;
 
 int
-valid_bonded_ethdev(struct rte_eth_dev *eth_dev);
+valid_bonded_ethdev(const struct rte_eth_dev *eth_dev);
 
 /* Search given slave array to find possition of given id.
  * Return slave pos or slaves_count if not found. */
-- 
2.1.4

Re: [PATCH 1/2] ethdev: make rte_eth_dev_is_valid_port public

[Bruce Richardson]

On Wed, Jun 10, 2015 at 03:06:24PM -0700, Stephen Hemminger wrote:
> From: Stephen Hemminger <shemming@brocade.com>
> 
> The function rte_eth_dev_is_valid_port is good way to have all
> drivers using same function and solves several hotplug related
> bugs from drivers not checking attached flag.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Just yesterday I was working on a patch to do the same thing! 

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH 2/2] bonding: fix name and port validation

[Declan Doherty]

On 10/06/15 23:06, Stephen Hemminger wrote:
> From: Stephen Hemminger <shemming@brocade.com>
>
> Cleanup the code in bonding that checks ports.
>    * Use standard rte_eth_dev_is_valid_port
>    * Change name of driver string to avoid variable namespace conflicts
>    * Get rid of unnecessary string comparison stuff. A simple pointer
>      check is enough here.
>    * Get rid of unnecessary assignment of driver_name, it is already
>      done by common code.
>    * Don't generate unnecessary log messages on error.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Declan Doherty <declan.doherty@intel.com>

Re: [PATCH 0/2] Trivial bonding patches

[Thomas Monjalon]

> These are a couple things found while doing code inspection for
> some other bonding related issues.
> 
> Stephen Hemminger (2):
>   ethdev: make rte_eth_dev_is_valid_port public
>   bonding: fix name and port validation

Applied, thanks