| Message ID | 20170202213321.11591-4-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
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, >
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.
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 :)
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.
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.
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?
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 --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,