brcmfmac: firmware: Fix firmware loading

PATCH Superseded Dmitry Osipenko 1 participant 2 messages Aug 8, 2021 Broadcom BRCM80211 IEEE802.11 wireless drivers

lore

Share & tools

lore

Dmitry Osipenko

author

2021-08-08 14:30 UTC #0 lore raw

PATCH v4 1/1 brcmfmac: firmware: Fix firmware loading

From: Linus Walleij <[email protected]>

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 from a dedicated brcm_fw_request_done_alt_path callback.

- Drop the unnecessary prototype of brcm_fw_request_done.

Fixes 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware binaries")

Cc Stefan Hansson <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

Signed-off-by Dmitry Osipenko <[email protected]>

---
ChangeLog v3->v4:
- Added brcmf_fw_request_done_alt_path() callback to replace the "tried_board_variant" variable, making code cleaner and errors handled consistently.
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    | 24 ++++++++++++++-----
 1 file changed, 18 insertions(+), 6 deletions(-)

drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c +18 -6

index adfdfc654b10..0eb13e5df517 100644
@@ -431,8 +431,6 @@ struct brcmf_fw {
 	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

@@ -658,6 +656,22 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx)
 	kfree(fwctx);
 }
 
+static void brcmf_fw_request_done_alt_path(const struct firmware *fw, void *ctx)
+{
+	struct brcmf_fw *fwctx = ctx;
+	struct brcmf_fw_item *first = &fwctx->req->items[0];
+	int ret = 0;
+
+	/* Fall back to canonical path if board firmware not found */
+	if (!fw)
+		ret = request_firmware_nowait(THIS_MODULE, true, first->path,
+					      fwctx->dev, GFP_KERNEL, fwctx,
+					      brcmf_fw_request_done);
+
+	if (fw || ret < 0)
+		brcmf_fw_request_done(fw, ctx);
+}
+
 static bool brcmf_fw_request_is_valid(struct brcmf_fw_request *req)
 {
 	struct brcmf_fw_item *item;

@@ -702,11 +716,9 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
 	if (alt_path) {
 		ret = request_firmware_nowait(THIS_MODULE, true, alt_path,
 					      fwctx->dev, GFP_KERNEL, fwctx,
-					      brcmf_fw_request_done);
+					      brcmf_fw_request_done_alt_path);
 		kfree(alt_path);
-	}
-	/* Else try canonical path */
-	if (ret) {
+	} else {
 		ret = request_firmware_nowait(THIS_MODULE, true, first->path,
 					      fwctx->dev, GFP_KERNEL, fwctx,
 					      brcmf_fw_request_done);

Signature · 1 line

2.32.0

Dmitry Osipenko

author

2021-08-08 16:24 UTC #1 lore raw

08.08.2021 17:28, Dmitry Osipenko пишет:

Quoted 36 lines

From: Linus Walleij <[email protected]>
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 from a dedicated brcm_fw_request_done_alt_path callback.
- Drop the unnecessary prototype of brcm_fw_request_done.
Fixes 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware binaries")
Cc Stefan Hansson <[email protected]>
Signed-off-by Linus Walleij <[email protected]>
Signed-off-by Dmitry Osipenko <[email protected]>
---
ChangeLog v3->v4:
- Added brcmf_fw_request_done_alt_path() callback to replace the "tried_board_variant" variable, making code cleaner and errors handled consistently.
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.

Quoted 3 lines

---
 .../broadcom/brcm80211/brcmfmac/firmware.c    | 24 ++++++++++++++-----
 1 file changed, 18 insertions(+), 6 deletions(-)

I'm now wondering whether we also need to specify the MODULE_FIRMWARE to cover the board binaries.

drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +3 -0

index 97ee9e2e2e35..e7150312d186 100644
@@ -625,6 +625,9 @@ BRCMF_FW_DEF(4359, "brcmfmac4359-sdio");
 BRCMF_FW_CLM_DEF(4373, "brcmfmac4373-sdio");
 BRCMF_FW_CLM_DEF(43012, "brcmfmac43012-sdio");
 
+/* firmware including board-specific binaries */
+MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-sdio.*.bin");
+
 /* firmware config files */
 MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-sdio.*.txt");
 MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.*.txt");