| 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
| ||
|
Date: Thu, 17 Mar 2022 08:32:47 +0100
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Manish Chopra <manishc@...vell.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, aelior@...vell.com,
regressions@...ts.linux.dev, stable@...r.kernel.org,
it+netdev@...gen.mpg.de
Subject: Re: [PATCH net] bnx2x: fix built-in kernel driver load failure
Dear Manish,
Am 16.03.22 um 22:46 schrieb Manish Chopra:
> commit b7a49f73059f ("bnx2x: Utilize firmware 7.13.21.0")
> added request_firmware() logic in probe() which caused
> built-in kernel driver (CONFIG_BNX2X=y) load failure (below),
> as access to firmware file is not feasible during the probe.
I think it’s important to document, that the firmware was not present in
the initrd.
> "Direct firmware load for bnx2x/bnx2x-e2-7.13.21.0.fw
> failed with error -2"
I’d say, no line break for log message. Maybe paste the excerpt below:
[ 20.534985] bnx2x 0000:45:00.0: msix capability found
[ 20.540342] bnx2x 0000:45:00.0: part number
394D4342-31373735-31314131-473331
[ 20.548605] bnx2x 0000:45:00.0: Direct firmware load for
bnx2x/bnx2x-e1h-7.13.21.0.fw failed with error -2
[ 20.558373] bnx2x 0000:45:00.0: Direct firmware load for
bnx2x/bnx2x-e1h-7.13.15.0.fw failed with error -2
[ 20.568319] bnx2x: probe of 0000:45:00.0 failed with error -2
> This patch fixes this issue by -
>
> 1. Removing request_firmware() logic from the probe()
> such that .ndo_open() handle it as it used to handle
> it earlier
>
> 2. Given request_firmware() is removed from probe(), so
> driver has to relax FW version comparisons a bit against
> the already loaded FW version (by some other PFs of same
> adapter) to allow different compatible/close FWs with which
> multiple PFs may run with (in different environments), as the
> given PF who is in probe flow has no idea now with which firmware
> file version it is going to initialize the device in ndo_open()
Please be specific and state, that the revision part in the version has
to be greater, and that downgrading is not allowed.
> Cc: stable@...r.kernel.org
> Link: https://lore.kernel.org/all/46f2d9d9-ae7f-b332-ddeb-b59802be2bab@molgen.mpg.de/
> Reported-by: Paul Menzel <pmenzel@...gen.mpg.de>
> Tested-by: Paul Menzel <pmenzel@...gen.mpg.de>
> Fixes: b7a49f73059f ("bnx2x: Utilize firmware 7.13.21.0")
> Signed-off-by: Manish Chopra <manishc@...vell.com>
> Signed-off-by: Ariel Elior <aelior@...vell.com>
> ---
> 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);
>
> /* 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 ||
The engineering version comes after the revision, so I’d assume they can
also be relaxed and differ?
> + loaded_fw_rev < BCM_5710_FW_REVISION_VERSION_V15) {
> if (print_err)
Unrelated, this print_err argument added in commit 91ebb929b6f8 (bnx2x:
Add support for Multi-Function UNDI) is not so elegant.
> - 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");
Please add the versions to the error message to give the user more clues.
> 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:
Kind regards,
Paul
Powered by blists - more mailing lists