| Message ID | 1436553071-32423-2-git-send-email-arend@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote: > @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp) > u32 cplen; > > c = nvp->data[nvp->pos]; > - if (!is_nvram_char(c)) { > + if (!is_nvram_char(c) && (c != ' ')) { This is redundant, please drop this change. See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces") > @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx) > struct brcmf_fw *fwctx = ctx; > u32 nvram_length = 0; > void *nvram = NULL; > + u8 *data = NULL; This can be const. > + size_t data_len; > + bool raw_nvram; > > brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev)); > - if (!fw && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) > - goto fail; > + if ((fw) && (fw->data)) { I think I was already pointing similar coding issue to you. There is no need for these extra braces. And if they are not needed, don't use them. There is no point in using if (((foo))) schema just because it works. You could be confused by macros where we sometimes need tricks like this, but this is a standard part of code. > + data = (u8 *)fw->data; Don't cast to workaround const != const. You won't need casting after making local "data" a const variable. > + data_len = fw->size; > + raw_nvram = false; > + } else { > + data = bcm47xx_nvram_get_contents(&data_len); > + if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) > + goto fail; > + raw_nvram = true; > + } > > - if (fw) { > - nvram = brcmf_fw_nvram_strip(fw->data, fw->size, &nvram_length, > + if (data) { > + nvram = brcmf_fw_nvram_strip(data, data_len, &nvram_length, > fwctx->domain_nr, fwctx->bus_nr); > - release_firmware(fw); > - if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) > - goto fail; > + if (raw_nvram) > + bcm47xx_nvram_release_contents(data); This is cosmetical but maybe you could move above 2 lines next to the release_firmware? So we have all freeing code at one please? Do you think it would improve readability? Nothing important thought. Feel free to ignore me here. > @@ -473,15 +490,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx) > if (!ret) > return; > > - /* when nvram is optional call .done() callback here */ > - if (fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL) { > - fwctx->done(fwctx->dev, fw, NULL, 0); > - kfree(fwctx); > - return; > - } > + brcmf_fw_request_nvram_done(NULL, fwctx); > + return; It gave me a 5 minutes headache ;) Could you add a short comment why you call _done anyway? Something like /* Even if we failed to init user space fw request we may get a platform one */
subject changed to v2. So let's go over your beef. On 07/11/2015 07:09 PM, Rafa? Mi?ecki wrote: > On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote: >> @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp) >> u32 cplen; >> >> c = nvp->data[nvp->pos]; >> - if (!is_nvram_char(c)) { >> + if (!is_nvram_char(c) && (c != ' ')) { > > This is redundant, please drop this change. > See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces") done >> @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx) >> struct brcmf_fw *fwctx = ctx; >> u32 nvram_length = 0; >> void *nvram = NULL; >> + u8 *data = NULL; > > This can be const. done >> + size_t data_len; >> + bool raw_nvram; >> >> brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev)); >> - if (!fw && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) >> - goto fail; >> + if ((fw) && (fw->data)) { > > I think I was already pointing similar coding issue to you. There is > no need for these extra braces. And if they are not needed, don't use > them. There is no point in using if (((foo))) schema just because it > works. You could be confused by macros where we sometimes need tricks > like this, but this is a standard part of code. No confusion, just paranoid. You clearly have never been on road of chasing compiler issues with logical condition, but indeed it can be removed although checkpatch does not seem to be bothered with it. Will change it. >> + data = (u8 *)fw->data; > > Don't cast to workaround const != const. You won't need casting after > making local "data" a const variable. done >> + data_len = fw->size; >> + raw_nvram = false; >> + } else { >> + data = bcm47xx_nvram_get_contents(&data_len); >> + if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) >> + goto fail; >> + raw_nvram = true; >> + } >> >> - if (fw) { >> - nvram = brcmf_fw_nvram_strip(fw->data, fw->size, &nvram_length, >> + if (data) { >> + nvram = brcmf_fw_nvram_strip(data, data_len, &nvram_length, >> fwctx->domain_nr, fwctx->bus_nr); >> - release_firmware(fw); >> - if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) >> - goto fail; >> + if (raw_nvram) >> + bcm47xx_nvram_release_contents(data); > > This is cosmetical but maybe you could move above 2 lines next to the > release_firmware? So we have all freeing code at one please? Do you > think it would improve readability? > Nothing important thought. Feel free to ignore me here. confused! The release_firmware call is removed here, right? >> @@ -473,15 +490,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx) >> if (!ret) >> return; >> >> - /* when nvram is optional call .done() callback here */ >> - if (fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL) { >> - fwctx->done(fwctx->dev, fw, NULL, 0); >> - kfree(fwctx); >> - return; >> - } >> + brcmf_fw_request_nvram_done(NULL, fwctx); >> + return; > > It gave me a 5 minutes headache ;) Could you add a short comment why > you call _done anyway? Something like > /* Even if we failed to init user space fw request we may get a platform one */ For the resulting code I don't see value adding such comment. Reading this patch you might want Hante to explain this change, but you figured it out. Sorry for the headache ;-) Regards, Arend -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/19/2015 11:21 PM, Arend van Spriel wrote: > subject changed to v2. So let's go over your beef. > > On 07/11/2015 07:09 PM, Rafa? Mi?ecki wrote: >> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote: >>> @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp) >>> u32 cplen; >>> >>> c = nvp->data[nvp->pos]; >>> - if (!is_nvram_char(c)) { >>> + if (!is_nvram_char(c) && (c != ' ')) { >> >> This is redundant, please drop this change. >> See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces") > > done > >>> @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const >>> struct firmware *fw, void *ctx) >>> struct brcmf_fw *fwctx = ctx; >>> u32 nvram_length = 0; >>> void *nvram = NULL; >>> + u8 *data = NULL; >> >> This can be const. > > done Actually this is not done, but either way will require a cast because bcm47xx_nvram_release_contents expects char* so there is nothing gained. Unless someone will change bcm47xx_nvram_get/release_contents api to const char*. Regards, Arend >>> + size_t data_len; >>> + bool raw_nvram; >>> >>> brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev)); >>> - if (!fw && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) >>> - goto fail; >>> + if ((fw) && (fw->data)) { >> >> I think I was already pointing similar coding issue to you. There is >> no need for these extra braces. And if they are not needed, don't use >> them. There is no point in using if (((foo))) schema just because it >> works. You could be confused by macros where we sometimes need tricks >> like this, but this is a standard part of code. > > No confusion, just paranoid. You clearly have never been on road of > chasing compiler issues with logical condition, but indeed it can be > removed although checkpatch does not seem to be bothered with it. Will > change it. > >>> + data = (u8 *)fw->data; >> >> Don't cast to workaround const != const. You won't need casting after >> making local "data" a const variable. > > done > >>> + data_len = fw->size; >>> + raw_nvram = false; >>> + } else { >>> + data = bcm47xx_nvram_get_contents(&data_len); >>> + if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) >>> + goto fail; >>> + raw_nvram = true; >>> + } >>> >>> - if (fw) { >>> - nvram = brcmf_fw_nvram_strip(fw->data, fw->size, >>> &nvram_length, >>> + if (data) { >>> + nvram = brcmf_fw_nvram_strip(data, data_len, >>> &nvram_length, >>> fwctx->domain_nr, >>> fwctx->bus_nr); >>> - release_firmware(fw); >>> - if (!nvram && !(fwctx->flags & >>> BRCMF_FW_REQ_NV_OPTIONAL)) >>> - goto fail; >>> + if (raw_nvram) >>> + bcm47xx_nvram_release_contents(data); >> >> This is cosmetical but maybe you could move above 2 lines next to the >> release_firmware? So we have all freeing code at one please? Do you >> think it would improve readability? >> Nothing important thought. Feel free to ignore me here. > > confused! The release_firmware call is removed here, right? > >>> @@ -473,15 +490,9 @@ static void brcmf_fw_request_code_done(const >>> struct firmware *fw, void *ctx) >>> if (!ret) >>> return; >>> >>> - /* when nvram is optional call .done() callback here */ >>> - if (fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL) { >>> - fwctx->done(fwctx->dev, fw, NULL, 0); >>> - kfree(fwctx); >>> - return; >>> - } >>> + brcmf_fw_request_nvram_done(NULL, fwctx); >>> + return; >> >> It gave me a 5 minutes headache ;) Could you add a short comment why >> you call _done anyway? Something like >> /* Even if we failed to init user space fw request we may get a >> platform one */ > > For the resulting code I don't see value adding such comment. Reading > this patch you might want Hante to explain this change, but you figured > it out. Sorry for the headache ;-) > > Regards, > Arend > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19 August 2015 at 23:43, Arend van Spriel <arend@broadcom.com> wrote: > On 08/19/2015 11:21 PM, Arend van Spriel wrote: >> >> subject changed to v2. So let's go over your beef. >> >> On 07/11/2015 07:09 PM, Rafa? Mi?ecki wrote: >>> >>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote: >>>> >>>> @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp) >>>> u32 cplen; >>>> >>>> c = nvp->data[nvp->pos]; >>>> - if (!is_nvram_char(c)) { >>>> + if (!is_nvram_char(c) && (c != ' ')) { >>> >>> >>> This is redundant, please drop this change. >>> See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces") >> >> >> done >> >>>> @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const >>>> struct firmware *fw, void *ctx) >>>> struct brcmf_fw *fwctx = ctx; >>>> u32 nvram_length = 0; >>>> void *nvram = NULL; >>>> + u8 *data = NULL; >>> >>> >>> This can be const. >> >> >> done > > > Actually this is not done, but either way will require a cast because > bcm47xx_nvram_release_contents expects char* so there is nothing gained. > Unless someone will change bcm47xx_nvram_get/release_contents api to const > char*. Passing non-const pointer to function taking const one is OK. You don't need casting, compiler won't complain about this. On the other hand casing const pointer to the non-const one is hacky and I believe you should avoid that. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19 August 2015 at 23:21, Arend van Spriel <arend@broadcom.com> wrote: > subject changed to v2. So let's go over your beef. I really hope none of my comment was mean or anything :) > On 07/11/2015 07:09 PM, Rafa? Mi?ecki wrote: >> >> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote: >>> + data_len = fw->size; >>> + raw_nvram = false; >>> + } else { >>> + data = bcm47xx_nvram_get_contents(&data_len); >>> + if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) >>> + goto fail; >>> + raw_nvram = true; >>> + } >>> >>> - if (fw) { >>> - nvram = brcmf_fw_nvram_strip(fw->data, fw->size, >>> &nvram_length, >>> + if (data) { >>> + nvram = brcmf_fw_nvram_strip(data, data_len, >>> &nvram_length, >>> fwctx->domain_nr, >>> fwctx->bus_nr); >>> - release_firmware(fw); >>> - if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) >>> - goto fail; >>> + if (raw_nvram) >>> + bcm47xx_nvram_release_contents(data); >> >> >> This is cosmetical but maybe you could move above 2 lines next to the >> release_firmware? So we have all freeing code at one please? Do you >> think it would improve readability? >> Nothing important thought. Feel free to ignore me here. > > > confused! The release_firmware call is removed here, right? Yes, you removed it from the "if (data) {" condition body but also re-added right after it. AFAIR I got an impression it may make more sense to have something like: if (raw_nvram) bcm47xx_nvram_release_contents(data); if (fw) release_firmware(fw); but you can just ignore it if it doesn't sound clear. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/20/2015 05:53 PM, Rafa? Mi?ecki wrote: > On 19 August 2015 at 23:43, Arend van Spriel <arend@broadcom.com> wrote: >> On 08/19/2015 11:21 PM, Arend van Spriel wrote: >>> >>> subject changed to v2. So let's go over your beef. >>> >>> On 07/11/2015 07:09 PM, Rafa? Mi?ecki wrote: >>>> >>>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote: >>>>> >>>>> @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp) >>>>> u32 cplen; >>>>> >>>>> c = nvp->data[nvp->pos]; >>>>> - if (!is_nvram_char(c)) { >>>>> + if (!is_nvram_char(c) && (c != ' ')) { >>>> >>>> >>>> This is redundant, please drop this change. >>>> See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces") >>> >>> >>> done >>> >>>>> @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const >>>>> struct firmware *fw, void *ctx) >>>>> struct brcmf_fw *fwctx = ctx; >>>>> u32 nvram_length = 0; >>>>> void *nvram = NULL; >>>>> + u8 *data = NULL; >>>> >>>> >>>> This can be const. >>> >>> >>> done >> >> >> Actually this is not done, but either way will require a cast because >> bcm47xx_nvram_release_contents expects char* so there is nothing gained. >> Unless someone will change bcm47xx_nvram_get/release_contents api to const >> char*. > > Passing non-const pointer to function taking const one is OK. You > don't need casting, compiler won't complain about this. bcm47xx_nvram_release_contents expect a non-const pointer so the const data pointer needs to be cast to non-const. Which you claim is hacky. Here is what happens when I make data pointer const: CC [M] drivers/net/wireless/brcm80211/brcmfmac/firmware.o drivers/net/wireless/brcm80211/brcmfmac/firmware.c: In function ???brcmf_fw_request_nvram_done???: drivers/net/wireless/brcm80211/brcmfmac/firmware.c:450:4: warning: passing argument 1 of ???bcm47xx_nvram_release_contents??? discards ???const??? qualifier from pointer target type [enabled by default] bcm47xx_nvram_release_contents(data); ^ In file included from drivers/net/wireless/brcm80211/brcmfmac/firmware.c:22:0: include/linux/bcm47xx_nvram.h:44:20: note: expected ???char *??? but argument is of type ???const u8 *??? static inline void bcm47xx_nvram_release_contents(char *nvram) ^ > On the other hand casing const pointer to the non-const one is hacky > and I believe you should avoid that. Either way you have to do a cast from const to non-const. u8 *data => data = (u8 *)fw->data; const u8 *data => bcm47xx_nvram_release_contents((char *)data); Regards, Arend -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/20/2015 05:59 PM, Rafa? Mi?ecki wrote: > On 19 August 2015 at 23:21, Arend van Spriel <arend@broadcom.com> wrote: >> subject changed to v2. So let's go over your beef. > > I really hope none of my comment was mean or anything :) > > >> On 07/11/2015 07:09 PM, Rafa? Mi?ecki wrote: >>> >>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote: >>>> + data_len = fw->size; >>>> + raw_nvram = false; >>>> + } else { >>>> + data = bcm47xx_nvram_get_contents(&data_len); >>>> + if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) >>>> + goto fail; >>>> + raw_nvram = true; >>>> + } >>>> >>>> - if (fw) { >>>> - nvram = brcmf_fw_nvram_strip(fw->data, fw->size, >>>> &nvram_length, >>>> + if (data) { >>>> + nvram = brcmf_fw_nvram_strip(data, data_len, >>>> &nvram_length, >>>> fwctx->domain_nr, >>>> fwctx->bus_nr); >>>> - release_firmware(fw); >>>> - if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) >>>> - goto fail; >>>> + if (raw_nvram) >>>> + bcm47xx_nvram_release_contents(data); >>> >>> >>> This is cosmetical but maybe you could move above 2 lines next to the >>> release_firmware? So we have all freeing code at one please? Do you >>> think it would improve readability? >>> Nothing important thought. Feel free to ignore me here. >> >> >> confused! The release_firmware call is removed here, right? > > Yes, you removed it from the "if (data) {" condition body but also > re-added right after it. AFAIR I got an impression it may make more > sense to have something like: > if (raw_nvram) > bcm47xx_nvram_release_contents(data); I did not check whether bcm47xx_nvram_release_contents deals with data being NULL pointer. If so, I can change it. Regards, Arend > if (fw) > release_firmware(fw); > but you can just ignore it if it doesn't sound clear. > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20 August 2015 at 18:06, Arend van Spriel <arend@broadcom.com> wrote: > On 08/20/2015 05:53 PM, Rafa? Mi?ecki wrote: >> >> On 19 August 2015 at 23:43, Arend van Spriel <arend@broadcom.com> wrote: >>> >>> On 08/19/2015 11:21 PM, Arend van Spriel wrote: >>>> >>>> >>>> subject changed to v2. So let's go over your beef. >>>> >>>> On 07/11/2015 07:09 PM, Rafa? Mi?ecki wrote: >>>>> >>>>> >>>>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote: >>>>>> >>>>>> >>>>>> @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp) >>>>>> u32 cplen; >>>>>> >>>>>> c = nvp->data[nvp->pos]; >>>>>> - if (!is_nvram_char(c)) { >>>>>> + if (!is_nvram_char(c) && (c != ' ')) { >>>>> >>>>> >>>>> >>>>> This is redundant, please drop this change. >>>>> See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces") >>>> >>>> >>>> >>>> done >>>> >>>>>> @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const >>>>>> struct firmware *fw, void *ctx) >>>>>> struct brcmf_fw *fwctx = ctx; >>>>>> u32 nvram_length = 0; >>>>>> void *nvram = NULL; >>>>>> + u8 *data = NULL; >>>>> >>>>> >>>>> >>>>> This can be const. >>>> >>>> >>>> >>>> done >>> >>> >>> >>> Actually this is not done, but either way will require a cast because >>> bcm47xx_nvram_release_contents expects char* so there is nothing gained. >>> Unless someone will change bcm47xx_nvram_get/release_contents api to >>> const >>> char*. >> >> >> Passing non-const pointer to function taking const one is OK. You >> don't need casting, compiler won't complain about this. > > > bcm47xx_nvram_release_contents expect a non-const pointer so the const data > pointer needs to be cast to non-const. Which you claim is hacky. > Here is what happens when I make data pointer const: > > CC [M] drivers/net/wireless/brcm80211/brcmfmac/firmware.o > drivers/net/wireless/brcm80211/brcmfmac/firmware.c: In function > ???brcmf_fw_request_nvram_done???: > drivers/net/wireless/brcm80211/brcmfmac/firmware.c:450:4: warning: passing > argument 1 of ???bcm47xx_nvram_release_contents??? discards ???const??? > qualifier from pointer target type [enabled by default] > bcm47xx_nvram_release_contents(data); > ^ > In file included from > drivers/net/wireless/brcm80211/brcmfmac/firmware.c:22:0: > include/linux/bcm47xx_nvram.h:44:20: note: expected ???char *??? but > argument is of type ???const u8 *??? > static inline void bcm47xx_nvram_release_contents(char *nvram) > ^ >> >> On the other hand casing const pointer to the non-const one is hacky >> and I believe you should avoid that. > > > Either way you have to do a cast from const to non-const. > > u8 *data => data = (u8 *)fw->data; > const u8 *data => bcm47xx_nvram_release_contents((char *)data); Oh, I feel silly. Yeah, you're right. In OpenWrt I was using two separated variables and it was what basically let it work. Just ignore that noise from me :|
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c index 743f16b..1503937 100644 --- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c +++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c @@ -19,6 +19,7 @@ #include <linux/device.h> #include <linux/firmware.h> #include <linux/module.h> +#include <linux/bcm47xx_nvram.h> #include "debug.h" #include "firmware.h" @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp) u32 cplen; c = nvp->data[nvp->pos]; - if (!is_nvram_char(c)) { + if (!is_nvram_char(c) && (c != ' ')) { /* key,value pair complete */ ekv = (u8 *)&nvp->data[nvp->pos]; skv = (u8 *)&nvp->data[nvp->entry]; @@ -207,6 +208,7 @@ static int brcmf_init_nvram_parser(struct nvram_parser *nvp, memset(nvp, 0, sizeof(*nvp)); nvp->data = data; + /* Limit size to MAX_NVRAM_SIZE, some files contain lot of comment */ if (data_len > BRCMF_FW_MAX_NVRAM_SIZE) size = BRCMF_FW_MAX_NVRAM_SIZE; @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx) struct brcmf_fw *fwctx = ctx; u32 nvram_length = 0; void *nvram = NULL; + u8 *data = NULL; + size_t data_len; + bool raw_nvram; brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev)); - if (!fw && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) - goto fail; + if ((fw) && (fw->data)) { + data = (u8 *)fw->data; + data_len = fw->size; + raw_nvram = false; + } else { + data = bcm47xx_nvram_get_contents(&data_len); + if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) + goto fail; + raw_nvram = true; + } - if (fw) { - nvram = brcmf_fw_nvram_strip(fw->data, fw->size, &nvram_length, + if (data) { + nvram = brcmf_fw_nvram_strip(data, data_len, &nvram_length, fwctx->domain_nr, fwctx->bus_nr); - release_firmware(fw); - if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) - goto fail; + if (raw_nvram) + bcm47xx_nvram_release_contents(data); } + if (fw) + release_firmware(fw); + if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) + goto fail; + fwctx->done(fwctx->dev, fwctx->code, nvram, nvram_length); kfree(fwctx); return; @@ -473,15 +490,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx) if (!ret) return; - /* when nvram is optional call .done() callback here */ - if (fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL) { - fwctx->done(fwctx->dev, fw, NULL, 0); - kfree(fwctx); - return; - } + brcmf_fw_request_nvram_done(NULL, fwctx); + return; - /* failed nvram request */ - release_firmware(fw); fail: brcmf_dbg(TRACE, "failed: dev=%s\n", dev_name(fwctx->dev)); device_release_driver(fwctx->dev);