[PATCH v1] Bluetooth: create CONFIG_BT_DEBUG_FUNC_NAME

[PATCH v1] Bluetooth: create CONFIG_BT_DEBUG_FUNC_NAME

[Alain Michaud]

Creates a CONFIG_BT_DEBUG_FUNC_NAME option to include function names in
debug statements.

Unlike other platforms __func__ isn't a string literal so it cannot be
automatically concatenated by the pre-processor.  As a result, the
function name is passed as a parameter to the tracing function.  Since
pr_debug is a function like macro, the normal expansion of BT_PREFIX_PARAM
does not work as it gets processed within the first parameter as well,
for this reason, BT_DBG is split into two versions.

This patch was built tested with all 4 possible combinations of
CONFIG_BT_DEBUG_FUNC_NAME and CONFIG_BT_FEATURE_DEBUG configurations.

Signed-off-by: Alain Michaud <alainm@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

 include/net/bluetooth/bluetooth.h | 32 +++++++++++++++++++++++--------
 net/bluetooth/Kconfig             | 11 +++++++++++
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 7ee8041af803..27ec8f2a7c28 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -162,22 +162,37 @@ void bt_dbg_set(bool enable);
 bool bt_dbg_get(void);
 __printf(1, 2)
 void bt_dbg(const char *fmt, ...);
+#define BT_DBG_INT	bt_dbg
+#else
+#define BT_DBG_INT	pr_debug
 #endif
 __printf(1, 2)
 void bt_warn_ratelimited(const char *fmt, ...);
 __printf(1, 2)
 void bt_err_ratelimited(const char *fmt, ...);
 
-#define BT_INFO(fmt, ...)	bt_info(fmt "\n", ##__VA_ARGS__)
-#define BT_WARN(fmt, ...)	bt_warn(fmt "\n", ##__VA_ARGS__)
-#define BT_ERR(fmt, ...)	bt_err(fmt "\n", ##__VA_ARGS__)
-
-#if IS_ENABLED(CONFIG_BT_FEATURE_DEBUG)
-#define BT_DBG(fmt, ...)	bt_dbg(fmt "\n", ##__VA_ARGS__)
+#if IS_ENABLED(CONFIG_BT_DEBUG_FUNC_NAMES)
+#define BT_PREFIX "%s() "
+#define BT_PREFIX_PARAM ,__func__
+#define BT_DBG(fmt, ...)	\
+	BT_DBG_INT(BT_PREFIX fmt "\n", __func__, ##__VA_ARGS__)
 #else
-#define BT_DBG(fmt, ...)	pr_debug(fmt "\n", ##__VA_ARGS__)
+#define BT_PREFIX
+#define BT_PREFIX_PARAM
+#define BT_DBG(fmt, ...)	\
+	BT_DBG_INT(fmt "\n", ##__VA_ARGS__)
 #endif
 
+#define BT_INFO(fmt, ...)	\
+	bt_info(BT_PREFIX fmt "\n" BT_PREFIX_PARAM, ##__VA_ARGS__)
+#define BT_WARN(fmt, ...)	\
+	bt_warn(BT_PREFIX fmt "\n" BT_PREFIX_PARAM, ##__VA_ARGS__)
+#define BT_ERR(fmt, ...)	\
+	bt_err(BT_PREFIX fmt "\n" BT_PREFIX_PARAM, ##__VA_ARGS__)
+
+#define BT_ERR_RATELIMITED(fmt, ...) \
+	bt_err_ratelimited(BT_PREFIX fmt "\n" BT_PREFIX_PARAM, ##__VA_ARGS__)
+
 #define bt_dev_info(hdev, fmt, ...)				\
 	BT_INFO("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
 #define bt_dev_warn(hdev, fmt, ...)				\
@@ -188,7 +203,8 @@ void bt_err_ratelimited(const char *fmt, ...);
 	BT_DBG("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
 
 #define bt_dev_warn_ratelimited(hdev, fmt, ...)			\
-	bt_warn_ratelimited("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
+	bt_warn_ratelimited("%s: " BT_PREFIX fmt, (hdev)->name	\
+			    BT_PREFIX_PARAM, ##__VA_ARGS__)
 #define bt_dev_err_ratelimited(hdev, fmt, ...)			\
 	bt_err_ratelimited("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
 
diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 1d6d243cdde9..de31c682c7b0 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -142,4 +142,15 @@ config BT_FEATURE_DEBUG
 	  This provides an option to enable/disable debugging statements
 	  at runtime via the experimental features interface.
 
+config BT_DEBUG_FUNC_NAMES
+	bool "Include function names in debugging statements"
+	depends on BT
+	default n
+	help
+	  Provides an option to include function names in debugging
+	  statements.
+
+	  When enabled, trace statements will include the function name as a
+	  prefix which may help identify the source code references.
+
 source "drivers/bluetooth/Kconfig"
-- 
2.27.0.212.ge8ba1cc988-goog

Re: [PATCH v1] Bluetooth: create CONFIG_BT_DEBUG_FUNC_NAME

[Alain Michaud]

This feature depends (soft dependency) on the kernel experimental
feature defined in this patch:
https://patchwork.kernel.org/patch/11646081/

On Mon, Jul 6, 2020 at 8:39 PM Alain Michaud <alainm@chromium.org> wrote:
>
> Creates a CONFIG_BT_DEBUG_FUNC_NAME option to include function names in
> debug statements.
>
> Unlike other platforms __func__ isn't a string literal so it cannot be
> automatically concatenated by the pre-processor.  As a result, the
> function name is passed as a parameter to the tracing function.  Since
> pr_debug is a function like macro, the normal expansion of BT_PREFIX_PARAM
> does not work as it gets processed within the first parameter as well,
> for this reason, BT_DBG is split into two versions.
>
> This patch was built tested with all 4 possible combinations of
> CONFIG_BT_DEBUG_FUNC_NAME and CONFIG_BT_FEATURE_DEBUG configurations.
>
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
>  include/net/bluetooth/bluetooth.h | 32 +++++++++++++++++++++++--------
>  net/bluetooth/Kconfig             | 11 +++++++++++
>  2 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 7ee8041af803..27ec8f2a7c28 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -162,22 +162,37 @@ void bt_dbg_set(bool enable);
>  bool bt_dbg_get(void);
>  __printf(1, 2)
>  void bt_dbg(const char *fmt, ...);
> +#define BT_DBG_INT     bt_dbg
> +#else
> +#define BT_DBG_INT     pr_debug
>  #endif
>  __printf(1, 2)
>  void bt_warn_ratelimited(const char *fmt, ...);
>  __printf(1, 2)
>  void bt_err_ratelimited(const char *fmt, ...);
>
> -#define BT_INFO(fmt, ...)      bt_info(fmt "\n", ##__VA_ARGS__)
> -#define BT_WARN(fmt, ...)      bt_warn(fmt "\n", ##__VA_ARGS__)
> -#define BT_ERR(fmt, ...)       bt_err(fmt "\n", ##__VA_ARGS__)
> -
> -#if IS_ENABLED(CONFIG_BT_FEATURE_DEBUG)
> -#define BT_DBG(fmt, ...)       bt_dbg(fmt "\n", ##__VA_ARGS__)
> +#if IS_ENABLED(CONFIG_BT_DEBUG_FUNC_NAMES)
> +#define BT_PREFIX "%s() "
> +#define BT_PREFIX_PARAM ,__func__
> +#define BT_DBG(fmt, ...)       \
> +       BT_DBG_INT(BT_PREFIX fmt "\n", __func__, ##__VA_ARGS__)
>  #else
> -#define BT_DBG(fmt, ...)       pr_debug(fmt "\n", ##__VA_ARGS__)
> +#define BT_PREFIX
> +#define BT_PREFIX_PARAM
> +#define BT_DBG(fmt, ...)       \
> +       BT_DBG_INT(fmt "\n", ##__VA_ARGS__)
>  #endif
>
> +#define BT_INFO(fmt, ...)      \
> +       bt_info(BT_PREFIX fmt "\n" BT_PREFIX_PARAM, ##__VA_ARGS__)
> +#define BT_WARN(fmt, ...)      \
> +       bt_warn(BT_PREFIX fmt "\n" BT_PREFIX_PARAM, ##__VA_ARGS__)
> +#define BT_ERR(fmt, ...)       \
> +       bt_err(BT_PREFIX fmt "\n" BT_PREFIX_PARAM, ##__VA_ARGS__)
> +
> +#define BT_ERR_RATELIMITED(fmt, ...) \
> +       bt_err_ratelimited(BT_PREFIX fmt "\n" BT_PREFIX_PARAM, ##__VA_ARGS__)
> +
>  #define bt_dev_info(hdev, fmt, ...)                            \
>         BT_INFO("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
>  #define bt_dev_warn(hdev, fmt, ...)                            \
> @@ -188,7 +203,8 @@ void bt_err_ratelimited(const char *fmt, ...);
>         BT_DBG("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
>
>  #define bt_dev_warn_ratelimited(hdev, fmt, ...)                        \
> -       bt_warn_ratelimited("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
> +       bt_warn_ratelimited("%s: " BT_PREFIX fmt, (hdev)->name  \
> +                           BT_PREFIX_PARAM, ##__VA_ARGS__)
>  #define bt_dev_err_ratelimited(hdev, fmt, ...)                 \
>         bt_err_ratelimited("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
>
> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
> index 1d6d243cdde9..de31c682c7b0 100644
> --- a/net/bluetooth/Kconfig
> +++ b/net/bluetooth/Kconfig
> @@ -142,4 +142,15 @@ config BT_FEATURE_DEBUG
>           This provides an option to enable/disable debugging statements
>           at runtime via the experimental features interface.
>
> +config BT_DEBUG_FUNC_NAMES
> +       bool "Include function names in debugging statements"
> +       depends on BT
> +       default n
> +       help
> +         Provides an option to include function names in debugging
> +         statements.
> +
> +         When enabled, trace statements will include the function name as a
> +         prefix which may help identify the source code references.
> +
>  source "drivers/bluetooth/Kconfig"
> --
> 2.27.0.212.ge8ba1cc988-goog
>

Re: [PATCH v1] Bluetooth: create CONFIG_BT_DEBUG_FUNC_NAME

[Marcel Holtmann]

Hi Alain,

> Creates a CONFIG_BT_DEBUG_FUNC_NAME option to include function names in
> debug statements.
> 
> Unlike other platforms __func__ isn't a string literal so it cannot be
> automatically concatenated by the pre-processor.  As a result, the
> function name is passed as a parameter to the tracing function.  Since
> pr_debug is a function like macro, the normal expansion of BT_PREFIX_PARAM
> does not work as it gets processed within the first parameter as well,
> for this reason, BT_DBG is split into two versions.
> 
> This patch was built tested with all 4 possible combinations of
> CONFIG_BT_DEBUG_FUNC_NAME and CONFIG_BT_FEATURE_DEBUG configurations.

can we please limit this to FEATURE_DEBUG since dynamic debug doesn’t need it. It can switch on function name debugging on a per debug statement. And even for FEATURE_DEBUG I would rather have it optional that can be enabled when needed via the experimental feature itself.

Regards

Marcel

Re: [PATCH v1] Bluetooth: create CONFIG_BT_DEBUG_FUNC_NAME

[Alain Michaud]

Hi Marcel

On Tue, Jul 7, 2020 at 2:32 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> > Creates a CONFIG_BT_DEBUG_FUNC_NAME option to include function names in
> > debug statements.
> >
> > Unlike other platforms __func__ isn't a string literal so it cannot be
> > automatically concatenated by the pre-processor.  As a result, the
> > function name is passed as a parameter to the tracing function.  Since
> > pr_debug is a function like macro, the normal expansion of BT_PREFIX_PARAM
> > does not work as it gets processed within the first parameter as well,
> > for this reason, BT_DBG is split into two versions.
> >
> > This patch was built tested with all 4 possible combinations of
> > CONFIG_BT_DEBUG_FUNC_NAME and CONFIG_BT_FEATURE_DEBUG configurations.
>
> can we please limit this to FEATURE_DEBUG since dynamic debug doesn’t need it. It can switch on function name debugging on a per debug statement. And even for FEATURE_DEBUG I would rather have it optional that can be enabled when needed via the experimental feature itself.
I agree on making this dependent on FEATURE_DEBUG as it may simplify
the configuration, but I don't think I like having this enabled via an
experimental feature as it complicates the tracing macros quite a bit
for no good reason.  I don't see a scenario where someone would turn
on CONFIG_DEBUG_FUNC_NAME but not want it enabled.

>
> Regards
>
> Marcel
>

Re: [PATCH v1] Bluetooth: create CONFIG_BT_DEBUG_FUNC_NAME

[Marcel Holtmann]

Hi Alain,

>>> Creates a CONFIG_BT_DEBUG_FUNC_NAME option to include function names in
>>> debug statements.
>>> 
>>> Unlike other platforms __func__ isn't a string literal so it cannot be
>>> automatically concatenated by the pre-processor.  As a result, the
>>> function name is passed as a parameter to the tracing function.  Since
>>> pr_debug is a function like macro, the normal expansion of BT_PREFIX_PARAM
>>> does not work as it gets processed within the first parameter as well,
>>> for this reason, BT_DBG is split into two versions.
>>> 
>>> This patch was built tested with all 4 possible combinations of
>>> CONFIG_BT_DEBUG_FUNC_NAME and CONFIG_BT_FEATURE_DEBUG configurations.
>> 
>> can we please limit this to FEATURE_DEBUG since dynamic debug doesn’t need it. It can switch on function name debugging on a per debug statement. And even for FEATURE_DEBUG I would rather have it optional that can be enabled when needed via the experimental feature itself.
> I agree on making this dependent on FEATURE_DEBUG as it may simplify
> the configuration, but I don't think I like having this enabled via an
> experimental feature as it complicates the tracing macros quite a bit
> for no good reason.  I don't see a scenario where someone would turn
> on CONFIG_DEBUG_FUNC_NAME but not want it enabled.

I left the Set Experimental Feature command flexible and so you could add an extra flags/settings bitmask additionally to choose how to format the debug strings. Similar to what dynamic debug is doing.

An alternative is that you have two debug features, one for with function names and one without. If you turn on both, then the one with function names supersedes.

Regards

Marcel