diff mbox series

[v3] brcmfmac: firmware: Fix firmware loading

Message ID 20210805093023.3465081-1-linus.walleij@linaro.org
State New
Headers show
Series [v3] brcmfmac: firmware: Fix firmware loading | expand

Commit Message

Linus Walleij Aug. 5, 2021, 9:30 a.m. UTC 
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 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(-)

-- 
2.31.1

Comments

Dmitry Osipenko Aug. 5, 2021, 10:31 a.m. UTC | #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 <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 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);
--
Arend Van Spriel Aug. 5, 2021, 11:35 a.m. UTC | #2 
On Thu, Aug 5, 2021 at 11:32 AM Linus Walleij <linus.walleij@linaro.org> 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@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 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.

Regards,
Arend
Dmitry Osipenko Aug. 6, 2021, 3:45 p.m. UTC | #3 
05.08.2021 14:35, Arend van Spriel пишет:
> On Thu, Aug 5, 2021 at 11:32 AM Linus Walleij <linus.walleij@linaro.org> 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@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 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.
Dmitry Osipenko Aug. 6, 2021, 4 p.m. UTC | #4 
06.08.2021 18:45, Dmitry Osipenko пишет:
..
>> 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)
...
Linus Walleij Aug. 7, 2021, 11:02 p.m. UTC | #5 
On Thu, Aug 5, 2021 at 12:31 PM Dmitry Osipenko <digetx@gmail.com> 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
Dmitry Osipenko Aug. 8, 2021, 2:23 p.m. UTC | #6 
08.08.2021 02:02, Linus Walleij пишет:
> On Thu, Aug 5, 2021 at 12:31 PM Dmitry Osipenko <digetx@gmail.com> 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.)


Alright, let's try that.
diff mbox series

Patch

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