| Message ID | DB3PR10MB6835DE6D279B65EC040B92AEE8AAA@DB3PR10MB6835.EURPRD10.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: Add documentation and replace printk calls | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
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
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`.
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 --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); }
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(-)