[dpdk-dev,v2,01/17] ixgbe: use the right debug macro

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

Commit Message

David Marchand Sept. 1, 2014, 10:24 a.m. UTC
  - 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 <david.marchand@6wind.com>
---
 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(-)
  

Comments

Jay Rolette Sept. 2, 2014, 1:43 p.m. UTC | #1
Hi David,

A couple of minor comments inline below. Looks good otherwise.

Jay


On Mon, Sep 1, 2014 at 5:24 AM, David Marchand <david.marchand@6wind.com>
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 <david.marchand@6wind.com>
> ---
>  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
>
>
  
David Marchand Sept. 2, 2014, 2:16 p.m. UTC | #2
Hello Jay,


On Tue, Sep 2, 2014 at 3:43 PM, Jay Rolette <rolette@infiniteio.com> wrote:

>
> On Mon, Sep 1, 2014 at 5:24 AM, David Marchand <david.marchand@6wind.com>
> wrote:
>
>>
>> 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
>

Actually, checkpatch complained about this.
So I can keep the parenthesis, but then I don't want Thomas to tell me my
patch does not pass checkpatch :-)



> 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?
>

Yep intended, a patch after this removes all \n (idem for the other
comments).

Thanks.
  
Thomas Monjalon Sept. 2, 2014, 2:21 p.m. UTC | #3
2014-09-02 16:16, David Marchand:
> >>  /* Macros to check for invlaid function pointers. */

Invlaid is an invalid word ;)

> >> -#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
> 
> Actually, checkpatch complained about this.
> So I can keep the parenthesis, but then I don't want Thomas to tell me my
> patch does not pass checkpatch :-)

You're right, I care about checkpatch :)
I don't see a case where parens are needed with return. Please give an example.
  
Jay Rolette Sept. 2, 2014, 5:57 p.m. UTC | #4
On Tue, Sep 2, 2014 at 9:21 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> >> -#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
> >
> > Actually, checkpatch complained about this.
> > So I can keep the parenthesis, but then I don't want Thomas to tell me my
> > patch does not pass checkpatch :-)
>
> You're right, I care about checkpatch :)
> I don't see a case where parens are needed with return. Please give an
> example.


Looking at it again, in this specific case you are correct. It is good
hygiene to always use parens around macro arguments, but this specific case
is ok without. It does add a small bit of danger if retval ends up getting
used elsewhere in the macro, but that's about it. Probably not worth
redoing the patch over that.

Jay
  

Patch

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;                            \
+	}                                                   \
 } 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,
+			     (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",
+			     (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",
+			     (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,
+			     (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,
+			     (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");
 		}
 	}
 
@@ -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");
 		}
 		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) !=