| Message ID | 20210804153434.3429348-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] brcmfmac: firmware: Fix firmware loading | expand |
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;
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".
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);
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
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
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
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.
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(); } }
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.
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; }
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(-)