Date:   Thu, 17 Mar 2022 14:31:45 +0100
From:   Paul Menzel <pmenzel@...gen.mpg.de>
To:     Manish Chopra <manishc@...vell.com>
Cc:     netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        Ariel Elior <aelior@...vell.com>, regressions@...ts.linux.dev,
        stable@...r.kernel.org, it+netdev@...gen.mpg.de
Subject: Re: [EXT] Re: [PATCH net] bnx2x: fix built-in kernel driver load
 failure


Dear Manish,


Am 17.03.22 um 10:55 schrieb Manish Chopra:
>> -----Original Message-----
>> From: Paul Menzel <pmenzel@...gen.mpg.de>
>> Sent: Thursday, March 17, 2022 1:03 PM

[…]

>> 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.
> 
> I believe this problem has nothing to do with initrd module/FW but
> rather a module built in the kernel/vmlinuz (CONFIG_BNX2X=y) itself, 
> A module load from initrd works fine and can access the initrd FW
> files present in initrd file system even during the probe. For
> example, when I had CONFIG_BNX2X=m, it loads the module fine from
> initrd with FW files present in initrd file system. When I had
> CONFIG_BNX2X=y, which I believe doesn't install/load module in/from
> initrd but in kernel (vmlinuz) itself, that's where it can't access
> the firmware file and cause the load failure.

I can only say, that adding the firmware to the initrd worked around the 
problem on our side with `CONFIG_BNX2X=y`.

>>> "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.
> 
> I didn't get it, you can't downgrade to any firmware unless you
> downgrade the kernel/driver, driver with these recent commits/fixes
> are always supposed to run with this minimum older FW version
> (7.13.15.0) or a greater FW version (for example now 7.13.21.0). if
> you want to use the FW version even older to these then you have to 
> downgrade the driver/kernel as well which won't have these
> commits/logics in them.
I just want you to explicitly describe the added condition in the commit
message.

>>> Cc: stable@...r.kernel.org
>>> Link: https://urldefense.proofpoint.com/v2/url?u=https-
>> 3A__lore.kernel.org_all_46f2d9d9-2Dae7f-2Db332-2Dddeb-
>> 2Db59802be2bab-
>> 40molgen.mpg.de_&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=bMTgx2X4
>> 8QVXyXOEL8ALyI4dsWoR-m74c5n3d-ruJI8&m=FmUvhi_ygxQI4mnQmg5pU-
>> th-BWb_aEXUni5bpt6e934rZh0Wp-
>> KuVdfW7N2O0za&s=t3mHF8L4_cacsuvE5TqHUBSqD70Yfsbk3or973FvyEQ&e=

Thank you Outlook for protecting the word. :/

>>> 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?
> 
> We don't change the engineering version at all, it's just for sanity
> and going to always remain as zero. We use this only for debugging
> purpose, to differentiate debug FW version vs official FW version.

Please add a comment to make that clean. (Also the existing comment
should be updated.)

>>> +		    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.
> 
> I didn't keep it purposely here (to get rid of 100 chars warning) :- ), given the debug print just before this already logs
> all the version info about the already loaded_fw which can be enabled to get those info. I would prefer a seeparate commit
> on refining/enabling messages or adding any new informative messages for user about FW versioning related.

Well, the warning does not apply to log messages, so – as it’s present 
before – and debug logging is cumbersome to enable, please leave it.


Kind regards,

Paul


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