[dpdk-dev,01/11] ixgbe: clean log messages

Message ID 1409062162-19575-2-git-send-email-david.marchand@6wind.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

David Marchand Aug. 26, 2014, 2:09 p.m. UTC
  Clean log messages:
- remove superfluous \n in log macros and add some \n where needed,
- remove leading \n in some messages,
- split multi lines messages,
- replace some PMD_INIT_LOG(DEBUG, "some_func\n") with PMD_INIT_FUNC_TRACE().

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c |   99 +++++++++++++++++------------------
 lib/librte_pmd_ixgbe/ixgbe_fdir.c   |   12 ++---
 lib/librte_pmd_ixgbe/ixgbe_logs.h   |   12 ++---
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c   |   14 ++---
 4 files changed, 68 insertions(+), 69 deletions(-)
  

Comments

Jay Rolette Aug. 26, 2014, 2:23 p.m. UTC | #1
Why are you adding newlines to log message strings? Shouldn't that be up to
whatever the messages end up getting routed to?

Jay


On Tue, Aug 26, 2014 at 9:09 AM, David Marchand <david.marchand@6wind.com>
wrote:

> Clean log messages:
> - remove superfluous \n in log macros and add some \n where needed,
> - remove leading \n in some messages,
> - split multi lines messages,
> - replace some PMD_INIT_LOG(DEBUG, "some_func\n") with
> PMD_INIT_FUNC_TRACE().
>
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> ---
>  lib/librte_pmd_ixgbe/ixgbe_ethdev.c |   99
> +++++++++++++++++------------------
>  lib/librte_pmd_ixgbe/ixgbe_fdir.c   |   12 ++---
>  lib/librte_pmd_ixgbe/ixgbe_logs.h   |   12 ++---
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c   |   14 ++---
>  4 files changed, 68 insertions(+), 69 deletions(-)
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> index 59122a1..ac00323 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> @@ -737,7 +737,7 @@ eth_ixgbe_dev_init(__attribute__((unused)) struct
> eth_driver *eth_drv,
>  #endif /* RTE_NIC_BYPASS */
>
>         if (diag != IXGBE_SUCCESS) {
> -               PMD_INIT_LOG(ERR, "Shared code init failed: %d", diag);
> +               PMD_INIT_LOG(ERR, "Shared code init failed: %d\n", diag);
>                 return -EIO;
>         }
>
> @@ -763,7 +763,7 @@ eth_ixgbe_dev_init(__attribute__((unused)) struct
> eth_driver *eth_drv,
>         /* Make sure we have a good EEPROM before we read from it */
>         diag = ixgbe_validate_eeprom_checksum(hw, &csum);
>         if (diag != IXGBE_SUCCESS) {
> -               PMD_INIT_LOG(ERR, "The EEPROM checksum is not valid: %d",
> diag);
> +               PMD_INIT_LOG(ERR, "The EEPROM checksum is not valid:
> %d\n", diag);
>                 return -EIO;
>         }
>
> @@ -791,13 +791,14 @@ eth_ixgbe_dev_init(__attribute__((unused)) struct
> eth_driver *eth_drv,
>         if (diag == IXGBE_ERR_EEPROM_VERSION) {
>                 PMD_INIT_LOG(ERR, "This device is a pre-production
> adapter/"
>                     "LOM.  Please be aware there may be issues associated "
> -                   "with your hardware.\n If you are experiencing
> problems "
> +                   "with your hardware.\n");
> +               PMD_INIT_LOG(ERR, "If you are experiencing problems "
>                     "please contact your Intel or hardware representative "
>                     "who provided you with this hardware.\n");
>         } else if (diag == IXGBE_ERR_SFP_NOT_SUPPORTED)
>                 PMD_INIT_LOG(ERR, "Unsupported SFP+ Module\n");
>         if (diag) {
> -               PMD_INIT_LOG(ERR, "Hardware Initialization Failure: %d",
> diag);
> +               PMD_INIT_LOG(ERR, "Hardware Initialization Failure: %d\n",
> diag);
>                 return -EIO;
>         }
>
> @@ -813,7 +814,7 @@ eth_ixgbe_dev_init(__attribute__((unused)) struct
> eth_driver *eth_drv,
>         if (eth_dev->data->mac_addrs == NULL) {
>                 PMD_INIT_LOG(ERR,
>                         "Failed to allocate %u bytes needed to store "
> -                       "MAC addresses",
> +                       "MAC addresses\n",
>                         ETHER_ADDR_LEN * hw->mac.num_rar_entries);
>                 return -ENOMEM;
>         }
> @@ -826,7 +827,8 @@ eth_ixgbe_dev_init(__attribute__((unused)) struct
> eth_driver *eth_drv,
>                         IXGBE_VMDQ_NUM_UC_MAC, 0);
>         if (eth_dev->data->hash_mac_addrs == NULL) {
>                 PMD_INIT_LOG(ERR,
> -                       "Failed to allocate %d bytes needed to store MAC
> addresses",
> +                       "Failed to allocate %d bytes needed to store "
> +                       "MAC addresses\n",
>                         ETHER_ADDR_LEN * IXGBE_VMDQ_NUM_UC_MAC);
>                 return -ENOMEM;
>         }
> @@ -850,14 +852,14 @@ eth_ixgbe_dev_init(__attribute__((unused)) struct
> eth_driver *eth_drv,
>
>         if (ixgbe_is_sfp(hw) && hw->phy.sfp_type !=
> ixgbe_sfp_type_not_present)
>                 PMD_INIT_LOG(DEBUG,
> -                            "MAC: %d, PHY: %d, SFP+: %d<n",
> +                            "MAC: %d, PHY: %d, SFP+: %d\n",
>                              (int) hw->mac.type, (int) hw->phy.type,
>                              (int) hw->phy.sfp_type);
>         else
>                 PMD_INIT_LOG(DEBUG, "MAC: %d, PHY: %d\n",
>                              (int) hw->mac.type, (int) hw->phy.type);
>
> -       PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x",
> +       PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x\n",
>                         eth_dev->data->port_id, pci_dev->id.vendor_id,
>                         pci_dev->id.device_id);
>
> @@ -933,7 +935,7 @@ eth_ixgbevf_dev_init(__attribute__((unused)) struct
> eth_driver *eth_drv,
>
> IXGBE_DEV_PRIVATE_TO_HWSTRIP_BITMAP(eth_dev->data->dev_private);
>         struct ether_addr *perm_addr = (struct ether_addr *)
> hw->mac.perm_addr;
>
> -       PMD_INIT_LOG(DEBUG, "eth_ixgbevf_dev_init");
> +       PMD_INIT_FUNC_TRACE();
>
>         eth_dev->dev_ops = &ixgbevf_eth_dev_ops;
>         eth_dev->rx_pkt_burst = &ixgbe_recv_pkts;
> @@ -963,7 +965,8 @@ eth_ixgbevf_dev_init(__attribute__((unused)) struct
> eth_driver *eth_drv,
>         /* Initialize the shared code (base driver) */
>         diag = ixgbe_init_shared_code(hw);
>         if (diag != IXGBE_SUCCESS) {
> -               PMD_INIT_LOG(ERR, "Shared code init failed for ixgbevf:
> %d", diag);
> +               PMD_INIT_LOG(ERR, "Shared code init failed for ixgbevf:
> %d\n",
> +                            diag);
>                 return -EIO;
>         }
>
> @@ -982,7 +985,7 @@ eth_ixgbevf_dev_init(__attribute__((unused)) struct
> eth_driver *eth_drv,
>          * In this case, assign a random MAC address.
>          */
>         if ((diag != IXGBE_SUCCESS) && (diag !=
> IXGBE_ERR_INVALID_MAC_ADDR)) {
> -               PMD_INIT_LOG(ERR, "VF Initialization Failure: %d", diag);
> +               PMD_INIT_LOG(ERR, "VF Initialization Failure: %d\n", diag);
>                 return (diag);
>         }
>
> @@ -998,7 +1001,7 @@ eth_ixgbevf_dev_init(__attribute__((unused)) struct
> eth_driver *eth_drv,
>         if (eth_dev->data->mac_addrs == NULL) {
>                 PMD_INIT_LOG(ERR,
>                         "Failed to allocate %u bytes needed to store "
> -                       "MAC addresses",
> +                       "MAC addresses\n",
>                         ETHER_ADDR_LEN * hw->mac.num_rar_entries);
>                 return -ENOMEM;
>         }
> @@ -1034,11 +1037,12 @@ eth_ixgbevf_dev_init(__attribute__((unused))
> struct eth_driver *eth_drv,
>                         break;
>
>                 default:
> -                       PMD_INIT_LOG(ERR, "VF Initialization Failure: %d",
> diag);
> +                       PMD_INIT_LOG(ERR, "VF Initialization Failure:
> %d\n",
> +                                    diag);
>                         return (-EIO);
>         }
>
> -       PMD_INIT_LOG(DEBUG, "\nport %d vendorID=0x%x deviceID=0x%x
> mac.type=%s\n",
> +       PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x
> mac.type=%s\n",
>                          eth_dev->data->port_id, pci_dev->id.vendor_id,
> pci_dev->id.device_id,
>                          "ixgbe_mac_82599_vf");
>
> @@ -1207,7 +1211,7 @@ ixgbe_vlan_hw_strip_disable(struct rte_eth_dev *dev,
> uint16_t queue)
>
>         if (hw->mac.type == ixgbe_mac_82598EB) {
>                 /* No queue level support */
> -               PMD_INIT_LOG(INFO, "82598EB not support queue level hw
> strip");
> +               PMD_INIT_LOG(INFO, "82598EB not support queue level hw
> strip\n");
>                 return;
>         }
>         else {
> @@ -1231,7 +1235,7 @@ ixgbe_vlan_hw_strip_enable(struct rte_eth_dev *dev,
> uint16_t queue)
>
>         if (hw->mac.type == ixgbe_mac_82598EB) {
>                 /* No queue level supported */
> -               PMD_INIT_LOG(INFO, "82598EB not support queue level hw
> strip");
> +               PMD_INIT_LOG(INFO, "82598EB not support queue level hw
> strip\n");
>                 return;
>         }
>         else {
> @@ -1543,7 +1547,7 @@ skip_link_setup:
>         return (0);
>
>  error:
> -       PMD_INIT_LOG(ERR, "failure in ixgbe_dev_start(): %d", err);
> +       PMD_INIT_LOG(ERR, "failure in ixgbe_dev_start(): %d\n", err);
>         ixgbe_dev_clear_queues(dev);
>         return -EIO;
>  }
> @@ -1599,10 +1603,8 @@ ixgbe_dev_set_link_up(struct rte_eth_dev *dev)
>  #ifdef RTE_NIC_BYPASS
>                 if (hw->device_id == IXGBE_DEV_ID_82599_BYPASS) {
>                         /* Not suported in bypass mode */
> -                       PMD_INIT_LOG(ERR,
> -                               "\nSet link up is not supported "
> -                               "by device id 0x%x\n",
> -                               hw->device_id);
> +                       PMD_INIT_LOG(ERR, "Set link up is not supported "
> +                                    "by device id 0x%x\n", hw->device_id);
>                         return -ENOTSUP;
>                 }
>  #endif
> @@ -1611,8 +1613,8 @@ ixgbe_dev_set_link_up(struct rte_eth_dev *dev)
>                 return 0;
>         }
>
> -       PMD_INIT_LOG(ERR, "\nSet link up is not supported by device id
> 0x%x\n",
> -               hw->device_id);
> +       PMD_INIT_LOG(ERR, "Set link up is not supported by device id
> 0x%x\n",
> +                    hw->device_id);
>         return -ENOTSUP;
>  }
>
> @@ -1628,10 +1630,8 @@ ixgbe_dev_set_link_down(struct rte_eth_dev *dev)
>  #ifdef RTE_NIC_BYPASS
>                 if (hw->device_id == IXGBE_DEV_ID_82599_BYPASS) {
>                         /* Not suported in bypass mode */
> -                       PMD_INIT_LOG(ERR,
> -                               "\nSet link down is not supported "
> -                               "by device id 0x%x\n",
> -                                hw->device_id);
> +                       PMD_INIT_LOG(ERR, "Set link down is not supported "
> +                                    "by device id 0x%x\n", hw->device_id);
>                         return -ENOTSUP;
>                 }
>  #endif
> @@ -1640,9 +1640,8 @@ ixgbe_dev_set_link_down(struct rte_eth_dev *dev)
>                 return 0;
>         }
>
> -       PMD_INIT_LOG(ERR,
> -               "\nSet link down is not supported by device id 0x%x\n",
> -                hw->device_id);
> +       PMD_INIT_LOG(ERR, "Set link down is not supported by device id
> 0x%x\n",
> +                    hw->device_id);
>         return -ENOTSUP;
>  }
>
> @@ -2113,7 +2112,7 @@ ixgbe_dev_interrupt_get_status(struct rte_eth_dev
> *dev)
>
>         /* read-on-clear nic registers here */
>         eicr = IXGBE_READ_REG(hw, IXGBE_EICR);
> -       PMD_DRV_LOG(INFO, "eicr %x", eicr);
> +       PMD_DRV_LOG(INFO, "eicr %x\n", eicr);
>
>         intr->flags = 0;
>         if (eicr & IXGBE_EICR_LSC) {
> @@ -2145,16 +2144,16 @@ ixgbe_dev_link_status_print(struct rte_eth_dev
> *dev)
>         memset(&link, 0, sizeof(link));
>         rte_ixgbe_dev_atomic_read_link_status(dev, &link);
>         if (link.link_status) {
> -               PMD_INIT_LOG(INFO, "Port %d: Link Up - speed %u Mbps - %s",
> +               PMD_INIT_LOG(INFO, "Port %d: Link Up - speed %u Mbps -
> %s\n",
>                                         (int)(dev->data->port_id),
>                                         (unsigned)link.link_speed,
>                         link.link_duplex == ETH_LINK_FULL_DUPLEX ?
>                                         "full-duplex" : "half-duplex");
>         } else {
> -               PMD_INIT_LOG(INFO, " Port %d: Link Down",
> +               PMD_INIT_LOG(INFO, " Port %d: Link Down\n",
>                                 (int)(dev->data->port_id));
>         }
> -       PMD_INIT_LOG(INFO, "PCI Address: %04d:%02d:%02d:%d",
> +       PMD_INIT_LOG(INFO, "PCI Address: %04d:%02d:%02d:%d\n",
>                                 dev->pci_dev->addr.domain,
>                                 dev->pci_dev->addr.bus,
>                                 dev->pci_dev->addr.devid,
> @@ -2211,9 +2210,9 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev)
>         if (intr_enable_delay) {
>                 if (rte_eal_alarm_set(timeout * 1000,
>                                       ixgbe_dev_interrupt_delayed_handler,
> (void*)dev) < 0)
> -                       PMD_DRV_LOG(ERR, "Error setting alarm");
> +                       PMD_DRV_LOG(ERR, "Error setting alarm\n");
>         } else {
> -               PMD_DRV_LOG(DEBUG, "enable intr immediately");
> +               PMD_DRV_LOG(DEBUG, "enable intr immediately\n");
>                 ixgbe_enable_intr(dev);
>                 rte_intr_enable(&(dev->pci_dev->intr_handle));
>         }
> @@ -2371,7 +2370,7 @@ ixgbe_flow_ctrl_set(struct rte_eth_dev *dev, struct
> rte_eth_fc_conf *fc_conf)
>         if (fc_conf->autoneg != !hw->fc.disable_fc_autoneg)
>                 return -ENOTSUP;
>         rx_buf_size = IXGBE_READ_REG(hw, IXGBE_RXPBSIZE(0));
> -       PMD_INIT_LOG(DEBUG, "Rx packet buffer size = 0x%x \n",
> rx_buf_size);
> +       PMD_INIT_LOG(DEBUG, "Rx packet buffer size = 0x%x\n", rx_buf_size);
>
>         /*
>          * At least reserve one Ethernet frame for watermark
> @@ -2413,7 +2412,7 @@ ixgbe_flow_ctrl_set(struct rte_eth_dev *dev, struct
> rte_eth_fc_conf *fc_conf)
>                 return 0;
>         }
>
> -       PMD_INIT_LOG(ERR, "ixgbe_fc_enable = 0x%x \n", err);
> +       PMD_INIT_LOG(ERR, "ixgbe_fc_enable = 0x%x\n", err);
>         return -EIO;
>  }
>
> @@ -2593,7 +2592,7 @@ ixgbe_priority_flow_ctrl_set(struct rte_eth_dev
> *dev, struct rte_eth_pfc_conf *p
>         ixgbe_dcb_unpack_map_cee(dcb_config, IXGBE_DCB_RX_CONFIG, map);
>         tc_num = map[pfc_conf->priority];
>         rx_buf_size = IXGBE_READ_REG(hw, IXGBE_RXPBSIZE(tc_num));
> -       PMD_INIT_LOG(DEBUG, "Rx packet buffer size = 0x%x \n",
> rx_buf_size);
> +       PMD_INIT_LOG(DEBUG, "Rx packet buffer size = 0x%x\n", rx_buf_size);
>         /*
>          * At least reserve one Ethernet frame for watermark
>          * high_water/low_water in kilo bytes for ixgbe
> @@ -2618,7 +2617,7 @@ ixgbe_priority_flow_ctrl_set(struct rte_eth_dev
> *dev, struct rte_eth_pfc_conf *p
>         if ((err == IXGBE_SUCCESS) || (err == IXGBE_ERR_FC_NOT_NEGOTIATED))
>                 return 0;
>
> -       PMD_INIT_LOG(ERR, "ixgbe_dcb_pfc_enable = 0x%x \n", err);
> +       PMD_INIT_LOG(ERR, "ixgbe_dcb_pfc_enable = 0x%x\n", err);
>         return -EIO;
>  }
>
> @@ -2765,7 +2764,7 @@ ixgbe_dev_mtu_set(struct rte_eth_dev *dev, uint16_t
> mtu)
>  static void
>  ixgbevf_intr_disable(struct ixgbe_hw *hw)
>  {
> -       PMD_INIT_LOG(DEBUG, "ixgbevf_intr_disable");
> +       PMD_INIT_FUNC_TRACE();
>
>         /* Clear interrupt mask to stop from interrupts being generated */
>         IXGBE_WRITE_REG(hw, IXGBE_VTEIMC, IXGBE_VF_IRQ_CLEAR_MASK);
> @@ -2778,8 +2777,8 @@ ixgbevf_dev_configure(struct rte_eth_dev *dev)
>  {
>         struct rte_eth_conf* conf = &dev->data->dev_conf;
>
> -       PMD_INIT_LOG(DEBUG, "\nConfigured Virtual Function port id: %d\n",
> -               dev->data->port_id);
> +       PMD_INIT_LOG(DEBUG, "Configured Virtual Function port id: %d\n",
> +                    dev->data->port_id);
>
>         /*
>          * VF has no ability to enable/disable HW CRC
> @@ -2807,7 +2806,7 @@ ixgbevf_dev_start(struct rte_eth_dev *dev)
>                 IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>         int err, mask = 0;
>
> -       PMD_INIT_LOG(DEBUG, "ixgbevf_dev_start");
> +       PMD_INIT_FUNC_TRACE();
>
>         hw->mac.ops.reset_hw(hw);
>
> @@ -2842,7 +2841,7 @@ ixgbevf_dev_stop(struct rte_eth_dev *dev)
>  {
>         struct ixgbe_hw *hw =
> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>
> -       PMD_INIT_LOG(DEBUG, "ixgbevf_dev_stop");
> +       PMD_INIT_FUNC_TRACE();
>
>         hw->adapter_stopped = TRUE;
>         ixgbe_stop_adapter(hw);
> @@ -2861,7 +2860,7 @@ ixgbevf_dev_close(struct rte_eth_dev *dev)
>  {
>         struct ixgbe_hw *hw =
> IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>
> -       PMD_INIT_LOG(DEBUG, "ixgbevf_dev_close");
> +       PMD_INIT_FUNC_TRACE();
>
>         ixgbe_reset_hw(hw);
>
> @@ -2908,7 +2907,7 @@ ixgbevf_vlan_filter_set(struct rte_eth_dev *dev,
> uint16_t vlan_id, int on)
>         /* vind is not used in VF driver, set to 0, check
> ixgbe_set_vfta_vf */
>         ret = ixgbe_set_vfta(hw, vlan_id, 0, !!on);
>         if(ret){
> -               PMD_INIT_LOG(ERR, "Unable to set VF vlan");
> +               PMD_INIT_LOG(ERR, "Unable to set VF vlan\n");
>                 return ret;
>         }
>         vid_idx = (uint32_t) ((vlan_id >> 5) & 0x7F);
> @@ -3477,7 +3476,7 @@ ixgbevf_add_mac_addr(struct rte_eth_dev *dev, struct
> ether_addr *mac_addr,
>         diag = ixgbevf_set_uc_addr_vf(hw, 2, mac_addr->addr_bytes);
>         if (diag == 0)
>                 return;
> -       PMD_DRV_LOG(ERR, "Unable to add MAC address - diag=%d", diag);
> +       PMD_DRV_LOG(ERR, "Unable to add MAC address - diag=%d\n", diag);
>  }
>
>  static void
> @@ -3517,7 +3516,7 @@ ixgbevf_remove_mac_addr(struct rte_eth_dev *dev,
> uint32_t index)
>                         PMD_DRV_LOG(ERR,
>                                     "Adding again MAC address "
>                                     "%02x:%02x:%02x:%02x:%02x:%02x failed "
> -                                   "diag=%d",
> +                                   "diag=%d\n",
>                                     mac_addr->addr_bytes[0],
>                                     mac_addr->addr_bytes[1],
>                                     mac_addr->addr_bytes[2],
> @@ -3806,7 +3805,7 @@ ixgbe_add_5tuple_filter(struct rte_eth_dev *dev,
> uint16_t index,
>                 return -EINVAL;  /* filter index is out of range. */
>
>         if (filter->tcp_flags) {
> -               PMD_INIT_LOG(INFO, "82599EB not tcp flags in 5tuple");
> +               PMD_INIT_LOG(INFO, "82599EB not tcp flags in 5tuple\n");
>                 return -EINVAL;
>         }
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_fdir.c
> b/lib/librte_pmd_ixgbe/ixgbe_fdir.c
> index 6c0a530..c78a450 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_fdir.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_fdir.c
> @@ -139,7 +139,7 @@ configure_fdir_flags(struct rte_fdir_conf *conf,
> uint32_t *fdirctrl)
>                 break;
>         default:
>                 /* bad value */
> -               PMD_INIT_LOG(ERR, "Invalid fdir_conf->pballoc value");
> +               PMD_INIT_LOG(ERR, "Invalid fdir_conf->pballoc value\n");
>                 return -EINVAL;
>         };
>
> @@ -158,7 +158,7 @@ configure_fdir_flags(struct rte_fdir_conf *conf,
> uint32_t *fdirctrl)
>                 break;
>         default:
>                 /* bad value */
> -               PMD_INIT_LOG(ERR, "Invalid fdir_conf->status value");
> +               PMD_INIT_LOG(ERR, "Invalid fdir_conf->status value\n");
>                 return -EINVAL;
>         };
>
> @@ -395,7 +395,7 @@ fdir_filter_to_atr_input(struct rte_fdir_filter
> *fdir_filter,
>         if ((fdir_filter->l4type == RTE_FDIR_L4TYPE_SCTP ||
>                         fdir_filter->l4type == RTE_FDIR_L4TYPE_NONE) &&
>                         (fdir_filter->port_src || fdir_filter->port_dst)) {
> -               PMD_INIT_LOG(ERR, "Invalid fdir_filter");
> +               PMD_INIT_LOG(ERR, "Invalid fdir_filter\n");
>                 return -EINVAL;
>         }
>
> @@ -420,7 +420,7 @@ fdir_filter_to_atr_input(struct rte_fdir_filter
> *fdir_filter,
>                 input->formatted.flow_type = IXGBE_ATR_FLOW_TYPE_IPV4;
>                 break;
>         default:
> -               PMD_INIT_LOG(ERR, " Error on l4type input");
> +               PMD_INIT_LOG(ERR, " Error on l4type input\n");
>                 return -EINVAL;
>         }
>
> @@ -524,7 +524,7 @@ fdir_erase_filter_82599(struct ixgbe_hw *hw,
>         }
>
>         if (!retry_count) {
> -               PMD_INIT_LOG(ERR, "Timeout querying for flow director
> filter");
> +               PMD_INIT_LOG(ERR, "Timeout querying for flow director
> filter\n");
>                 err = -EIO;
>         }
>
> @@ -678,7 +678,7 @@ ixgbe_fdir_set_masks(struct rte_eth_dev *dev, struct
> rte_fdir_masks *fdir_masks)
>
>         err = ixgbe_reinit_fdir_tables_82599(hw);
>         if (err) {
> -               PMD_INIT_LOG(ERR, "reinit of fdir tables failed");
> +               PMD_INIT_LOG(ERR, "reinit of fdir tables failed\n");
>                 return -EIO;
>         }
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_logs.h
> b/lib/librte_pmd_ixgbe/ixgbe_logs.h
> index 9f0a684..22d8cfb 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_logs.h
> +++ b/lib/librte_pmd_ixgbe/ixgbe_logs.h
> @@ -36,8 +36,8 @@
>
>  #ifdef RTE_LIBRTE_IXGBE_DEBUG_INIT
>  #define PMD_INIT_LOG(level, fmt, args...) \
> -       RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
> -#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
> +       RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
> +#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>\n")
>  #else
>  #define PMD_INIT_LOG(level, fmt, args...) do { } while(0)
>  #define PMD_INIT_FUNC_TRACE() do { } while(0)
> @@ -45,28 +45,28 @@
>
>  #ifdef RTE_LIBRTE_IXGBE_DEBUG_RX
>  #define PMD_RX_LOG(level, fmt, args...) \
> -       RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
> +       RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
>  #else
>  #define PMD_RX_LOG(level, fmt, args...) do { } while(0)
>  #endif
>
>  #ifdef RTE_LIBRTE_IXGBE_DEBUG_TX
>  #define PMD_TX_LOG(level, fmt, args...) \
> -       RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
> +       RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
>  #else
>  #define PMD_TX_LOG(level, fmt, args...) do { } while(0)
>  #endif
>
>  #ifdef RTE_LIBRTE_IXGBE_DEBUG_TX_FREE
>  #define PMD_TX_FREE_LOG(level, fmt, args...) \
> -       RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
> +       RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
>  #else
>  #define PMD_TX_FREE_LOG(level, fmt, args...) do { } while(0)
>  #endif
>
>  #ifdef RTE_LIBRTE_IXGBE_DEBUG_DRIVER
>  #define PMD_DRV_LOG(level, fmt, args...) \
> -       RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
> +       RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
>  #else
>  #define PMD_DRV_LOG(level, fmt, args...) do { } while(0)
>  #endif
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index dfc2076..cbec821 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -492,7 +492,7 @@ ixgbe_xmit_cleanup(struct igb_tx_queue *txq)
>         {
>                 PMD_TX_FREE_LOG(DEBUG,
>                                 "TX descriptor %4u is not done"
> -                               "(port=%d queue=%d)",
> +                               "(port=%d queue=%d)\n",
>                                 desc_to_clean_to,
>                                 txq->port_id, txq->queue_id);
>                 /* Failed to clean any descriptors, better luck next time
> */
> @@ -509,7 +509,7 @@ ixgbe_xmit_cleanup(struct igb_tx_queue *txq)
>
>         PMD_TX_FREE_LOG(DEBUG,
>                         "Cleaning %4u TX descriptors: %4u to %4u "
> -                       "(port=%d queue=%d)",
> +                       "(port=%d queue=%d)\n",
>                         nb_tx_to_clean, last_desc_cleaned,
> desc_to_clean_to,
>                         txq->port_id, txq->queue_id);
>
> @@ -630,7 +630,7 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts,
>                         PMD_TX_FREE_LOG(DEBUG,
>                                         "Not enough free TX descriptors "
>                                         "nb_used=%4u nb_free=%4u "
> -                                       "(port=%d queue=%d)",
> +                                       "(port=%d queue=%d)\n",
>                                         nb_used, txq->nb_tx_free,
>                                         txq->port_id, txq->queue_id);
>
> @@ -650,7 +650,7 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts,
>                                         "performance."
>                                         "nb_used=%4u nb_free=%4u "
>                                         "tx_rs_thresh=%4u. "
> -                                       "(port=%d queue=%d)",
> +                                       "(port=%d queue=%d)\n",
>                                         nb_used, txq->nb_tx_free,
>                                         txq->tx_rs_thresh,
>                                         txq->port_id, txq->queue_id);
> @@ -782,7 +782,7 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts,
>                 if (txq->nb_tx_used >= txq->tx_rs_thresh) {
>                         PMD_TX_FREE_LOG(DEBUG,
>                                         "Setting RS bit on TXD id="
> -                                       "%4u (port=%d queue=%d)",
> +                                       "%4u (port=%d queue=%d)\n",
>                                         tx_last, txq->port_id,
> txq->queue_id);
>
>                         cmd_type_len |= IXGBE_TXD_CMD_RS;
> @@ -798,7 +798,7 @@ end_of_tx:
>         /*
>          * Set the Transmit Descriptor Tail (TDT)
>          */
> -       PMD_TX_LOG(DEBUG, "port_id=%u queue_id=%u tx_tail=%u nb_tx=%u",
> +       PMD_TX_LOG(DEBUG, "port_id=%u queue_id=%u tx_tail=%u nb_tx=%u\n",
>                    (unsigned) txq->port_id, (unsigned) txq->queue_id,
>                    (unsigned) tx_id, (unsigned) nb_tx);
>         IXGBE_PCI_REG_WRITE(txq->tdt_reg_addr, tx_id);
> @@ -1383,7 +1383,7 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct
> rte_mbuf **rx_pkts,
>                  * to happen by sending specific "back-pressure" flow
> control
>                  * frames to its peer(s).
>                  */
> -               PMD_RX_LOG(DEBUG, "\nport_id=%u queue_id=%u rx_id=%u "
> +               PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_id=%u "
>                            "staterr=0x%x data_len=%u\n",
>                            (unsigned) rxq->port_id, (unsigned)
> rxq->queue_id,
>                            (unsigned) rx_id, (unsigned) staterr,
> --
> 1.7.10.4
>
>
  
David Marchand Aug. 26, 2014, 2:55 p.m. UTC | #2
Hello Jay,

On Tue, Aug 26, 2014 at 4:23 PM, Jay Rolette <rolette@infiniteio.com> wrote:

> Why are you adding newlines to log message strings? Shouldn't that be up
> to whatever the messages end up getting routed to?
>

Actually, I wanted to have consistent log formats in the PMDs so that the
log messages displayed at runtime are aligned (and not bouncing with \n
before or after the log message).
There was two solutions from my point of view :
- either always add a \n in the log macro and remove all trailing \n
- do the opposite

I preferred the latter as it let users of the macro set the message format
as they want.


Before this change :

Configuring Port 0 (socket 1)
PMD: ixgbe_dev_tx_queue_setup(): sw_ring=0x7f3e59cd8080
hw_ring=0x7f3e59d02080 dma_addr=0x727702080

PMD: ixgbe_dev_tx_queue_setup(): Using simple tx code path

PMD: ixgbe_dev_tx_queue_setup(): Vector tx enabled.

PMD: ixgbe_dev_rx_queue_setup(): sw_ring=0x7f3e59cd57c0
hw_ring=0x7f3e59d12080 dma_addr=0x727712080

PMD: ixgbe_dev_rx_queue_setup(): Rx Burst Bulk Alloc Preconditions are
satisfied. Rx Burst Bulk Alloc function will be used on port=0, queue=0.

PMD: ixgbe_dev_rx_queue_setup(): Vector rx enabled, please make sure RX
burst size no less than 32.


After this change :

Configuring Port 0 (socket 1)
PMD: ixgbe_dev_tx_queue_setup(): sw_ring=0x7fd47ecd8080
hw_ring=0x7fd47ed02080 dma_addr=0x727702080
PMD: ixgbe_dev_tx_queue_setup(): Using simple tx code path
PMD: ixgbe_dev_tx_queue_setup(): Vector tx enabled.
PMD: ixgbe_dev_rx_queue_setup(): sw_ring=0x7fd47ecd57c0
hw_ring=0x7fd47ed12080 dma_addr=0x727712080
PMD: ixgbe_dev_rx_queue_setup(): Rx Burst Bulk Alloc Preconditions are
satisfied. Rx Burst Bulk Alloc function will be used on port=0, queue=0.
PMD: ixgbe_dev_rx_queue_setup(): Vector rx enabled, please make sure RX
burst size no less than 32.
Port 0: 90:E2:BA:29:DF:58
  
Jay Rolette Aug. 27, 2014, 1:53 p.m. UTC | #3
Hi David,

The updated output is definitely an improvement, but if you'll go with the
first solution you described (adding \n in the log macro), it works much
better for products using DPDK. For developer use, I think it ends up being
a wash either way.

At least for my product (embedded network appliance), we want to capture
anything that is logging in syslog. The log macros in DPDK make that very
reasonable to do, but if there are embedded newlines, it ends up screwing
up log parsing when someone wants to process logs later.

We end up having to remove all of those newlines, which makes it harder to
merge as new releases coming out.

I'm assuming most products have similar requirements for logging. That's at
least been the case for the products I've been involved with over the years.

If the PMDs are using PMD_LOG as a replacement macro for debugging
printf's, I can see where this might be a little more pain, but having
PMD_LOG is a lot more useful it if easily integrates with central logging
facilities.

Thanks,
Jay


On Tue, Aug 26, 2014 at 9:55 AM, David Marchand <david.marchand@6wind.com>
wrote:

> Hello Jay,
>
> On Tue, Aug 26, 2014 at 4:23 PM, Jay Rolette <rolette@infiniteio.com>
> wrote:
>
>> Why are you adding newlines to log message strings? Shouldn't that be up
>> to whatever the messages end up getting routed to?
>>
>
> Actually, I wanted to have consistent log formats in the PMDs so that the
> log messages displayed at runtime are aligned (and not bouncing with \n
> before or after the log message).
> There was two solutions from my point of view :
> - either always add a \n in the log macro and remove all trailing \n
> - do the opposite
>
> I preferred the latter as it let users of the macro set the message format
> as they want.
>
>
> Before this change :
>
> Configuring Port 0 (socket 1)
> PMD: ixgbe_dev_tx_queue_setup(): sw_ring=0x7f3e59cd8080
> hw_ring=0x7f3e59d02080 dma_addr=0x727702080
>
> PMD: ixgbe_dev_tx_queue_setup(): Using simple tx code path
>
> PMD: ixgbe_dev_tx_queue_setup(): Vector tx enabled.
>
> PMD: ixgbe_dev_rx_queue_setup(): sw_ring=0x7f3e59cd57c0
> hw_ring=0x7f3e59d12080 dma_addr=0x727712080
>
> PMD: ixgbe_dev_rx_queue_setup(): Rx Burst Bulk Alloc Preconditions are
> satisfied. Rx Burst Bulk Alloc function will be used on port=0, queue=0.
>
> PMD: ixgbe_dev_rx_queue_setup(): Vector rx enabled, please make sure RX
> burst size no less than 32.
>
>
> After this change :
>
> Configuring Port 0 (socket 1)
> PMD: ixgbe_dev_tx_queue_setup(): sw_ring=0x7fd47ecd8080
> hw_ring=0x7fd47ed02080 dma_addr=0x727702080
> PMD: ixgbe_dev_tx_queue_setup(): Using simple tx code path
> PMD: ixgbe_dev_tx_queue_setup(): Vector tx enabled.
> PMD: ixgbe_dev_rx_queue_setup(): sw_ring=0x7fd47ecd57c0
> hw_ring=0x7fd47ed12080 dma_addr=0x727712080
> PMD: ixgbe_dev_rx_queue_setup(): Rx Burst Bulk Alloc Preconditions are
> satisfied. Rx Burst Bulk Alloc function will be used on port=0, queue=0.
> PMD: ixgbe_dev_rx_queue_setup(): Vector rx enabled, please make sure RX
> burst size no less than 32.
> Port 0: 90:E2:BA:29:DF:58
>
>
> --
> David Marchand
>
  
Stephen Hemminger Aug. 27, 2014, 6:06 p.m. UTC | #4
On Wed, 27 Aug 2014 08:53:46 -0500
Jay Rolette <rolette@infiniteio.com> wrote:

> The updated output is definitely an improvement, but if you'll go with the
> first solution you described (adding \n in the log macro), it works much
> better for products using DPDK. For developer use, I think it ends up being
> a wash either way.

Also for driver consistency, all drivers must do the same thing. I.e the PMD_INIT_LOG
macro should either add or not add newline in the same way in all drivers.
I fixed virtio and vmxnet3 by just taking extra newlines out of messages
and leaving the macro alone.
  
David Marchand Aug. 29, 2014, 7:45 a.m. UTC | #5
On Wed, Aug 27, 2014 at 8:06 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Wed, 27 Aug 2014 08:53:46 -0500
> Jay Rolette <rolette@infiniteio.com> wrote:
>
> > The updated output is definitely an improvement, but if you'll go with
> the
> > first solution you described (adding \n in the log macro), it works much
> > better for products using DPDK. For developer use, I think it ends up
> being
> > a wash either way.
>
> Also for driver consistency, all drivers must do the same thing. I.e the
> PMD_INIT_LOG
> macro should either add or not add newline in the same way in all drivers.
> I fixed virtio and vmxnet3 by just taking extra newlines out of messages
> and leaving the macro alone.
>

Not sure that we can talk about consistency at the moment ... (all the more
so as if you only fixed two pmds).
Anyway.


If we go with this approach, there is still something that won't work.
ixgbe shared code uses DEBUGOUT* macros which are called a lot in this
shared code.
The macros call sites use trailing \n.
I suppose I will have a big NO from Intel guys if I remove the trailing \n,
so I suppose we can use an alternate macro that calls RTE_LOG rather than
PMD_DRV_LOG in ixgbe_os_dep.h.

I will also remove any reference to DEBUGOUT* in non-shared code.
>From my point of view, these macro are only here for compat with shared
code.


By the way, did you look at the other patches of this series ?

I want to send a v2 later (maybe not today), so any comment on the other
patches are welcome.
  

Patch

diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
index 59122a1..ac00323 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
@@ -737,7 +737,7 @@  eth_ixgbe_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 #endif /* RTE_NIC_BYPASS */
 
 	if (diag != IXGBE_SUCCESS) {
-		PMD_INIT_LOG(ERR, "Shared code init failed: %d", diag);
+		PMD_INIT_LOG(ERR, "Shared code init failed: %d\n", diag);
 		return -EIO;
 	}
 
@@ -763,7 +763,7 @@  eth_ixgbe_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 	/* Make sure we have a good EEPROM before we read from it */
 	diag = ixgbe_validate_eeprom_checksum(hw, &csum);
 	if (diag != IXGBE_SUCCESS) {
-		PMD_INIT_LOG(ERR, "The EEPROM checksum is not valid: %d", diag);
+		PMD_INIT_LOG(ERR, "The EEPROM checksum is not valid: %d\n", diag);
 		return -EIO;
 	}
 
@@ -791,13 +791,14 @@  eth_ixgbe_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 	if (diag == IXGBE_ERR_EEPROM_VERSION) {
 		PMD_INIT_LOG(ERR, "This device is a pre-production adapter/"
 		    "LOM.  Please be aware there may be issues associated "
-		    "with your hardware.\n If you are experiencing problems "
+		    "with your hardware.\n");
+		PMD_INIT_LOG(ERR, "If you are experiencing problems "
 		    "please contact your Intel or hardware representative "
 		    "who provided you with this hardware.\n");
 	} else if (diag == IXGBE_ERR_SFP_NOT_SUPPORTED)
 		PMD_INIT_LOG(ERR, "Unsupported SFP+ Module\n");
 	if (diag) {
-		PMD_INIT_LOG(ERR, "Hardware Initialization Failure: %d", diag);
+		PMD_INIT_LOG(ERR, "Hardware Initialization Failure: %d\n", diag);
 		return -EIO;
 	}
 
@@ -813,7 +814,7 @@  eth_ixgbe_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 	if (eth_dev->data->mac_addrs == NULL) {
 		PMD_INIT_LOG(ERR,
 			"Failed to allocate %u bytes needed to store "
-			"MAC addresses",
+			"MAC addresses\n",
 			ETHER_ADDR_LEN * hw->mac.num_rar_entries);
 		return -ENOMEM;
 	}
@@ -826,7 +827,8 @@  eth_ixgbe_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 			IXGBE_VMDQ_NUM_UC_MAC, 0);
 	if (eth_dev->data->hash_mac_addrs == NULL) {
 		PMD_INIT_LOG(ERR,
-			"Failed to allocate %d bytes needed to store MAC addresses",
+			"Failed to allocate %d bytes needed to store "
+			"MAC addresses\n",
 			ETHER_ADDR_LEN * IXGBE_VMDQ_NUM_UC_MAC);
 		return -ENOMEM;
 	}
@@ -850,14 +852,14 @@  eth_ixgbe_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 
 	if (ixgbe_is_sfp(hw) && hw->phy.sfp_type != ixgbe_sfp_type_not_present)
 		PMD_INIT_LOG(DEBUG,
-			     "MAC: %d, PHY: %d, SFP+: %d<n",
+			     "MAC: %d, PHY: %d, SFP+: %d\n",
 			     (int) hw->mac.type, (int) hw->phy.type,
 			     (int) hw->phy.sfp_type);
 	else
 		PMD_INIT_LOG(DEBUG, "MAC: %d, PHY: %d\n",
 			     (int) hw->mac.type, (int) hw->phy.type);
 
-	PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x",
+	PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x\n",
 			eth_dev->data->port_id, pci_dev->id.vendor_id,
 			pci_dev->id.device_id);
 
@@ -933,7 +935,7 @@  eth_ixgbevf_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 		IXGBE_DEV_PRIVATE_TO_HWSTRIP_BITMAP(eth_dev->data->dev_private);
 	struct ether_addr *perm_addr = (struct ether_addr *) hw->mac.perm_addr;
 
-	PMD_INIT_LOG(DEBUG, "eth_ixgbevf_dev_init");
+	PMD_INIT_FUNC_TRACE();
 
 	eth_dev->dev_ops = &ixgbevf_eth_dev_ops;
 	eth_dev->rx_pkt_burst = &ixgbe_recv_pkts;
@@ -963,7 +965,8 @@  eth_ixgbevf_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 	/* Initialize the shared code (base driver) */
 	diag = ixgbe_init_shared_code(hw);
 	if (diag != IXGBE_SUCCESS) {
-		PMD_INIT_LOG(ERR, "Shared code init failed for ixgbevf: %d", diag);
+		PMD_INIT_LOG(ERR, "Shared code init failed for ixgbevf: %d\n",
+			     diag);
 		return -EIO;
 	}
 
@@ -982,7 +985,7 @@  eth_ixgbevf_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 	 * In this case, assign a random MAC address.
 	 */
 	if ((diag != IXGBE_SUCCESS) && (diag != IXGBE_ERR_INVALID_MAC_ADDR)) {
-		PMD_INIT_LOG(ERR, "VF Initialization Failure: %d", diag);
+		PMD_INIT_LOG(ERR, "VF Initialization Failure: %d\n", diag);
 		return (diag);
 	}
 
@@ -998,7 +1001,7 @@  eth_ixgbevf_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 	if (eth_dev->data->mac_addrs == NULL) {
 		PMD_INIT_LOG(ERR,
 			"Failed to allocate %u bytes needed to store "
-			"MAC addresses",
+			"MAC addresses\n",
 			ETHER_ADDR_LEN * hw->mac.num_rar_entries);
 		return -ENOMEM;
 	}
@@ -1034,11 +1037,12 @@  eth_ixgbevf_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 			break;
 
 		default:
-			PMD_INIT_LOG(ERR, "VF Initialization Failure: %d", diag);
+			PMD_INIT_LOG(ERR, "VF Initialization Failure: %d\n",
+				     diag);
 			return (-EIO);
 	}
 
-	PMD_INIT_LOG(DEBUG, "\nport %d vendorID=0x%x deviceID=0x%x mac.type=%s\n",
+	PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x mac.type=%s\n",
 			 eth_dev->data->port_id, pci_dev->id.vendor_id, pci_dev->id.device_id,
 			 "ixgbe_mac_82599_vf");
 
@@ -1207,7 +1211,7 @@  ixgbe_vlan_hw_strip_disable(struct rte_eth_dev *dev, uint16_t queue)
 
 	if (hw->mac.type == ixgbe_mac_82598EB) {
 		/* No queue level support */
-		PMD_INIT_LOG(INFO, "82598EB not support queue level hw strip");
+		PMD_INIT_LOG(INFO, "82598EB not support queue level hw strip\n");
 		return;
 	}
 	else {
@@ -1231,7 +1235,7 @@  ixgbe_vlan_hw_strip_enable(struct rte_eth_dev *dev, uint16_t queue)
 
 	if (hw->mac.type == ixgbe_mac_82598EB) {
 		/* No queue level supported */
-		PMD_INIT_LOG(INFO, "82598EB not support queue level hw strip");
+		PMD_INIT_LOG(INFO, "82598EB not support queue level hw strip\n");
 		return;
 	}
 	else {
@@ -1543,7 +1547,7 @@  skip_link_setup:
 	return (0);
 
 error:
-	PMD_INIT_LOG(ERR, "failure in ixgbe_dev_start(): %d", err);
+	PMD_INIT_LOG(ERR, "failure in ixgbe_dev_start(): %d\n", err);
 	ixgbe_dev_clear_queues(dev);
 	return -EIO;
 }
@@ -1599,10 +1603,8 @@  ixgbe_dev_set_link_up(struct rte_eth_dev *dev)
 #ifdef RTE_NIC_BYPASS
 		if (hw->device_id == IXGBE_DEV_ID_82599_BYPASS) {
 			/* Not suported in bypass mode */
-			PMD_INIT_LOG(ERR,
-				"\nSet link up is not supported "
-				"by device id 0x%x\n",
-				hw->device_id);
+			PMD_INIT_LOG(ERR, "Set link up is not supported "
+				     "by device id 0x%x\n", hw->device_id);
 			return -ENOTSUP;
 		}
 #endif
@@ -1611,8 +1613,8 @@  ixgbe_dev_set_link_up(struct rte_eth_dev *dev)
 		return 0;
 	}
 
-	PMD_INIT_LOG(ERR, "\nSet link up is not supported by device id 0x%x\n",
-		hw->device_id);
+	PMD_INIT_LOG(ERR, "Set link up is not supported by device id 0x%x\n",
+		     hw->device_id);
 	return -ENOTSUP;
 }
 
@@ -1628,10 +1630,8 @@  ixgbe_dev_set_link_down(struct rte_eth_dev *dev)
 #ifdef RTE_NIC_BYPASS
 		if (hw->device_id == IXGBE_DEV_ID_82599_BYPASS) {
 			/* Not suported in bypass mode */
-			PMD_INIT_LOG(ERR,
-				"\nSet link down is not supported "
-				"by device id 0x%x\n",
-				 hw->device_id);
+			PMD_INIT_LOG(ERR, "Set link down is not supported "
+				     "by device id 0x%x\n", hw->device_id);
 			return -ENOTSUP;
 		}
 #endif
@@ -1640,9 +1640,8 @@  ixgbe_dev_set_link_down(struct rte_eth_dev *dev)
 		return 0;
 	}
 
-	PMD_INIT_LOG(ERR,
-		"\nSet link down is not supported by device id 0x%x\n",
-		 hw->device_id);
+	PMD_INIT_LOG(ERR, "Set link down is not supported by device id 0x%x\n",
+		     hw->device_id);
 	return -ENOTSUP;
 }
 
@@ -2113,7 +2112,7 @@  ixgbe_dev_interrupt_get_status(struct rte_eth_dev *dev)
 
 	/* read-on-clear nic registers here */
 	eicr = IXGBE_READ_REG(hw, IXGBE_EICR);
-	PMD_DRV_LOG(INFO, "eicr %x", eicr);
+	PMD_DRV_LOG(INFO, "eicr %x\n", eicr);
 
 	intr->flags = 0;
 	if (eicr & IXGBE_EICR_LSC) {
@@ -2145,16 +2144,16 @@  ixgbe_dev_link_status_print(struct rte_eth_dev *dev)
 	memset(&link, 0, sizeof(link));
 	rte_ixgbe_dev_atomic_read_link_status(dev, &link);
 	if (link.link_status) {
-		PMD_INIT_LOG(INFO, "Port %d: Link Up - speed %u Mbps - %s",
+		PMD_INIT_LOG(INFO, "Port %d: Link Up - speed %u Mbps - %s\n",
 					(int)(dev->data->port_id),
 					(unsigned)link.link_speed,
 			link.link_duplex == ETH_LINK_FULL_DUPLEX ?
 					"full-duplex" : "half-duplex");
 	} else {
-		PMD_INIT_LOG(INFO, " Port %d: Link Down",
+		PMD_INIT_LOG(INFO, " Port %d: Link Down\n",
 				(int)(dev->data->port_id));
 	}
-	PMD_INIT_LOG(INFO, "PCI Address: %04d:%02d:%02d:%d",
+	PMD_INIT_LOG(INFO, "PCI Address: %04d:%02d:%02d:%d\n",
 				dev->pci_dev->addr.domain,
 				dev->pci_dev->addr.bus,
 				dev->pci_dev->addr.devid,
@@ -2211,9 +2210,9 @@  ixgbe_dev_interrupt_action(struct rte_eth_dev *dev)
 	if (intr_enable_delay) {
 		if (rte_eal_alarm_set(timeout * 1000,
 				      ixgbe_dev_interrupt_delayed_handler, (void*)dev) < 0)
-			PMD_DRV_LOG(ERR, "Error setting alarm");
+			PMD_DRV_LOG(ERR, "Error setting alarm\n");
 	} else {
-		PMD_DRV_LOG(DEBUG, "enable intr immediately");
+		PMD_DRV_LOG(DEBUG, "enable intr immediately\n");
 		ixgbe_enable_intr(dev);
 		rte_intr_enable(&(dev->pci_dev->intr_handle));
 	}
@@ -2371,7 +2370,7 @@  ixgbe_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf)
 	if (fc_conf->autoneg != !hw->fc.disable_fc_autoneg)
 		return -ENOTSUP;
 	rx_buf_size = IXGBE_READ_REG(hw, IXGBE_RXPBSIZE(0));
-	PMD_INIT_LOG(DEBUG, "Rx packet buffer size = 0x%x \n", rx_buf_size);
+	PMD_INIT_LOG(DEBUG, "Rx packet buffer size = 0x%x\n", rx_buf_size);
 
 	/*
 	 * At least reserve one Ethernet frame for watermark
@@ -2413,7 +2412,7 @@  ixgbe_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_fc_conf *fc_conf)
 		return 0;
 	}
 
-	PMD_INIT_LOG(ERR, "ixgbe_fc_enable = 0x%x \n", err);
+	PMD_INIT_LOG(ERR, "ixgbe_fc_enable = 0x%x\n", err);
 	return -EIO;
 }
 
@@ -2593,7 +2592,7 @@  ixgbe_priority_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_pfc_conf *p
 	ixgbe_dcb_unpack_map_cee(dcb_config, IXGBE_DCB_RX_CONFIG, map);
 	tc_num = map[pfc_conf->priority];
 	rx_buf_size = IXGBE_READ_REG(hw, IXGBE_RXPBSIZE(tc_num));
-	PMD_INIT_LOG(DEBUG, "Rx packet buffer size = 0x%x \n", rx_buf_size);
+	PMD_INIT_LOG(DEBUG, "Rx packet buffer size = 0x%x\n", rx_buf_size);
 	/*
 	 * At least reserve one Ethernet frame for watermark
 	 * high_water/low_water in kilo bytes for ixgbe
@@ -2618,7 +2617,7 @@  ixgbe_priority_flow_ctrl_set(struct rte_eth_dev *dev, struct rte_eth_pfc_conf *p
 	if ((err == IXGBE_SUCCESS) || (err == IXGBE_ERR_FC_NOT_NEGOTIATED))
 		return 0;
 
-	PMD_INIT_LOG(ERR, "ixgbe_dcb_pfc_enable = 0x%x \n", err);
+	PMD_INIT_LOG(ERR, "ixgbe_dcb_pfc_enable = 0x%x\n", err);
 	return -EIO;
 }
 
@@ -2765,7 +2764,7 @@  ixgbe_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
 static void
 ixgbevf_intr_disable(struct ixgbe_hw *hw)
 {
-	PMD_INIT_LOG(DEBUG, "ixgbevf_intr_disable");
+	PMD_INIT_FUNC_TRACE();
 
 	/* Clear interrupt mask to stop from interrupts being generated */
 	IXGBE_WRITE_REG(hw, IXGBE_VTEIMC, IXGBE_VF_IRQ_CLEAR_MASK);
@@ -2778,8 +2777,8 @@  ixgbevf_dev_configure(struct rte_eth_dev *dev)
 {
 	struct rte_eth_conf* conf = &dev->data->dev_conf;
 
-	PMD_INIT_LOG(DEBUG, "\nConfigured Virtual Function port id: %d\n",
-		dev->data->port_id);
+	PMD_INIT_LOG(DEBUG, "Configured Virtual Function port id: %d\n",
+		     dev->data->port_id);
 
 	/*
 	 * VF has no ability to enable/disable HW CRC
@@ -2807,7 +2806,7 @@  ixgbevf_dev_start(struct rte_eth_dev *dev)
 		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	int err, mask = 0;
 
-	PMD_INIT_LOG(DEBUG, "ixgbevf_dev_start");
+	PMD_INIT_FUNC_TRACE();
 
 	hw->mac.ops.reset_hw(hw);
 
@@ -2842,7 +2841,7 @@  ixgbevf_dev_stop(struct rte_eth_dev *dev)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
-	PMD_INIT_LOG(DEBUG, "ixgbevf_dev_stop");
+	PMD_INIT_FUNC_TRACE();
 
 	hw->adapter_stopped = TRUE;
 	ixgbe_stop_adapter(hw);
@@ -2861,7 +2860,7 @@  ixgbevf_dev_close(struct rte_eth_dev *dev)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
-	PMD_INIT_LOG(DEBUG, "ixgbevf_dev_close");
+	PMD_INIT_FUNC_TRACE();
 
 	ixgbe_reset_hw(hw);
 
@@ -2908,7 +2907,7 @@  ixgbevf_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on)
 	/* vind is not used in VF driver, set to 0, check ixgbe_set_vfta_vf */
 	ret = ixgbe_set_vfta(hw, vlan_id, 0, !!on);
 	if(ret){
-		PMD_INIT_LOG(ERR, "Unable to set VF vlan");
+		PMD_INIT_LOG(ERR, "Unable to set VF vlan\n");
 		return ret;
 	}
 	vid_idx = (uint32_t) ((vlan_id >> 5) & 0x7F);
@@ -3477,7 +3476,7 @@  ixgbevf_add_mac_addr(struct rte_eth_dev *dev, struct ether_addr *mac_addr,
 	diag = ixgbevf_set_uc_addr_vf(hw, 2, mac_addr->addr_bytes);
 	if (diag == 0)
 		return;
-	PMD_DRV_LOG(ERR, "Unable to add MAC address - diag=%d", diag);
+	PMD_DRV_LOG(ERR, "Unable to add MAC address - diag=%d\n", diag);
 }
 
 static void
@@ -3517,7 +3516,7 @@  ixgbevf_remove_mac_addr(struct rte_eth_dev *dev, uint32_t index)
 			PMD_DRV_LOG(ERR,
 				    "Adding again MAC address "
 				    "%02x:%02x:%02x:%02x:%02x:%02x failed "
-				    "diag=%d",
+				    "diag=%d\n",
 				    mac_addr->addr_bytes[0],
 				    mac_addr->addr_bytes[1],
 				    mac_addr->addr_bytes[2],
@@ -3806,7 +3805,7 @@  ixgbe_add_5tuple_filter(struct rte_eth_dev *dev, uint16_t index,
 		return -EINVAL;  /* filter index is out of range. */
 
 	if (filter->tcp_flags) {
-		PMD_INIT_LOG(INFO, "82599EB not tcp flags in 5tuple");
+		PMD_INIT_LOG(INFO, "82599EB not tcp flags in 5tuple\n");
 		return -EINVAL;
 	}
 
diff --git a/lib/librte_pmd_ixgbe/ixgbe_fdir.c b/lib/librte_pmd_ixgbe/ixgbe_fdir.c
index 6c0a530..c78a450 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_fdir.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_fdir.c
@@ -139,7 +139,7 @@  configure_fdir_flags(struct rte_fdir_conf *conf, uint32_t *fdirctrl)
 		break;
 	default:
 		/* bad value */
-		PMD_INIT_LOG(ERR, "Invalid fdir_conf->pballoc value");
+		PMD_INIT_LOG(ERR, "Invalid fdir_conf->pballoc value\n");
 		return -EINVAL;
 	};
 
@@ -158,7 +158,7 @@  configure_fdir_flags(struct rte_fdir_conf *conf, uint32_t *fdirctrl)
 		break;
 	default:
 		/* bad value */
-		PMD_INIT_LOG(ERR, "Invalid fdir_conf->status value");
+		PMD_INIT_LOG(ERR, "Invalid fdir_conf->status value\n");
 		return -EINVAL;
 	};
 
@@ -395,7 +395,7 @@  fdir_filter_to_atr_input(struct rte_fdir_filter *fdir_filter,
 	if ((fdir_filter->l4type == RTE_FDIR_L4TYPE_SCTP ||
 			fdir_filter->l4type == RTE_FDIR_L4TYPE_NONE) &&
 			(fdir_filter->port_src || fdir_filter->port_dst)) {
-		PMD_INIT_LOG(ERR, "Invalid fdir_filter");
+		PMD_INIT_LOG(ERR, "Invalid fdir_filter\n");
 		return -EINVAL;
 	}
 
@@ -420,7 +420,7 @@  fdir_filter_to_atr_input(struct rte_fdir_filter *fdir_filter,
 		input->formatted.flow_type = IXGBE_ATR_FLOW_TYPE_IPV4;
 		break;
 	default:
-		PMD_INIT_LOG(ERR, " Error on l4type input");
+		PMD_INIT_LOG(ERR, " Error on l4type input\n");
 		return -EINVAL;
 	}
 
@@ -524,7 +524,7 @@  fdir_erase_filter_82599(struct ixgbe_hw *hw,
 	}
 
 	if (!retry_count) {
-		PMD_INIT_LOG(ERR, "Timeout querying for flow director filter");
+		PMD_INIT_LOG(ERR, "Timeout querying for flow director filter\n");
 		err = -EIO;
 	}
 
@@ -678,7 +678,7 @@  ixgbe_fdir_set_masks(struct rte_eth_dev *dev, struct rte_fdir_masks *fdir_masks)
 
 	err = ixgbe_reinit_fdir_tables_82599(hw);
 	if (err) {
-		PMD_INIT_LOG(ERR, "reinit of fdir tables failed");
+		PMD_INIT_LOG(ERR, "reinit of fdir tables failed\n");
 		return -EIO;
 	}
 
diff --git a/lib/librte_pmd_ixgbe/ixgbe_logs.h b/lib/librte_pmd_ixgbe/ixgbe_logs.h
index 9f0a684..22d8cfb 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_logs.h
+++ b/lib/librte_pmd_ixgbe/ixgbe_logs.h
@@ -36,8 +36,8 @@ 
 
 #ifdef RTE_LIBRTE_IXGBE_DEBUG_INIT
 #define PMD_INIT_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
-#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
+	RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
+#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>\n")
 #else
 #define PMD_INIT_LOG(level, fmt, args...) do { } while(0)
 #define PMD_INIT_FUNC_TRACE() do { } while(0)
@@ -45,28 +45,28 @@ 
 
 #ifdef RTE_LIBRTE_IXGBE_DEBUG_RX
 #define PMD_RX_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
+	RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
 #else
 #define PMD_RX_LOG(level, fmt, args...) do { } while(0)
 #endif
 
 #ifdef RTE_LIBRTE_IXGBE_DEBUG_TX
 #define PMD_TX_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
+	RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
 #else
 #define PMD_TX_LOG(level, fmt, args...) do { } while(0)
 #endif
 
 #ifdef RTE_LIBRTE_IXGBE_DEBUG_TX_FREE
 #define PMD_TX_FREE_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
+	RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
 #else
 #define PMD_TX_FREE_LOG(level, fmt, args...) do { } while(0)
 #endif
 
 #ifdef RTE_LIBRTE_IXGBE_DEBUG_DRIVER
 #define PMD_DRV_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args)
+	RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
 #else
 #define PMD_DRV_LOG(level, fmt, args...) do { } while(0)
 #endif
diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
index dfc2076..cbec821 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
@@ -492,7 +492,7 @@  ixgbe_xmit_cleanup(struct igb_tx_queue *txq)
 	{
 		PMD_TX_FREE_LOG(DEBUG,
 				"TX descriptor %4u is not done"
-				"(port=%d queue=%d)",
+				"(port=%d queue=%d)\n",
 				desc_to_clean_to,
 				txq->port_id, txq->queue_id);
 		/* Failed to clean any descriptors, better luck next time */
@@ -509,7 +509,7 @@  ixgbe_xmit_cleanup(struct igb_tx_queue *txq)
 
 	PMD_TX_FREE_LOG(DEBUG,
 			"Cleaning %4u TX descriptors: %4u to %4u "
-			"(port=%d queue=%d)",
+			"(port=%d queue=%d)\n",
 			nb_tx_to_clean, last_desc_cleaned, desc_to_clean_to,
 			txq->port_id, txq->queue_id);
 
@@ -630,7 +630,7 @@  ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
 			PMD_TX_FREE_LOG(DEBUG,
 					"Not enough free TX descriptors "
 					"nb_used=%4u nb_free=%4u "
-					"(port=%d queue=%d)",
+					"(port=%d queue=%d)\n",
 					nb_used, txq->nb_tx_free,
 					txq->port_id, txq->queue_id);
 
@@ -650,7 +650,7 @@  ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
 					"performance."
 					"nb_used=%4u nb_free=%4u "
 					"tx_rs_thresh=%4u. "
-					"(port=%d queue=%d)",
+					"(port=%d queue=%d)\n",
 					nb_used, txq->nb_tx_free,
 					txq->tx_rs_thresh,
 					txq->port_id, txq->queue_id);
@@ -782,7 +782,7 @@  ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
 		if (txq->nb_tx_used >= txq->tx_rs_thresh) {
 			PMD_TX_FREE_LOG(DEBUG,
 					"Setting RS bit on TXD id="
-					"%4u (port=%d queue=%d)",
+					"%4u (port=%d queue=%d)\n",
 					tx_last, txq->port_id, txq->queue_id);
 
 			cmd_type_len |= IXGBE_TXD_CMD_RS;
@@ -798,7 +798,7 @@  end_of_tx:
 	/*
 	 * Set the Transmit Descriptor Tail (TDT)
 	 */
-	PMD_TX_LOG(DEBUG, "port_id=%u queue_id=%u tx_tail=%u nb_tx=%u",
+	PMD_TX_LOG(DEBUG, "port_id=%u queue_id=%u tx_tail=%u nb_tx=%u\n",
 		   (unsigned) txq->port_id, (unsigned) txq->queue_id,
 		   (unsigned) tx_id, (unsigned) nb_tx);
 	IXGBE_PCI_REG_WRITE(txq->tdt_reg_addr, tx_id);
@@ -1383,7 +1383,7 @@  ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		 * to happen by sending specific "back-pressure" flow control
 		 * frames to its peer(s).
 		 */
-		PMD_RX_LOG(DEBUG, "\nport_id=%u queue_id=%u rx_id=%u "
+		PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_id=%u "
 			   "staterr=0x%x data_len=%u\n",
 			   (unsigned) rxq->port_id, (unsigned) rxq->queue_id,
 			   (unsigned) rx_id, (unsigned) staterr,