Re: [EXT] Re: [PATCH net-next 1/2] bnx2x: Utilize firmware 7.13.20.0

From: Andrew Lunn <andrew@lunn.ch>
To: Ariel Elior <aelior@marvell.com>
Cc: Jakub Kicinski <kuba@kernel.org>,
	Manish Chopra <manishc@marvell.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	Sudarsana Reddy Kalluru <skalluru@marvell.com>,
	"malin1024@gmail.com" <malin1024@gmail.com>,
	Shai Malin <smalin@marvell.com>,
	Omkar Kulkarni <okulkarni@marvell.com>,
	Nilesh Javali <njavali@marvell.com>,
	"GR-everest-linux-l2@marvell.com"
	<GR-everest-linux-l2@marvell.com>
Subject: Re: [EXT] Re: [PATCH net-next 1/2] bnx2x: Utilize firmware 7.13.20.0
Date: Wed, 27 Oct 2021 14:18:36 +0200	[thread overview]
Message-ID: <YXlDnCZIlVl1Etgs@lunn.ch> (raw)
In-Reply-To: <PH0PR18MB465598CDD29377C300C3184CC4859@PH0PR18MB4655.namprd18.prod.outlook.com>

> > ----------------------------------------------------------------------
> > On Tue, 26 Oct 2021 12:37:16 -0700 Manish Chopra wrote:
> > > Commit 0050dcf3e848 ("bnx2x: Add FW 7.13.20.0") added a new .bin
> > > firmware file to linux-firmware.git tree. This new firmware addresses
> > > few important issues and enhancements as mentioned below -
> > >
> > > - Support direct invalidation of FP HSI Ver per function ID, required for
> > >   invalidating FP HSI Ver prior to each VF start, as there is no VF
> > > start
> > > - BRB hardware block parity error detection support for the driver
> > > - Fix the FCOE underrun flow
> > > - Fix PSOD during FCoE BFS over the NIC ports after preboot driver
> > >
> > > This patch incorporates this new firmware 7.13.20.0 in bnx2x driver.
> > 
> > How is this expected to work? Your drivers seems to select a very specific FW
> > version:
> > 
> > 	/* Check FW version */
> > 	offset = be32_to_cpu(fw_hdr->fw_version.offset);
> > 	fw_ver = firmware->data + offset;
> > 	if ((fw_ver[0] != BCM_5710_FW_MAJOR_VERSION) ||
> > 	    (fw_ver[1] != BCM_5710_FW_MINOR_VERSION) ||
> > 	    (fw_ver[2] != BCM_5710_FW_REVISION_VERSION) ||
> > 	    (fw_ver[3] != BCM_5710_FW_ENGINEERING_VERSION)) {
> > 		BNX2X_ERR("Bad FW version:%d.%d.%d.%d. Should be
> > %d.%d.%d.%d\n",
> > 		       fw_ver[0], fw_ver[1], fw_ver[2], fw_ver[3],
> > 		       BCM_5710_FW_MAJOR_VERSION,
> > 		       BCM_5710_FW_MINOR_VERSION,
> > 		       BCM_5710_FW_REVISION_VERSION,
> > 		       BCM_5710_FW_ENGINEERING_VERSION);
> > 		return -EINVAL;
> > 	}
> > 
> > so this change has a dependency on user updating their /lib/firmware.
> > 
> > Is it really okay to break the systems for people who do not have that FW
> > version with a stable backport?
> > 
> > Greg, do you have general guidance for this or is it subsystem by subsystem?

I have been pushing back on a similar change for the Marvell Prestera
driver, which also loads the firmware from /lib/firmware and they are
proposing to break the ABI to the firmware, and not support older
version.

I don't like this. As Jakub points out, you are going to break systems
which don't update the firmware and the kernel at the same time. I
really would prefer you support two versions of the firmware, and
detect what features it supports to runtime.

	Andrew