diff mbox series

[2/2] Bluetooth: Replaces printk with pr_debug in bt_dbg

Message ID DB3PR10MB6835DE6D279B65EC040B92AEE8AAA@DB3PR10MB6835.EURPRD10.PROD.OUTLOOK.COM
State New
Headers show
Series Bluetooth: Add documentation and replace printk calls | expand

Commit Message

Yuran Pereira Nov. 6, 2023, 10:26 p.m. UTC
bt_dbg() uses printk, as opposed to other functions in this file
which use pr_* family of logging functions.

This patch changes that by replacing `printk(KERN_DEBUG` with
the equivalent pr_debug() call which makes the overall file
look more uniform and cleaner.

Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
---
 net/bluetooth/lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Greg KH Nov. 7, 2023, 6:31 a.m. UTC | #1
On Tue, Nov 07, 2023 at 03:56:08AM +0530, Yuran Pereira wrote:
> bt_dbg() uses printk, as opposed to other functions in this file
> which use pr_* family of logging functions.
> 
> This patch changes that by replacing `printk(KERN_DEBUG` with
> the equivalent pr_debug() call which makes the overall file
> look more uniform and cleaner.
> 
> Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
> ---
>  net/bluetooth/lib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c
> index 063032fe9c68..96ba39f8b461 100644
> --- a/net/bluetooth/lib.c
> +++ b/net/bluetooth/lib.c
> @@ -329,7 +329,7 @@ void bt_dbg(const char *format, ...)
>  	vaf.fmt = format;
>  	vaf.va = &args;
>  
> -	printk(KERN_DEBUG pr_fmt("%pV"), &vaf);
> +	pr_debug("%pV", &vaf);

You might have just changed the functionality here, are you SURE this is
identical to the original code?  How was it tested?

I'm not saying this is a bad idea to do, just be aware of the
consequences for this change and document it properly (hint, the
changelog does not document the user-visible change that just happened.)

Note, pr_debug() is NOT identical to printk(), look at the source for
the full details.

thanks,

greg k-h
Yuran Pereira Nov. 7, 2023, 4:02 p.m. UTC | #2
Hello Greg,
On Tue, Nov 07, 2023 at 07:31:27AM +0100, Greg KH wrote:
> 
> You might have just changed the functionality here, are you SURE this is
> identical to the original code?  How was it tested?
> 
> I'm not saying this is a bad idea to do, just be aware of the
> consequences for this change and document it properly (hint, the
> changelog does not document the user-visible change that just happened.)
> 
> Note, pr_debug() is NOT identical to printk(), look at the source for
> the full details.
> 

Thank you for the heads-up. 
Yes, you're right.

I just took another look and it seems that using pr_debug here
does defeat the purpose of bt_dbg which was created for situations
where `DYNAMIC_DEBUG` and `DEBUG` are disabled.

The likely equivalent would have been `pr_devel` but that also
depends on `DEBUG`.

Do you think that a new `pr_devel_uncond` like the one below
(only to be used in exceptional scenarios) would be a good idea?

```
#define pr_devel_uncond(fmt, ...) \
    printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
```

This would neither depend on `DYNAMIC_DEBUG` nor on `DEBUG`.
Greg KH Nov. 7, 2023, 6:13 p.m. UTC | #3
On Tue, Nov 07, 2023 at 09:32:51PM +0530, Yuran Pereira wrote:
> Hello Greg,
> On Tue, Nov 07, 2023 at 07:31:27AM +0100, Greg KH wrote:
> > 
> > You might have just changed the functionality here, are you SURE this is
> > identical to the original code?  How was it tested?
> > 
> > I'm not saying this is a bad idea to do, just be aware of the
> > consequences for this change and document it properly (hint, the
> > changelog does not document the user-visible change that just happened.)
> > 
> > Note, pr_debug() is NOT identical to printk(), look at the source for
> > the full details.
> > 
> 
> Thank you for the heads-up. 
> Yes, you're right.
> 
> I just took another look and it seems that using pr_debug here
> does defeat the purpose of bt_dbg which was created for situations
> where `DYNAMIC_DEBUG` and `DEBUG` are disabled.
> 
> The likely equivalent would have been `pr_devel` but that also
> depends on `DEBUG`.
> 
> Do you think that a new `pr_devel_uncond` like the one below
> (only to be used in exceptional scenarios) would be a good idea?
> 
> ```
> #define pr_devel_uncond(fmt, ...) \
>     printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> ```
> 
> This would neither depend on `DYNAMIC_DEBUG` nor on `DEBUG`.

No, not at all, the bluetooth subsystem should move to actually use the
proper dynamic debug infrastructure and not have their own "special"
subsystem loging macros/functions.  What you are doing here is the
proper way forward, BUT you need to make everyone aware that it is going
to change how things work from what they do today.

In other words, it's not just a "trivial" change, you need to get
approval to change this type of functionality from the Bluetooth
developers/maintainers.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c
index 063032fe9c68..96ba39f8b461 100644
--- a/net/bluetooth/lib.c
+++ b/net/bluetooth/lib.c
@@ -329,7 +329,7 @@  void bt_dbg(const char *format, ...)
 	vaf.fmt = format;
 	vaf.va = &args;
 
-	printk(KERN_DEBUG pr_fmt("%pV"), &vaf);
+	pr_debug("%pV", &vaf);
 
 	va_end(args);
 }