bnx2x: Latest firmware requirement breaks no regression policy

bnx2x: Latest firmware requirement breaks no regression policy

[Paul Menzel]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

Dear Linux folks,


Updating a server from 4.19.x to 5.4.x, the network device

    Ethernet controller [0200]: Broadcom Inc. and subsidiaries NetXtreme II BCM57711 10-Gigabit PCIe [14e4:164f]

failed to initialize due to missing firmware.

    bnx2x 0000:41:00.0: Direct firmware load for bnx2x/bnx2x-e1h-7.13.11.0.fw failed with error -2
    bnx2x: [bnx2x_init_firmware:13557(net02)]Can't load firmware file bnx2x/bnx2x-e1h-7.13.11.0.fw
    bnx2x: [bnx2x_func_hw_init:6002(net02)]Error loading firmware
    bnx2x: [bnx2x_nic_load:2730(net02)]HW init failed, aborting

That is undesired, because without network access one has to have
direct system access to find out what is wrong.

Could you please change the policy to only print a big warning,
if the latest firmware is not available, and an update suggestion?


Kind regards,

Paul


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

RE: bnx2x: Latest firmware requirement breaks no regression policy

[Sudarsana Reddy Kalluru]

Hi Paul,
    The firmware file referred below (i.e., storm FW) should be present on the host (i.e., /lib/firmware/bnx2x/ path), not the device. Driver must require this version of the FW to initialize the device, and hence provide the network functionality. Also, the driver is not backward compatible with older FW versions.
So it's not possible to handle the below error scenario in the driver,
	>     bnx2x 0000:41:00.0: Direct firmware load for bnx2x/bnx2x-e1h-7.13.11.0.fw failed with error -2
	>     bnx2x: [bnx2x_init_firmware:13557(net02)]Can't load firmware file bnx2x/bnx2x-e1h-7.13.11.0.fw
At the most, we can validate the existence of FW file on the host during the kernel build or installation.
FW image name from driver sources:
	drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:
	#define FW_FILE_NAME_E1         "bnx2x/bnx2x-e1-" FW_FILE_VERSION ".fw"
	#define FW_FILE_NAME_E1H        "bnx2x/bnx2x-e1h-" FW_FILE_VERSION ".fw"
	#define FW_FILE_NAME_E2         "bnx2x/bnx2x-e2-" FW_FILE_VERSION ".fw"
FW image path on the host:
	/lib/firmware/bnx2x/bnx2x-e1h-7.13.11.0.fw

Thanks,
Sudarsana
> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Paul Menzel
> Sent: Monday, February 17, 2020 6:48 PM
> To: Ariel Elior <aelior@marvell.com>; Sudarsana Reddy Kalluru
> <skalluru@marvell.com>; GR-everest-linux-l2 <GR-everest-linux-
> l2@marvell.com>
> Cc: netdev@vger.kernel.org; LKML <linux-kernel@vger.kernel.org>; it+linux-
> netdev@molgen.mpg.de
> Subject: bnx2x: Latest firmware requirement breaks no regression policy
> 
> Dear Linux folks,
> 
> 
> Updating a server from 4.19.x to 5.4.x, the network device
> 
>     Ethernet controller [0200]: Broadcom Inc. and subsidiaries NetXtreme II
> BCM57711 10-Gigabit PCIe [14e4:164f]
> 
> failed to initialize due to missing firmware.
> 
>     bnx2x 0000:41:00.0: Direct firmware load for bnx2x/bnx2x-e1h-7.13.11.0.fw
> failed with error -2
>     bnx2x: [bnx2x_init_firmware:13557(net02)]Can't load firmware file
> bnx2x/bnx2x-e1h-7.13.11.0.fw
>     bnx2x: [bnx2x_func_hw_init:6002(net02)]Error loading firmware
>     bnx2x: [bnx2x_nic_load:2730(net02)]HW init failed, aborting
> 
> That is undesired, because without network access one has to have
> direct system access to find out what is wrong.
> 
> Could you please change the policy to only print a big warning,
> if the latest firmware is not available, and an update suggestion?
> 
> 
> Kind regards,
> 
> Paul

Re: bnx2x: Latest firmware requirement breaks no regression policy

[Paul Menzel]

[-- Attachment #1: Type: text/plain, Size: 2200 bytes --]

Dear Sudarsana,


Thank you for your reply.


On 2020-02-19 09:49, Sudarsana Reddy Kalluru wrote:

> The firmware file referred below (i.e., storm FW) should be present
> on the host (i.e., /lib/firmware/bnx2x/ path), not the device. Driver
> must require this version of the FW to initialize the device, and
> hence provide the network functionality. Also, the driver is not
> backward compatible with older FW versions.
> 
> So it's not possible to handle the below error scenario in the driver,
> 
> 	>     bnx2x 0000:41:00.0: Direct firmware load for bnx2x/bnx2x-e1h-7.13.11.0.fw failed with error -2
> 	>     bnx2x: [bnx2x_init_firmware:13557(net02)]Can't load firmware file bnx2x/bnx2x-e1h-7.13.11.0.fw
> 
> At the most, we can validate the existence of FW file on the host
> during the kernel build or installation.

That is what I thought about the current state. But why was this
design decision made? It’s not user-friendly, and as written breaks
the no regression policy. Users can update the Linux kernel without
any regressions, and everything working as before. Dave, what is
your opinion?

Where are the driver requirements/implementation short-comings
documented?

If an older Linux kernel works with a certain firmware version, why
shouldn’t a newer Linux kernel work with that firmware version.
Maybe some features are missing, but at least I should get the same
state as with the older version.

Do you have plans to switch the driver to a model, where the
features/requirements of the firmware are queried by the driver, so
older versions can be supported?

> FW image name from driver sources:
> 	drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:
> 	#define FW_FILE_NAME_E1         "bnx2x/bnx2x-e1-" FW_FILE_VERSION ".fw"
> 	#define FW_FILE_NAME_E1H        "bnx2x/bnx2x-e1h-" FW_FILE_VERSION ".fw"
> 	#define FW_FILE_NAME_E2         "bnx2x/bnx2x-e2-" FW_FILE_VERSION ".fw"
> FW image path on the host:
> 	/lib/firmware/bnx2x/bnx2x-e1h-7.13.11.0.fw
Yes, that is what I found in my original research, and that is how
we fixed it, but with the non-working interface it was more work
than necessary.


Kind regards,

Paul


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

RE: [EXT] Re: bnx2x: Latest firmware requirement breaks no regression policy

[Sudarsana Reddy Kalluru]

Hi Paul,
    Bnx2x driver and the storm FW are tightly coupled, and the info is exchanged between them via shmem (i.e., common structures which might change between the releases). Also, FW provides some offset addresses to the driver which could change between the FW releases, following is one such commit,
	https://www.spinics.net/lists/netdev/msg609889.html
Hence it's not very straight forward to provide the backward compatibility i.e., newer (updated) kernel driver with the older FW.
Currently we don’t have plans to implement the new model mentioned below.

Thanks,
Sudarsana
> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Wednesday, February 19, 2020 6:14 PM
> To: Sudarsana Reddy Kalluru <skalluru@marvell.com>; Ariel Elior
> <aelior@marvell.com>; GR-everest-linux-l2 <GR-everest-linux-
> l2@marvell.com>
> Cc: netdev@vger.kernel.org; LKML <linux-kernel@vger.kernel.org>; it+linux-
> netdev@molgen.mpg.de; David S. Miller <davem@davemloft.net>
> Subject: [EXT] Re: bnx2x: Latest firmware requirement breaks no regression
> policy
> 
> External Email
> 
> ----------------------------------------------------------------------
> Dear Sudarsana,
> 
> 
> Thank you for your reply.
> 
> 
> On 2020-02-19 09:49, Sudarsana Reddy Kalluru wrote:
> 
> > The firmware file referred below (i.e., storm FW) should be present
> > on the host (i.e., /lib/firmware/bnx2x/ path), not the device. Driver
> > must require this version of the FW to initialize the device, and
> > hence provide the network functionality. Also, the driver is not
> > backward compatible with older FW versions.
> >
> > So it's not possible to handle the below error scenario in the driver,
> >
> > 	>     bnx2x 0000:41:00.0: Direct firmware load for bnx2x/bnx2x-e1h-
> 7.13.11.0.fw failed with error -2
> > 	>     bnx2x: [bnx2x_init_firmware:13557(net02)]Can't load firmware file
> bnx2x/bnx2x-e1h-7.13.11.0.fw
> >
> > At the most, we can validate the existence of FW file on the host
> > during the kernel build or installation.
> 
> That is what I thought about the current state. But why was this
> design decision made? It’s not user-friendly, and as written breaks
> the no regression policy. Users can update the Linux kernel without
> any regressions, and everything working as before. Dave, what is
> your opinion?
> 
> Where are the driver requirements/implementation short-comings
> documented?
> 
> If an older Linux kernel works with a certain firmware version, why
> shouldn’t a newer Linux kernel work with that firmware version.
> Maybe some features are missing, but at least I should get the same
> state as with the older version.
> 
> Do you have plans to switch the driver to a model, where the
> features/requirements of the firmware are queried by the driver, so
> older versions can be supported?
> 
> > FW image name from driver sources:
> > 	drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:
> > 	#define FW_FILE_NAME_E1         "bnx2x/bnx2x-e1-" FW_FILE_VERSION
> ".fw"
> > 	#define FW_FILE_NAME_E1H        "bnx2x/bnx2x-e1h-"
> FW_FILE_VERSION ".fw"
> > 	#define FW_FILE_NAME_E2         "bnx2x/bnx2x-e2-" FW_FILE_VERSION
> ".fw"
> > FW image path on the host:
> > 	/lib/firmware/bnx2x/bnx2x-e1h-7.13.11.0.fw
> Yes, that is what I found in my original research, and that is how
> we fixed it, but with the non-working interface it was more work
> than necessary.
> 
> 
> Kind regards,
> 
> Paul

RE: [EXT] Re: bnx2x: Latest firmware requirement breaks no regression policy

[Ariel Elior]

> -----Original Message-----
> From: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> Sent: Thursday, February 20, 2020 11:17 AM
> To: Paul Menzel <pmenzel@molgen.mpg.de>; Ariel Elior
> <aelior@marvell.com>; GR-everest-linux-l2 <GR-everest-linux-
> l2@marvell.com>
> Cc: netdev@vger.kernel.org; LKML <linux-kernel@vger.kernel.org>; it+linux-
> netdev@molgen.mpg.de; David S. Miller <davem@davemloft.net>
> Subject: RE: [EXT] Re: bnx2x: Latest firmware requirement breaks no regression
> policy
> 
> Hi Paul,
>     Bnx2x driver and the storm FW are tightly coupled, and the info is exchanged
> between them via shmem (i.e., common structures which might change
> between the releases). Also, FW provides some offset addresses to the driver
> which could change between the FW releases, following is one such commit,
> 	https://www.spinics.net/lists/netdev/msg609889.html
> Hence it's not very straight forward to provide the backward compatibility i.e.,
> newer (updated) kernel driver with the older FW.
> Currently we don’t have plans to implement the new model mentioned below.
> 
> Thanks,
> Sudarsana
Hi,
There are additional reasons why backwards/forwards compatibility considerations
are not applicable here. This Fw is not nvram based, and does not reside in the
device. It is programed to the device on every driver load. The driver will
never face a device "already initialized" with a version of FW it is not
familiar with.
The device also has traditional management FW in nvram in the device with which
we have a backwards and forwards compatibility mechanism, which works just
fine.
But the FW under discussion is fastpath Fw, used to craft every packet going out
of the device and analyze and place every packet coming into the device.
Supporting multiple versions of FW would be tantamount to implementing dozens of
versions of start_xmit and napi_poll in the driver (not to mention multiple
fastpath handles of all the offloads the device supports, roce, iscsi, fcoe and
iwarp, as all of these are offloaded by the FW).
The entire device initialization sequence also changes significantly from one FW
version to the Next. All of these differences are abstracted away in the FW
file, which includes the init sequence and the compiled FW. The amount of
changes required in driver are very significant when moving from one version to
the next. Trying to keep all those versions alive concurrently would cause this
already very large driver to be 20x larger.
We don't have a method of keeping the device operational if the kernel was
upgraded but firmware tree was not updated. The best that can be done is report
the problem, which is what we do.
Thanks,
Ariel

Re: [EXT] Re: bnx2x: Latest firmware requirement breaks no regression policy

[Jakub Kicinski]

On Thu, 20 Feb 2020 15:40:37 +0000 Ariel Elior wrote:
> > -----Original Message-----
> > From: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> > Sent: Thursday, February 20, 2020 11:17 AM
> > To: Paul Menzel <pmenzel@molgen.mpg.de>; Ariel Elior
> > <aelior@marvell.com>; GR-everest-linux-l2 <GR-everest-linux-  
> > l2@marvell.com>  
> > Cc: netdev@vger.kernel.org; LKML <linux-kernel@vger.kernel.org>; it+linux-
> > netdev@molgen.mpg.de; David S. Miller <davem@davemloft.net>
> > Subject: RE: [EXT] Re: bnx2x: Latest firmware requirement breaks no regression
> > policy
> > 
> > Hi Paul,
> >     Bnx2x driver and the storm FW are tightly coupled, and the info is exchanged
> > between them via shmem (i.e., common structures which might change
> > between the releases). Also, FW provides some offset addresses to the driver
> > which could change between the FW releases, following is one such commit,
> > 	https://www.spinics.net/lists/netdev/msg609889.html
> > Hence it's not very straight forward to provide the backward compatibility i.e.,
> > newer (updated) kernel driver with the older FW.
> > Currently we don’t have plans to implement the new model mentioned below.
> > 
> Hi,
> There are additional reasons why backwards/forwards compatibility considerations
> are not applicable here. This Fw is not nvram based, and does not reside in the
> device. It is programed to the device on every driver load. The driver will
> never face a device "already initialized" with a version of FW it is not
> familiar with.

How do you deal with VFs?

> The device also has traditional management FW in nvram in the device with which
> we have a backwards and forwards compatibility mechanism, which works just
> fine.
> But the FW under discussion is fastpath Fw, used to craft every packet going out
> of the device and analyze and place every packet coming into the device.
> Supporting multiple versions of FW would be tantamount to implementing dozens of
> versions of start_xmit and napi_poll in the driver (not to mention multiple
> fastpath handles of all the offloads the device supports, roce, iscsi, fcoe and
> iwarp, as all of these are offloaded by the FW).
> The entire device initialization sequence also changes significantly from one FW
> version to the Next. All of these differences are abstracted away in the FW
> file, which includes the init sequence and the compiled FW. The amount of
> changes required in driver are very significant when moving from one version to
> the next. Trying to keep all those versions alive concurrently would cause this
> already very large driver to be 20x larger.

All your points are disproved by all the devices which have drivers
upstream and don't break backward compatibility on every release.

> We don't have a method of keeping the device operational if the kernel was
> upgraded but firmware tree was not updated. The best that can be done is report
> the problem, which is what we do.

RE: [EXT] Re: bnx2x: Latest firmware requirement breaks no regression policy

[Ariel Elior]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, February 21, 2020 2:38 AM
> To: Ariel Elior <aelior@marvell.com>
> Cc: Sudarsana Reddy Kalluru <skalluru@marvell.com>; Paul Menzel
> <pmenzel@molgen.mpg.de>; GR-everest-linux-l2 <GR-everest-linux-
> l2@marvell.com>; netdev@vger.kernel.org; LKML <linux-
> kernel@vger.kernel.org>; it+linux-netdev@molgen.mpg.de; David S. Miller
> <davem@davemloft.net>
> Subject: Re: [EXT] Re: bnx2x: Latest firmware requirement breaks no regression
> policy
> 
> On Thu, 20 Feb 2020 15:40:37 +0000 Ariel Elior wrote:
> > > -----Original Message-----
> > > From: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> > > Sent: Thursday, February 20, 2020 11:17 AM
> > > To: Paul Menzel <pmenzel@molgen.mpg.de>; Ariel Elior
> > > <aelior@marvell.com>; GR-everest-linux-l2 <GR-everest-linux-
> > > l2@marvell.com>
> > > Cc: netdev@vger.kernel.org; LKML <linux-kernel@vger.kernel.org>;
> > > it+linux- netdev@molgen.mpg.de; David S. Miller
> > > <davem@davemloft.net>
> > > Subject: RE: [EXT] Re: bnx2x: Latest firmware requirement breaks no
> > > regression policy
> > >
> > > Hi Paul,
> > >     Bnx2x driver and the storm FW are tightly coupled, and the info
> > > is exchanged between them via shmem (i.e., common structures which
> > > might change between the releases). Also, FW provides some offset
> > > addresses to the driver which could change between the FW releases,
> following is one such commit,
> > >
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net
> > >
> _lists_netdev_msg609889.html&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=c
> WB
> > >
> gNIFUifZRx2xhypdcaYrfIsMGt93NxP1r8GQtC0s&m=W4pEZSYmEGyvbakqtsYcE7
> OaK
> > >
> vjaEvlWjSFfUUOc65A&s=A7UihKukjHlFL16Z1MFf9zY0BItRgbDSAzwq48PkmvY&e
> =
> > > Hence it's not very straight forward to provide the backward
> > > compatibility i.e., newer (updated) kernel driver with the older FW.
> > > Currently we don’t have plans to implement the new model mentioned
> below.
> > >
> > Hi,
> > There are additional reasons why backwards/forwards compatibility
> > considerations are not applicable here. This Fw is not nvram based,
> > and does not reside in the device. It is programed to the device on
> > every driver load. The driver will never face a device "already
> > initialized" with a version of FW it is not familiar with.
> 
> How do you deal with VFs?
> 
> > The device also has traditional management FW in nvram in the device
> > with which we have a backwards and forwards compatibility mechanism,
> > which works just fine.
> > But the FW under discussion is fastpath Fw, used to craft every packet
> > going out of the device and analyze and place every packet coming into the
> device.
> > Supporting multiple versions of FW would be tantamount to implementing
> > dozens of versions of start_xmit and napi_poll in the driver (not to
> > mention multiple fastpath handles of all the offloads the device
> > supports, roce, iscsi, fcoe and iwarp, as all of these are offloaded by the FW).
> > The entire device initialization sequence also changes significantly
> > from one FW version to the Next. All of these differences are
> > abstracted away in the FW file, which includes the init sequence and
> > the compiled FW. The amount of changes required in driver are very
> > significant when moving from one version to the next. Trying to keep
> > all those versions alive concurrently would cause this already very large
> driver to be 20x larger.
> 
> All your points are disproved by all the devices which have drivers upstream
> and don't break backward compatibility on every release.
Note that we are talking about compatibility with a file on the disk, not with
the FW in the device. I don't think any of the other drivers have fastpath
FW which needs to be programmed to the device on every driver load. As
noted, this is not FW that resides in flash. It has to be reprogrammed on
driver load. Note that we do always make sure to have the new FW file
accepted in the Linux FW tree before we send to Dave the series for making
use of it. And the driver does detect that the FW file is not available and
fails the load flow gracefully.

I think this boils down to a simple question: Is the use case of upgrading
the kernel without pulling the FW tree a legitimate one? If so, I think the
solution we can offer is to create an .h file with the required init sequence
(FW an all) and have the driver compile with it, so it can always initialize
the device. This is how this device is initialized in OSes which don't support
dynamic FW load. This is not difficult to do and would solve the problem
you are facing (FW file not available on disk) but would also mean that the
compiled driver size would be much larger. Is that desirable?

As an aside, please note that this model is implemented qede for a few
years and in bnx2x for many years, and I did not get this complaint yet.

> 
> > We don't have a method of keeping the device operational if the kernel
> > was upgraded but firmware tree was not updated. The best that can be
> > done is report the problem, which is what we do.