Re: [PATCH V5 2/2] brcmfmac: don't warn user about NVRAM if fallback to the platform one succeeds

[Arend van Spriel]

On 31-07-17 17:09, RafaÅ MiÅecki wrote:
> From: RafaÅ MiÅecki <rafal@xxxxxxxxxx>
> 
> Failing to load NVRAM *file* isn't critical if we manage to get platform
> NVRAM in the fallback path. It means warnings like:
> [   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2
> are unnecessary & disturbing for people with *platform* NVRAM as they
> are not expected to have NVRAM file. This is a very common case for
> Broadcom home routers.
> 
> Instead of printing warning immediately within the firmware subsystem
> let's try our fallback code first. If that fails as well, then it's a
> right moment to print an error.
> 
> This should reduce amount of false reports from users seeing this
> warning while having wireless working perfectly fine with the platform
> NVRAM.

Reviewed-by: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx>
> Signed-off-by: RafaÅ MiÅecki <rafal@xxxxxxxxxx>
> ---
> V2: Update commit message as it wasn't clear enough (thanks Andy) & add extra
>     messages to the firmware.c.
> V3: Set FW_OPT_UEVENT to don't change behavior
> V4: Switch to the new request_firmware_async syntax
> V5: Rebase, update commit message, resend after drvdata discussion
> ---
>  .../wireless/broadcom/brcm80211/brcmfmac/firmware.c    | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> index d231042f19d6..524442b3870f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> @@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
>  		raw_nvram = false;
>  	} else {
>  		data = bcm47xx_nvram_get_contents(&data_len);
> -		if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
> -			goto fail;
> +		if (!data) {
> +			brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");

Better make this INFO level instead of TRACE. The intent of TRACE level
is for entry/exit points in functions.

Regards,
Arend