Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210805093023.3465081-1-linus.walleij@linaro.org>
In-Reply-To: <20210805093023.3465081-1-linus.walleij@linaro.org>
From:   Arend van Spriel <aspriel@gmail.com>
Date:   Thu, 5 Aug 2021 13:35:03 +0200
Message-ID: <CAJ65rDx1ab8gBZRpnyp5kb__VVA+_Vy3VS-0xesYC_+VmEKchw@mail.gmail.com>
Subject: Re: [PATCH v3] brcmfmac: firmware: Fix firmware loading
To:     Linus Walleij <linus.walleij@linaro.org>
Cc:     Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        Chi-hsien Lin <chi-hsien.lin@infineon.com>,
        Wright Feng <wright.feng@infineon.com>,
        Chung-hsien Hsu <chung-hsien.hsu@infineon.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Dmitry Osipenko <digetx@gmail.com>,
        Stefan Hansson <newbyte@disroot.org>,
        Arend van Spriel <arend.vanspriel@broadcom.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Aug 5, 2021 at 11:32 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> The patch that would first try the board-specific firmware
> had a bug because the fallback would not be called: the
> asynchronous interface is used meaning request_firmware_nowait()
> returns 0 immediately.
>
> Harden the firmware loading like this:
>
> - If we cannot build an alt_path (like if no board_type is
>   specified) just request the first firmware without any
>   suffix, like in the past.
>
> - If the lookup of a board specific firmware fails, we get
>   a NULL fw in the async callback, so just try again without
>   the alt_path. Use a context state variable to check that
>   we do not try this indefinitely.
>
> - Rename the brcm_fw_request_done to brcm_fw_request_done_first
>   reflecting the fact that this callback is only used for the
>   first (main) firmware file, and drop the unnecessary
>   prototype.

While implementing the firmware.c module at first I was doing every
firmware request with the 'nowait' variant hence the callback was used
repeatedly. However, I abandoned that as the reason for async request
was to avoid delay it may cause on kernel boot. Decoupling the initial
firmware request was sufficient for that and simplified things quite a
bit. As to the naming maybe 'brcmf_fw_async_request_done()' is a clear
alternative.

> Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware binaries")
> Cc: Dmitry Osipenko <digetx@gmail.com>
> Cc: Stefan Hansson <newbyte@disroot.org>
> Tested-by: Dmitry Osipenko <digetx@gmail.com>
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v2->v3:
> - Rename state variable to "tried_board_variant".
> ChangeLog v1->v2:
> - Instead of using a static variable, add a context variable
>   "tested_board_variant"
> - Collect Arend's review tag.
> - Collect Tested-by from Dmitry.
> ---
>  .../broadcom/brcm80211/brcmfmac/firmware.c    | 31 +++++++++++++------
>  1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> index adfdfc654b10..d32491fd74fe 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> @@ -428,11 +428,10 @@ struct brcmf_fw {
>         struct device *dev;
>         struct brcmf_fw_request *req;
>         u32 curpos;
> +       bool tried_board_variant;
>         void (*done)(struct device *dev, int err, struct brcmf_fw_request *req);
>  };
>
> -static void brcmf_fw_request_done(const struct firmware *fw, void *ctx);
> -
>  #ifdef CONFIG_EFI
>  /* In some cases the EFI-var stored nvram contains "ccode=ALL" or "ccode=XV"
>   * to specify "worldwide" compatible settings, but these 2 ccode-s do not work
> @@ -638,11 +637,25 @@ static int brcmf_fw_request_firmware(const struct firmware **fw,
>         return request_firmware(fw, cur->path, fwctx->dev);
>  }
>
> -static void brcmf_fw_request_done(const struct firmware *fw, void *ctx)
> +static void brcmf_fw_request_done_first(const struct firmware *fw, void *ctx)
>  {
>         struct brcmf_fw *fwctx = ctx;
> +       struct brcmf_fw_item *first = &fwctx->req->items[0];
>         int ret;
>
> +       /* Something failed with the first firmware request, such as not
> +        * getting the per-board firmware. Retry this, now using the less
> +        * specific path for the first firmware item, i.e. without the board
> +        * suffix.
> +        */
> +       if (!fw && !fwctx->tried_board_variant) {
> +               fwctx->tried_board_variant = true;
> +               ret = request_firmware_nowait(THIS_MODULE, true, first->path,
> +                                             fwctx->dev, GFP_KERNEL, fwctx,
> +                                             brcmf_fw_request_done_first);
> +               return;
> +       }
> +

So here we could use the synchronous variant instead for the reason
explained earlier.

Regards,
Arend