[v5] brcmfmac: firmware: Fix firmware loading

Commit Message

Dmitry Osipenko Aug. 8, 2021, 6:05 p.m. UTC

From: Linus Walleij <[e-mail]>

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 <[e-mail]>
Signed-off-by: Linus Walleij <[e-mail]>
Signed-off-by: Dmitry Osipenko <[e-mail]>
---
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(-)

Comments

Arend van Spriel Aug. 9, 2021, 8:23 a.m. UTC | #1

On 8/8/2021 8:05 PM, Dmitry Osipenko wrote:
> From: Linus Walleij <[e-mail]>
> 
> 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.
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

Dmitry Osipenko Aug. 9, 2021, 2:56 p.m. UTC | #2

09.08.2021 11:23, Arend van Spriel пишет:
> On 8/8/2021 8:05 PM, Dmitry Osipenko wrote:
>> From: Linus Walleij <[e-mail]>
>>
>> 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.
> 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.

> 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 Aug. 9, 2021, 3:42 p.m. UTC | #3

On August 9, 2021 4:56:46 PM Dmitry Osipenko <[e-mail]> wrote:

> 09.08.2021 11:23, Arend van Spriel пишет:
>> On 8/8/2021 8:05 PM, Dmitry Osipenko wrote:
>>> From: Linus Walleij <[e-mail]>
>>>
>>> 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.
>> 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.
>
>> 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 Aug. 9, 2021, 3:58 p.m. UTC | #4

09.08.2021 18:42, Arend van Spriel пишет:
> On August 9, 2021 4:56:46 PM Dmitry Osipenko <[e-mail]> wrote:
> 
>> 09.08.2021 11:23, Arend van Spriel пишет:
>>> On 8/8/2021 8:05 PM, Dmitry Osipenko wrote:
>>>> From: Linus Walleij <[e-mail]>
>>>>
>>>> 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.
>>> 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.
>>
>>> 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.

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

Dmitry Osipenko Aug. 10, 2021, 2:15 p.m. UTC | #5

08.08.2021 21:05, Dmitry Osipenko пишет:
> From: Linus Walleij <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Linus Walleij <[e-mail]>
> Signed-off-by: Dmitry Osipenko <[e-mail]>
> ---

Kalle, please apply it with this tag. Thanks!

Reviewed-by: Arend van Spriel <[e-mail]>

Kalle Valo Aug. 21, 2021, 3:46 p.m. UTC | #6

Dmitry Osipenko <[e-mail]> wrote:

> From: Linus Walleij <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Linus Walleij <[e-mail]>
> Signed-off-by: Dmitry Osipenko <[e-mail]>
> Reviewed-by: Arend van Spriel <[e-mail]>

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

c2dac3d2d3f1 brcmfmac: firmware: Fix firmware loading

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index adfdfc654b10..0eb13e5df517 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,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);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 97ee9e2e2e35..1d1b0b7d8d9b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -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),