diff mbox series

[v2] Bluetooth: create CONFIG_BT_DEBUG_FEATURE_FUNC_NAME

Message ID 20200707141628.368748-1-alainm@chromium.org (mailing list archive)
State Accepted
Headers show
Series [v2] Bluetooth: create CONFIG_BT_DEBUG_FEATURE_FUNC_NAME | expand

Commit Message

Alain Michaud July 7, 2020, 2:16 p.m. UTC
Creates a CONFIG_BT_DEBUG_FEATURE_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: Archie Pusaka <apusaka@chromium.org>
---

Changes in v2:
 - Making CONFIG_BT_DEBUG_FEATURE_FUNC_NAME dependent on
 CONFIG_BT_DEBUG_FEATURE

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

Comments

Marcel Holtmann July 7, 2020, 3:53 p.m. UTC | #1
Hi Alain,

> Creates a CONFIG_BT_DEBUG_FEATURE_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: Archie Pusaka <apusaka@chromium.org>
> ---
> 
> Changes in v2:
> - Making CONFIG_BT_DEBUG_FEATURE_FUNC_NAME dependent on
> CONFIG_BT_DEBUG_FEATURE
> 
> 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..8506dd268d4b 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(BT_FEATURE_DEBUG_FUNC_NAMES)

are you sure you tested this?

And frankly I don’t get the point for the new Kconfig option. It is rather useless in this patch. Tell me one thing, do you prefer that FEATURE_DEBUG prints the function names or not. Because if dynamic debug is used, we don’t need it since that is all configurable via dynamic debug itself and we don’t need it there (and I also don’t want it in the dynamic debug case).

Regards

Marcel
Alain Michaud July 7, 2020, 5:15 p.m. UTC | #2
On Tue, Jul 7, 2020 at 11:53 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> > Creates a CONFIG_BT_DEBUG_FEATURE_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: Archie Pusaka <apusaka@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Making CONFIG_BT_DEBUG_FEATURE_FUNC_NAME dependent on
> > CONFIG_BT_DEBUG_FEATURE
> >
> > 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..8506dd268d4b 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(BT_FEATURE_DEBUG_FUNC_NAMES)
>
> are you sure you tested this?

This was tested on chromeos using this patch chain which also sets the
configuration and revert our original patch:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2279613

>
>
> And frankly I don’t get the point for the new Kconfig option. It is rather useless in this patch. Tell me one thing, do you prefer that FEATURE_DEBUG prints the function names or not. Because if dynamic debug is used, we don’t need it since that is all configurable via dynamic debug itself and we don’t need it there (and I also don’t want it in the dynamic debug case).
I'd be ok if we unconditionally print function names if
CONFIG_BT_FEATURE_DEBUG is set.  what I want to avoid is to overly
complicate the debug macros to have the function names configurable at
runtime.

>
> Regards
>
> Marcel
>
Alain Michaud July 7, 2020, 8:02 p.m. UTC | #3
On Tue, Jul 7, 2020 at 1:15 PM Alain Michaud <alainmichaud@google.com> wrote:
>
> On Tue, Jul 7, 2020 at 11:53 AM Marcel Holtmann <marcel@holtmann.org> wrote:
> >
> > Hi Alain,
> >
> > > Creates a CONFIG_BT_DEBUG_FEATURE_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: Archie Pusaka <apusaka@chromium.org>
> > > ---
> > >
> > > Changes in v2:
> > > - Making CONFIG_BT_DEBUG_FEATURE_FUNC_NAME dependent on
> > > CONFIG_BT_DEBUG_FEATURE
> > >
> > > 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..8506dd268d4b 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(BT_FEATURE_DEBUG_FUNC_NAMES)
> >
> > are you sure you tested this?
>
> This was tested on chromeos using this patch chain which also sets the
> configuration and revert our original patch:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2279613
(facepalm) You were right, I introduced this bug in v2's rename.  This
is now fixed in V3.
>
> >
> >
> > And frankly I don’t get the point for the new Kconfig option. It is rather useless in this patch. Tell me one thing, do you prefer that FEATURE_DEBUG prints the function names or not. Because if dynamic debug is used, we don’t need it since that is all configurable via dynamic debug itself and we don’t need it there (and I also don’t want it in the dynamic debug case).
> I'd be ok if we unconditionally print function names if
> CONFIG_BT_FEATURE_DEBUG is set.  what I want to avoid is to overly
> complicate the debug macros to have the function names configurable at
> runtime.
>
> >
> > Regards
> >
> > Marcel
> >
diff mbox series

Patch

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 7ee8041af803..8506dd268d4b 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(BT_FEATURE_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..ccbf762053f1 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_FEATURE_DEBUG_FUNC_NAMES
+	bool "Include function names in debugging statements"
+	depends on BT_FEATURE_DEBUG
+	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"