Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200707141628.368748-1-alainm@chromium.org> <67268C4E-674E-4DE9-B642-E1CAE8748E28@holtmann.org>
In-Reply-To: <67268C4E-674E-4DE9-B642-E1CAE8748E28@holtmann.org>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Tue, 7 Jul 2020 13:15:32 -0400
Message-ID: <CALWDO_UDxA3maSxKr_wc_VKo3bXuT+GtSmSrMAJ5u4640cyKxw@mail.gmail.com>
Subject: Re: [PATCH v2] Bluetooth: create CONFIG_BT_DEBUG_FEATURE_FUNC_NAME
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Alain Michaud <alainm@chromium.org>,
        BlueZ <linux-bluetooth@vger.kernel.org>,
        Archie Pusaka <apusaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

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_PA=
RAM
> > 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=E2=80=99t get the point for the new Kconfig option. It =
is rather useless in this patch. Tell me one thing, do you prefer that FEAT=
URE_DEBUG prints the function names or not. Because if dynamic debug is use=
d, we don=E2=80=99t need it since that is all configurable via dynamic debu=
g itself and we don=E2=80=99t need it there (and I also don=E2=80=99t 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
>