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 <redacted>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Linus Walleij <redacted>
---
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(-)
@@ -428,11 +428,10 @@ struct brcmf_fw {structdevice*dev;structbrcmf_fw_request*req;u32curpos;+booltested_board_variant;void(*done)(structdevice*dev,interr,structbrcmf_fw_request*req);};-staticvoidbrcmf_fw_request_done(conststructfirmware*fw,void*ctx);-#ifdef CONFIG_EFI/* In some cases the EFI-var stored nvram contains "ccode=ALL" or "ccode=XV"*tospecify"worldwide"compatiblesettings,butthese2ccode-sdonotwork
@@ -638,11 +637,25 @@ static int brcmf_fw_request_firmware(const struct firmware **fw,returnrequest_firmware(fw,cur->path,fwctx->dev);}-staticvoidbrcmf_fw_request_done(conststructfirmware*fw,void*ctx)+staticvoidbrcmf_fw_request_done_first(conststructfirmware*fw,void*ctx){structbrcmf_fw*fwctx=ctx;+structbrcmf_fw_item*first=&fwctx->req->items[0];intret;+/* Something failed with the first firmware request, such as not+*gettingtheper-boardfirmware.Retrythis,nowusingtheless+*specificpathforthefirstfirmwareitem,i.e.withouttheboard+*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);return0;}
+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;
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
On Thu, Aug 5, 2021 at 3:22 AM Dmitry Osipenko [off-list ref] wrote:
04.08.2021 18:34, Linus Walleij пишет:
quoted
+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
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
On Thu, Aug 5, 2021 at 3:44 AM Dmitry Osipenko [off-list ref] wrote:
04.08.2021 18:34, Linus Walleij пишет:
quoted
+ 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.
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
On Thu, Aug 5, 2021 at 3:22 AM Dmitry Osipenko [off-list ref] wrote:
quoted
04.08.2021 18:34, Linus Walleij пишет:
quoted
+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.
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.
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();
}
}
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.