Re: [PATCH 1/2] brcmfmac: modify __brcmf_err() to take bus as a parameter

[Arend Van Spriel <arend.vanspriel@broadcom.com>]

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);
>   }
>