[PATCH v2] brcmfmac: firmware: Fix firmware loading

[PATCH v2] brcmfmac: firmware: Fix firmware loading

[Linus Walleij]

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.

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 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..2b0fb79c5ede 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 tested_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->tested_board_variant) {
+		fwctx->tested_board_variant = true;
+		ret = request_firmware_nowait(THIS_MODULE, true, first->path,
+					      fwctx->dev, GFP_KERNEL, fwctx,
+					      brcmf_fw_request_done_first);
+		return;
+	}
+
 	ret = brcmf_fw_complete_request(fw, fwctx);
 
 	while (ret == 0 && ++fwctx->curpos < fwctx->req->n_items) {
@@ -700,19 +713,19 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
 	/* First try alternative board-specific path if any */
 	alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type);
 	if (alt_path) {
+		fwctx->tested_board_variant = false;
 		ret = request_firmware_nowait(THIS_MODULE, true, alt_path,
 					      fwctx->dev, GFP_KERNEL, fwctx,
-					      brcmf_fw_request_done);
+					      brcmf_fw_request_done_first);
 		kfree(alt_path);
-	}
-	/* Else try canonical path */
-	if (ret) {
+	} else {
+		fwctx->tested_board_variant = true;
 		ret = request_firmware_nowait(THIS_MODULE, true, first->path,
 					      fwctx->dev, GFP_KERNEL, fwctx,
-					      brcmf_fw_request_done);
+					      brcmf_fw_request_done_first);
 	}
 	if (ret < 0)
-		brcmf_fw_request_done(NULL, fwctx);
+		brcmf_fw_request_done_first(NULL, fwctx);
 
 	return 0;
 }
-- 
2.31.1

Re: [PATCH v2] brcmfmac: firmware: Fix firmware loading

[Dmitry Osipenko]

04.08.2021 18:34, Linus Walleij пишет:
> +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->tested_board_variant) {
> +		fwctx->tested_board_variant = true;
> +		ret = request_firmware_nowait(THIS_MODULE, true, first->path,
> +					      fwctx->dev, GFP_KERNEL, fwctx,
> +					      brcmf_fw_request_done_first);
> +		return;

The original code was proceeding on error. Is this a typo here?

if (!ret)
	return;

Re: [PATCH v2] brcmfmac: firmware: Fix firmware loading

[Dmitry Osipenko]

04.08.2021 18:34, Linus Walleij пишет:
> +	bool tested_board_variant;

What about s/tested/tried/?

>  	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)

Is it really worthwhile to rename this function? There is no "done_second".

Re: [PATCH v2] brcmfmac: firmware: Fix firmware loading

[Dmitry Osipenko]

04.08.2021 18:34, Linus Walleij пишет:
> +		fwctx->tested_board_variant = false;

This shouldn't be really needed, isn't it?

>  		ret = request_firmware_nowait(THIS_MODULE, true, alt_path,
>  					      fwctx->dev, GFP_KERNEL, fwctx,
> -					      brcmf_fw_request_done);
> +					      brcmf_fw_request_done_first);
>  		kfree(alt_path);
> -	}
> -	/* Else try canonical path */
> -	if (ret) {


> +	} else {
> +		fwctx->tested_board_variant = true;
>  		ret = request_firmware_nowait(THIS_MODULE, true, first->path,
>  					      fwctx->dev, GFP_KERNEL, fwctx,
> -					      brcmf_fw_request_done);
> +					      brcmf_fw_request_done_first);
>  	}
>  	if (ret < 0)
> -		brcmf_fw_request_done(NULL, fwctx);
> +		brcmf_fw_request_done_first(NULL, fwctx);

This "else" can be replaced with:

if (!alt_path || ret < 0)
	brcmf_fw_request_done(NULL, fwctx);

Re: [PATCH v2] brcmfmac: firmware: Fix firmware loading

[Linus Walleij]

On Thu, Aug 5, 2021 at 3:22 AM Dmitry Osipenko <digetx@gmail.com> wrote:
> 04.08.2021 18:34, Linus Walleij пишет:
> > +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->tested_board_variant) {
> > +             fwctx->tested_board_variant = true;
> > +             ret = request_firmware_nowait(THIS_MODULE, true, first->path,
> > +                                           fwctx->dev, GFP_KERNEL, fwctx,
> > +                                           brcmf_fw_request_done_first);
> > +             return;
>
> The original code was proceeding on error. Is this a typo here?

No, we are testing specifically for fw being NULL and in that case we issue
a new request_firmware_nowait() call with ourselves as "done" callback,
so we really need to return here.

The worker will call the same function again after this but now
tested_board_variant is true.

Yours,
Linus Walleij

Re: [PATCH v2] brcmfmac: firmware: Fix firmware loading

[Linus Walleij]

On Thu, Aug 5, 2021 at 3:35 AM Dmitry Osipenko <digetx@gmail.com> wrote:
> 04.08.2021 18:34, Linus Walleij пишет:
> > +     bool tested_board_variant;
>
> What about s/tested/tried/?

OK that is clearer, I fix!

> > -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)
>
> Is it really worthwhile to rename this function? There is no "done_second".

It is to reflect the actual use, because it fooled me as it could
be interpreted (intuitively) as "this is called when all firmware requests
are done" since it doesn't specify. But that is not the case, it is
only called when done with the first first firmware in the list.
Hence the name change.

The philosophy is in line with Rusty Russell's API design hierarchy:
http://sweng.the-davies.net/Home/rustys-api-design-manifesto

Yours,
Linus Walleij

Re: [PATCH v2] brcmfmac: firmware: Fix firmware loading

[Linus Walleij]

On Thu, Aug 5, 2021 at 3:44 AM Dmitry Osipenko <digetx@gmail.com> wrote:

> 04.08.2021 18:34, Linus Walleij пишет:
> > +             fwctx->tested_board_variant = false;
>
> This shouldn't be really needed, isn't it?

It is true in the sense that fwctx is allocated with kzalloc.

But it is done in a different function brcmf_fw_alloc_request()
and it is not entirely clear that the allocated struct is not reused,
and if someone refactors the code they could reuse it, so I add
this just to be 100% sure that this gets set to false.

Usually in the kernel when we have functions named *alloc*
it is allocating objects that can later be reused several times
(see the block layer for example), so I try to follow that pattern
here and assume that fwctx can be reused.

The only time I really rely on fields being zero is when the
allocation is in the same base block, e.g. inside probe() or
so.

> > +     } else {
> > +             fwctx->tested_board_variant = true;
> >               ret = request_firmware_nowait(THIS_MODULE, true, first->path,
> >                                             fwctx->dev, GFP_KERNEL, fwctx,
> > -                                           brcmf_fw_request_done);
> > +                                           brcmf_fw_request_done_first);
> >       }
> >       if (ret < 0)
> > -             brcmf_fw_request_done(NULL, fwctx);
> > +             brcmf_fw_request_done_first(NULL, fwctx);
>
> This "else" can be replaced with:
>
> if (!alt_path || ret < 0)
>         brcmf_fw_request_done(NULL, fwctx);

Sorry I don't quite get this... both branches of the if/else clause will
assign ret also if alt_path is set request_firmware_nowait() can return
nonzero and then brcmf_fw_request_done() needs to get
called?

Yours,
Linus Walleij

Re: [PATCH v2] brcmfmac: firmware: Fix firmware loading

[Dmitry Osipenko]

05.08.2021 12:14, Linus Walleij пишет:
> On Thu, Aug 5, 2021 at 3:22 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>> 04.08.2021 18:34, Linus Walleij пишет:
>>> +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->tested_board_variant) {
>>> +             fwctx->tested_board_variant = true;
>>> +             ret = request_firmware_nowait(THIS_MODULE, true, first->path,
>>> +                                           fwctx->dev, GFP_KERNEL, fwctx,
>>> +                                           brcmf_fw_request_done_first);
>>> +             return;
>>
>> The original code was proceeding on error. Is this a typo here?
> 
> No, we are testing specifically for fw being NULL and in that case we issue
> a new request_firmware_nowait() call with ourselves as "done" callback,
> so we really need to return here.
> 
> The worker will call the same function again after this but now
> tested_board_variant is true.

The worker won't call the same function if request_firmware_nowait() fails.

Re: [PATCH v2] brcmfmac: firmware: Fix firmware loading

[Dmitry Osipenko]

05.08.2021 12:17, Linus Walleij пишет:
> On Thu, Aug 5, 2021 at 3:35 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>> 04.08.2021 18:34, Linus Walleij пишет:
>>> +     bool tested_board_variant;
>>
>> What about s/tested/tried/?
> 
> OK that is clearer, I fix!
> 
>>> -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)
>>
>> Is it really worthwhile to rename this function? There is no "done_second".
> 
> It is to reflect the actual use, because it fooled me as it could
> be interpreted (intuitively) as "this is called when all firmware requests
> are done" since it doesn't specify. But that is not the case, it is
> only called when done with the first first firmware in the list.
> Hence the name change.

AFAICS, it's called for both first and when done.

> The philosophy is in line with Rusty Russell's API design hierarchy:
> http://sweng.the-davies.net/Home/rustys-api-design-manifesto

Why you can't split that function in two then?

brcmf_fw_request_done_first()
{
	if (!fw) {
		request_firmware_nowait(THIS_MODULE, true, first->path,
					fwctx->dev, GFP_KERNEL, fwctx,
					brcmf_fw_request_done);
	} else {
		brcmf_fw_request_done();
	}
}

Re: [PATCH v2] brcmfmac: firmware: Fix firmware loading

[Dmitry Osipenko]

05.08.2021 12:27, Linus Walleij пишет:
>>> +     } else {
>>> +             fwctx->tested_board_variant = true;
>>>               ret = request_firmware_nowait(THIS_MODULE, true, first->path,
>>>                                             fwctx->dev, GFP_KERNEL, fwctx,
>>> -                                           brcmf_fw_request_done);
>>> +                                           brcmf_fw_request_done_first);
>>>       }
>>>       if (ret < 0)
>>> -             brcmf_fw_request_done(NULL, fwctx);
>>> +             brcmf_fw_request_done_first(NULL, fwctx);
>> This "else" can be replaced with:
>>
>> if (!alt_path || ret < 0)
>>         brcmf_fw_request_done(NULL, fwctx);
> Sorry I don't quite get this... both branches of the if/else clause will
> assign ret also if alt_path is set request_firmware_nowait() can return
> nonzero and then brcmf_fw_request_done() needs to get
> called?

That call will request the first->path from brcmf_fw_request_done()
since fw=NULL and fwctx->tested_board_variant=false. Hence the "else"
branch can be omitted.