From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-gw3-out.broadcom.com ([216.31.210.64]:46114 "EHLO mail-gw3-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751878AbbFDPCm (ORCPT ); Thu, 4 Jun 2015 11:02:42 -0400 Message-ID: <5570688E.8060307@broadcom.com> (sfid-20150604_170245_587708_D77B89FF) Date: Thu, 4 Jun 2015 17:02:38 +0200 From: Arend van Spriel MIME-Version: 1.0 To: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= CC: Kalle Valo , , Brett Rudley , "Franky (Zhenhui) Lin" , Hante Meuleman , Subject: Re: [PATCH] brcmfmac: use direct data pointer in NVRAM parser struct References: <1432813063-30922-1-git-send-email-zajec5@gmail.com> In-Reply-To: <1432813063-30922-1-git-send-email-zajec5@gmail.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: On 05/28/15 13:37, Rafał Miłecki wrote: > As we plan to add support for platform NVRAM we should store direct > data pointer without the extra struct firmware layer. This will allow > us to support other sources with the only requirement being u8 buffer. > > Signed-off-by: Rafał Miłecki > Signed-off-by: Hante Meuleman > Signed-off-by: Arend van Spriel This look a bit contradictory. You mention below you written this from 'scratch' so it seems odd that it is signed off by me and Hante. You may add: Acked-by: Arend van Spriel > --- > Tested on router with BCM43602-s using /lib/firmware/brcm/brcmfmac43602-pcie.txt > > I've written this patch from scratch, it's inspired by the dropped: > [PATCH 6/6] brcmfmac: Add support for host platform NVRAM loading. > --- > drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 40 +++++++++++----------- > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c > index 7ae6461..b72df87 100644 > --- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c > +++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c > @@ -43,7 +43,7 @@ enum nvram_parser_state { > * struct nvram_parser - internal info for parser. > * > * @state: current parser state. > - * @fwnv: input buffer being parsed. > + * @data: input buffer being parsed. So you are not adding the data_len to the structure, which means you found it is not needed? Just checking as it is different from the aforementioned patch. Regards, Arend > * @nvram: output buffer with parse result. > * @nvram_len: lenght of parse result. > * @line: current line. > @@ -55,7 +55,7 @@ enum nvram_parser_state { > */ > struct nvram_parser { > enum nvram_parser_state state; > - const struct firmware *fwnv; > + const u8 *data; > u8 *nvram; > u32 nvram_len; > u32 line; > @@ -91,7 +91,7 @@ static enum nvram_parser_state brcmf_nvram_handle_idle(struct nvram_parser *nvp) > { > char c; > > - c = nvp->fwnv->data[nvp->pos]; > + c = nvp->data[nvp->pos]; > if (c == '\n') > return COMMENT; > if (is_whitespace(c)) > @@ -115,16 +115,16 @@ static enum nvram_parser_state brcmf_nvram_handle_key(struct nvram_parser *nvp) > enum nvram_parser_state st = nvp->state; > char c; > > - c = nvp->fwnv->data[nvp->pos]; > + c = nvp->data[nvp->pos]; > if (c == '=') { > /* ignore RAW1 by treating as comment */ > - if (strncmp(&nvp->fwnv->data[nvp->entry], "RAW1", 4) == 0) > + if (strncmp(&nvp->data[nvp->entry], "RAW1", 4) == 0) > st = COMMENT; > else > st = VALUE; > - if (strncmp(&nvp->fwnv->data[nvp->entry], "devpath", 7) == 0) > + if (strncmp(&nvp->data[nvp->entry], "devpath", 7) == 0) > nvp->multi_dev_v1 = true; > - if (strncmp(&nvp->fwnv->data[nvp->entry], "pcie/", 5) == 0) > + if (strncmp(&nvp->data[nvp->entry], "pcie/", 5) == 0) > nvp->multi_dev_v2 = true; > } else if (!is_nvram_char(c) || c == ' ') { > brcmf_dbg(INFO, "warning: ln=%d:col=%d: '=' expected, skip invalid key entry\n", > @@ -145,11 +145,11 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp) > char *ekv; > u32 cplen; > > - c = nvp->fwnv->data[nvp->pos]; > + c = nvp->data[nvp->pos]; > if (!is_nvram_char(c)) { > /* key,value pair complete */ > - ekv = (u8 *)&nvp->fwnv->data[nvp->pos]; > - skv = (u8 *)&nvp->fwnv->data[nvp->entry]; > + ekv = (u8 *)&nvp->data[nvp->pos]; > + skv = (u8 *)&nvp->data[nvp->entry]; > cplen = ekv - skv; > if (nvp->nvram_len + cplen + 1>= BRCMF_FW_MAX_NVRAM_SIZE) > return END; > @@ -170,7 +170,7 @@ brcmf_nvram_handle_comment(struct nvram_parser *nvp) > { > char *eoc, *sol; > > - sol = (char *)&nvp->fwnv->data[nvp->pos]; > + sol = (char *)&nvp->data[nvp->pos]; > eoc = strchr(sol, '\n'); > if (!eoc) { > eoc = strchr(sol, '\0'); > @@ -201,17 +201,17 @@ static enum nvram_parser_state > }; > > static int brcmf_init_nvram_parser(struct nvram_parser *nvp, > - const struct firmware *nv) > + const u8 *data, size_t data_len) > { > size_t size; > > memset(nvp, 0, sizeof(*nvp)); > - nvp->fwnv = nv; > + nvp->data = data; > /* Limit size to MAX_NVRAM_SIZE, some files contain lot of comment */ > - if (nv->size> BRCMF_FW_MAX_NVRAM_SIZE) > + if (data_len> BRCMF_FW_MAX_NVRAM_SIZE) > size = BRCMF_FW_MAX_NVRAM_SIZE; > else > - size = nv->size; > + size = data_len; > /* Alloc for extra 0 byte + roundup by 4 + length field */ > size += 1 + 3 + sizeof(u32); > nvp->nvram = kzalloc(size, GFP_KERNEL); > @@ -356,18 +356,18 @@ fail: > * and converts newlines to NULs. Shortens buffer as needed and pads with NULs. > * End of buffer is completed with token identifying length of buffer. > */ > -static void *brcmf_fw_nvram_strip(const struct firmware *nv, u32 *new_length, > - u16 domain_nr, u16 bus_nr) > +static void *brcmf_fw_nvram_strip(const u8 *data, size_t data_len, > + u32 *new_length, u16 domain_nr, u16 bus_nr) > { > struct nvram_parser nvp; > u32 pad; > u32 token; > __le32 token_le; > > - if (brcmf_init_nvram_parser(&nvp, nv)< 0) > + if (brcmf_init_nvram_parser(&nvp, data, data_len)< 0) > return NULL; > > - while (nvp.pos< nv->size) { > + while (nvp.pos< data_len) { > nvp.state = nv_parser_states[nvp.state](&nvp); > if (nvp.state == END) > break; > @@ -426,7 +426,7 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx) > goto fail; > > if (fw) { > - nvram = brcmf_fw_nvram_strip(fw,&nvram_length, > + nvram = brcmf_fw_nvram_strip(fw->data, fw->size,&nvram_length, > fwctx->domain_nr, fwctx->bus_nr); > release_firmware(fw); > if (!nvram&& !(fwctx->flags& BRCMF_FW_REQ_NV_OPTIONAL))