From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-f193.google.com (mail-io0-f193.google.com [209.85.223.193]) by dpdk.org (Postfix) with ESMTP id 51792D14C for ; Fri, 20 Apr 2018 15:13:52 +0200 (CEST) Received: by mail-io0-f193.google.com with SMTP id d11-v6so2631836iof.11 for ; Fri, 20 Apr 2018 06:13:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qwilt-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Xk4ACviQWfLl8pamv6vKfJlWqpypGEhEUYi8gHtgwek=; b=dA/3Ws8rHM33W+BNpWGe1Y4eiyk4eiFqRjIOj8Zci0TKR0CDdSuQWZ4uSPjwM/8AWg iBJrPQ+uW5LKNZIPrdDbXwvDlfmpY5yEQUaFd3+wk3pMWsz1a5nWVadyWiJDnON4L82S zDqy0aHhWRbJ86AUw+bPp0sLlG5j6J2ZTjGbAl6ZyCjdA/07kCIevl9NjA7bjsyYamHT msS+jPiYQGZG/BqDkFKBHskGotjRMCtyYHwzCsidK4hIg1X1J8RhTx5gj1/KRlu+Livr hh7VSmj6tzwT2RjKnIibM+8HaLrd+cvsFdZDoc1SOivPIHxf/ana6q2t1hpWJaKGgknA aWgw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Xk4ACviQWfLl8pamv6vKfJlWqpypGEhEUYi8gHtgwek=; b=LzPb422ASz61Wii3tPJIJoagQssbDtehmBiTXOzCPZFwh+4XHUcV+7nmBYiWARTdQ5 dESGyRyTGUemHuickHcWG4exYz1frmytc3Cew04N3xBhQowhBysgwU8o1Bp4NrrMECuS Po56xFctwPtB8ZJxmXLo/lXYUWygL4D5T6sD/8SkYkcn6MwTWHfX7gNo0dxSreFG0E9n CIqNuaw5IQdwsVhFvP92ZywyrL7lolN51DvgeV7KGgUw8ojd+qrVPNyzmu48nbs63jum JNwREGXZHZVnHRKXTvFnPCUyuCC2t7zf1e93AsgT+UkjUU2XIo8/0IUagHam5p79XSY7 RsVQ== X-Gm-Message-State: ALQs6tD0PAIYMcDD0s4QL1asaXYlmzLxd34yYuUMYJulxOZ9Ld5gJNUI EDmg2aRTp0adocdqEwM2now6k+WzeZJJzOT0vREJnQ== X-Google-Smtp-Source: AIpwx4+0Sau2OhWApItCpSW9wZtcAnAIlsYq6m32TuK8kvPsJ1cHumItqTvtpkkk1RqBMi0kRkEkDqp6ES+A3GbFEkE= X-Received: by 2002:a6b:81a:: with SMTP id 26-v6mr10219109ioi.136.1524230031527; Fri, 20 Apr 2018 06:13:51 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.142.145 with HTTP; Fri, 20 Apr 2018 06:13:51 -0700 (PDT) In-Reply-To: References: <1524117669-25729-1-git-send-email-arnon@qwilt.com> <1524117669-25729-3-git-send-email-arnon@qwilt.com> From: Arnon Warshavsky Date: Fri, 20 Apr 2018 16:13:51 +0300 Message-ID: To: Kevin Traynor Cc: Thomas Monjalon , "Burakov, Anatoly" , "Lu, Wenzhuo" , "Doherty, Declan" , jerin.jacob@caviumnetworks.com, Bruce Richardson , "Yigit, Ferruh" , dev@dpdk.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v4 02/11] bond: replace rte_panic instances in bonding driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Apr 2018 13:13:52 -0000 Will do. Thanks On Thu, Apr 19, 2018 at 8:25 PM, Kevin Traynor wrote: > 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 > > > > -- *Arnon Warshavsky* *Qwilt | work: +972-72-2221634 | mobile: +972-50-8583058 | arnon@qwilt.com *