[PATCH option A] bcma: use custom printing functions

[PATCH option A] bcma: use custom printing functions

[Rafał Miłecki]

Having bus number printed makes it much easier to anaylze logs on
systems with more buses. For example Netgear WNDR4500 has 3 AMBA buses
in total, which makes standard log really messy.
---
This patch (option A) keeps using pr_<level> in custom functions.

Pros: We use prefered functions (pr_<level>) in custom functions
Cons: Sometimes we may need to call pr_<level> directly. We won't get
	messages prefixed then (because of not setting pr_fmt).
---
 drivers/bcma/bcma_private.h |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/bcma/bcma_private.h b/drivers/bcma/bcma_private.h
index b81755b..1f638c5 100644
--- a/drivers/bcma/bcma_private.h
+++ b/drivers/bcma/bcma_private.h
@@ -1,10 +1,6 @@
 #ifndef LINUX_BCMA_PRIVATE_H_
 #define LINUX_BCMA_PRIVATE_H_
 
-#ifndef pr_fmt
-#define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
-#endif
-
 #include <linux/bcma/bcma.h>
 #include <linux/delay.h>
 
@@ -12,6 +8,11 @@
 
 struct bcma_bus;
 
+#define bcma_err(fmt, ...) \
+	pr_err(KBUILD_MODNAME "-%d: " fmt, bus->num, ##__VA_ARGS__)
+#define bcma_info(fmt, ...) \
+	pr_info(KBUILD_MODNAME "-%d: " fmt, bus->num, ##__VA_ARGS__)
+
 /* main.c */
 int __devinit bcma_bus_register(struct bcma_bus *bus);
 void bcma_bus_unregister(struct bcma_bus *bus);
-- 
1.7.7

Re: [PATCH option A] bcma: use custom printing functions

[Johannes Berg]

On Thu, 2012-06-28 at 21:44 +0200, Rafał Miłecki wrote:

> +#define bcma_err(fmt, ...) \
> +	pr_err(KBUILD_MODNAME "-%d: " fmt, bus->num, ##__VA_ARGS__)

both of your options seem to rely on "bus" being a variable in the
context, is that really a good idea?

johannes

Re: [PATCH option A] bcma: use custom printing functions

[Rafał Miłecki]

2012/6/28 Johannes Berg <johannes@sipsolutions.net>:
> On Thu, 2012-06-28 at 21:44 +0200, Rafał Miłecki wrote:
>
>> +#define bcma_err(fmt, ...) \
>> +     pr_err(KBUILD_MODNAME "-%d: " fmt, bus->num, ##__VA_ARGS__)
>
> both of your options seem to rely on "bus" being a variable in the
> context, is that really a good idea?

Yeah, I made that assumption to make calls nicer & shorter. We may
need to get reference to "bus" in function or two.

I saw such a solution in "radeon" gpu driver, example:
value = RREG32(R600_AUDIO_STATUS_BITS);
(they assume "rdev" is available in every function calling RREG32).

If you believe it's ugly, I can change that. I also wonder what Joe
will respond.

P.S.
Both patches are not signed yet and they are supposed to be RFC. Sorry
for missing that in subject line.

-- 
Rafał

Re: [PATCH option A] bcma: use custom printing functions

[Joe Perches]

On Thu, 2012-06-28 at 21:56 +0200, Rafał Miłecki wrote:
> 2012/6/28 Johannes Berg <johannes@sipsolutions.net>:
> > On Thu, 2012-06-28 at 21:44 +0200, Rafał Miłecki wrote:
> >
> >> +#define bcma_err(fmt, ...) \
> >> +     pr_err(KBUILD_MODNAME "-%d: " fmt, bus->num, ##__VA_ARGS__)
> >
> > both of your options seem to rely on "bus" being a variable in the
> > context, is that really a good idea?
> 
> Yeah, I made that assumption to make calls nicer & shorter. We may
> need to get reference to "bus" in function or two.
> 
> I saw such a solution in "radeon" gpu driver, example:
> value = RREG32(R600_AUDIO_STATUS_BITS);
> (they assume "rdev" is available in every function calling RREG32).

I think that radeon use is ugly myself.

> If you believe it's ugly, I can change that. I also wonder what Joe
> will respond.
> 
> P.S.
> Both patches are not signed yet and they are supposed to be RFC. Sorry
> for missing that in subject line.

I think it'd be better to add and use:

#define bcma_bus_err(bus, fmt, ...)				\
	pr_err("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)

#define bcma_bus_info(bus, fmt, ...)				\
	pr_info("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)

or some other equivalent use if you're wedded
to wanting "bcma-%d:" prefixed output.

I'd rather have the prefix be something like "bcma: <bus#>: ",
so a dmesg grep pattern can be "^bcma:" but hey, it ain't my code.

Do what you think best.

cheers, Joe

Re: [PATCH option A] bcma: use custom printing functions

[Rafał Miłecki]

2012/6/28 Joe Perches <joe@perches.com>:
> On Thu, 2012-06-28 at 21:56 +0200, Rafał Miłecki wrote:
>> 2012/6/28 Johannes Berg <johannes@sipsolutions.net>:
>> > On Thu, 2012-06-28 at 21:44 +0200, Rafał Miłecki wrote:
>> >
>> >> +#define bcma_err(fmt, ...) \
>> >> +     pr_err(KBUILD_MODNAME "-%d: " fmt, bus->num, ##__VA_ARGS__)
>> >
>> > both of your options seem to rely on "bus" being a variable in the
>> > context, is that really a good idea?
>>
>> Yeah, I made that assumption to make calls nicer & shorter. We may
>> need to get reference to "bus" in function or two.
>>
>> I saw such a solution in "radeon" gpu driver, example:
>> value = RREG32(R600_AUDIO_STATUS_BITS);
>> (they assume "rdev" is available in every function calling RREG32).
>
> I think that radeon use is ugly myself.
>
>> If you believe it's ugly, I can change that. I also wonder what Joe
>> will respond.
>>
>> P.S.
>> Both patches are not signed yet and they are supposed to be RFC. Sorry
>> for missing that in subject line.
>
> I think it'd be better to add and use:
>
> #define bcma_bus_err(bus, fmt, ...)                             \
>        pr_err("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)
>
> #define bcma_bus_info(bus, fmt, ...)                            \
>        pr_info("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)
>
> or some other equivalent use if you're wedded
> to wanting "bcma-%d:" prefixed output.
>
> I'd rather have the prefix be something like "bcma: <bus#>: ",
> so a dmesg grep pattern can be "^bcma:" but hey, it ain't my code.

OK, thanks for your opinion. Just to be sure, did you mean:
bcma: bus0: FOO BAR
or
bcma: <bus0>: FOO BAR
?

Personally I don't really care, but maybe there is already similar
case in some other driver you know about? It could be nice to be
consistent across the kernel.

-- 
Rafał

Re: [PATCH option A] bcma: use custom printing functions

[Hauke Mehrtens]

On 06/28/2012 10:18 PM, Rafał Miłecki wrote:
> 2012/6/28 Joe Perches <joe@perches.com>:
>> On Thu, 2012-06-28 at 21:56 +0200, Rafał Miłecki wrote:
>>> 2012/6/28 Johannes Berg <johannes@sipsolutions.net>:
>>>> On Thu, 2012-06-28 at 21:44 +0200, Rafał Miłecki wrote:
>>>>
>>>>> +#define bcma_err(fmt, ...) \
>>>>> +     pr_err(KBUILD_MODNAME "-%d: " fmt, bus->num, ##__VA_ARGS__)
>>>>
>>>> both of your options seem to rely on "bus" being a variable in the
>>>> context, is that really a good idea?
>>>
>>> Yeah, I made that assumption to make calls nicer & shorter. We may
>>> need to get reference to "bus" in function or two.
>>>
>>> I saw such a solution in "radeon" gpu driver, example:
>>> value = RREG32(R600_AUDIO_STATUS_BITS);
>>> (they assume "rdev" is available in every function calling RREG32).
>>
>> I think that radeon use is ugly myself.
>>
>>> If you believe it's ugly, I can change that. I also wonder what Joe
>>> will respond.
>>>
>>> P.S.
>>> Both patches are not signed yet and they are supposed to be RFC. Sorry
>>> for missing that in subject line.
>>
>> I think it'd be better to add and use:
>>
>> #define bcma_bus_err(bus, fmt, ...)                             \
>>        pr_err("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)
>>
>> #define bcma_bus_info(bus, fmt, ...)                            \
>>        pr_info("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)
>>
>> or some other equivalent use if you're wedded
>> to wanting "bcma-%d:" prefixed output.
>>
>> I'd rather have the prefix be something like "bcma: <bus#>: ",
>> so a dmesg grep pattern can be "^bcma:" but hey, it ain't my code.
> 
> OK, thanks for your opinion. Just to be sure, did you mean:
> bcma: bus0: FOO BAR
> or
> bcma: <bus0>: FOO BAR
> ?
> 
> Personally I don't really care, but maybe there is already similar
> case in some other driver you know about? It could be nice to be
> consistent across the kernel.
> 
In b43 it looks like this if you have multiple devices logging, but if
there is some common way of doing it bcma should use that.

b43-phy0 debug: Found PHY: Analog 8, Type 4, Revision 6
b43-phy0 debug: Found Radio: Manuf 0x17F, Version 0x2056, Revision 11
b43-phy1: Broadcom 4716 WLAN found (core revision 17)
b43-phy1 debug: Found PHY: Analog 8, Type 4, Revision 5
b43-phy1 debug: Found Radio: Manuf 0x17F, Version 0x2056, Revision 7

Hauke

Re: [PATCH option A] bcma: use custom printing functions

[Rafał Miłecki]

2012/6/28 Hauke Mehrtens <hauke@hauke-m.de>:
> On 06/28/2012 10:18 PM, Rafał Miłecki wrote:
>> 2012/6/28 Joe Perches <joe@perches.com>:
>>> On Thu, 2012-06-28 at 21:56 +0200, Rafał Miłecki wrote:
>>>> 2012/6/28 Johannes Berg <johannes@sipsolutions.net>:
>>>>> On Thu, 2012-06-28 at 21:44 +0200, Rafał Miłecki wrote:
>>>>>
>>>>>> +#define bcma_err(fmt, ...) \
>>>>>> +     pr_err(KBUILD_MODNAME "-%d: " fmt, bus->num, ##__VA_ARGS__)
>>>>>
>>>>> both of your options seem to rely on "bus" being a variable in the
>>>>> context, is that really a good idea?
>>>>
>>>> Yeah, I made that assumption to make calls nicer & shorter. We may
>>>> need to get reference to "bus" in function or two.
>>>>
>>>> I saw such a solution in "radeon" gpu driver, example:
>>>> value = RREG32(R600_AUDIO_STATUS_BITS);
>>>> (they assume "rdev" is available in every function calling RREG32).
>>>
>>> I think that radeon use is ugly myself.
>>>
>>>> If you believe it's ugly, I can change that. I also wonder what Joe
>>>> will respond.
>>>>
>>>> P.S.
>>>> Both patches are not signed yet and they are supposed to be RFC. Sorry
>>>> for missing that in subject line.
>>>
>>> I think it'd be better to add and use:
>>>
>>> #define bcma_bus_err(bus, fmt, ...)                             \
>>>        pr_err("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)
>>>
>>> #define bcma_bus_info(bus, fmt, ...)                            \
>>>        pr_info("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)
>>>
>>> or some other equivalent use if you're wedded
>>> to wanting "bcma-%d:" prefixed output.
>>>
>>> I'd rather have the prefix be something like "bcma: <bus#>: ",
>>> so a dmesg grep pattern can be "^bcma:" but hey, it ain't my code.
>>
>> OK, thanks for your opinion. Just to be sure, did you mean:
>> bcma: bus0: FOO BAR
>> or
>> bcma: <bus0>: FOO BAR
>> ?
>>
>> Personally I don't really care, but maybe there is already similar
>> case in some other driver you know about? It could be nice to be
>> consistent across the kernel.
>>
> In b43 it looks like this if you have multiple devices logging, but if
> there is some common way of doing it bcma should use that.
>
> b43-phy0 debug: Found PHY: Analog 8, Type 4, Revision 6
> b43-phy0 debug: Found Radio: Manuf 0x17F, Version 0x2056, Revision 11
> b43-phy1: Broadcom 4716 WLAN found (core revision 17)
> b43-phy1 debug: Found PHY: Analog 8, Type 4, Revision 5
> b43-phy1 debug: Found Radio: Manuf 0x17F, Version 0x2056, Revision 7

I don't think we should follow b43 way. For example I remember myself
logging "debug" and "error" strings in early bcma times. I was told to
avoid that and just use correct pr_<level> calls.

-- 
Rafał

Re: [PATCH option A] bcma: use custom printing functions

[Joe Perches]

On Thu, 2012-06-28 at 22:18 +0200, Rafał Miłecki wrote:
> 2012/6/28 Joe Perches <joe@perches.com>:
> > On Thu, 2012-06-28 at 21:56 +0200, Rafał Miłecki wrote:
> >> 2012/6/28 Johannes Berg <johannes@sipsolutions.net>:
> >> > On Thu, 2012-06-28 at 21:44 +0200, Rafał Miłecki wrote:
> >> >
> >> >> +#define bcma_err(fmt, ...) \
> >> >> +     pr_err(KBUILD_MODNAME "-%d: " fmt, bus->num, ##__VA_ARGS__)
> >> >
> >> > both of your options seem to rely on "bus" being a variable in the
> >> > context, is that really a good idea?
> >>
> >> Yeah, I made that assumption to make calls nicer & shorter. We may
> >> need to get reference to "bus" in function or two.
> >>
> >> I saw such a solution in "radeon" gpu driver, example:
> >> value = RREG32(R600_AUDIO_STATUS_BITS);
> >> (they assume "rdev" is available in every function calling RREG32).
> >
> > I think that radeon use is ugly myself.
> >
> >> If you believe it's ugly, I can change that. I also wonder what Joe
> >> will respond.
> >>
> >> P.S.
> >> Both patches are not signed yet and they are supposed to be RFC. Sorry
> >> for missing that in subject line.
> >
> > I think it'd be better to add and use:
> >
> > #define bcma_bus_err(bus, fmt, ...)                             \
> >        pr_err("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)
> >
> > #define bcma_bus_info(bus, fmt, ...)                            \
> >        pr_info("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)
> >
> > or some other equivalent use if you're wedded
> > to wanting "bcma-%d:" prefixed output.
> >
> > I'd rather have the prefix be something like "bcma: <bus#>: ",
> > so a dmesg grep pattern can be "^bcma:" but hey, it ain't my code.
> 
> OK, thanks for your opinion. Just to be sure, did you mean:
> bcma: bus0: FOO BAR
> or
> bcma: <bus0>: FOO BAR
> ?
> 
> Personally I don't really care, but maybe there is already similar
> case in some other driver you know about? It could be nice to be
> consistent across the kernel.

There's no real consistency in prefixes.

Most of the cases I've seen have used either something like
"prefix: (#): " or "prefix-#:"

cheers, Joe

Re: [PATCH option A] bcma: use custom printing functions

[Rafał Miłecki]

2012/6/28 Joe Perches <joe@perches.com>:
> I think it'd be better to add and use:
>
> #define bcma_bus_err(bus, fmt, ...)                             \
>        pr_err("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)
>
> #define bcma_bus_info(bus, fmt, ...)                            \
>        pr_info("bus %d: " fmt, (bus)->num, ##__VA_ARGS__)
>
> or some other equivalent use if you're wedded
> to wanting "bcma-%d:" prefixed output.
>
> I'd rather have the prefix be something like "bcma: <bus#>: ",
> so a dmesg grep pattern can be "^bcma:" but hey, it ain't my code.

It doesn't seem to be sth common to have grep-pable "bcma:". Some examples:
pci_root PNP0A08:00: host bridge window [mem 0x000d8000-0x000dbfff]
pci 0000:00:01.0: [8086:2a41] type 1 class 0x000604
pnp 00:00: [bus 00-ff]
system 00:03: [mem 0xfed00000-0xfed003ff] has been reserved
pci_bus 0000:0a: resource 5 [io  0x0d00-0xffff]
ahci 0000:00:1f.2: version 3.0
scsi0 : ahci
scsi1 : ahci
usb 2-6: New USB device found, idVendor=054c, idProduct=0377
sky2 0000:08:00.0: Yukon-2 EC Ultra chip revision 3
iwlagn 0000:06:00.0: HW Revision ID = 0x0

My decision will be to use
bcma 0:
bcma 1:
bcma 2:
etc. That will be the most compatible solution with the rest of the kernel.

-- 
Rafał