2021-08-08 18:17:35

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v5] 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(-)

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),
--
2.32.0


2021-08-09 08:25:14

by Arend van Spriel

[permalink] [raw]
Subject: Re: [PATCH v5] brcmfmac: firmware: Fix firmware loading

On 8/8/2021 8:05 PM, Dmitry Osipenko wrote:
> 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.
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

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


Attachments:
smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2021-08-09 14:57:22

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v5] brcmfmac: firmware: Fix firmware loading

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

2021-08-09 15:46:11

by Arend Van Spriel

[permalink] [raw]
Subject: Re: [PATCH v5] brcmfmac: firmware: Fix firmware loading

On August 9, 2021 4:56:46 PM Dmitry Osipenko <[email protected]> wrote:

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


2021-08-09 16:03:35

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v5] brcmfmac: firmware: Fix firmware loading

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

2021-08-10 14:32:04

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v5] brcmfmac: firmware: Fix firmware loading

08.08.2021 21:05, 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.
>
> - 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]>

2021-08-21 15:47:04

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v5] brcmfmac: firmware: Fix firmware loading

Dmitry Osipenko <[email protected]> wrote:

> 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]>
> Reviewed-by: Arend van Spriel <[email protected]>

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

c2dac3d2d3f1 brcmfmac: firmware: Fix firmware loading

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

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