From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.molgen.mpg.de (mx3.molgen.mpg.de [141.14.17.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0882D7A for ; Thu, 17 Mar 2022 07:32:50 +0000 (UTC) Received: from [192.168.0.3] (ip5f5aef3c.dynamic.kabel-deutschland.de [95.90.239.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) (Authenticated sender: pmenzel) by mx.molgen.mpg.de (Postfix) with ESMTPSA id 22B4C61EA192E; Thu, 17 Mar 2022 08:32:48 +0100 (CET) Message-ID: <35d305f5-aa84-2c47-7efd-66fffb91c398@molgen.mpg.de> Date: Thu, 17 Mar 2022 08:32:47 +0100 Precedence: bulk X-Mailing-List: regressions@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH net] bnx2x: fix built-in kernel driver load failure Content-Language: en-US To: Manish Chopra Cc: netdev@vger.kernel.org, kuba@kernel.org, aelior@marvell.com, regressions@lists.linux.dev, stable@vger.kernel.org, it+netdev@molgen.mpg.de References: <20220316214613.6884-1-manishc@marvell.com> From: Paul Menzel In-Reply-To: <20220316214613.6884-1-manishc@marvell.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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@vger.kernel.org > Link: https://lore.kernel.org/all/46f2d9d9-ae7f-b332-ddeb-b59802be2bab@molgen.mpg.de/ > Reported-by: Paul Menzel > Tested-by: Paul Menzel > Fixes: b7a49f73059f ("bnx2x: Utilize firmware 7.13.21.0") > Signed-off-by: Manish Chopra > Signed-off-by: Ariel Elior > --- > 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