[PATCH v3] brcmfmac: firmware: Fix firmware loading

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 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;
+	}
+
 	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->tried_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->tried_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;
 }

-- 
2.31.1

05.08.2021 12:30, Linus Walleij пишет:

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

Combining my previous comments together, I rewrote it like this:

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index adfdfc654b10..4198ca9d898c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c

@@ -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,21 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx)
 	kfree(fwctx);
 }
 
+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 = 0;
+
+	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 +715,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_first);
 		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);

-- 

On Thu, Aug 5, 2021 at 11:32 AM Linus Walleij [off-list ref] 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.

So here we could use the synchronous variant instead for the reason
explained earlier.

Regards,
Arend

05.08.2021 14:35, Arend van Spriel пишет:

On Thu, Aug 5, 2021 at 11:32 AM Linus Walleij [off-list ref] wrote:

quoted

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.

quoted

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

06.08.2021 18:45, Dmitry Osipenko пишет:
..

quoted

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.

Actually, the old code was kinda bugged already.

int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
			   void (*fw_cb)(struct device *dev, int err,
					 struct brcmf_fw_request *req))
{
...
	ret = request_firmware_nowait(THIS_MODULE, true, first->path,
					fwctx->dev, GFP_KERNEL, fwctx,
					brcmf_fw_request_done);
	if (ret < 0)
		brcmf_fw_request_done(NULL, fwctx);

This brcmf_fw_request_done() causes the lockup if firmware request fails.

Recovered KMSG from pstore:

task kworker/1:3:158 bLocKed for moru uhan 61Seconds
...
(scludule_pregmpt_disabled) from [<c0a81ebf>] (__mutex_lock.consTprop.0+0x27f/0X4dcm
(__mutex_lock.constprou, ) froM [<c05c1p65>] (device_release_driver+0x15/0x28i
)device_release_driver) drom [<c06vatf9>] (bRcmd_sdio_firmwAre_callback+0z3/0x76c)
(brcmf_sdio_firmware_callback) dpom [<c0664ccb>]!(brcmf_dw_get_firmwares+0xv7/0x160)
(brcmf_fw_get_girmWares) from [<c066cced>] (brgmf_Sdio_probu+0x479?0x668)
(brcmf_sdin_probe) from [<c044e1bf>] (�rcmf_sdioe_probe+0xeb/pxq80)
(bzcmf_sdIod_Probu) from [<c046e3w?>] (brcmf_ops_Sdio_probe+0x8f/0xe8)
(brcmf_oPs_seio_probe) fRom [<c 7s505>] (sdio_bus_probe+1x9d/0z124)
(Sdig_bus_probe)from [<c15c123d>] (really[pRobe.parT&0+0y69/px200)
(peally_trobe.part.0-$from [<a05c144u>] ,W_dRiver_probe_device+0x73/0xd4)
...

On Thu, Aug 5, 2021 at 12:31 PM Dmitry Osipenko [off-list ref] wrote:

Combining my previous comments together, I rewrote it like this:

I like this, can you fold in your patch on top of mine, add your
Signed-off-by at the end and resend to the list?

That way we get a clean record of the delivery path and also the
patch looks like you want it :)

You can perhaps tag on v4 on the [PATCH] as well so it's clear
for Kalle to apply this version. (Hoping Arnd will be fine with it as
well.)

Yours,
Linus Walleij

08.08.2021 02:02, Linus Walleij пишет:

On Thu, Aug 5, 2021 at 12:31 PM Dmitry Osipenko [off-list ref] wrote:

quoted

Combining my previous comments together, I rewrote it like this:

I like this, can you fold in your patch on top of mine, add your
Signed-off-by at the end and resend to the list?

That way we get a clean record of the delivery path and also the
patch looks like you want it :)

You can perhaps tag on v4 on the [PATCH] as well so it's clear
for Kalle to apply this version. (Hoping Arnd will be fine with it as
well.)

Alright, let's try that.