Re: [PATCH v5] brcmfmac: add CLM download support

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

On 29-08-17 08:23, Wright Feng wrote:
> From: Chung-Hsien Hsu <stanley.hsu@cypress.com>
> 
> The firmware for brcmfmac devices includes information regarding
> regulatory constraints. For certain devices this information is kept
> separately in a binary form that needs to be downloaded to the device.
> This patch adds support to download this so-called CLM blob file. It
> uses the same naming scheme as the other firmware files with extension
> of .clm_blob.
> 
> The CLM blob file is optional. If the file does not exist, the download
> process will be bypassed. It will not affect the driver loading.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Chung-Hsien Hsu <stanley.hsu@cypress.com>
> ---
> v2: Revise commit message to describe in more detail
> v3: Add error handling in brcmf_c_get_clm_name function
> v4: Correct the length of dload_buf in brcmf_c_download function
> v5: Remove unnecessary cast and alignment
> ---
>   .../net/wireless/broadcom/brcm80211/brcmfmac/bus.h |  10 ++
>   .../wireless/broadcom/brcm80211/brcmfmac/common.c  | 160 +++++++++++++++++++++
>   .../wireless/broadcom/brcm80211/brcmfmac/core.c    |   2 +
>   .../wireless/broadcom/brcm80211/brcmfmac/core.h    |   2 +
>   .../broadcom/brcm80211/brcmfmac/fwil_types.h       |  31 ++++
>   .../wireless/broadcom/brcm80211/brcmfmac/pcie.c    |  19 +++
>   .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    |  19 +++
>   .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c |  18 +++
>   8 files changed, 261 insertions(+)

[...]

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> index 7a2b495..f6268e0 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> @@ -18,6 +18,7 @@
>   #include <linux/string.h>
>   #include <linux/netdevice.h>
>   #include <linux/module.h>
> +#include <linux/firmware.h>
>   #include <brcmu_wifi.h>
>   #include <brcmu_utils.h>
>   #include "core.h"
> @@ -28,6 +29,7 @@
>   #include "tracepoint.h"
>   #include "common.h"
>   #include "of.h"
> +#include "firmware.h"

You are not calling anything in firmware.c from this source file so I do 
not think you need to include firmware.h here. Or did I miss something?

>   MODULE_AUTHOR("Broadcom Corporation");
>   MODULE_DESCRIPTION("Broadcom 802.11 wireless LAN fullmac driver.");
> @@ -104,12 +106,138 @@ void brcmf_c_set_joinpref_default(struct brcmf_if *ifp)
>   		brcmf_err("Set join_pref error (%d)\n", err);
>   }

[...]

> +static int brcmf_c_process_clm_blob(struct brcmf_if *ifp)
> +{
> +	struct device *dev = ifp->drvr->bus_if->dev;
> +	struct brcmf_dload_data_le *chunk_buf;
> +	const struct firmware *clm = NULL;
> +	u8 clm_name[BRCMF_FW_NAME_LEN];
> +	u32 chunk_len;
> +	u32 datalen;
> +	u32 cumulative_len = 0;
> +	u16 dl_flag = DL_BEGIN;
> +	u32 status;
> +	s32 err;
> +
> +	brcmf_dbg(INFO, "Enter\n");

Please use TRACE level for function entry logging.

> +	memset(clm_name, 0, BRCMF_FW_NAME_LEN);
> +	err = brcmf_c_get_clm_name(ifp, clm_name);
> +	if (err) {
> +		brcmf_err("get CLM blob file name failed (%d)\n", err);
> +		return err;
> +	}
> +
> +	err = request_firmware(&clm, clm_name, dev);
> +	if (err) {
> +		if (err == -ENOENT)
> +			return 0;

This exit point is worth a comment or even a brcmf_dbg(INFO, ...) to 
clarify what is happening here, ie. continue with CLM data currently 
present in firmware.

> +		brcmf_err("request CLM blob file failed (%d)\n", err);
> +		return err;
> +	}
> +
> +	datalen = clm->size;

move this initialization just before the do-while loop.

> +	chunk_buf = kzalloc(sizeof(*chunk_buf) + MAX_CHUNK_LEN - 1, GFP_KERNEL);
> +	if (!chunk_buf) {
> +		err = -ENOMEM;
> +		goto done;
> +	}

initialize datalen and cumulative_len here.

> +	do {
> +		if (datalen > MAX_CHUNK_LEN) {
> +			chunk_len = MAX_CHUNK_LEN;
> +		} else {
> +			chunk_len = datalen;
> +			dl_flag |= DL_END;
> +		}
> +		memcpy(chunk_buf->data, clm->data + cumulative_len, chunk_len);
> +
> +		err = brcmf_c_download(ifp, dl_flag, chunk_buf, chunk_len);
> +
> +		dl_flag &= ~DL_BEGIN;
> +
> +		cumulative_len += chunk_len;
> +		datalen -= chunk_len;
> +	} while ((datalen > 0) && (err == 0));
> +
> +	if (err) {
> +		brcmf_err("clmload (%d byte file) failed (%d); ",
> +			  (u32)clm->size, err);

Instead of casting clm->size it seems better to use the proper format 
specifier, ie. %zu (see format_decode() [1]).

> +		/* Retrieve clmload_status and print */
> +		err = brcmf_fil_iovar_int_get(ifp, "clmload_status", &status);
> +		if (err)
> +			brcmf_err("get clmload_status failed (%d)\n", err);
> +		else
> +			brcmf_dbg(INFO, "clmload_status=%d\n", status);
> +		err = -EIO;
> +	}
> +
> +	kfree(chunk_buf);
> +done:
> +	release_firmware(clm);
> +	return err;
> +}
> +