diff mbox

[V3,4/9] brcmfmac: add struct brcmf_pub parameter to the __brcmf_err

Message ID 20170202213321.11591-4-zajec5@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show

Commit Message

Rafał Miłecki Feb. 2, 2017, 9:33 p.m. UTC
From: Rafał Miłecki <rafal@milecki.pl>

This will allow getting struct device reference from the passed
brcmf_pub for the needs of dev_err. More detailed messages are really
important for home routers which frequently have 2 (or even 3) wireless
cards supported by brcmfmac.

Note that all calls are yet to be updated as for now brcmf_err macro
always passes NULL. This will be handled in following patch to make this
change easier to review.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Add missing include
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h        | 2 ++
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c     | 2 +-
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h      | 9 +++++----
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c | 4 +++-
 4 files changed, 11 insertions(+), 6 deletions(-)

Comments

Arend van Spriel Feb. 8, 2017, 9:54 a.m. UTC | #1
On 2-2-2017 22:33, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This will allow getting struct device reference from the passed
> brcmf_pub for the needs of dev_err. More detailed messages are really
> important for home routers which frequently have 2 (or even 3) wireless
> cards supported by brcmfmac.
> 
> Note that all calls are yet to be updated as for now brcmf_err macro
> always passes NULL. This will be handled in following patch to make this
> change easier to review.

I prefer brcmf_err() to have struct device reference directly instead of
using brcmf_pub. That would remove the need for patches 5 till 7 as bus
specific code already has struct device. So I have acked the first three
patches, but would like to revise 8 and 9.

Kalle,

I acked the first three patches. Can those three still go in for 4.11?

Regards,
Arend
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Add missing include
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h        | 2 ++
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c     | 2 +-
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h      | 9 +++++----
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c | 4 +++-
>  4 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> index 76693df34742..35d4801a62e6 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
> @@ -17,6 +17,8 @@
>  #ifndef BRCMFMAC_BUS_H
>  #define BRCMFMAC_BUS_H
>  
> +#include <linux/device.h>
> +
>  #include "debug.h"
>  
>  /* IDs of the 6 default common rings of msgbuf protocol */
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> index 33b133f7e63a..f8ce597c4781 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> @@ -219,7 +219,7 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
>  }
>  
>  #ifndef CONFIG_BRCM_TRACING
> -void __brcmf_err(const char *func, const char *fmt, ...)
> +void __brcmf_err(struct brcmf_pub *pub, const char *func, const char *fmt, ...)
>  {
>  	struct va_format vaf;
>  	va_list args;
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
> index 066126123e96..99acc095c834 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
> @@ -19,6 +19,8 @@
>  
>  #include <linux/net.h>	/* net_ratelimit() */
>  
> +struct brcmf_pub;
> +
>  /* message levels */
>  #define BRCMF_TRACE_VAL		0x00000002
>  #define BRCMF_INFO_VAL		0x00000004
> @@ -45,8 +47,8 @@
>  #undef pr_fmt
>  #define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
>  
> -__printf(2, 3)
> -void __brcmf_err(const char *func, const char *fmt, ...);
> +__printf(3, 4)
> +void __brcmf_err(struct brcmf_pub *pub, const char *func, const char *fmt, ...);
>  /* Macro for error messages. When debugging / tracing the driver all error
>   * messages are important to us.
>   */
> @@ -55,7 +57,7 @@ void __brcmf_err(const char *func, const char *fmt, ...);
>  		if (IS_ENABLED(CONFIG_BRCMDBG) ||			\
>  		    IS_ENABLED(CONFIG_BRCM_TRACING) ||			\
>  		    net_ratelimit())					\
> -			__brcmf_err(__func__, fmt, ##__VA_ARGS__);	\
> +			__brcmf_err(NULL, __func__, fmt, ##__VA_ARGS__);\
>  	} while (0)
>  
>  #if defined(DEBUG) || defined(CONFIG_BRCM_TRACING)
> @@ -99,7 +101,6 @@ do {									\
>  
>  extern int brcmf_msg_level;
>  
> -struct brcmf_pub;
>  #ifdef DEBUG
>  void brcmf_debugfs_init(void);
>  void brcmf_debugfs_exit(void);
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c
> index fe6755944b7b..329cb65eb78b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c
> @@ -18,10 +18,12 @@
>  
>  #ifndef __CHECKER__
>  #define CREATE_TRACE_POINTS
> +#include "bus.h"
> +#include "core.h"
>  #include "tracepoint.h"
>  #include "debug.h"
>  
> -void __brcmf_err(const char *func, const char *fmt, ...)
> +void __brcmf_err(struct brcmf_pub *pub, const char *func, const char *fmt, ...)
>  {
>  	struct va_format vaf = {
>  		.fmt = fmt,
>
Rafał Miłecki Feb. 8, 2017, 10 a.m. UTC | #2
On 2017-02-08 10:54, Arend Van Spriel wrote:
> On 2-2-2017 22:33, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> This will allow getting struct device reference from the passed
>> brcmf_pub for the needs of dev_err. More detailed messages are really
>> important for home routers which frequently have 2 (or even 3) 
>> wireless
>> cards supported by brcmfmac.
>> 
>> Note that all calls are yet to be updated as for now brcmf_err macro
>> always passes NULL. This will be handled in following patch to make 
>> this
>> change easier to review.
> 
> I prefer brcmf_err() to have struct device reference directly instead 
> of
> using brcmf_pub. That would remove the need for patches 5 till 7 as bus
> specific code already has struct device. So I have acked the first 
> three
> patches, but would like to revise 8 and 9.
> 
> Kalle,
> 
> I acked the first three patches. Can those three still go in for 4.11?

Sounds OK to me. Kalle, I ack Arend's request if it isn't too late.
Kalle Valo Feb. 8, 2017, 2:52 p.m. UTC | #3
Rafał Miłecki <rafal@milecki.pl> writes:

> On 2017-02-08 10:54, Arend Van Spriel wrote:
>> On 2-2-2017 22:33, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> This will allow getting struct device reference from the passed
>>> brcmf_pub for the needs of dev_err. More detailed messages are really
>>> important for home routers which frequently have 2 (or even 3)
>>> wireless
>>> cards supported by brcmfmac.
>>>
>>> Note that all calls are yet to be updated as for now brcmf_err macro
>>> always passes NULL. This will be handled in following patch to make
>>> this
>>> change easier to review.
>>
>> I prefer brcmf_err() to have struct device reference directly
>> instead of
>> using brcmf_pub. That would remove the need for patches 5 till 7 as bus
>> specific code already has struct device. So I have acked the first
>> three
>> patches, but would like to revise 8 and 9.
>>
>> Kalle,
>>
>> I acked the first three patches. Can those three still go in for 4.11?
>
> Sounds OK to me. Kalle, I ack Arend's request if it isn't too late.

Ok, I'll try. My plan is to get everything ready for linux-next by
tomorrow morning (Finland time), let's see how it goes.

Related to this, Rafał are you still deleting the patches from patchwork
which should be dropped? I think you are as I can't see patches 4-9
anymore.

Now that my patchwork setup is much better (compared to how it was over
a year ago) I would actually prefer that you don't do that anymore. The
problem is that when you delete the patch from patchwork it completely
disappears from patchwork and I can't check the patch or discussion
anymore. And I'm so accustomed to use patchwork that only seldom I use
email to find the patch.

So it would be better to leave the patches as is and let me drop them
(=change the state Changes Requested, Rejected or similar), which is
trivial with my script. Otherwise I get this unsure feeling of what
happened to the patch :)
Rafał Miłecki Feb. 8, 2017, 3:09 p.m. UTC | #4
On 2017-02-08 15:52, Kalle Valo wrote:
> Rafał Miłecki <rafal@milecki.pl> writes:
> 
>> On 2017-02-08 10:54, Arend Van Spriel wrote:
>>> On 2-2-2017 22:33, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>> 
>>>> This will allow getting struct device reference from the passed
>>>> brcmf_pub for the needs of dev_err. More detailed messages are 
>>>> really
>>>> important for home routers which frequently have 2 (or even 3)
>>>> wireless
>>>> cards supported by brcmfmac.
>>>> 
>>>> Note that all calls are yet to be updated as for now brcmf_err macro
>>>> always passes NULL. This will be handled in following patch to make
>>>> this
>>>> change easier to review.
>>> 
>>> I prefer brcmf_err() to have struct device reference directly
>>> instead of
>>> using brcmf_pub. That would remove the need for patches 5 till 7 as 
>>> bus
>>> specific code already has struct device. So I have acked the first
>>> three
>>> patches, but would like to revise 8 and 9.
>>> 
>>> Kalle,
>>> 
>>> I acked the first three patches. Can those three still go in for 
>>> 4.11?
>> 
>> Sounds OK to me. Kalle, I ack Arend's request if it isn't too late.
> 
> Ok, I'll try. My plan is to get everything ready for linux-next by
> tomorrow morning (Finland time), let's see how it goes.
> 
> Related to this, Rafał are you still deleting the patches from 
> patchwork
> which should be dropped? I think you are as I can't see patches 4-9
> anymore.
> 
> Now that my patchwork setup is much better (compared to how it was over
> a year ago) I would actually prefer that you don't do that anymore. The
> problem is that when you delete the patch from patchwork it completely
> disappears from patchwork and I can't check the patch or discussion
> anymore. And I'm so accustomed to use patchwork that only seldom I use
> email to find the patch.
> 
> So it would be better to leave the patches as is and let me drop them
> (=change the state Changes Requested, Rejected or similar), which is
> trivial with my script. Otherwise I get this unsure feeling of what
> happened to the patch :)

Yeah, that was me (marked 4-9 as Changes Requested), sorry ;) I won't be 
messing with patches in the future.
Rafał Miłecki Feb. 23, 2017, 9:08 p.m. UTC | #5
On 8 February 2017 at 10:54, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 2-2-2017 22:33, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This will allow getting struct device reference from the passed
>> brcmf_pub for the needs of dev_err. More detailed messages are really
>> important for home routers which frequently have 2 (or even 3) wireless
>> cards supported by brcmfmac.
>>
>> Note that all calls are yet to be updated as for now brcmf_err macro
>> always passes NULL. This will be handled in following patch to make this
>> change easier to review.
>
> I prefer brcmf_err() to have struct device reference directly instead of
> using brcmf_pub. That would remove the need for patches 5 till 7 as bus
> specific code already has struct device. So I have acked the first three
> patches, but would like to revise 8 and 9.

I'm a bit disappointed as this change of mind means I wasted quite
some time by reworking all brcmf_err calls :(

I intentionally sent
[PATCH RFC 2/2] brcmfmac: add brcmf_err_pub macro for better error messages
2 weeks before the final patch, to make sure my design is alright &
can be accepted :(

I'll work on another version of this patch & will send it as early RFC.
Rafał Miłecki Feb. 23, 2017, 9:41 p.m. UTC | #6
Hey Arend,

On 8 February 2017 at 10:54, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 2-2-2017 22:33, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This will allow getting struct device reference from the passed
>> brcmf_pub for the needs of dev_err. More detailed messages are really
>> important for home routers which frequently have 2 (or even 3) wireless
>> cards supported by brcmfmac.
>>
>> Note that all calls are yet to be updated as for now brcmf_err macro
>> always passes NULL. This will be handled in following patch to make this
>> change easier to review.
>
> I prefer brcmf_err() to have struct device reference directly instead of
> using brcmf_pub. That would remove the need for patches 5 till 7 as bus
> specific code already has struct device. So I have acked the first three
> patches, but would like to revise 8 and 9.

I wanted to have some better perspective so I analyzed few upstream
Linux wireless drivers. It seems to be a pretty common pattern to have
one struct describing hardware that gets passed across the whole
driver. This way you can access any important info from any place of
the driver & some will probably say it also simplifies a driver.

For example following drivers use following struct in almost every
part of their code (across multiple files):
ath9k: struct ath_hw
ath10k: struct ath10k
iwlmvm: struct iwl_mvm
mt7601u: struct mt7601u_dev
mwifiex: struct mwifiex_private
rt2x00: struct rt2x00_dev

The most common / generic struct I found in brcmfmac was struct
brcmf_pub. That's why I tried to use it for the commonly used
brcmf_err.

What's the reason for hiding this struct from few random parts of the
code? Is there any real gain from this?
Arend van Spriel March 21, 2017, 1:49 p.m. UTC | #7
On 23-2-2017 22:41, Rafał Miłecki wrote:
> Hey Arend,
> 
> On 8 February 2017 at 10:54, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 2-2-2017 22:33, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> This will allow getting struct device reference from the passed
>>> brcmf_pub for the needs of dev_err. More detailed messages are really
>>> important for home routers which frequently have 2 (or even 3) wireless
>>> cards supported by brcmfmac.
>>>
>>> Note that all calls are yet to be updated as for now brcmf_err macro
>>> always passes NULL. This will be handled in following patch to make this
>>> change easier to review.
>>
>> I prefer brcmf_err() to have struct device reference directly instead of
>> using brcmf_pub. That would remove the need for patches 5 till 7 as bus
>> specific code already has struct device. So I have acked the first three
>> patches, but would like to revise 8 and 9.
> 
> I wanted to have some better perspective so I analyzed few upstream
> Linux wireless drivers. It seems to be a pretty common pattern to have
> one struct describing hardware that gets passed across the whole
> driver. This way you can access any important info from any place of
> the driver & some will probably say it also simplifies a driver.
> 
> For example following drivers use following struct in almost every
> part of their code (across multiple files):
> ath9k: struct ath_hw
> ath10k: struct ath10k
> iwlmvm: struct iwl_mvm
> mt7601u: struct mt7601u_dev
> mwifiex: struct mwifiex_private
> rt2x00: struct rt2x00_dev
> 
> The most common / generic struct I found in brcmfmac was struct
> brcmf_pub. That's why I tried to use it for the commonly used
> brcmf_err.
> 
> What's the reason for hiding this struct from few random parts of the
> code? Is there any real gain from this?

Hi Rafał,

Time flies when you are ... well, me apparently. I realized I did not
get back on this and looked at it myself. In brcmfmac we have layering
with a common layer and bus specific layer. The interface between those
layers is in bus.h, ie. struct brcmf_bus which holds reference to struct
device. So I would think using struct brcmf_bus in brcmf_err() would be
best fit. I also looked at doing some type introspection using
__builtin_types_compatible_p() which is built-in function available in
both GCC and Clang. Might be worth exploring.

Regards,
Arend
diff mbox

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
index 76693df34742..35d4801a62e6 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
@@ -17,6 +17,8 @@ 
 #ifndef BRCMFMAC_BUS_H
 #define BRCMFMAC_BUS_H
 
+#include <linux/device.h>
+
 #include "debug.h"
 
 /* IDs of the 6 default common rings of msgbuf protocol */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 33b133f7e63a..f8ce597c4781 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -219,7 +219,7 @@  int brcmf_c_preinit_dcmds(struct brcmf_if *ifp)
 }
 
 #ifndef CONFIG_BRCM_TRACING
-void __brcmf_err(const char *func, const char *fmt, ...)
+void __brcmf_err(struct brcmf_pub *pub, const char *func, const char *fmt, ...)
 {
 	struct va_format vaf;
 	va_list args;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
index 066126123e96..99acc095c834 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h
@@ -19,6 +19,8 @@ 
 
 #include <linux/net.h>	/* net_ratelimit() */
 
+struct brcmf_pub;
+
 /* message levels */
 #define BRCMF_TRACE_VAL		0x00000002
 #define BRCMF_INFO_VAL		0x00000004
@@ -45,8 +47,8 @@ 
 #undef pr_fmt
 #define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
 
-__printf(2, 3)
-void __brcmf_err(const char *func, const char *fmt, ...);
+__printf(3, 4)
+void __brcmf_err(struct brcmf_pub *pub, const char *func, const char *fmt, ...);
 /* Macro for error messages. When debugging / tracing the driver all error
  * messages are important to us.
  */
@@ -55,7 +57,7 @@  void __brcmf_err(const char *func, const char *fmt, ...);
 		if (IS_ENABLED(CONFIG_BRCMDBG) ||			\
 		    IS_ENABLED(CONFIG_BRCM_TRACING) ||			\
 		    net_ratelimit())					\
-			__brcmf_err(__func__, fmt, ##__VA_ARGS__);	\
+			__brcmf_err(NULL, __func__, fmt, ##__VA_ARGS__);\
 	} while (0)
 
 #if defined(DEBUG) || defined(CONFIG_BRCM_TRACING)
@@ -99,7 +101,6 @@  do {									\
 
 extern int brcmf_msg_level;
 
-struct brcmf_pub;
 #ifdef DEBUG
 void brcmf_debugfs_init(void);
 void brcmf_debugfs_exit(void);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c
index fe6755944b7b..329cb65eb78b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c
@@ -18,10 +18,12 @@ 
 
 #ifndef __CHECKER__
 #define CREATE_TRACE_POINTS
+#include "bus.h"
+#include "core.h"
 #include "tracepoint.h"
 #include "debug.h"
 
-void __brcmf_err(const char *func, const char *fmt, ...)
+void __brcmf_err(struct brcmf_pub *pub, const char *func, const char *fmt, ...)
 {
 	struct va_format vaf = {
 		.fmt = fmt,