From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yh0-f53.google.com (mail-yh0-f53.google.com [209.85.213.53]) by dpdk.org (Postfix) with ESMTP id EB4F5683B for ; Tue, 2 Sep 2014 15:39:23 +0200 (CEST) Received: by mail-yh0-f53.google.com with SMTP id a41so4249849yho.12 for ; Tue, 02 Sep 2014 06:43:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=kI2jaxZJ5WNYJUZUrH+bgyxM9kEvOCss8yTS/wCECfs=; b=cvGVqsaGMPBwHuTpI0PE2lMt9J0PSL1fk++Ojn7g9zTNIU2uWyCoCyvDF73CGP8P6a abce6J0hD03GHoryOSGfhVx2DoLhvCQnyk0XaYIRAjhijkYxbTdCPht202avhnfzIufS DsR8uOJTVMAgtpl/K0/4w98bYyXMzT3+dxtZEXX690hZIUIUvgpOh6pxxtXavCLn5+yb tTYIDKpZIgBQx3uNv1ozaSDAySnAbm6sHY3WidXGnlSiTUxUSzW5j2H3t1q7EAv/tdke B2MCcqq7vpZKENo/k9rFO0yg1tq4EDwvFt1/XBTsw0JrvGm7MvdgIOf9+rMCWVUDAI94 7tiw== X-Gm-Message-State: ALoCoQnT5IYvrDzjTjwAZhlSkD1ovrhWK779pfNRXTc+l4+WGW0cxeexpeNHbhdtaXY/3mpPKHVl MIME-Version: 1.0 X-Received: by 10.236.117.37 with SMTP id i25mr25624655yhh.85.1409665435617; Tue, 02 Sep 2014 06:43:55 -0700 (PDT) Received: by 10.170.96.213 with HTTP; Tue, 2 Sep 2014 06:43:55 -0700 (PDT) In-Reply-To: <1409567080-27083-2-git-send-email-david.marchand@6wind.com> References: <1409567080-27083-1-git-send-email-david.marchand@6wind.com> <1409567080-27083-2-git-send-email-david.marchand@6wind.com> Date: Tue, 2 Sep 2014 08:43:55 -0500 Message-ID: From: Jay Rolette To: David Marchand Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2 01/17] ixgbe: use the right debug macro X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Sep 2014 13:39:24 -0000 Hi David, A couple of minor comments inline below. Looks good otherwise. Jay On Mon, Sep 1, 2014 at 5:24 AM, David Marchand wrote: > - We should not use DEBUGOUT*/DEBUGFUNC macros in non-shared code. > These macros come as compat wrappers for shared code. > - We should avoid calling RTE_LOG directly as pmd provides a wrapper for > logs. > > Signed-off-by: David Marchand > --- > lib/librte_pmd_ixgbe/ixgbe_82599_bypass.c | 14 ++++---- > lib/librte_pmd_ixgbe/ixgbe_bypass.c | 26 +++++++------- > lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 27 +++++++-------- > lib/librte_pmd_ixgbe/ixgbe_pf.c | 4 +-- > lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 53 > +++++++++++++++-------------- > 5 files changed, 63 insertions(+), 61 deletions(-) > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_82599_bypass.c > b/lib/librte_pmd_ixgbe/ixgbe_82599_bypass.c > index 0f0000c..2623419 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_82599_bypass.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_82599_bypass.c > @@ -63,7 +63,7 @@ ixgbe_set_fiber_fixed_speed(struct ixgbe_hw *hw, > ixgbe_link_speed speed) > rs = IXGBE_SFF_SOFT_RS_SELECT_1G; > break; > default: > - DEBUGOUT("Invalid fixed module speed\n"); > + PMD_DRV_LOG("Invalid fixed module speed"); > return; > } > > @@ -72,7 +72,7 @@ ixgbe_set_fiber_fixed_speed(struct ixgbe_hw *hw, > ixgbe_link_speed speed) > IXGBE_I2C_EEPROM_DEV_ADDR2, > &eeprom_data); > if (status) { > - DEBUGOUT("Failed to read Rx Rate Select RS0\n"); > + PMD_DRV_LOG("Failed to read Rx Rate Select RS0"); > goto out; > } > > @@ -82,7 +82,7 @@ ixgbe_set_fiber_fixed_speed(struct ixgbe_hw *hw, > ixgbe_link_speed speed) > IXGBE_I2C_EEPROM_DEV_ADDR2, > eeprom_data); > if (status) { > - DEBUGOUT("Failed to write Rx Rate Select RS0\n"); > + PMD_DRV_LOG("Failed to write Rx Rate Select RS0"); > goto out; > } > > @@ -91,7 +91,7 @@ ixgbe_set_fiber_fixed_speed(struct ixgbe_hw *hw, > ixgbe_link_speed speed) > IXGBE_I2C_EEPROM_DEV_ADDR2, > &eeprom_data); > if (status) { > - DEBUGOUT("Failed to read Rx Rate Select RS1\n"); > + PMD_DRV_LOG("Failed to read Rx Rate Select RS1"); > goto out; > } > > @@ -101,7 +101,7 @@ ixgbe_set_fiber_fixed_speed(struct ixgbe_hw *hw, > ixgbe_link_speed speed) > IXGBE_I2C_EEPROM_DEV_ADDR2, > eeprom_data); > if (status) { > - DEBUGOUT("Failed to write Rx Rate Select RS1\n"); > + PMD_DRV_LOG("Failed to write Rx Rate Select RS1"); > goto out; > } > out: > @@ -130,7 +130,7 @@ ixgbe_setup_mac_link_multispeed_fixed_fiber(struct > ixgbe_hw *hw, > bool link_up = false; > bool negotiation; > > - DEBUGFUNC(""); > + PMD_INIT_FUNC_TRACE(); > > /* Mask off requested but non-supported speeds */ > status = ixgbe_get_link_capabilities(hw, &link_speed, > &negotiation); > @@ -261,7 +261,7 @@ ixgbe_bypass_get_media_type(struct ixgbe_hw *hw) > { > enum ixgbe_media_type media_type; > > - DEBUGFUNC(""); > + PMD_INIT_FUNC_TRACE(); > > if (hw->device_id == IXGBE_DEV_ID_82599_BYPASS) { > media_type = ixgbe_media_type_fiber; > diff --git a/lib/librte_pmd_ixgbe/ixgbe_bypass.c > b/lib/librte_pmd_ixgbe/ixgbe_bypass.c > index 1d21dc0..1a980b8 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_bypass.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_bypass.c > @@ -40,20 +40,20 @@ > #define BYPASS_STATUS_OFF_MASK 3 > > /* Macros to check for invlaid function pointers. */ > -#define FUNC_PTR_OR_ERR_RET(func, retval) do { \ > - if ((func) == NULL) { \ > - DEBUGOUT("%s:%d function not supported\n", \ > - __func__, __LINE__); \ > - return (retval); \ > - } \ > +#define FUNC_PTR_OR_ERR_RET(func, retval) do { \ > + if ((func) == NULL) { \ > + PMD_DRV_LOG("%s:%d function not supported", \ > + __func__, __LINE__); \ > + return retval; \ > Need to keep the parens around retval in your macro > + } \ > } while(0) > > -#define FUNC_PTR_OR_RET(func) do { \ > - if ((func) == NULL) { \ > - DEBUGOUT("%s:%d function not supported\n", \ > - __func__, __LINE__); \ > - return; \ > - } \ > +#define FUNC_PTR_OR_RET(func) do { \ > + if ((func) == NULL) { \ > + PMD_DRV_LOG("%s:%d function not supported", \ > + __func__, __LINE__); \ > + return; \ > + } \ > } while(0) > > > @@ -114,7 +114,7 @@ ixgbe_bypass_init(struct rte_eth_dev *dev) > /* Only allow BYPASS ops on the first port */ > if (hw->device_id != IXGBE_DEV_ID_82599_BYPASS || > hw->bus.func != 0) { > - DEBUGOUT("bypass function is not supported on that > device\n"); > + PMD_DRV_LOG("bypass function is not supported on that > device"); > return; > } > > diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c > b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c > index 59122a1..a8a7ed6 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c > @@ -667,7 +667,7 @@ ixgbe_swfw_lock_reset(struct ixgbe_hw *hw) > */ > mask = IXGBE_GSSR_PHY0_SM << hw->bus.func; > if (ixgbe_acquire_swfw_semaphore(hw, mask) < 0) { > - DEBUGOUT1("SWFW phy%d lock released", hw->bus.func); > + PMD_DRV_LOG(DEBUG, "SWFW phy%d lock released", > hw->bus.func); > } > ixgbe_release_swfw_semaphore(hw, mask); > > @@ -679,7 +679,7 @@ ixgbe_swfw_lock_reset(struct ixgbe_hw *hw) > */ > mask = IXGBE_GSSR_EEP_SM | IXGBE_GSSR_MAC_CSR_SM | > IXGBE_GSSR_SW_MNG_SM; > if (ixgbe_acquire_swfw_semaphore(hw, mask) < 0) { > - DEBUGOUT("SWFW common locks released"); > + PMD_DRV_LOG(DEBUG, "SWFW common locks released"); > } > ixgbe_release_swfw_semaphore(hw, mask); > } > @@ -1012,16 +1012,15 @@ eth_ixgbevf_dev_init(__attribute__((unused)) > struct eth_driver *eth_drv, > eth_dev->data->mac_addrs = NULL; > return diag; > } > - RTE_LOG(INFO, PMD, > - "\tVF MAC address not assigned by Host PF\n" > - "\tAssign randomly generated MAC address " > - "%02x:%02x:%02x:%02x:%02x:%02x\n", > - perm_addr->addr_bytes[0], > - perm_addr->addr_bytes[1], > - perm_addr->addr_bytes[2], > - perm_addr->addr_bytes[3], > - perm_addr->addr_bytes[4], > - perm_addr->addr_bytes[5]); > + PMD_INIT_LOG(INFO, "\tVF MAC address not assigned by Host > PF"); > + PMD_INIT_LOG(INFO, "\tAssign randomly generated MAC > address " > + "%02x:%02x:%02x:%02x:%02x:%02x", > + perm_addr->addr_bytes[0], > + perm_addr->addr_bytes[1], > + perm_addr->addr_bytes[2], > + perm_addr->addr_bytes[3], > + perm_addr->addr_bytes[4], > + perm_addr->addr_bytes[5]); > } > > /* Copy the permanent MAC address */ > @@ -1090,7 +1089,7 @@ rte_ixgbe_pmd_init(const char *name __rte_unused, > const char *params __rte_unuse > static int > rte_ixgbevf_pmd_init(const char *name __rte_unused, const char *param > __rte_unused) > { > - DEBUGFUNC("rte_ixgbevf_pmd_init"); > + PMD_INIT_FUNC_TRACE(); > > rte_eth_driver_register(&rte_ixgbevf_pmd); > return (0); > @@ -2515,7 +2514,7 @@ ixgbe_dcb_pfc_enable_generic(struct ixgbe_hw > *hw,uint8_t tc_num) > fccfg_reg |= IXGBE_FCCFG_TFCE_PRIORITY; > break; > default: > - DEBUGOUT("Flow control param set incorrectly\n"); > + PMD_DRV_LOG(DEBUG, "Flow control param set incorrectly"); > ret_val = IXGBE_ERR_CONFIG; > goto out; > break; > diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c > b/lib/librte_pmd_ixgbe/ixgbe_pf.c > index 170944d..59fb58b 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_pf.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c > @@ -478,7 +478,7 @@ ixgbe_rcv_msg_from_vf(struct rte_eth_dev *dev, > uint16_t vf) > > retval = ixgbe_read_mbx(hw, msgbuf, mbx_size, vf); > if (retval) { > - RTE_LOG(ERR, PMD, "Error mbx recv msg from VF %d\n", vf); > + PMD_DRV_LOG(ERR, "Error mbx recv msg from VF %d", vf); > return retval; > } > > @@ -511,7 +511,7 @@ ixgbe_rcv_msg_from_vf(struct rte_eth_dev *dev, > uint16_t vf) > retval = ixgbe_vf_set_vlan(dev, vf, msgbuf); > break; > default: > - RTE_LOG(DEBUG, PMD, "Unhandled Msg %8.8x\n", (unsigned) > msgbuf[0]); > + PMD_DRV_LOG(DEBUG, "Unhandled Msg %8.8x", > (unsigned)msgbuf[0]); > retval = IXGBE_ERR_MBX; > break; > } > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > index dfc2076..46962bc 100644 > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c > @@ -1765,33 +1765,36 @@ ixgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, > tx_free_thresh = (uint16_t)((tx_conf->tx_free_thresh) ? > tx_conf->tx_free_thresh : DEFAULT_TX_FREE_THRESH); > if (tx_rs_thresh >= (nb_desc - 2)) { > - RTE_LOG(ERR, PMD, "tx_rs_thresh must be less than the > number " > - "of TX descriptors minus 2. (tx_rs_thresh=%u > port=%d " > - "queue=%d)\n", (unsigned int)tx_rs_thresh, > - (int)dev->data->port_id, (int)queue_idx); > + PMD_INIT_LOG(ERR, "tx_rs_thresh must be less than the > number " > + "of TX descriptors minus 2. (tx_rs_thresh=%u " > + "port=%d queue=%d)\n", (unsigned > int)tx_rs_thresh, > Embedded newline on this log message. I noticed you have \n still on all of the PMD_INIT_LOG() calls following. Intended? > + (int)dev->data->port_id, (int)queue_idx); > return -(EINVAL); > } > if (tx_free_thresh >= (nb_desc - 3)) { > - RTE_LOG(ERR, PMD, "tx_rs_thresh must be less than the " > - "tx_free_thresh must be less than the number of TX > " > - "descriptors minus 3. (tx_free_thresh=%u port=%d " > - "queue=%d)\n", (unsigned > int)tx_free_thresh, > - (int)dev->data->port_id, (int)queue_idx); > + PMD_INIT_LOG(ERR, "tx_rs_thresh must be less than the " > + "tx_free_thresh must be less than the number > of " > + "TX descriptors minus 3. (tx_free_thresh=%u " > + "port=%d queue=%d)\n", > Embedded newline > + (unsigned int)tx_free_thresh, > + (int)dev->data->port_id, (int)queue_idx); > return -(EINVAL); > } > if (tx_rs_thresh > tx_free_thresh) { > - RTE_LOG(ERR, PMD, "tx_rs_thresh must be less than or equal > to " > - "tx_free_thresh. (tx_free_thresh=%u > tx_rs_thresh=%u " > - "port=%d queue=%d)\n", (unsigned > int)tx_free_thresh, > - (unsigned int)tx_rs_thresh, > (int)dev->data->port_id, > - (int)queue_idx); > + PMD_INIT_LOG(ERR, "tx_rs_thresh must be less than or equal > to " > + "tx_free_thresh. (tx_free_thresh=%u " > + "tx_rs_thresh=%u port=%d queue=%d)\n", > Embedded newline > + (unsigned int)tx_free_thresh, > + (unsigned int)tx_rs_thresh, > + (int)dev->data->port_id, > + (int)queue_idx); > return -(EINVAL); > } > if ((nb_desc % tx_rs_thresh) != 0) { > - RTE_LOG(ERR, PMD, "tx_rs_thresh must be a divisor of the " > - "number of TX descriptors. (tx_rs_thresh=%u > port=%d " > - "queue=%d)\n", (unsigned int)tx_rs_thresh, > - (int)dev->data->port_id, (int)queue_idx); > + PMD_INIT_LOG(ERR, "tx_rs_thresh must be a divisor of the " > + "number of TX descriptors. (tx_rs_thresh=%u " > + "port=%d queue=%d)\n", (unsigned > int)tx_rs_thresh, > Embedded newline > + (int)dev->data->port_id, (int)queue_idx); > return -(EINVAL); > } > > @@ -1802,10 +1805,10 @@ ixgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, > * accumulates WTHRESH descriptors. > */ > if ((tx_rs_thresh > 1) && (tx_conf->tx_thresh.wthresh != 0)) { > - RTE_LOG(ERR, PMD, "TX WTHRESH must be set to 0 if " > - "tx_rs_thresh is greater than 1. (tx_rs_thresh=%u " > - "port=%d queue=%d)\n", (unsigned int)tx_rs_thresh, > - (int)dev->data->port_id, (int)queue_idx); > + PMD_INIT_LOG(ERR, "TX WTHRESH must be set to 0 if " > + "tx_rs_thresh is greater than 1. > (tx_rs_thresh=%u " > + "port=%d queue=%d)\n", (unsigned > int)tx_rs_thresh, > Embedded newline > + (int)dev->data->port_id, (int)queue_idx); > return -(EINVAL); > } > > @@ -3279,7 +3282,7 @@ ixgbe_dev_mq_rx_configure(struct rte_eth_dev *dev) > IXGBE_WRITE_REG(hw, IXGBE_MRQC, > IXGBE_MRQC_VMDQRT8TCEN); > break; > default: > - RTE_LOG(ERR, PMD, "invalid pool number in IOV > mode\n"); > + PMD_INIT_LOG(ERR, "invalid pool number in IOV > mode\n"); > Embedded newline > } > } > > @@ -3332,7 +3335,7 @@ ixgbe_dev_mq_tx_configure(struct rte_eth_dev *dev) > break; > default: > mtqc = IXGBE_MTQC_64Q_1PB; > - RTE_LOG(ERR, PMD, "invalid pool number in IOV > mode\n"); > + PMD_INIT_LOG(ERR, "invalid pool number in IOV > mode\n"); > Embedded newline > } > IXGBE_WRITE_REG(hw, IXGBE_MTQC, mtqc); > } > @@ -3595,7 +3598,7 @@ ixgbe_dev_tx_init(struct rte_eth_dev *dev) > static inline void > ixgbe_setup_loopback_link_82599(struct ixgbe_hw *hw) > { > - DEBUGFUNC("ixgbe_setup_loopback_link_82599"); > + PMD_INIT_FUNC_TRACE(); > > if (ixgbe_verify_lesm_fw_enabled_82599(hw)) { > if (hw->mac.ops.acquire_swfw_sync(hw, > IXGBE_GSSR_MAC_CSR_SM) != > -- > 1.7.10.4 > >