Re: [PATCH v2 1/7] brcmfmac: Add support for host platform NVRAM loading.

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

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
>