[dpdk-dev,1/7] net/qede: reduce noise in debug logs
Checks
Commit Message
From: Rasesh Mody <Rasesh.Mody@cavium.com>
Replace CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER with
CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL which is a 32-bit bitmapped value
where each bit represent a particular submodule to debug. Also move
notice messages under CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO.
Signed-off-by: Harish Patil <harish.patil@qlogic.com>
Signed-off-by: Rasesh Mody <Rasesh.Mody@cavium.com>
---
config/common_base | 2 +-
doc/guides/nics/qede.rst | 4 ++--
drivers/net/qede/qede_ethdev.c | 4 ++--
drivers/net/qede/qede_logs.h | 21 +++++----------------
4 files changed, 10 insertions(+), 21 deletions(-)
Comments
On 12/3/2016 2:43 AM, Harish Patil wrote:
> From: Rasesh Mody <Rasesh.Mody@cavium.com>
>
> Replace CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER with
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL which is a 32-bit bitmapped value
> where each bit represent a particular submodule to debug. Also move
> notice messages under CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO.
>
> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
> Signed-off-by: Rasesh Mody <Rasesh.Mody@cavium.com>
> ---
> config/common_base | 2 +-
> doc/guides/nics/qede.rst | 4 ++--
> drivers/net/qede/qede_ethdev.c | 4 ++--
> drivers/net/qede/qede_logs.h | 21 +++++----------------
> 4 files changed, 10 insertions(+), 21 deletions(-)
>
> diff --git a/config/common_base b/config/common_base
> index 4bff83a..2ffd557 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -320,7 +320,7 @@ CONFIG_RTE_LIBRTE_BOND_DEBUG_ALB_L1=n
> CONFIG_RTE_LIBRTE_QEDE_PMD=y
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INIT=n
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO=n
> -CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER=n
> +CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL=0
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX=n
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_RX=n
> #Provides abs path/name of the firmware file.
> diff --git a/doc/guides/nics/qede.rst b/doc/guides/nics/qede.rst
> index d22ecdd..ddf4248 100644
> --- a/doc/guides/nics/qede.rst
> +++ b/doc/guides/nics/qede.rst
> @@ -103,9 +103,9 @@ enabling debugging options may affect system performance.
>
> Toggle display of generic debugging messages.
>
> -- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER`` (default **n**)
> +- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL`` (default **0**)
Does it make sense to document how DEBUG_VAL used?
Also commit log says bitmapped value to enable/disable a particular
submodule, you may want to document here which value enable/disable
which submodule.
>
> - Toggle display of ecore related messages.
> + Control driver debug verbosity using 32-bit bitmap flags.
>
> - ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX`` (default **n**)
>
<...>
> diff --git a/drivers/net/qede/qede_logs.h b/drivers/net/qede/qede_logs.h
> index 45c4af0..08fdf04 100644
> --- a/drivers/net/qede/qede_logs.h
> +++ b/drivers/net/qede/qede_logs.h
> @@ -16,15 +16,18 @@
> (p_dev)->name ? (p_dev)->name : "", \
> ##__VA_ARGS__)
>
> +#ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
Is "_INFO" carries any meaning in this config option, why not just
RTE_LIBRTE_QEDE_DEBUG?
> #define DP_NOTICE(p_dev, is_assert, fmt, ...) \
> rte_log(RTE_LOG_NOTICE, RTE_LOGTYPE_PMD,\
> "[QEDE PMD: (%s)]%s:" fmt, \
> (p_dev)->name ? (p_dev)->name : "", \
> __func__, \
> ##__VA_ARGS__)
> +#else
> +#define DP_NOTICE(p_dev, fmt, ...) do { } while (0)
> +#endif
>
> #ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
> -
> #define DP_INFO(p_dev, fmt, ...) \
> rte_log(RTE_LOG_INFO, RTE_LOGTYPE_PMD, \
> "[%s:%d(%s)]" fmt, \
> @@ -33,10 +36,8 @@
> ##__VA_ARGS__)
> #else
> #define DP_INFO(p_dev, fmt, ...) do { } while (0)
> -
> #endif
>
> -#ifdef RTE_LIBRTE_QEDE_DEBUG_DRIVER
Are you sure you want to enable DP_VERBOSE, I guess most verbose log
macro, by default? Perhaps may want to control it via
RTE_LIBRTE_QEDE_DEBUG_INFO?
> #define DP_VERBOSE(p_dev, module, fmt, ...) \
> do { \
> if ((p_dev)->dp_module & module) \
> @@ -46,9 +47,7 @@
> (p_dev)->name ? (p_dev)->name : "", \
> ##__VA_ARGS__); \
> } while (0)
> -#else
> -#define DP_VERBOSE(p_dev, fmt, ...) do { } while (0)
> -#endif
> +
>
<...>
On 12/3/2016 2:43 AM, Harish Patil wrote:
> From: Rasesh Mody <Rasesh.Mody@cavium.com>
>
> Replace CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER with
> CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL which is a 32-bit bitmapped value
> where each bit represent a particular submodule to debug. Also move
> notice messages under CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO.
>
> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
> Signed-off-by: Rasesh Mody <Rasesh.Mody@cavium.com>
> ---
Is 32bit supported by driver?
If so it is throwing a compilation error for it [1], if not can you
please document it?
[1]
In file included from .../drivers/net/qede/base/ecore.h:35:0,
from .../drivers/net/qede/qede_ethdev.h:22,
from .../drivers/net/qede/qede_ethdev.c:9:
.../drivers/net/qede/qede_ethdev.c: In function ‘qede_rss_hash_update’:
.../drivers/net/qede/base/../qede_logs.h:33:3: error: format ‘%lx’
expects argument of type ‘long unsigned int’, but argument 7 has type
‘uint64_t {aka long long unsigned int}’ [-Werror=format=]
"[%s:%d(%s)]" fmt, \
^
.../drivers/net/qede/qede_ethdev.c:1472:2: note: in expansion of macro
‘DP_INFO’
DP_INFO(edev, "RSS hf = 0x%lx len = %u key = %p\n", hf, len, key);
^~~~~~~
cc1: all warnings being treated as errors
<...>
Hi Ferruh,
>On 12/3/2016 2:43 AM, Harish Patil wrote:
>> From: Rasesh Mody <Rasesh.Mody@cavium.com>
>>
>> Replace CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER with
>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL which is a 32-bit bitmapped value
>> where each bit represent a particular submodule to debug. Also move
>> notice messages under CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO.
>>
>> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
>> Signed-off-by: Rasesh Mody <Rasesh.Mody@cavium.com>
>> ---
>> config/common_base | 2 +-
>> doc/guides/nics/qede.rst | 4 ++--
>> drivers/net/qede/qede_ethdev.c | 4 ++--
>> drivers/net/qede/qede_logs.h | 21 +++++----------------
>> 4 files changed, 10 insertions(+), 21 deletions(-)
>>
>> diff --git a/config/common_base b/config/common_base
>> index 4bff83a..2ffd557 100644
>> --- a/config/common_base
>> +++ b/config/common_base
>> @@ -320,7 +320,7 @@ CONFIG_RTE_LIBRTE_BOND_DEBUG_ALB_L1=n
>> CONFIG_RTE_LIBRTE_QEDE_PMD=y
>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INIT=n
>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO=n
>> -CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER=n
>> +CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL=0
>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX=n
>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_RX=n
>> #Provides abs path/name of the firmware file.
>> diff --git a/doc/guides/nics/qede.rst b/doc/guides/nics/qede.rst
>> index d22ecdd..ddf4248 100644
>> --- a/doc/guides/nics/qede.rst
>> +++ b/doc/guides/nics/qede.rst
>> @@ -103,9 +103,9 @@ enabling debugging options may affect system
>>performance.
>>
>> Toggle display of generic debugging messages.
>>
>> -- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER`` (default **n**)
>> +- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL`` (default **0**)
>
>Does it make sense to document how DEBUG_VAL used?
>
>Also commit log says bitmapped value to enable/disable a particular
>submodule, you may want to document here which value enable/disable
>which submodule.
>
>>
>> - Toggle display of ecore related messages.
>> + Control driver debug verbosity using 32-bit bitmap flags.
>>
>> - ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX`` (default **n**)
>>
Not really, I think that would be too much. But if you think it really
helps then perhaps yes we can document.
Otherwise it is just for internal debugging.
>
><...>
>
>> diff --git a/drivers/net/qede/qede_logs.h b/drivers/net/qede/qede_logs.h
>> index 45c4af0..08fdf04 100644
>> --- a/drivers/net/qede/qede_logs.h
>> +++ b/drivers/net/qede/qede_logs.h
>> @@ -16,15 +16,18 @@
>> (p_dev)->name ? (p_dev)->name : "", \
>> ##__VA_ARGS__)
>>
>> +#ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
>
>Is "_INFO" carries any meaning in this config option, why not just
>RTE_LIBRTE_QEDE_DEBUG?
INFO is used to mean just informational type of messages.
If you think it doesn’t make sense then I can rename it.
>
>> #define DP_NOTICE(p_dev, is_assert, fmt, ...) \
>> rte_log(RTE_LOG_NOTICE, RTE_LOGTYPE_PMD,\
>> "[QEDE PMD: (%s)]%s:" fmt, \
>> (p_dev)->name ? (p_dev)->name : "", \
>> __func__, \
>> ##__VA_ARGS__)
>> +#else
>> +#define DP_NOTICE(p_dev, fmt, ...) do { } while (0)
>> +#endif
>>
>> #ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
>> -
>> #define DP_INFO(p_dev, fmt, ...) \
>> rte_log(RTE_LOG_INFO, RTE_LOGTYPE_PMD, \
>> "[%s:%d(%s)]" fmt, \
>> @@ -33,10 +36,8 @@
>> ##__VA_ARGS__)
>> #else
>> #define DP_INFO(p_dev, fmt, ...) do { } while (0)
>> -
>> #endif
>>
>> -#ifdef RTE_LIBRTE_QEDE_DEBUG_DRIVER
>
>Are you sure you want to enable DP_VERBOSE, I guess most verbose log
>macro, by default? Perhaps may want to control it via
>RTE_LIBRTE_QEDE_DEBUG_INFO?
DP_VERBOSE is enabled but it has a check:
if ((p_dev)->dp_module & module)
which controls what to print.
Here dp_module is controlled by CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL flag.
Hope it is clear.
>
>> #define DP_VERBOSE(p_dev, module, fmt, ...) \
>> do { \
>> if ((p_dev)->dp_module & module) \
>> @@ -46,9 +47,7 @@
>> (p_dev)->name ? (p_dev)->name : "", \
>> ##__VA_ARGS__); \
>> } while (0)
>> -#else
>> -#define DP_VERBOSE(p_dev, fmt, ...) do { } while (0)
>> -#endif
>> +
>>
><...>
>
>
On 12/12/2016 5:15 PM, Harish Patil wrote:
> Hi Ferruh,
>
>> On 12/3/2016 2:43 AM, Harish Patil wrote:
>>> From: Rasesh Mody <Rasesh.Mody@cavium.com>
>>>
>>> Replace CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER with
>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL which is a 32-bit bitmapped value
>>> where each bit represent a particular submodule to debug. Also move
>>> notice messages under CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO.
>>>
>>> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
>>> Signed-off-by: Rasesh Mody <Rasesh.Mody@cavium.com>
>>> ---
>>> config/common_base | 2 +-
>>> doc/guides/nics/qede.rst | 4 ++--
>>> drivers/net/qede/qede_ethdev.c | 4 ++--
>>> drivers/net/qede/qede_logs.h | 21 +++++----------------
>>> 4 files changed, 10 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/config/common_base b/config/common_base
>>> index 4bff83a..2ffd557 100644
>>> --- a/config/common_base
>>> +++ b/config/common_base
>>> @@ -320,7 +320,7 @@ CONFIG_RTE_LIBRTE_BOND_DEBUG_ALB_L1=n
>>> CONFIG_RTE_LIBRTE_QEDE_PMD=y
>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INIT=n
>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO=n
>>> -CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER=n
>>> +CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL=0
>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX=n
>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_RX=n
>>> #Provides abs path/name of the firmware file.
>>> diff --git a/doc/guides/nics/qede.rst b/doc/guides/nics/qede.rst
>>> index d22ecdd..ddf4248 100644
>>> --- a/doc/guides/nics/qede.rst
>>> +++ b/doc/guides/nics/qede.rst
>>> @@ -103,9 +103,9 @@ enabling debugging options may affect system
>>> performance.
>>>
>>> Toggle display of generic debugging messages.
>>>
>>> -- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER`` (default **n**)
>>> +- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL`` (default **0**)
>>
>> Does it make sense to document how DEBUG_VAL used?
>>
>> Also commit log says bitmapped value to enable/disable a particular
>> submodule, you may want to document here which value enable/disable
>> which submodule.
>>
>>>
>>> - Toggle display of ecore related messages.
>>> + Control driver debug verbosity using 32-bit bitmap flags.
>>>
>>> - ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX`` (default **n**)
>>>
>
> Not really, I think that would be too much. But if you think it really
> helps then perhaps yes we can document.
> Otherwise it is just for internal debugging.
As a user of your driver, how can I know how to enable / disable a
module log?
I know VAL enables / disables them, but I don't know what modules exists
and what values are required.
If this is just for internal debugging and user not need to configure
this one, does it needs to be a config option in configuration file?
>
>
>>
>> <...>
>>
>>> diff --git a/drivers/net/qede/qede_logs.h b/drivers/net/qede/qede_logs.h
>>> index 45c4af0..08fdf04 100644
>>> --- a/drivers/net/qede/qede_logs.h
>>> +++ b/drivers/net/qede/qede_logs.h
>>> @@ -16,15 +16,18 @@
>>> (p_dev)->name ? (p_dev)->name : "", \
>>> ##__VA_ARGS__)
>>>
>>> +#ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
>>
>> Is "_INFO" carries any meaning in this config option, why not just
>> RTE_LIBRTE_QEDE_DEBUG?
>
> INFO is used to mean just informational type of messages.
> If you think it doesn’t make sense then I can rename it.
I don't have a strong opinion, I think that _INFO is not adding value
unless you have different config options per each level like _VERBOSE,
_INFO, _NOTICE. But if you believe your users will benefit from it, it
is your call.
>
>>
>>> #define DP_NOTICE(p_dev, is_assert, fmt, ...) \
>>> rte_log(RTE_LOG_NOTICE, RTE_LOGTYPE_PMD,\
>>> "[QEDE PMD: (%s)]%s:" fmt, \
>>> (p_dev)->name ? (p_dev)->name : "", \
>>> __func__, \
>>> ##__VA_ARGS__)
>>> +#else
>>> +#define DP_NOTICE(p_dev, fmt, ...) do { } while (0)
>>> +#endif
>>>
>>> #ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
>>> -
>>> #define DP_INFO(p_dev, fmt, ...) \
>>> rte_log(RTE_LOG_INFO, RTE_LOGTYPE_PMD, \
>>> "[%s:%d(%s)]" fmt, \
>>> @@ -33,10 +36,8 @@
>>> ##__VA_ARGS__)
>>> #else
>>> #define DP_INFO(p_dev, fmt, ...) do { } while (0)
>>> -
>>> #endif
>>>
>>> -#ifdef RTE_LIBRTE_QEDE_DEBUG_DRIVER
>>
>> Are you sure you want to enable DP_VERBOSE, I guess most verbose log
>> macro, by default? Perhaps may want to control it via
>> RTE_LIBRTE_QEDE_DEBUG_INFO?
>
> DP_VERBOSE is enabled but it has a check:
> if ((p_dev)->dp_module & module)
> which controls what to print.
> Here dp_module is controlled by CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL flag.
> Hope it is clear.
Clear thanks,
I guess right now:
DEBUG_VAL: enables verbose debug for selected module(s)
DEBUG_INFO: Enables NOTICE and INFO level for ? (all modules?)
ERR level is always enabled
Again it is your call, but I think CONFIG_RTE_LIBRTE_QEDE_DEBUG can be
used to enable/disable all debugs, and when enabled QEDE_DEBUG_VAL can
select which modules to enable verbose debug ...
>>
>>> #define DP_VERBOSE(p_dev, module, fmt, ...) \
>>> do { \
>>> if ((p_dev)->dp_module & module) \
>>> @@ -46,9 +47,7 @@
>>> (p_dev)->name ? (p_dev)->name : "", \
>>> ##__VA_ARGS__); \
>>> } while (0)
>>> -#else
>>> -#define DP_VERBOSE(p_dev, fmt, ...) do { } while (0)
>>> -#endif
>>> +
>>>
>> <...>
>>
>>
>
>
>On 12/12/2016 5:15 PM, Harish Patil wrote:
>> Hi Ferruh,
>>
>>> On 12/3/2016 2:43 AM, Harish Patil wrote:
>>>> From: Rasesh Mody <Rasesh.Mody@cavium.com>
>>>>
>>>> Replace CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER with
>>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL which is a 32-bit bitmapped value
>>>> where each bit represent a particular submodule to debug. Also move
>>>> notice messages under CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO.
>>>>
>>>> Signed-off-by: Harish Patil <harish.patil@qlogic.com>
>>>> Signed-off-by: Rasesh Mody <Rasesh.Mody@cavium.com>
>>>> ---
>>>> config/common_base | 2 +-
>>>> doc/guides/nics/qede.rst | 4 ++--
>>>> drivers/net/qede/qede_ethdev.c | 4 ++--
>>>> drivers/net/qede/qede_logs.h | 21 +++++----------------
>>>> 4 files changed, 10 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/config/common_base b/config/common_base
>>>> index 4bff83a..2ffd557 100644
>>>> --- a/config/common_base
>>>> +++ b/config/common_base
>>>> @@ -320,7 +320,7 @@ CONFIG_RTE_LIBRTE_BOND_DEBUG_ALB_L1=n
>>>> CONFIG_RTE_LIBRTE_QEDE_PMD=y
>>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INIT=n
>>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO=n
>>>> -CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER=n
>>>> +CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL=0
>>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX=n
>>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_RX=n
>>>> #Provides abs path/name of the firmware file.
>>>> diff --git a/doc/guides/nics/qede.rst b/doc/guides/nics/qede.rst
>>>> index d22ecdd..ddf4248 100644
>>>> --- a/doc/guides/nics/qede.rst
>>>> +++ b/doc/guides/nics/qede.rst
>>>> @@ -103,9 +103,9 @@ enabling debugging options may affect system
>>>> performance.
>>>>
>>>> Toggle display of generic debugging messages.
>>>>
>>>> -- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER`` (default **n**)
>>>> +- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL`` (default **0**)
>>>
>>> Does it make sense to document how DEBUG_VAL used?
>>>
>>> Also commit log says bitmapped value to enable/disable a particular
>>> submodule, you may want to document here which value enable/disable
>>> which submodule.
>>>
>>>>
>>>> - Toggle display of ecore related messages.
>>>> + Control driver debug verbosity using 32-bit bitmap flags.
>>>>
>>>> - ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX`` (default **n**)
>>>>
>>
>> Not really, I think that would be too much. But if you think it really
>> helps then perhaps yes we can document.
>> Otherwise it is just for internal debugging.
>
>As a user of your driver, how can I know how to enable / disable a
>module log?
>I know VAL enables / disables them, but I don't know what modules exists
>and what values are required.
>
>If this is just for internal debugging and user not need to configure
>this one, does it needs to be a config option in configuration file?
Okay then let me revert this change as before in v2 series. Earlier, it
was boolean config option and CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER=y would
enable all debug logs.
>
>>
>>
>>>
>>> <...>
>>>
>>>> diff --git a/drivers/net/qede/qede_logs.h
>>>>b/drivers/net/qede/qede_logs.h
>>>> index 45c4af0..08fdf04 100644
>>>> --- a/drivers/net/qede/qede_logs.h
>>>> +++ b/drivers/net/qede/qede_logs.h
>>>> @@ -16,15 +16,18 @@
>>>> (p_dev)->name ? (p_dev)->name : "", \
>>>> ##__VA_ARGS__)
>>>>
>>>> +#ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
>>>
>>> Is "_INFO" carries any meaning in this config option, why not just
>>> RTE_LIBRTE_QEDE_DEBUG?
>>
>> INFO is used to mean just informational type of messages.
>> If you think it doesn’t make sense then I can rename it.
>
>I don't have a strong opinion, I think that _INFO is not adding value
>unless you have different config options per each level like _VERBOSE,
>_INFO, _NOTICE. But if you believe your users will benefit from it, it
>is your call.
Yes, I would like to retain INFO as it provides high-level informational
messages currently.
>
>>
>>>
>>>> #define DP_NOTICE(p_dev, is_assert, fmt, ...) \
>>>> rte_log(RTE_LOG_NOTICE, RTE_LOGTYPE_PMD,\
>>>> "[QEDE PMD: (%s)]%s:" fmt, \
>>>> (p_dev)->name ? (p_dev)->name : "", \
>>>> __func__, \
>>>> ##__VA_ARGS__)
>>>> +#else
>>>> +#define DP_NOTICE(p_dev, fmt, ...) do { } while (0)
>>>> +#endif
>>>>
>>>> #ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
>>>> -
>>>> #define DP_INFO(p_dev, fmt, ...) \
>>>> rte_log(RTE_LOG_INFO, RTE_LOGTYPE_PMD, \
>>>> "[%s:%d(%s)]" fmt, \
>>>> @@ -33,10 +36,8 @@
>>>> ##__VA_ARGS__)
>>>> #else
>>>> #define DP_INFO(p_dev, fmt, ...) do { } while (0)
>>>> -
>>>> #endif
>>>>
>>>> -#ifdef RTE_LIBRTE_QEDE_DEBUG_DRIVER
>>>
>>> Are you sure you want to enable DP_VERBOSE, I guess most verbose log
>>> macro, by default? Perhaps may want to control it via
>>> RTE_LIBRTE_QEDE_DEBUG_INFO?
>>
>> DP_VERBOSE is enabled but it has a check:
>> if ((p_dev)->dp_module & module)
>> which controls what to print.
>> Here dp_module is controlled by CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL flag.
>> Hope it is clear.
>
>Clear thanks,
>
>I guess right now:
>DEBUG_VAL: enables verbose debug for selected module(s)
>DEBUG_INFO: Enables NOTICE and INFO level for ? (all modules?)
>ERR level is always enabled
>
>Again it is your call, but I think CONFIG_RTE_LIBRTE_QEDE_DEBUG can be
>used to enable/disable all debugs, and when enabled QEDE_DEBUG_VAL can
>select which modules to enable verbose debug ...
>
>>>
>>>> #define DP_VERBOSE(p_dev, module, fmt, ...) \
>>>> do { \
>>>> if ((p_dev)->dp_module & module) \
>>>> @@ -46,9 +47,7 @@
>>>> (p_dev)->name ? (p_dev)->name : "", \
>>>> ##__VA_ARGS__); \
>>>> } while (0)
>>>> -#else
>>>> -#define DP_VERBOSE(p_dev, fmt, ...) do { } while (0)
>>>> -#endif
>>>> +
>>>>
>>> <...>
>>>
>>>
>>
>
>
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Thursday, December 08, 2016 8:49 AM
>
> On 12/3/2016 2:43 AM, Harish Patil wrote:
> > From: Rasesh Mody <Rasesh.Mody@cavium.com>
> >
> > Replace CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER with
> > CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL which is a 32-bit bitmapped value
> > where each bit represent a particular submodule to debug. Also move
> > notice messages under CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO.
> >
> > Signed-off-by: Harish Patil <harish.patil@qlogic.com>
> > Signed-off-by: Rasesh Mody <Rasesh.Mody@cavium.com>
> > ---
>
> Is 32bit supported by driver?
We compile test our drivers for 32bit.
> If so it is throwing a compilation error for it [1], if not can you please
> document it?
>
> [1]
> In file included from .../drivers/net/qede/base/ecore.h:35:0,
> from .../drivers/net/qede/qede_ethdev.h:22,
> from .../drivers/net/qede/qede_ethdev.c:9:
> .../drivers/net/qede/qede_ethdev.c: In function 'qede_rss_hash_update':
> .../drivers/net/qede/base/../qede_logs.h:33:3: error: format '%lx'
> expects argument of type 'long unsigned int', but argument 7 has type
> 'uint64_t {aka long long unsigned int}' [-Werror=format=]
> "[%s:%d(%s)]" fmt, \
> ^
> .../drivers/net/qede/qede_ethdev.c:1472:2: note: in expansion of macro
> 'DP_INFO'
> DP_INFO(edev, "RSS hf = 0x%lx len = %u key = %p\n", hf, len, key);
> ^~~~~~~
> cc1: all warnings being treated as errors
We'll fix the compilation error.
>
>
> <...>
@@ -320,7 +320,7 @@ CONFIG_RTE_LIBRTE_BOND_DEBUG_ALB_L1=n
CONFIG_RTE_LIBRTE_QEDE_PMD=y
CONFIG_RTE_LIBRTE_QEDE_DEBUG_INIT=n
CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO=n
-CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER=n
+CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL=0
CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX=n
CONFIG_RTE_LIBRTE_QEDE_DEBUG_RX=n
#Provides abs path/name of the firmware file.
@@ -103,9 +103,9 @@ enabling debugging options may affect system performance.
Toggle display of generic debugging messages.
-- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER`` (default **n**)
+- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_VAL`` (default **0**)
- Toggle display of ecore related messages.
+ Control driver debug verbosity using 32-bit bitmap flags.
- ``CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX`` (default **n**)
@@ -1393,8 +1393,8 @@ static int qede_common_dev_init(struct rte_eth_dev *eth_dev, bool is_vf)
uint8_t is_mac_forced;
bool is_mac_exist;
/* Fix up ecore debug level */
- uint32_t dp_module = ~0 & ~ECORE_MSG_HW;
- uint8_t dp_level = ECORE_LEVEL_VERBOSE;
+ uint32_t dp_module = RTE_LIBRTE_QEDE_DEBUG_VAL;
+ uint8_t dp_level = ECORE_LEVEL_NOTICE;
uint32_t max_mac_addrs;
int rc;
@@ -16,15 +16,18 @@
(p_dev)->name ? (p_dev)->name : "", \
##__VA_ARGS__)
+#ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
#define DP_NOTICE(p_dev, is_assert, fmt, ...) \
rte_log(RTE_LOG_NOTICE, RTE_LOGTYPE_PMD,\
"[QEDE PMD: (%s)]%s:" fmt, \
(p_dev)->name ? (p_dev)->name : "", \
__func__, \
##__VA_ARGS__)
+#else
+#define DP_NOTICE(p_dev, fmt, ...) do { } while (0)
+#endif
#ifdef RTE_LIBRTE_QEDE_DEBUG_INFO
-
#define DP_INFO(p_dev, fmt, ...) \
rte_log(RTE_LOG_INFO, RTE_LOGTYPE_PMD, \
"[%s:%d(%s)]" fmt, \
@@ -33,10 +36,8 @@
##__VA_ARGS__)
#else
#define DP_INFO(p_dev, fmt, ...) do { } while (0)
-
#endif
-#ifdef RTE_LIBRTE_QEDE_DEBUG_DRIVER
#define DP_VERBOSE(p_dev, module, fmt, ...) \
do { \
if ((p_dev)->dp_module & module) \
@@ -46,9 +47,7 @@
(p_dev)->name ? (p_dev)->name : "", \
##__VA_ARGS__); \
} while (0)
-#else
-#define DP_VERBOSE(p_dev, fmt, ...) do { } while (0)
-#endif
+
#define PMD_INIT_LOG(level, edev, fmt, args...) \
rte_log(RTE_LOG_ ## level, RTE_LOGTYPE_PMD, \
@@ -77,14 +76,4 @@
#define PMD_RX_LOG(level, q, fmt, args...) do { } while (0)
#endif
-#ifdef RTE_LIBRTE_QEDE_DEBUG_DRIVER
-#define PMD_DRV_LOG_RAW(level, fmt, args...) \
- RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)
-#else
-#define PMD_DRV_LOG_RAW(level, fmt, args...) do { } while (0)
-#endif
-
-#define PMD_DRV_LOG(level, fmt, args...) \
- PMD_DRV_LOG_RAW(level, fmt "\n", ## args)
-
#endif /* _QEDE_LOGS_H_ */