[PATCH v3] brcmfmac: Do not print the firmware version as an error

[PATCH v3] brcmfmac: Do not print the firmware version as an error

[Hans de Goede]

Using pr_err for things which are not errors is a bad idea. E.g. it
will cause the plymouth bootsplash screen to drop back to the text
console so that the user can see the error, which is not what we
normally want to happen.

Instead add a new brcmf_info macro and use that.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Fix brcm_err typo (should be brcmf_err) in CONFIG_BRCM_TRACING case
Changes in v3:
-Use do { } while (0) around macro
-Rebase on top of v4.11-rc1
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 2 +-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h  | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 33b133f..7a2b495 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -161,7 +161,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
 	strsep(&ptr, "\n");
 
 	/* Print fw version info */
-	brcmf_err("Firmware version = %s\n", buf);
+	brcmf_info("Firmware version = %s\n", buf);
 
 	/* locate firmware version number for ethtool */
 	ptr = strrchr(buf, ' ') + 1;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
index 0661261..afb2436 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
@@ -59,6 +59,10 @@ void __brcmf_err(const char *func, const char *fmt, ...);
 	} while (0)
 
 #if defined(DEBUG) || defined(CONFIG_BRCM_TRACING)
+
+/* For debug/tracing purposes treat info messages as errors */
+#define brcmf_info brcmf_err
+
 __printf(3, 4)
 void __brcmf_dbg(u32 level, const char *func, const char *fmt, ...);
 #define brcmf_dbg(level, fmt, ...)				\
@@ -77,6 +81,11 @@ do {								\
 
 #else /* defined(DEBUG) || defined(CONFIG_BRCM_TRACING) */
 
+#define brcmf_info(fmt, ...)						\
+	do {								\
+		pr_info("%s: " fmt, __func__, ##__VA_ARGS__);		\
+	} while (0)
+
 #define brcmf_dbg(level, fmt, ...) no_printk(fmt, ##__VA_ARGS__)
 
 #define BRCMF_DATA_ON()		0
-- 
2.9.3

Re: [PATCH v3] brcmfmac: Do not print the firmware version as an error

[Joe Perches]

On Wed, 2017-03-08 at 09:23 +0100, Hans de Goede wrote:
> Using pr_err for things which are not errors is a bad idea. E.g. it
> will cause the plymouth bootsplash screen to drop back to the text
> console so that the user can see the error, which is not what we
> normally want to happen.
> 
> Instead add a new brcmf_info macro and use that.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Fix brcm_err typo (should be brcmf_err) in CONFIG_BRCM_TRACING case
> Changes in v3:
> -Use do { } while (0) around macro

why?  Single statement macros do not need a do/while

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
[]
> @@ -59,6 +59,10 @@ void __brcmf_err(const char *func, const char *fmt, ...);
>  	} while (0)
>  
>  #if defined(DEBUG) || defined(CONFIG_BRCM_TRACING)
> +
> +/* For debug/tracing purposes treat info messages as errors */
> +#define brcmf_info brcmf_err
> +
>  __printf(3, 4)
>  void __brcmf_dbg(u32 level, const char *func, const char *fmt, ...);
>  #define brcmf_dbg(level, fmt, ...)				\
> @@ -77,6 +81,11 @@ do {								\
>  
>  #else /* defined(DEBUG) || defined(CONFIG_BRCM_TRACING) */
>  
> +#define brcmf_info(fmt, ...)						\
> +	do {								\
> +		pr_info("%s: " fmt, __func__, ##__VA_ARGS__);		\
> +	} while (0)

#define brcmf_info(fmt, ...)
	pr_info("%s: " fmt, __func__, ##__VA_ARGS__)

> +
>  #define brcmf_dbg(level, fmt, ...) no_printk(fmt, ##__VA_ARGS__)

I think the separate defines for DEBUG/CONFIG_BRCM_TRACING
are not necessary.

Re: [PATCH v3] brcmfmac: Do not print the firmware version as an error

[Hans de Goede]

Hi,

On 08-03-17 17:34, Joe Perches wrote:
> On Wed, 2017-03-08 at 09:23 +0100, Hans de Goede wrote:
>> Using pr_err for things which are not errors is a bad idea. E.g. it
>> will cause the plymouth bootsplash screen to drop back to the text
>> console so that the user can see the error, which is not what we
>> normally want to happen.
>>
>> Instead add a new brcmf_info macro and use that.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Fix brcm_err typo (should be brcmf_err) in CONFIG_BRCM_TRACING case
>> Changes in v3:
>> -Use do { } while (0) around macro
>
> why?  Single statement macros do not need a do/while

Because Arend ask me to during review of v2.

>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
> []
>> @@ -59,6 +59,10 @@ void __brcmf_err(const char *func, const char *fmt, ...);
>>  	} while (0)
>>
>>  #if defined(DEBUG) || defined(CONFIG_BRCM_TRACING)
>> +
>> +/* For debug/tracing purposes treat info messages as errors */
>> +#define brcmf_info brcmf_err
>> +
>>  __printf(3, 4)
>>  void __brcmf_dbg(u32 level, const char *func, const char *fmt, ...);
>>  #define brcmf_dbg(level, fmt, ...)				\
>> @@ -77,6 +81,11 @@ do {								\
>>
>>  #else /* defined(DEBUG) || defined(CONFIG_BRCM_TRACING) */
>>
>> +#define brcmf_info(fmt, ...)						\
>> +	do {								\
>> +		pr_info("%s: " fmt, __func__, ##__VA_ARGS__);		\
>> +	} while (0)
>
> #define brcmf_info(fmt, ...)
> 	pr_info("%s: " fmt, __func__, ##__VA_ARGS__)
>
>> +
>>  #define brcmf_dbg(level, fmt, ...) no_printk(fmt, ##__VA_ARGS__)
>
> I think the separate defines for DEBUG/CONFIG_BRCM_TRACING
> are not necessary.

When tracing we want the message logging the firmware version
to show up in the trace, which requires calling __brcmf_err()

Regards,

Hans

Re: [PATCH v3] brcmfmac: Do not print the firmware version as an error

[Joe Perches]

On Wed, 2017-03-08 at 17:57 +0100, Hans de Goede wrote:
> Hi,

And hello back to you.

> On 08-03-17 17:34, Joe Perches wrote:
> > On Wed, 2017-03-08 at 09:23 +0100, Hans de Goede wrote:
> > > Using pr_err for things which are not errors is a bad idea. E.g. it
> > > will cause the plymouth bootsplash screen to drop back to the text
> > > console so that the user can see the error, which is not what we
> > > normally want to happen.
> > > 
> > > Instead add a new brcmf_info macro and use that.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > Changes in v2:
> > > -Fix brcm_err typo (should be brcmf_err) in CONFIG_BRCM_TRACING case
> > > Changes in v3:
> > > -Use do { } while (0) around macro
> > 
> > why?  Single statement macros do not need a do/while
> 
> Because Arend ask me to during review of v2.

Well, maybe Arend should learn that single statement macros
don't need do/while guards and that do/while guards are
generally not used in the kernel for single statements.

Re: [PATCH v3] brcmfmac: Do not print the firmware version as an error

[Arend Van Spriel]

On 8-3-2017 18:53, Joe Perches wrote:
> On Wed, 2017-03-08 at 17:57 +0100, Hans de Goede wrote:
>> Hi,
> 
> And hello back to you.

Also hello.

>> On 08-03-17 17:34, Joe Perches wrote:
>>> On Wed, 2017-03-08 at 09:23 +0100, Hans de Goede wrote:
>>>> Using pr_err for things which are not errors is a bad idea. E.g. it
>>>> will cause the plymouth bootsplash screen to drop back to the text
>>>> console so that the user can see the error, which is not what we
>>>> normally want to happen.
>>>>
>>>> Instead add a new brcmf_info macro and use that.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v2:
>>>> -Fix brcm_err typo (should be brcmf_err) in CONFIG_BRCM_TRACING case
>>>> Changes in v3:
>>>> -Use do { } while (0) around macro
>>>
>>> why?  Single statement macros do not need a do/while
>>
>> Because Arend ask me to during review of v2.
> 
> Well, maybe Arend should learn that single statement macros
> don't need do/while guards and that do/while guards are
> generally not used in the kernel for single statements.

Always good to learn from an expert. The intent behind my remark was to
follow the same pattern as brcmf_err for the sake of consistency. I was
not clear.

Re: [PATCH v3] brcmfmac: Do not print the firmware version as an error

[Hans de Goede]

Hi all,

On 08-03-17 21:44, Arend Van Spriel wrote:
>
>
> On 8-3-2017 18:53, Joe Perches wrote:
>> On Wed, 2017-03-08 at 17:57 +0100, Hans de Goede wrote:
>>> Hi,
>>
>> And hello back to you.
>
> Also hello.
>
>>> On 08-03-17 17:34, Joe Perches wrote:
>>>> On Wed, 2017-03-08 at 09:23 +0100, Hans de Goede wrote:
>>>>> Using pr_err for things which are not errors is a bad idea. E.g. it
>>>>> will cause the plymouth bootsplash screen to drop back to the text
>>>>> console so that the user can see the error, which is not what we
>>>>> normally want to happen.
>>>>>
>>>>> Instead add a new brcmf_info macro and use that.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> -Fix brcm_err typo (should be brcmf_err) in CONFIG_BRCM_TRACING case
>>>>> Changes in v3:
>>>>> -Use do { } while (0) around macro
>>>>
>>>> why?  Single statement macros do not need a do/while
>>>
>>> Because Arend ask me to during review of v2.
>>
>> Well, maybe Arend should learn that single statement macros
>> don't need do/while guards and that do/while guards are
>> generally not used in the kernel for single statements.
>
> Always good to learn from an expert. The intent behind my remark was to
> follow the same pattern as brcmf_err for the sake of consistency. I was
> not clear.

Ok, so what is it going to be, are we going to keep this as is
with the do .. while added or shall I do a v4 dropping it again?

Regards,

Hans

Re: [PATCH v3] brcmfmac: Do not print the firmware version as an error

[Arend Van Spriel]

On 9-3-2017 9:09, Hans de Goede wrote:
> Hi all,
> 
> On 08-03-17 21:44, Arend Van Spriel wrote:
>>
>>
>> On 8-3-2017 18:53, Joe Perches wrote:
>>> On Wed, 2017-03-08 at 17:57 +0100, Hans de Goede wrote:
>>>> Hi,
>>>
>>> And hello back to you.
>>
>> Also hello.
>>
>>>> On 08-03-17 17:34, Joe Perches wrote:
>>>>> On Wed, 2017-03-08 at 09:23 +0100, Hans de Goede wrote:
>>>>>> Using pr_err for things which are not errors is a bad idea. E.g. it
>>>>>> will cause the plymouth bootsplash screen to drop back to the text
>>>>>> console so that the user can see the error, which is not what we
>>>>>> normally want to happen.
>>>>>>
>>>>>> Instead add a new brcmf_info macro and use that.
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> -Fix brcm_err typo (should be brcmf_err) in CONFIG_BRCM_TRACING case
>>>>>> Changes in v3:
>>>>>> -Use do { } while (0) around macro
>>>>>
>>>>> why?  Single statement macros do not need a do/while
>>>>
>>>> Because Arend ask me to during review of v2.
>>>
>>> Well, maybe Arend should learn that single statement macros
>>> don't need do/while guards and that do/while guards are
>>> generally not used in the kernel for single statements.
>>
>> Always good to learn from an expert. The intent behind my remark was to
>> follow the same pattern as brcmf_err for the sake of consistency. I was
>> not clear.
> 
> Ok, so what is it going to be, are we going to keep this as is
> with the do .. while added or shall I do a v4 dropping it again?

Sorry, Hans

My bad. Let's stick with v3 and I will do a follow-up patch.

Regards,
Arend

> Regards,
> 
> Hans

Re: [PATCH v3] brcmfmac: Do not print the firmware version as an error

[Hans de Goede]

Hi,

On 09-03-17 09:17, Arend Van Spriel wrote:
> On 9-3-2017 9:09, Hans de Goede wrote:
>> Hi all,
>>
>> On 08-03-17 21:44, Arend Van Spriel wrote:
>>>
>>>
>>> On 8-3-2017 18:53, Joe Perches wrote:
>>>> On Wed, 2017-03-08 at 17:57 +0100, Hans de Goede wrote:
>>>>> Hi,
>>>>
>>>> And hello back to you.
>>>
>>> Also hello.
>>>
>>>>> On 08-03-17 17:34, Joe Perches wrote:
>>>>>> On Wed, 2017-03-08 at 09:23 +0100, Hans de Goede wrote:
>>>>>>> Using pr_err for things which are not errors is a bad idea. E.g. it
>>>>>>> will cause the plymouth bootsplash screen to drop back to the text
>>>>>>> console so that the user can see the error, which is not what we
>>>>>>> normally want to happen.
>>>>>>>
>>>>>>> Instead add a new brcmf_info macro and use that.
>>>>>>>
>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>> -Fix brcm_err typo (should be brcmf_err) in CONFIG_BRCM_TRACING case
>>>>>>> Changes in v3:
>>>>>>> -Use do { } while (0) around macro
>>>>>>
>>>>>> why?  Single statement macros do not need a do/while
>>>>>
>>>>> Because Arend ask me to during review of v2.
>>>>
>>>> Well, maybe Arend should learn that single statement macros
>>>> don't need do/while guards and that do/while guards are
>>>> generally not used in the kernel for single statements.
>>>
>>> Always good to learn from an expert. The intent behind my remark was to
>>> follow the same pattern as brcmf_err for the sake of consistency. I was
>>> not clear.
>>
>> Ok, so what is it going to be, are we going to keep this as is
>> with the do .. while added or shall I do a v4 dropping it again?
>
> Sorry, Hans
>
> My bad. Let's stick with v3 and I will do a follow-up patch.

Ok, that works for me :)

Regards,

Hans