From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-gw1-out.broadcom.com ([216.31.210.62]:34381 "EHLO mail-gw1-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751406AbbHSVnE (ORCPT ); Wed, 19 Aug 2015 17:43:04 -0400 Message-ID: <55D4F864.4000604@broadcom.com> (sfid-20150819_234310_337801_D3FC57B4) Date: Wed, 19 Aug 2015 23:43:00 +0200 From: Arend van Spriel MIME-Version: 1.0 To: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= CC: Kalle Valo , linux-wireless , Hante Meuleman Subject: Re: [PATCH v2 1/7] brcmfmac: Add support for host platform NVRAM loading. References: <1436553071-32423-1-git-send-email-arend@broadcom.com> <1436553071-32423-2-git-send-email-arend@broadcom.com> <55D4F34D.2010505@broadcom.com> In-Reply-To: <55D4F34D.2010505@broadcom.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: 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 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 >