Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
* [RFC] Bluetooth: Add debugfs option to enable runtime debug statements
@ 2020-01-27 16:49 Marcel Holtmann
       [not found] ` <CALWDO_Vbd=KNhjGDEiHSaSDyZh2gc+nJVSaf3ZwN4pVMjaW9sg@mail.gmail.com>
  2020-01-30 14:19 ` Alain Michaud
  0 siblings, 2 replies; 5+ messages in thread
From: Marcel Holtmann @ 2020-01-27 16:49 UTC (permalink / raw)
  To: linux-bluetooth

Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/bluetooth.h | 10 ++++-
 net/bluetooth/Kconfig             |  4 ++
 net/bluetooth/af_bluetooth.c      |  2 +
 net/bluetooth/lib.c               | 69 +++++++++++++++++++++++++++++++
 4 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index e42bb8e03c09..d32d1f401efb 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -129,6 +129,8 @@ void bt_warn(const char *fmt, ...);
 __printf(1, 2)
 void bt_err(const char *fmt, ...);
 __printf(1, 2)
+void bt_dbg(const char *fmt, ...);
+__printf(1, 2)
 void bt_warn_ratelimited(const char *fmt, ...);
 __printf(1, 2)
 void bt_err_ratelimited(const char *fmt, ...);
@@ -136,7 +138,11 @@ 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__)
-#define BT_DBG(fmt, ...)	pr_debug(fmt "\n", ##__VA_ARGS__)
+#if IS_ENABLED(CONFIG_BT_DEBUGFS_OPTION)
+#define BT_DBG(fmt, ...)	bt_dbg(fmt "\n", ##__VA_ARGS__)
+#else
+#define BT_DBG(fmt, ...)	pr_debug(mt "\n", ##__VA_ARGS__)
+#endif
 
 #define bt_dev_info(hdev, fmt, ...)				\
 	BT_INFO("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
@@ -393,6 +399,8 @@ void bt_procfs_cleanup(struct net *net, const char *name);
 
 extern struct dentry *bt_debugfs;
 
+void bt_lib_debugfs_init(void);
+
 int l2cap_init(void);
 void l2cap_exit(void);
 
diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 165148c7c4ce..64833640bf3d 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -128,4 +128,8 @@ config BT_DEBUGFS
 	  Provide extensive information about internal Bluetooth states
 	  in debugfs.
 
+config BT_DEBUGFS_OPTION
+	bool
+	default y if BT_DEBUGFS && !DYNAMIC_DEBUG
+
 source "drivers/bluetooth/Kconfig"
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 3fd124927d4d..fa0cd665f32a 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -731,6 +731,8 @@ static int __init bt_init(void)
 
 	bt_debugfs = debugfs_create_dir("bluetooth", NULL);
 
+	bt_lib_debugfs_init();
+
 	bt_leds_init();
 
 	err = bt_sysfs_init();
diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c
index c09e0a3a0ed9..28cfc3fd8fbe 100644
--- a/net/bluetooth/lib.c
+++ b/net/bluetooth/lib.c
@@ -27,6 +27,7 @@
 #define pr_fmt(fmt) "Bluetooth: " fmt
 
 #include <linux/export.h>
+#include <linux/debugfs.h>
 
 #include <net/bluetooth/bluetooth.h>
 
@@ -135,6 +136,55 @@ int bt_to_errno(__u16 code)
 }
 EXPORT_SYMBOL(bt_to_errno);
 
+#ifdef CONFIG_BT_DEBUGFS_OPTION
+static bool debug_enable;
+
+static ssize_t debug_enable_read(struct file *file, char __user *user_buf,
+				 size_t count, loff_t *ppos)
+{
+	char buf[3];
+
+	buf[0] = debug_enable ? 'Y': 'N';
+	buf[1] = '\n';
+	buf[2] = '\0';
+	return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
+}
+
+static ssize_t debug_enable_write(struct file *file,
+				  const char __user *user_buf,
+				  size_t count, loff_t *ppos)
+{
+	bool enable;
+	int err;
+
+	err = kstrtobool_from_user(user_buf, count, &enable);
+	if (err)
+		return err;
+
+	if (enable == debug_enable)
+		return -EALREADY;
+
+	debug_enable = enable;
+
+	return count;
+}
+
+static const struct file_operations debug_enable_fops = {
+	.open		= simple_open,
+	.read		= debug_enable_read,
+	.write		= debug_enable_write,
+	.llseek		= default_llseek,
+};
+
+void bt_lib_debugfs_init(void)
+{
+	debugfs_create_file("debug_enable", 0644, bt_debugfs, NULL,
+			    &debug_enable_fops);
+}
+#else
+void bt_lib_debugfs_init(void);
+#endif
+
 void bt_info(const char *format, ...)
 {
 	struct va_format vaf;
@@ -183,6 +233,25 @@ void bt_err(const char *format, ...)
 }
 EXPORT_SYMBOL(bt_err);
 
+void bt_dbg(const char *format, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	if (likely(!debug_enable))
+		return;
+
+	va_start(args, format);
+
+	vaf.fmt = format;
+	vaf.va = &args;
+
+	printk(KERN_DEBUG pr_fmt("%pV"), &vaf);
+
+	va_end(args);
+}
+EXPORT_SYMBOL(bt_dbg);
+
 void bt_warn_ratelimited(const char *format, ...)
 {
 	struct va_format vaf;
-- 
2.24.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC] Bluetooth: Add debugfs option to enable runtime debug statements
       [not found] ` <CALWDO_Vbd=KNhjGDEiHSaSDyZh2gc+nJVSaf3ZwN4pVMjaW9sg@mail.gmail.com>
@ 2020-01-29  0:50   ` Alain Michaud
  0 siblings, 0 replies; 5+ messages in thread
From: Alain Michaud @ 2020-01-29  0:50 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: BlueZ

Resending in plain text... Sorry, old dog, new tricks... (or is it the
opposite? :))


On Tue, Jan 28, 2020 at 7:48 PM Alain Michaud <alainmichaud@google.com> wrote:
>
> Hi Marcel,
>
> Just a quick ack from me.
>
> It looks good for me, but I'd like to get feedback from our team which has implemented the earlier proposal.  The team however in Taipei aren't due back in from holiday until next week, so it may take some time before we get back to you.
>
> Thanks,
> Alain
>
> On Mon, Jan 27, 2020 at 11:49 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>>
>> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>> ---
>>  include/net/bluetooth/bluetooth.h | 10 ++++-
>>  net/bluetooth/Kconfig             |  4 ++
>>  net/bluetooth/af_bluetooth.c      |  2 +
>>  net/bluetooth/lib.c               | 69 +++++++++++++++++++++++++++++++
>>  4 files changed, 84 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index e42bb8e03c09..d32d1f401efb 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -129,6 +129,8 @@ void bt_warn(const char *fmt, ...);
>>  __printf(1, 2)
>>  void bt_err(const char *fmt, ...);
>>  __printf(1, 2)
>> +void bt_dbg(const char *fmt, ...);
>> +__printf(1, 2)
>>  void bt_warn_ratelimited(const char *fmt, ...);
>>  __printf(1, 2)
>>  void bt_err_ratelimited(const char *fmt, ...);
>> @@ -136,7 +138,11 @@ 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__)
>> -#define BT_DBG(fmt, ...)       pr_debug(fmt "\n", ##__VA_ARGS__)
>> +#if IS_ENABLED(CONFIG_BT_DEBUGFS_OPTION)
>> +#define BT_DBG(fmt, ...)       bt_dbg(fmt "\n", ##__VA_ARGS__)
>> +#else
>> +#define BT_DBG(fmt, ...)       pr_debug(mt "\n", ##__VA_ARGS__)
>> +#endif
>>
>>  #define bt_dev_info(hdev, fmt, ...)                            \
>>         BT_INFO("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
>> @@ -393,6 +399,8 @@ void bt_procfs_cleanup(struct net *net, const char *name);
>>
>>  extern struct dentry *bt_debugfs;
>>
>> +void bt_lib_debugfs_init(void);
>> +
>>  int l2cap_init(void);
>>  void l2cap_exit(void);
>>
>> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
>> index 165148c7c4ce..64833640bf3d 100644
>> --- a/net/bluetooth/Kconfig
>> +++ b/net/bluetooth/Kconfig
>> @@ -128,4 +128,8 @@ config BT_DEBUGFS
>>           Provide extensive information about internal Bluetooth states
>>           in debugfs.
>>
>> +config BT_DEBUGFS_OPTION
>> +       bool
>> +       default y if BT_DEBUGFS && !DYNAMIC_DEBUG
>> +
>>  source "drivers/bluetooth/Kconfig"
>> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
>> index 3fd124927d4d..fa0cd665f32a 100644
>> --- a/net/bluetooth/af_bluetooth.c
>> +++ b/net/bluetooth/af_bluetooth.c
>> @@ -731,6 +731,8 @@ static int __init bt_init(void)
>>
>>         bt_debugfs = debugfs_create_dir("bluetooth", NULL);
>>
>> +       bt_lib_debugfs_init();
>> +
>>         bt_leds_init();
>>
>>         err = bt_sysfs_init();
>> diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c
>> index c09e0a3a0ed9..28cfc3fd8fbe 100644
>> --- a/net/bluetooth/lib.c
>> +++ b/net/bluetooth/lib.c
>> @@ -27,6 +27,7 @@
>>  #define pr_fmt(fmt) "Bluetooth: " fmt
>>
>>  #include <linux/export.h>
>> +#include <linux/debugfs.h>
>>
>>  #include <net/bluetooth/bluetooth.h>
>>
>> @@ -135,6 +136,55 @@ int bt_to_errno(__u16 code)
>>  }
>>  EXPORT_SYMBOL(bt_to_errno);
>>
>> +#ifdef CONFIG_BT_DEBUGFS_OPTION
>> +static bool debug_enable;
>> +
>> +static ssize_t debug_enable_read(struct file *file, char __user *user_buf,
>> +                                size_t count, loff_t *ppos)
>> +{
>> +       char buf[3];
>> +
>> +       buf[0] = debug_enable ? 'Y': 'N';
>> +       buf[1] = '\n';
>> +       buf[2] = '\0';
>> +       return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
>> +}
>> +
>> +static ssize_t debug_enable_write(struct file *file,
>> +                                 const char __user *user_buf,
>> +                                 size_t count, loff_t *ppos)
>> +{
>> +       bool enable;
>> +       int err;
>> +
>> +       err = kstrtobool_from_user(user_buf, count, &enable);
>> +       if (err)
>> +               return err;
>> +
>> +       if (enable == debug_enable)
>> +               return -EALREADY;
>> +
>> +       debug_enable = enable;
>> +
>> +       return count;
>> +}
>> +
>> +static const struct file_operations debug_enable_fops = {
>> +       .open           = simple_open,
>> +       .read           = debug_enable_read,
>> +       .write          = debug_enable_write,
>> +       .llseek         = default_llseek,
>> +};
>> +
>> +void bt_lib_debugfs_init(void)
>> +{
>> +       debugfs_create_file("debug_enable", 0644, bt_debugfs, NULL,
>> +                           &debug_enable_fops);
>> +}
>> +#else
>> +void bt_lib_debugfs_init(void);
>> +#endif
>> +
>>  void bt_info(const char *format, ...)
>>  {
>>         struct va_format vaf;
>> @@ -183,6 +233,25 @@ void bt_err(const char *format, ...)
>>  }
>>  EXPORT_SYMBOL(bt_err);
>>
>> +void bt_dbg(const char *format, ...)
>> +{
>> +       struct va_format vaf;
>> +       va_list args;
>> +
>> +       if (likely(!debug_enable))
>> +               return;
>> +
>> +       va_start(args, format);
>> +
>> +       vaf.fmt = format;
>> +       vaf.va = &args;
>> +
>> +       printk(KERN_DEBUG pr_fmt("%pV"), &vaf);
>> +
>> +       va_end(args);
>> +}
>> +EXPORT_SYMBOL(bt_dbg);
>> +
>>  void bt_warn_ratelimited(const char *format, ...)
>>  {
>>         struct va_format vaf;
>> --
>> 2.24.1
>>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC] Bluetooth: Add debugfs option to enable runtime debug statements
  2020-01-27 16:49 [RFC] Bluetooth: Add debugfs option to enable runtime debug statements Marcel Holtmann
       [not found] ` <CALWDO_Vbd=KNhjGDEiHSaSDyZh2gc+nJVSaf3ZwN4pVMjaW9sg@mail.gmail.com>
@ 2020-01-30 14:19 ` Alain Michaud
  2020-01-31  3:27   ` Archie Pusaka
  1 sibling, 1 reply; 5+ messages in thread
From: Alain Michaud @ 2020-01-30 14:19 UTC (permalink / raw)
  To: Marcel Holtmann, Archie Pusaka; +Cc: BlueZ

Adding +Archie Pusaka for comments.

Thanks,
Alain


On Mon, Jan 27, 2020 at 11:49 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
>  include/net/bluetooth/bluetooth.h | 10 ++++-
>  net/bluetooth/Kconfig             |  4 ++
>  net/bluetooth/af_bluetooth.c      |  2 +
>  net/bluetooth/lib.c               | 69 +++++++++++++++++++++++++++++++
>  4 files changed, 84 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index e42bb8e03c09..d32d1f401efb 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -129,6 +129,8 @@ void bt_warn(const char *fmt, ...);
>  __printf(1, 2)
>  void bt_err(const char *fmt, ...);
>  __printf(1, 2)
> +void bt_dbg(const char *fmt, ...);
> +__printf(1, 2)
>  void bt_warn_ratelimited(const char *fmt, ...);
>  __printf(1, 2)
>  void bt_err_ratelimited(const char *fmt, ...);
> @@ -136,7 +138,11 @@ 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__)
> -#define BT_DBG(fmt, ...)       pr_debug(fmt "\n", ##__VA_ARGS__)
> +#if IS_ENABLED(CONFIG_BT_DEBUGFS_OPTION)
> +#define BT_DBG(fmt, ...)       bt_dbg(fmt "\n", ##__VA_ARGS__)
> +#else
> +#define BT_DBG(fmt, ...)       pr_debug(mt "\n", ##__VA_ARGS__)
> +#endif
>
>  #define bt_dev_info(hdev, fmt, ...)                            \
>         BT_INFO("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
> @@ -393,6 +399,8 @@ void bt_procfs_cleanup(struct net *net, const char *name);
>
>  extern struct dentry *bt_debugfs;
>
> +void bt_lib_debugfs_init(void);
> +
>  int l2cap_init(void);
>  void l2cap_exit(void);
>
> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
> index 165148c7c4ce..64833640bf3d 100644
> --- a/net/bluetooth/Kconfig
> +++ b/net/bluetooth/Kconfig
> @@ -128,4 +128,8 @@ config BT_DEBUGFS
>           Provide extensive information about internal Bluetooth states
>           in debugfs.
>
> +config BT_DEBUGFS_OPTION
> +       bool
> +       default y if BT_DEBUGFS && !DYNAMIC_DEBUG
> +
>  source "drivers/bluetooth/Kconfig"
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 3fd124927d4d..fa0cd665f32a 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -731,6 +731,8 @@ static int __init bt_init(void)
>
>         bt_debugfs = debugfs_create_dir("bluetooth", NULL);
>
> +       bt_lib_debugfs_init();
> +
>         bt_leds_init();
>
>         err = bt_sysfs_init();
> diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c
> index c09e0a3a0ed9..28cfc3fd8fbe 100644
> --- a/net/bluetooth/lib.c
> +++ b/net/bluetooth/lib.c
> @@ -27,6 +27,7 @@
>  #define pr_fmt(fmt) "Bluetooth: " fmt
>
>  #include <linux/export.h>
> +#include <linux/debugfs.h>
>
>  #include <net/bluetooth/bluetooth.h>
>
> @@ -135,6 +136,55 @@ int bt_to_errno(__u16 code)
>  }
>  EXPORT_SYMBOL(bt_to_errno);
>
> +#ifdef CONFIG_BT_DEBUGFS_OPTION
> +static bool debug_enable;
> +
> +static ssize_t debug_enable_read(struct file *file, char __user *user_buf,
> +                                size_t count, loff_t *ppos)
> +{
> +       char buf[3];
> +
> +       buf[0] = debug_enable ? 'Y': 'N';
> +       buf[1] = '\n';
> +       buf[2] = '\0';
> +       return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
> +}
> +
> +static ssize_t debug_enable_write(struct file *file,
> +                                 const char __user *user_buf,
> +                                 size_t count, loff_t *ppos)
> +{
> +       bool enable;
> +       int err;
> +
> +       err = kstrtobool_from_user(user_buf, count, &enable);
> +       if (err)
> +               return err;
> +
> +       if (enable == debug_enable)
> +               return -EALREADY;
> +
> +       debug_enable = enable;
> +
> +       return count;
> +}
> +
> +static const struct file_operations debug_enable_fops = {
> +       .open           = simple_open,
> +       .read           = debug_enable_read,
> +       .write          = debug_enable_write,
> +       .llseek         = default_llseek,
> +};
> +
> +void bt_lib_debugfs_init(void)
> +{
> +       debugfs_create_file("debug_enable", 0644, bt_debugfs, NULL,
> +                           &debug_enable_fops);
> +}
> +#else
> +void bt_lib_debugfs_init(void);
> +#endif
> +
>  void bt_info(const char *format, ...)
>  {
>         struct va_format vaf;
> @@ -183,6 +233,25 @@ void bt_err(const char *format, ...)
>  }
>  EXPORT_SYMBOL(bt_err);
>
> +void bt_dbg(const char *format, ...)
> +{
> +       struct va_format vaf;
> +       va_list args;
> +
> +       if (likely(!debug_enable))
> +               return;
> +
> +       va_start(args, format);
> +
> +       vaf.fmt = format;
> +       vaf.va = &args;
> +
> +       printk(KERN_DEBUG pr_fmt("%pV"), &vaf);
> +
> +       va_end(args);
> +}
> +EXPORT_SYMBOL(bt_dbg);
> +
>  void bt_warn_ratelimited(const char *format, ...)
>  {
>         struct va_format vaf;
> --
> 2.24.1
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC] Bluetooth: Add debugfs option to enable runtime debug statements
  2020-01-30 14:19 ` Alain Michaud
@ 2020-01-31  3:27   ` Archie Pusaka
  2020-01-31 15:17     ` Marcel Holtmann
  0 siblings, 1 reply; 5+ messages in thread
From: Archie Pusaka @ 2020-01-31  3:27 UTC (permalink / raw)
  To: Alain Michaud; +Cc: Marcel Holtmann, BlueZ

Hi Marcel,

Thanks for the fantastic patch! My comments inline.

>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
>  include/net/bluetooth/bluetooth.h | 10 ++++-
>  net/bluetooth/Kconfig             |  4 ++
>  net/bluetooth/af_bluetooth.c      |  2 +
>  net/bluetooth/lib.c               | 69 +++++++++++++++++++++++++++++++
>  4 files changed, 84 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index e42bb8e03c09..d32d1f401efb 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -129,6 +129,8 @@ void bt_warn(const char *fmt, ...);
>  __printf(1, 2)
>  void bt_err(const char *fmt, ...);
>  __printf(1, 2)
> +void bt_dbg(const char *fmt, ...);
> +__printf(1, 2)
>  void bt_warn_ratelimited(const char *fmt, ...);
>  __printf(1, 2)
>  void bt_err_ratelimited(const char *fmt, ...);
> @@ -136,7 +138,11 @@ 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__)
> -#define BT_DBG(fmt, ...)       pr_debug(fmt "\n", ##__VA_ARGS__)
> +#if IS_ENABLED(CONFIG_BT_DEBUGFS_OPTION)
> +#define BT_DBG(fmt, ...)       bt_dbg(fmt "\n", ##__VA_ARGS__)
> +#else
> +#define BT_DBG(fmt, ...)       pr_debug(mt "\n", ##__VA_ARGS__)

Should be fmt instead of mt.

> +#endif
>
>  #define bt_dev_info(hdev, fmt, ...)                            \
>         BT_INFO("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
> @@ -393,6 +399,8 @@ void bt_procfs_cleanup(struct net *net, const char *name);
>
>  extern struct dentry *bt_debugfs;
>
> +void bt_lib_debugfs_init(void);
> +
>  int l2cap_init(void);
>  void l2cap_exit(void);
>
> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
> index 165148c7c4ce..64833640bf3d 100644
> --- a/net/bluetooth/Kconfig
> +++ b/net/bluetooth/Kconfig
> @@ -128,4 +128,8 @@ config BT_DEBUGFS
>           Provide extensive information about internal Bluetooth states
>           in debugfs.
>
> +config BT_DEBUGFS_OPTION
> +       bool
> +       default y if BT_DEBUGFS && !DYNAMIC_DEBUG

If I understand it correctly, setting BT_DEBUGFS_OPTION to true
will cause all of the BT_DBG logs to be loaded into the resulting
build. Prior to the introduction of this config setting, the logs by
default are discarded from the build. Therefore, by introducing this
option, there will be an increase in the size of the build.

Shouldn't this option defaults to false instead?

> +
>  source "drivers/bluetooth/Kconfig"
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 3fd124927d4d..fa0cd665f32a 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -731,6 +731,8 @@ static int __init bt_init(void)
>
>         bt_debugfs = debugfs_create_dir("bluetooth", NULL);
>
> +       bt_lib_debugfs_init();
> +
>         bt_leds_init();
>
>         err = bt_sysfs_init();
> diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c
> index c09e0a3a0ed9..28cfc3fd8fbe 100644
> --- a/net/bluetooth/lib.c
> +++ b/net/bluetooth/lib.c
> @@ -27,6 +27,7 @@
>  #define pr_fmt(fmt) "Bluetooth: " fmt
>
>  #include <linux/export.h>
> +#include <linux/debugfs.h>
>
>  #include <net/bluetooth/bluetooth.h>
>
> @@ -135,6 +136,55 @@ int bt_to_errno(__u16 code)
>  }
>  EXPORT_SYMBOL(bt_to_errno);
>
> +#ifdef CONFIG_BT_DEBUGFS_OPTION
> +static bool debug_enable;
> +
> +static ssize_t debug_enable_read(struct file *file, char __user *user_buf,
> +                                size_t count, loff_t *ppos)
> +{
> +       char buf[3];
> +
> +       buf[0] = debug_enable ? 'Y': 'N';
> +       buf[1] = '\n';
> +       buf[2] = '\0';
> +       return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
> +}
> +
> +static ssize_t debug_enable_write(struct file *file,
> +                                 const char __user *user_buf,
> +                                 size_t count, loff_t *ppos)
> +{
> +       bool enable;
> +       int err;
> +
> +       err = kstrtobool_from_user(user_buf, count, &enable);
> +       if (err)
> +               return err;
> +
> +       if (enable == debug_enable)
> +               return -EALREADY;
> +
> +       debug_enable = enable;
> +
> +       return count;
> +}
> +
> +static const struct file_operations debug_enable_fops = {
> +       .open           = simple_open,
> +       .read           = debug_enable_read,
> +       .write          = debug_enable_write,
> +       .llseek         = default_llseek,
> +};
> +
> +void bt_lib_debugfs_init(void)
> +{
> +       debugfs_create_file("debug_enable", 0644, bt_debugfs, NULL,
> +                           &debug_enable_fops);
> +}
> +#else
> +void bt_lib_debugfs_init(void);

Should be the empty function
bt_lib_debugfs_init(void)
{
}

> +#endif
> +
>  void bt_info(const char *format, ...)
>  {
>         struct va_format vaf;
> @@ -183,6 +233,25 @@ void bt_err(const char *format, ...)
>  }
>  EXPORT_SYMBOL(bt_err);
>
> +void bt_dbg(const char *format, ...)
> +{
> +       struct va_format vaf;
> +       va_list args;
> +
> +       if (likely(!debug_enable))
> +               return;
> +
> +       va_start(args, format);
> +
> +       vaf.fmt = format;
> +       vaf.va = &args;
> +
> +       printk(KERN_DEBUG pr_fmt("%pV"), &vaf);
> +
> +       va_end(args);
> +}
> +EXPORT_SYMBOL(bt_dbg);
> +
>  void bt_warn_ratelimited(const char *format, ...)
>  {
>         struct va_format vaf;
> --
> 2.24.1
>

Thanks,
Archie

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC] Bluetooth: Add debugfs option to enable runtime debug statements
  2020-01-31  3:27   ` Archie Pusaka
@ 2020-01-31 15:17     ` Marcel Holtmann
  0 siblings, 0 replies; 5+ messages in thread
From: Marcel Holtmann @ 2020-01-31 15:17 UTC (permalink / raw)
  To: Archie Pusaka; +Cc: Alain Michaud, BlueZ

Hi Archie,

> Thanks for the fantastic patch! My comments inline.
> 
>> 
>> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>> ---
>> include/net/bluetooth/bluetooth.h | 10 ++++-
>> net/bluetooth/Kconfig             |  4 ++
>> net/bluetooth/af_bluetooth.c      |  2 +
>> net/bluetooth/lib.c               | 69 +++++++++++++++++++++++++++++++
>> 4 files changed, 84 insertions(+), 1 deletion(-)
>> 
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index e42bb8e03c09..d32d1f401efb 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -129,6 +129,8 @@ void bt_warn(const char *fmt, ...);
>> __printf(1, 2)
>> void bt_err(const char *fmt, ...);
>> __printf(1, 2)
>> +void bt_dbg(const char *fmt, ...);
>> +__printf(1, 2)
>> void bt_warn_ratelimited(const char *fmt, ...);
>> __printf(1, 2)
>> void bt_err_ratelimited(const char *fmt, ...);
>> @@ -136,7 +138,11 @@ 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__)
>> -#define BT_DBG(fmt, ...)       pr_debug(fmt "\n", ##__VA_ARGS__)
>> +#if IS_ENABLED(CONFIG_BT_DEBUGFS_OPTION)
>> +#define BT_DBG(fmt, ...)       bt_dbg(fmt "\n", ##__VA_ARGS__)
>> +#else
>> +#define BT_DBG(fmt, ...)       pr_debug(mt "\n", ##__VA_ARGS__)
> 
> Should be fmt instead of mt.

Indeed.

> 
>> +#endif
>> 
>> #define bt_dev_info(hdev, fmt, ...)                            \
>>        BT_INFO("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
>> @@ -393,6 +399,8 @@ void bt_procfs_cleanup(struct net *net, const char *name);
>> 
>> extern struct dentry *bt_debugfs;
>> 
>> +void bt_lib_debugfs_init(void);
>> +
>> int l2cap_init(void);
>> void l2cap_exit(void);
>> 
>> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
>> index 165148c7c4ce..64833640bf3d 100644
>> --- a/net/bluetooth/Kconfig
>> +++ b/net/bluetooth/Kconfig
>> @@ -128,4 +128,8 @@ config BT_DEBUGFS
>>          Provide extensive information about internal Bluetooth states
>>          in debugfs.
>> 
>> +config BT_DEBUGFS_OPTION
>> +       bool
>> +       default y if BT_DEBUGFS && !DYNAMIC_DEBUG
> 
> If I understand it correctly, setting BT_DEBUGFS_OPTION to true
> will cause all of the BT_DBG logs to be loaded into the resulting
> build. Prior to the introduction of this config setting, the logs by
> default are discarded from the build. Therefore, by introducing this
> option, there will be an increase in the size of the build.
> 
> Shouldn't this option defaults to false instead?

It defaults to y only if you a) selected BT_DEBUGFS and b) have disabled DYNAMIC_DEBUG. Otherwise it is off and defaults back to using pr_debug. Which is in case of disabled DYNAMIC_DEBUG results in disabled debug statement.

So it is not worth than with dynamic debug. We could tweak the bt_dbg part a bit to reduce the overhead, but it is not as bad. However what we can add is an extra comment in the BT_DEBUGFS help text to mention that case with DYNAMIC_DEBUG disabled.

>> +
>> source "drivers/bluetooth/Kconfig"
>> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
>> index 3fd124927d4d..fa0cd665f32a 100644
>> --- a/net/bluetooth/af_bluetooth.c
>> +++ b/net/bluetooth/af_bluetooth.c
>> @@ -731,6 +731,8 @@ static int __init bt_init(void)
>> 
>>        bt_debugfs = debugfs_create_dir("bluetooth", NULL);
>> 
>> +       bt_lib_debugfs_init();
>> +
>>        bt_leds_init();
>> 
>>        err = bt_sysfs_init();
>> diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c
>> index c09e0a3a0ed9..28cfc3fd8fbe 100644
>> --- a/net/bluetooth/lib.c
>> +++ b/net/bluetooth/lib.c
>> @@ -27,6 +27,7 @@
>> #define pr_fmt(fmt) "Bluetooth: " fmt
>> 
>> #include <linux/export.h>
>> +#include <linux/debugfs.h>
>> 
>> #include <net/bluetooth/bluetooth.h>
>> 
>> @@ -135,6 +136,55 @@ int bt_to_errno(__u16 code)
>> }
>> EXPORT_SYMBOL(bt_to_errno);
>> 
>> +#ifdef CONFIG_BT_DEBUGFS_OPTION
>> +static bool debug_enable;
>> +
>> +static ssize_t debug_enable_read(struct file *file, char __user *user_buf,
>> +                                size_t count, loff_t *ppos)
>> +{
>> +       char buf[3];
>> +
>> +       buf[0] = debug_enable ? 'Y': 'N';
>> +       buf[1] = '\n';
>> +       buf[2] = '\0';
>> +       return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
>> +}
>> +
>> +static ssize_t debug_enable_write(struct file *file,
>> +                                 const char __user *user_buf,
>> +                                 size_t count, loff_t *ppos)
>> +{
>> +       bool enable;
>> +       int err;
>> +
>> +       err = kstrtobool_from_user(user_buf, count, &enable);
>> +       if (err)
>> +               return err;
>> +
>> +       if (enable == debug_enable)
>> +               return -EALREADY;
>> +
>> +       debug_enable = enable;
>> +
>> +       return count;
>> +}
>> +
>> +static const struct file_operations debug_enable_fops = {
>> +       .open           = simple_open,
>> +       .read           = debug_enable_read,
>> +       .write          = debug_enable_write,
>> +       .llseek         = default_llseek,
>> +};
>> +
>> +void bt_lib_debugfs_init(void)
>> +{
>> +       debugfs_create_file("debug_enable", 0644, bt_debugfs, NULL,
>> +                           &debug_enable_fops);
>> +}
>> +#else
>> +void bt_lib_debugfs_init(void);
> 
> Should be the empty function
> bt_lib_debugfs_init(void)
> {
> }
> 

Correct. I screwed this up.

Regards

Marcel


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-27 16:49 [RFC] Bluetooth: Add debugfs option to enable runtime debug statements Marcel Holtmann
     [not found] ` <CALWDO_Vbd=KNhjGDEiHSaSDyZh2gc+nJVSaf3ZwN4pVMjaW9sg@mail.gmail.com>
2020-01-29  0:50   ` Alain Michaud
2020-01-30 14:19 ` Alain Michaud
2020-01-31  3:27   ` Archie Pusaka
2020-01-31 15:17     ` Marcel Holtmann

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org
	public-inbox-index linux-bluetooth

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git