On 08/20/2015 05:59 PM, Rafał Miłecki wrote:

On 19 August 2015 at 23:21, Arend van Spriel [off-list ref] wrote:

quoted

subject changed to v2. So let's go over your beef.

I really hope none of my comment was mean or anything :)

quoted

On 07/11/2015 07:09 PM, Rafał Miłecki wrote:

quoted

On 10 July 2015 at 20:31, Arend van Spriel [off-list ref] wrote:

quoted

+               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.