| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5f136c0c-2e16-d176-3d4a-caed6c3420a7@molgen.mpg.de>
Date: Wed, 16 Mar 2022 18:09:30 +0100
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Manish Chopra <manishc@...vell.com>
Cc: buczek@...gen.mpg.de, kuba@...nel.org, netdev@...r.kernel.org,
aelior@...vell.com, it+netdev@...gen.mpg.de,
regressions@...ts.linux.dev
Subject: Re: [RFC net] bnx2x: fix built-in kernel driver load failure
Dear Manish,
Thank you for the patch.
Am 16.03.22 um 12:18 schrieb Manish Chopra:
> commit b7a49f73059f ("bnx2x: Utilize firmware 7.13.21.0")
> added request_firmware() logic in probe() which caused
> built-in kernel driver load failure as access to firmware
> file is not feasible during the probe time.
… for example, when the initrd does not provide the firmware files.
Please also paste one example error message.
> This patch fixes this issue by -
>
> 1. Removing request_firmware() logic from the probe() such
> that open() handle it as it used to handle it earlier
>
> 2. Relaxing a bit FW version comparisons against the loaded FW
> (to allow many close/compatible FWs to run together now)
I’d prefer if you also pasted one error message, and even split this out
into a separate commit with elaborate problem description.
Style note: For the commit message, it’d be great if you used 75
characters per line.
> Reported-by: Paul Menzel <pmenzel@...gen.mpg.de>
> Fixes: b7a49f73059f ("bnx2x: Utilize firmware 7.13.21.0")
The regzbot also asks to add the tag below [1].
Link:
https://lore.kernel.org/r/46f2d9d9-ae7f-b332-ddeb-b59802be2bab@molgen.mpg.de
> Signed-off-by: Manish Chopra <manishc@...vell.com>
> Signed-off-by: Ariel Elior <aelior@...vell.com>
> ---
>
> Note that this patch is just for test and get feedback
> from Paul Menzel about the issue reported by him on built-in
> driver probe failure due to firmware files not found
Tested-by: Paul Menzel <pmenzel@...gen.mpg.de>
Dell PowerEdge R910/0KYD3D, BIOS 2.10.0 08/29/2013 with patch on top of
Linux 5.10.103 with 7.13.15.0 on the root partition:
$ lspci -nn -s 45:00.1
45:00.1 Ethernet controller [0200]: Broadcom Inc. and subsidiaries
NetXtreme II BCM57711 10-Gigabit PCIe [14e4:164f]
$ ethtool -i net05
driver: bnx2x
version: 5.10.103.mx64.429-00016-g597b02
firmware-version: 7.8.16 bc 6.2.26 phy aa0.406
expansion-rom-version:
bus-info: 0000:45:00.1
supports-statistics: yes
supports-test: yes
supports-eeprom-access: yes
supports-register-dump: yes
supports-priv-flags: yes
```
> drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 --
> .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 28 +++++++++++--------
> .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 15 ++--------
> 3 files changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> index a19dd6797070..2209d99b3404 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> @@ -2533,6 +2533,4 @@ void bnx2x_register_phc(struct bnx2x *bp);
> * Meant for implicit re-load flows.
> */
> int bnx2x_vlan_reconfigure_vid(struct bnx2x *bp);
> -int bnx2x_init_firmware(struct bnx2x *bp);
> -void bnx2x_release_firmware(struct bnx2x *bp);
> #endif /* bnx2x.h */
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> index 8d36ebbf08e1..5729a5ab059d 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -2364,24 +2364,30 @@ int bnx2x_compare_fw_ver(struct bnx2x *bp, u32 load_code, bool print_err)
> /* is another pf loaded on this engine? */
> if (load_code != FW_MSG_CODE_DRV_LOAD_COMMON_CHIP &&
> load_code != FW_MSG_CODE_DRV_LOAD_COMMON) {
> - /* build my FW version dword */
> - u32 my_fw = (bp->fw_major) + (bp->fw_minor << 8) +
> - (bp->fw_rev << 16) + (bp->fw_eng << 24);
> + u8 loaded_fw_major, loaded_fw_minor, loaded_fw_rev, loaded_fw_eng;
> + u32 loaded_fw;
>
> /* read loaded FW from chip */
> - u32 loaded_fw = REG_RD(bp, XSEM_REG_PRAM);
> + loaded_fw = REG_RD(bp, XSEM_REG_PRAM);
>
> - DP(BNX2X_MSG_SP, "loaded fw %x, my fw %x\n",
> - loaded_fw, my_fw);
> + loaded_fw_major = loaded_fw & 0xff;
> + loaded_fw_minor = (loaded_fw >> 8) & 0xff;
> + loaded_fw_rev = (loaded_fw >> 16) & 0xff;
> + loaded_fw_eng = (loaded_fw >> 24) & 0xff;
> +
> + DP(BNX2X_MSG_SP, "loaded fw 0x%x major 0x%x minor 0x%x rev 0x%x eng 0x%x\n",
> + loaded_fw, loaded_fw_major, loaded_fw_minor, loaded_fw_rev, loaded_fw_eng);
Hmm, with `CONFIG_BNX2X=y` and `bnx2x.debug=0x0100000`, bringing up
net05 (.1) and then net04 (.0), I only see:
[ 3333.883697] bnx2x: [bnx2x_compare_fw_ver:2378(net04)]loaded fw
f0d07 major 7 minor d rev f eng 0
For another patch, but the currently loaded firmware, and when loading
new firmware, the version of it, should also be logged by Linux (by
default, and not with debug level).
Also copying the 7.13.21.0 firmware on the running system, bringing the
interfaces down and up again, the newer firmware is not loaded, but it
stays with the 7.13.15.0:
[ 3533.374046] bnx2x: [bnx2x_compare_fw_ver:2378(net04)]loaded fw
f0d07 major 7 minor d rev f eng 0
> /* abort nic load if version mismatch */
> - if (my_fw != loaded_fw) {
> + if (loaded_fw_major != BCM_5710_FW_MAJOR_VERSION ||
> + loaded_fw_minor != BCM_5710_FW_MINOR_VERSION ||
> + loaded_fw_eng != BCM_5710_FW_ENGINEERING_VERSION ||
> + loaded_fw_rev < BCM_5710_FW_REVISION_VERSION_V15) {
> if (print_err)
> - BNX2X_ERR("bnx2x with FW %x was already loaded which mismatches my %x FW. Aborting\n",
> - loaded_fw, my_fw);
> + BNX2X_ERR("loaded FW incompatible. Aborting\n");
> else
> - BNX2X_DEV_INFO("bnx2x with FW %x was already loaded which mismatches my %x FW, possibly due to MF UNDI\n",
> - loaded_fw, my_fw);
> + BNX2X_DEV_INFO("loaded FW incompatible, possibly due to MF UNDI\n");
> +
> return -EBUSY;
> }
> }
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index eedb48d945ed..c19b072f3a23 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -12319,15 +12319,6 @@ static int bnx2x_init_bp(struct bnx2x *bp)
>
> bnx2x_read_fwinfo(bp);
>
> - if (IS_PF(bp)) {
> - rc = bnx2x_init_firmware(bp);
> -
> - if (rc) {
> - bnx2x_free_mem_bp(bp);
> - return rc;
> - }
> - }
> -
> func = BP_FUNC(bp);
>
> /* need to reset chip if undi was active */
> @@ -12340,7 +12331,6 @@ static int bnx2x_init_bp(struct bnx2x *bp)
>
> rc = bnx2x_prev_unload(bp);
> if (rc) {
> - bnx2x_release_firmware(bp);
> bnx2x_free_mem_bp(bp);
> return rc;
> }
> @@ -13409,7 +13399,7 @@ do { \
> (u8 *)bp->arr, len); \
> } while (0)
>
> -int bnx2x_init_firmware(struct bnx2x *bp)
> +static int bnx2x_init_firmware(struct bnx2x *bp)
> {
> const char *fw_file_name, *fw_file_name_v15;
> struct bnx2x_fw_file_hdr *fw_hdr;
> @@ -13509,7 +13499,7 @@ int bnx2x_init_firmware(struct bnx2x *bp)
> return rc;
> }
>
> -void bnx2x_release_firmware(struct bnx2x *bp)
> +static void bnx2x_release_firmware(struct bnx2x *bp)
> {
> kfree(bp->init_ops_offsets);
> kfree(bp->init_ops);
> @@ -14026,7 +14016,6 @@ static int bnx2x_init_one(struct pci_dev *pdev,
> return 0;
>
> init_one_freemem:
> - bnx2x_release_firmware(bp);
> bnx2x_free_mem_bp(bp);
>
> init_one_exit:
> --
> 2.35.1.273.ge6ebfd0
So why was the earlier firmware version comparison needed in commit
b7a49f73059f ("bnx2x: Utilize firmware 7.13.21.0")?
I let the maintainers decide how to best go forward.
Kind regards,
Paul
[1]: https://linux-regtracking.leemhuis.info/regzbot/mainline/
(click on the array to expand the information)
Powered by blists - more mailing lists