| Message ID | 20190115061949.27260-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/2] brcmfmac: modify __brcmf_err() to take bus as a parameter | expand |
On 1/15/2019 7:19 AM, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > So far __brcmf_err() was using pr_err() which didn't allow identifying > device that was affected by an error. It's crucial for systems with more > than 1 device supported by brcmfmac (a common case for home routers). > > This change allows passing struct brcmf_bus to the __brcmf_err(). That > struct has been agreed to be the most common one. It allows accessing > struct device easily & using dev_err() printing helper. Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > This is my another try on improving brcmf_err after the failure from 2 > years ago: > [PATCH V3 4/9] brcmfmac: add struct brcmf_pub parameter to the __brcmf_err > https://patchwork.kernel.org/patch/9553255/ > > Back then my change has been rejected due to miscommunication and late > realisation that struct brcmf_pub (a previous choice instead of struct > brcmf_bus) was a bad idea. Back then Arend wrote: >> So I would think using struct brcmf_bus in brcmf_err() would be best >> fit. > > So this patch follows that suggestion & updates __brcmf_err() > accordingly. Thanks, Rafał Little less than two years ago I played with your idea and using GCC builtin __builtin_types_compatible_p(t1,t2). Anyway, it looks good. So you want to limit it to brcmf_err() or brcmf_dbg() as well? Regards, Arend > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 7 +++++-- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h | 8 +++++--- > .../net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c | 7 +++++-- > 3 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > index 0ce1d8174e6d..c62009a06617 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > @@ -350,7 +350,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_bus *bus, const char *func, const char *fmt, ...) > { > struct va_format vaf; > va_list args; > @@ -359,7 +359,10 @@ void __brcmf_err(const char *func, const char *fmt, ...) > > vaf.fmt = fmt; > vaf.va = &args; > - pr_err("%s: %pV", func, &vaf); > + if (bus) > + dev_err(bus->dev, "%s: %pV", func, &vaf); > + else > + pr_err("%s: %pV", func, &vaf); > > va_end(args); > } > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h > index cfed0626bf5a..b499f90d94f6 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h > @@ -45,8 +45,10 @@ > #undef pr_fmt > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > -__printf(2, 3) > -void __brcmf_err(const char *func, const char *fmt, ...); > +struct brcmf_bus; > + > +__printf(3, 4) > +void __brcmf_err(struct brcmf_bus *bus, 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) > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c > index fe6755944b7b..f9359ea9cb13 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c > @@ -21,7 +21,7 @@ > #include "tracepoint.h" > #include "debug.h" > > -void __brcmf_err(const char *func, const char *fmt, ...) > +void __brcmf_err(struct brcmf_bus *bus, const char *func, const char *fmt, ...) > { > struct va_format vaf = { > .fmt = fmt, > @@ -30,7 +30,10 @@ void __brcmf_err(const char *func, const char *fmt, ...) > > va_start(args, fmt); > vaf.va = &args; > - pr_err("%s: %pV", func, &vaf); > + if (bus) > + dev_err(bus->dev, "%s: %pV", func, &vaf); > + else > + pr_err("%s: %pV", func, &vaf); > trace_brcmf_err(func, &vaf); > va_end(args); > } >
Hi Rafał,
I love your patch! Yet something to improve:
[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on v5.0-rc2 next-20190115]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/brcmfmac-modify-__brcmf_err-to-take-bus-as-a-parameter/20190115-183015
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: i386-randconfig-x006-201902 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c: In function '__brcmf_err':
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c:34:3: error: implicit declaration of function 'dev_err'; did you mean 'xa_err'? [-Werror=implicit-function-declaration]
dev_err(bus->dev, "%s: %pV", func, &vaf);
^~~~~~~
xa_err
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c:34:14: error: dereferencing pointer to incomplete type 'struct brcmf_bus'
dev_err(bus->dev, "%s: %pV", func, &vaf);
^~
cc1: some warnings being treated as errors
vim +34 drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c
23
24 void __brcmf_err(struct brcmf_bus *bus, const char *func, const char *fmt, ...)
25 {
26 struct va_format vaf = {
27 .fmt = fmt,
28 };
29 va_list args;
30
31 va_start(args, fmt);
32 vaf.va = &args;
33 if (bus)
> 34 dev_err(bus->dev, "%s: %pV", func, &vaf);
35 else
36 pr_err("%s: %pV", func, &vaf);
37 trace_brcmf_err(func, &vaf);
38 va_end(args);
39 }
40
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, 15 Jan 2019 at 09:48, Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > On 1/15/2019 7:19 AM, Rafał Miłecki wrote: > > From: Rafał Miłecki <rafal@milecki.pl> > > > > So far __brcmf_err() was using pr_err() which didn't allow identifying > > device that was affected by an error. It's crucial for systems with more > > than 1 device supported by brcmfmac (a common case for home routers). > > > > This change allows passing struct brcmf_bus to the __brcmf_err(). That > > struct has been agreed to be the most common one. It allows accessing > > struct device easily & using dev_err() printing helper. > > Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > > --- > > This is my another try on improving brcmf_err after the failure from 2 > > years ago: > > [PATCH V3 4/9] brcmfmac: add struct brcmf_pub parameter to the __brcmf_err > > https://patchwork.kernel.org/patch/9553255/ > > > > Back then my change has been rejected due to miscommunication and late > > realisation that struct brcmf_pub (a previous choice instead of struct > > brcmf_bus) was a bad idea. Back then Arend wrote: > >> So I would think using struct brcmf_bus in brcmf_err() would be best > >> fit. > > > > So this patch follows that suggestion & updates __brcmf_err() > > accordingly. > > Thanks, Rafał > > Little less than two years ago I played with your idea and using GCC > builtin __builtin_types_compatible_p(t1,t2). Anyway, it looks good. So > you want to limit it to brcmf_err() or brcmf_dbg() as well? I believe all messages printed by brcmfmac should specify a device. brcmf_err, brcmf_info & brcmf_dbg. I can work on brcmf_info & brcmf_dbg once I get done with brcmf_err :)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c index 0ce1d8174e6d..c62009a06617 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c @@ -350,7 +350,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_bus *bus, const char *func, const char *fmt, ...) { struct va_format vaf; va_list args; @@ -359,7 +359,10 @@ void __brcmf_err(const char *func, const char *fmt, ...) vaf.fmt = fmt; vaf.va = &args; - pr_err("%s: %pV", func, &vaf); + if (bus) + dev_err(bus->dev, "%s: %pV", func, &vaf); + else + pr_err("%s: %pV", func, &vaf); va_end(args); } diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h index cfed0626bf5a..b499f90d94f6 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h @@ -45,8 +45,10 @@ #undef pr_fmt #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt -__printf(2, 3) -void __brcmf_err(const char *func, const char *fmt, ...); +struct brcmf_bus; + +__printf(3, 4) +void __brcmf_err(struct brcmf_bus *bus, 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) diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c index fe6755944b7b..f9359ea9cb13 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/tracepoint.c @@ -21,7 +21,7 @@ #include "tracepoint.h" #include "debug.h" -void __brcmf_err(const char *func, const char *fmt, ...) +void __brcmf_err(struct brcmf_bus *bus, const char *func, const char *fmt, ...) { struct va_format vaf = { .fmt = fmt, @@ -30,7 +30,10 @@ void __brcmf_err(const char *func, const char *fmt, ...) va_start(args, fmt); vaf.va = &args; - pr_err("%s: %pV", func, &vaf); + if (bus) + dev_err(bus->dev, "%s: %pV", func, &vaf); + else + pr_err("%s: %pV", func, &vaf); trace_brcmf_err(func, &vaf); va_end(args); }