brcmfmac: firmware: Fix firmware loading

PATCH In review Dmitry Osipenko 4 participants 7 messages Aug 8, 2021 – Aug 21, 2021 Broadcom BRCM80211 IEEE802.11 wireless drivers

lore

Share & tools

lore

Dmitry Osipenko

author

2021-08-08 18:05 UTC #0 lore raw

PATCH v5 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.

- Added MODULE_FIRMWARE match for per-board SDIO bins, making userspace tools to pull all the relevant firmware files.

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 v4->v5:
- Added MODULE_FIRMWARE to catch per-board SDIO firmware files. 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 ++++++++++++++-----
 .../broadcom/brcm80211/brcmfmac/sdio.c        |  3 +++
 2 files changed, 21 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);

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

index 97ee9e2e2e35..1d1b0b7d8d9b 100644
@@ -629,6 +629,9 @@ BRCMF_FW_CLM_DEF(43012, "brcmfmac43012-sdio");
 MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-sdio.*.txt");
 MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.*.txt");
 
+/* per-board firmware binaries */
+MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-sdio.*.bin");
+
 static const struct brcmf_firmware_mapping brcmf_sdio_fwnames[] = {
 	BRCMF_FW_ENTRY(BRCM_CC_43143_CHIP_ID, 0xFFFFFFFF, 43143),
 	BRCMF_FW_ENTRY(BRCM_CC_43241_CHIP_ID, 0x0000001F, 43241B0),

Signature · 1 line

2.32.0

Arend van Spriel

maintainer

2021-08-09 08:23 UTC #1 lore raw

Quoted 18 lines · Dmitry Osipenko

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.

Quoted 4 lines

- Drop the unnecessary prototype of brcm_fw_request_done.
- Added MODULE_FIRMWARE match for per-board SDIO bins, making userspace tools to pull all the relevant firmware files.

The original idea was to setup the path names in brcmf_fw_alloc_request() function, but with the introduction of the board_type for NVRAM files that was abandoned and we cook up alternative paths. Now similar is done for the firmware files. So I would want to rework the code, but for now I am going with Linus's/Your fix for the sake of having the regression more or less quickly resolved.

You reported an issue earlier where the firmware callback was called from the probe context causing it to hang and it is not clear to me whether that is fixed with this version of the patch.

Regards,
Arend

Signature · 1 line

This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it.

Dmitry Osipenko

author

2021-08-09 14:56 UTC #2 lore raw

09.08.2021 11:23, Arend van Spriel пишет:

Quoted 21 lines · Dmitry Osipenko

Quoted 20 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.

Quoted 8 lines

Quoted 2 lines
- Added MODULE_FIRMWARE match for per-board SDIO bins, making userspace tools to pull all the relevant firmware files.
The original idea was to setup the path names in brcmf_fw_alloc_request() function, but with the introduction of the board_type for NVRAM files that was abandoned and we cook up alternative paths. Now similar is done for the firmware files. So I would want to rework the code, but for now I am going with Linus's/Your fix for the sake of having the regression more or less quickly resolved.

Thanks, indeed it could be improved further later on.

Quoted 3 lines

You reported an issue earlier where the firmware callback was called from the probe context causing it to hang and it is not clear to me whether that is fixed with this version of the patch.

It's independent minor problem that can't be easily reproduced in practice and seems it existed for a long time. It's not fixed by this patch.

Arend van Spriel

2021-08-09 15:42 UTC #3 lore raw

Quoted 31 lines · Dmitry Osipenko

09.08.2021 11:23, Arend van Spriel пишет:
Quoted 21 lines · Dmitry Osipenko
Quoted 20 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.
Quoted 8 lines
Quoted 2 lines
- Added MODULE_FIRMWARE match for per-board SDIO bins, making userspace tools to pull all the relevant firmware files.
The original idea was to setup the path names in brcmf_fw_alloc_request() function, but with the introduction of the board_type for NVRAM files that was abandoned and we cook up alternative paths. Now similar is done for the firmware files. So I would want to rework the code, but for now I am going with Linus's/Your fix for the sake of having the regression more or less quickly resolved.

Quoted 8 lines

Thanks, indeed it could be improved further later on.
Quoted 3 lines
You reported an issue earlier where the firmware callback was called from the probe context causing it to hang and it is not clear to me whether that is fixed with this version of the patch.
It's independent minor problem that can't be easily reproduced in practice and seems it existed for a long time. It's not fixed by this patch.

Ok. I understand the issue and I have a fix lined up for it. I noticed my reviewed-by tag got lost between V2 and latest version. Feel free to add it back.

Regards,
Arend

Dmitry Osipenko

author

2021-08-09 15:58 UTC #4 lore raw

09.08.2021 18:42, Arend van Spriel пишет:

Quoted 39 lines · Dmitry Osipenko

Quoted 38 lines
09.08.2021 11:23, Arend van Spriel пишет:
Quoted 21 lines · Dmitry Osipenko
Quoted 20 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.
Quoted 8 lines
Quoted 2 lines
- Added MODULE_FIRMWARE match for per-board SDIO bins, making userspace tools to pull all the relevant firmware files.
The original idea was to setup the path names in brcmf_fw_alloc_request() function, but with the introduction of the board_type for NVRAM files that was abandoned and we cook up alternative paths. Now similar is done for the firmware files. So I would want to rework the code, but for now I am going with Linus's/Your fix for the sake of having the regression more or less quickly resolved.
Thanks, indeed it could be improved further later on.
Quoted 3 lines
You reported an issue earlier where the firmware callback was called from the probe context causing it to hang and it is not clear to me whether that is fixed with this version of the patch.

Quoted 7 lines

Quoted 3 lines
It's independent minor problem that can't be easily reproduced in practice and seems it existed for a long time. It's not fixed by this patch.
Ok. I understand the issue and I have a fix lined up for it. I noticed my reviewed-by tag got lost between V2 and latest version. Feel free to add it back.

Please reply with yours r-b to the patch, it should be enough.

Dmitry Osipenko

author Reviewed-by Arend van Spriel on v5 1/1

2021-08-10 14:21 UTC #5 lore raw

In reply to Dmitry Osipenko

08.08.2021 21:05, Dmitry Osipenko пишет:

Quoted 20 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.

Quoted 8 lines

- Added MODULE_FIRMWARE match for per-board SDIO bins, making userspace tools to pull all the relevant firmware files.
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]>
---

Kalle, please apply it with this tag. Thanks!

Reviewed-by Arend van Spriel <[email protected]>

Kalle Valo

2021-08-21 15:46 UTC #6 lore raw

In reply to Dmitry Osipenko

Quoted 20 lines · Dmitry Osipenko

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.

Quoted 8 lines

- Added MODULE_FIRMWARE match for per-board SDIO bins, making userspace tools to pull all the relevant firmware files.
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]>
Reviewed-by Arend van Spriel <[email protected]>

Patch applied to wireless-drivers-next.git, thanks.

c2dac3d2d3f1 brcmfmac: firmware: Fix firmware loading

Signature · 3 lines

https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches