From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Traynor Subject: Re: [PATCH v4 02/11] bond: replace rte_panic instances in bonding driver Date: Thu, 19 Apr 2018 18:25:27 +0100 Message-ID: References: <1524117669-25729-1-git-send-email-arnon@qwilt.com> <1524117669-25729-3-git-send-email-arnon@qwilt.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org To: Arnon Warshavsky , thomas@monjalon.net, anatoly.burakov@intel.com, wenzhuo.lu@intel.com, declan.doherty@intel.com, jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com, ferruh.yigit@intel.com Return-path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 03710AABA for ; Thu, 19 Apr 2018 19:25:31 +0200 (CEST) In-Reply-To: <1524117669-25729-3-git-send-email-arnon@qwilt.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 04/19/2018 07:01 AM, Arnon Warshavsky wrote: > replace panic calls with log and retrun value. > Local functions to this file, > changing from void to int are non-abi-breaking > -- > v4 - fix split literal strings in log messages > > Signed-off-by: Arnon Warshavsky > --- > drivers/net/bonding/rte_eth_bond_8023ad.c | 28 +++++++++++++++-------- > drivers/net/bonding/rte_eth_bond_8023ad_private.h | 2 +- > drivers/net/bonding/rte_eth_bond_api.c | 20 +++++++++++----- > drivers/net/bonding/rte_eth_bond_pmd.c | 9 +++++--- > drivers/net/bonding/rte_eth_bond_private.h | 2 +- > 5 files changed, 40 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c > index c452318..7512901 100644 > --- a/drivers/net/bonding/rte_eth_bond_8023ad.c > +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c > @@ -893,7 +893,7 @@ > bond_mode_8023ad_periodic_cb, arg); > } > > -void > +int > bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev, > uint16_t slave_id) > { > @@ -939,7 +939,7 @@ > timer_cancel(&port->warning_timer); > > if (port->mbuf_pool != NULL) > - return; > + return 0; > > RTE_ASSERT(port->rx_ring == NULL); > RTE_ASSERT(port->tx_ring == NULL); > @@ -968,8 +968,9 @@ > /* Any memory allocation failure in initialization is critical because > * resources can't be free, so reinitialization is impossible. */ > if (port->mbuf_pool == NULL) { > - rte_panic("Slave %u: Failed to create memory pool '%s': %s\n", > - slave_id, mem_name, rte_strerror(rte_errno)); > + RTE_LOG(ERR, PMD, "%s() Slave %u: Failed to create memory pool '%s': %s\n", > + __func__, slave_id, mem_name, rte_strerror(rte_errno)); > + return -1; > } > > snprintf(mem_name, RTE_DIM(mem_name), "slave_%u_rx", slave_id); > @@ -977,8 +978,9 @@ > rte_align32pow2(BOND_MODE_8023AX_SLAVE_RX_PKTS), socket_id, 0); > > if (port->rx_ring == NULL) { > - rte_panic("Slave %u: Failed to create rx ring '%s': %s\n", slave_id, > - mem_name, rte_strerror(rte_errno)); > + RTE_LOG(ERR, PMD, "%s() Slave %u: Failed to create rx ring '%s': %s\n", > + __func__, slave_id, mem_name, rte_strerror(rte_errno)); > + return -1; > } > > /* TX ring is at least one pkt longer to make room for marker packet. */ > @@ -987,9 +989,12 @@ > rte_align32pow2(BOND_MODE_8023AX_SLAVE_TX_PKTS + 1), socket_id, 0); > > if (port->tx_ring == NULL) { > - rte_panic("Slave %u: Failed to create tx ring '%s': %s\n", slave_id, > - mem_name, rte_strerror(rte_errno)); > + RTE_LOG(ERR, PMD, "%s() Slave %u: Fail to create tx ring '%s': %s\n", > + __func__, slave_id, mem_name, rte_strerror(rte_errno)); > + return -1; > } > + > + return 0; > } > > int > @@ -1143,9 +1148,12 @@ > struct bond_dev_private *internals = bond_dev->data->dev_private; > uint8_t i; > > - for (i = 0; i < internals->active_slave_count; i++) > - bond_mode_8023ad_activate_slave(bond_dev, > + for (i = 0; i < internals->active_slave_count; i++) { > + int rc = bond_mode_8023ad_activate_slave(bond_dev, > internals->active_slaves[i]); > + if (rc != 0) > + return rc; > + } > > return 0; > } > diff --git a/drivers/net/bonding/rte_eth_bond_8023ad_private.h b/drivers/net/bonding/rte_eth_bond_8023ad_private.h > index 0f490a5..96a42f2 100644 > --- a/drivers/net/bonding/rte_eth_bond_8023ad_private.h > +++ b/drivers/net/bonding/rte_eth_bond_8023ad_private.h > @@ -263,7 +263,7 @@ struct mode8023ad_private { > * @return > * 0 on success, negative value otherwise. > */ > -void > +int > bond_mode_8023ad_activate_slave(struct rte_eth_dev *dev, uint16_t port_id); > > /** > diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c > index aa89425..96aa1ff 100644 > --- a/drivers/net/bonding/rte_eth_bond_api.c > +++ b/drivers/net/bonding/rte_eth_bond_api.c > @@ -69,14 +69,15 @@ > return 0; > } > > -void > +int > activate_slave(struct rte_eth_dev *eth_dev, uint16_t port_id) > { > struct bond_dev_private *internals = eth_dev->data->dev_private; > uint8_t active_count = internals->active_slave_count; > > if (internals->mode == BONDING_MODE_8023AD) > - bond_mode_8023ad_activate_slave(eth_dev, port_id); > + if (bond_mode_8023ad_activate_slave(eth_dev, port_id) != 0) > + return -1; > > if (internals->mode == BONDING_MODE_TLB > || internals->mode == BONDING_MODE_ALB) { > @@ -357,10 +358,17 @@ > bond_ethdev_primary_set(internals, > slave_port_id); > > - if (find_slave_by_id(internals->active_slaves, > - internals->active_slave_count, > - slave_port_id) == internals->active_slave_count) > - activate_slave(bonded_eth_dev, slave_port_id); > + int rc = There's no need for the rc variables, the existing check would suffice here > + find_slave_by_id(internals->active_slaves, > + internals->active_slave_count, > + slave_port_id); > + > + if (rc == internals->active_slave_count) { > + int rc = activate_slave(bonded_eth_dev, > + slave_port_id); > + if (rc != 0) > + return -1; and this could be if (activate_slave(bonded_eth_dev, slave_port_id)) return -1; > + } > } > } > > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c > index 2805c71..2d9052d 100644 > --- a/drivers/net/bonding/rte_eth_bond_pmd.c > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c > @@ -1741,8 +1741,10 @@ struct bwg_slave { > /* Any memory allocation failure in initialization is critical because > * resources can't be free, so reinitialization is impossible. */ > if (port->slow_pool == NULL) { > - rte_panic("Slave %u: Failed to create memory pool '%s': %s\n", > - slave_id, mem_name, rte_strerror(rte_errno)); > + RTE_LOG(ERR, PMD, "%s() Slave %u: Failed to create memory pool '%s': %s\n", > + __func__, slave_id, > + mem_name, rte_strerror(rte_errno)); > + return -1; > } > } > > @@ -2673,7 +2675,8 @@ struct bwg_slave { > mac_address_slaves_update(bonded_eth_dev); > } > > - activate_slave(bonded_eth_dev, port_id); > + if (activate_slave(bonded_eth_dev, port_id) != 0) > + return -1; it's more consistent with the rest of the function to do, if(activate_slave(bonded_eth_dev, port_id)) return rc; There's other places through the patches where "!= 0" is used but not really needed > > /* If user has defined the primary port then default to using it */ > if (internals->user_defined_primary_port && > diff --git a/drivers/net/bonding/rte_eth_bond_private.h b/drivers/net/bonding/rte_eth_bond_private.h > index 94eca88..d99d42c 100644 > --- a/drivers/net/bonding/rte_eth_bond_private.h > +++ b/drivers/net/bonding/rte_eth_bond_private.h > @@ -187,7 +187,7 @@ struct bond_dev_private { > void > deactivate_slave(struct rte_eth_dev *eth_dev, uint16_t port_id); > > -void > +int > activate_slave(struct rte_eth_dev *eth_dev, uint16_t port_id); > > void >