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>
 <CALWDO_UDxA3maSxKr_wc_VKo3bXuT+GtSmSrMAJ5u4640cyKxw@mail.gmail.com>
In-Reply-To: <CALWDO_UDxA3maSxKr_wc_VKo3bXuT+GtSmSrMAJ5u4640cyKxw@mail.gmail.com>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Tue, 7 Jul 2020 16:02:21 -0400
Message-ID: <CALWDO_VMohmimfoZdPR4w5heFJLERa7tNHTQD9qPMYpCkoemmg@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 1:15 PM Alain Michaud <alainmichaud@google.com> wrot=
e:
>
> On Tue, Jul 7, 2020 at 11:53 AM Marcel Holtmann <marcel@holtmann.org> wro=
te:
> >
> > Hi Alain,
> >
> > > Creates a CONFIG_BT_DEBUG_FEATURE_FUNC_NAME option to include functio=
n names in
> > > debug statements.
> > >
> > > Unlike other platforms __func__ isn't a string literal so it cannot b=
e
> > > automatically concatenated by the pre-processor.  As a result, the
> > > function name is passed as a parameter to the tracing function.  Sinc=
e
> > > 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/bluetoot=
h/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=E2=80=99t get the point for the new Kconfig option. I=
t is rather useless in this patch. Tell me one thing, do you prefer that FE=
ATURE_DEBUG prints the function names or not. Because if dynamic debug is u=
sed, we don=E2=80=99t need it since that is all configurable via dynamic de=
bug itself and we don=E2=80=99t need it there (and I also don=E2=80=99t wan=
t 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
> >