Subject: Re: [PATCH 1/2] brcmfmac: modify __brcmf_err() to take bus as a
 parameter
To:     =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>,
        Kalle Valo <kvalo@codeaurora.org>
Cc:     linux-wireless@vger.kernel.org,
        brcm80211-dev-list.pdl@broadcom.com,
        brcm80211-dev-list@cypress.com,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>
References: <20190115061949.27260-1-zajec5@gmail.com>
From:   Arend Van Spriel <arend.vanspriel@broadcom.com>
Message-ID: <daa97557-03fb-c266-239e-f5f1723bcef7@broadcom.com>
Date:   Tue, 15 Jan 2019 09:48:22 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <20190115061949.27260-1-zajec5@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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