05.08.2021 14:35, Arend van Spriel пишет:
> On Thu, Aug 5, 2021 at 11:32 AM Linus Walleij <linus.walleij@xxxxxxxxxx> 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@xxxxxxxxx>
>> Cc: Stefan Hansson <newbyte@xxxxxxxxxxx>
>> Tested-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>> Reviewed-by: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx>
>> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>> ---
>> 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.
The blocking variant doesn't work as-is, it locks up the probe.
