Re: [PATCH] bnx2: Use request_firmware()

Previous thread: [PATCH net-next] sctp: Clean up TEST_FRAME hacks. by Vlad Yasevich on Thursday, March 19, 2009 - 2:43 pm. (2 messages)

Next thread: claims of by Mrs Joan Thomas on Thursday, March 19, 2009 - 4:03 pm. (1 message)
From: Ben Hutchings
Date: Thursday, March 19, 2009 - 3:29 pm

Based on a patch for Debian by Bastian Blank <waldi@debian.org>.

This patch contains the code changes for review, but should not be
applied.  The complete patch, which is about 1 MB in size and therefore
too large for the mailing list, is at:
<http://womble.decadent.org.uk/tmp/0001-bnx2-Use-request_firmware.patch>.

Ben.

---
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 45403e6..0769f09 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -2206,6 +2206,7 @@ config BNX2
 	tristate "Broadcom NetXtremeII support"
 	depends on PCI
 	select CRC32
+	select FW_LOADER
 	select ZLIB_INFLATE
 	help
 	  This driver supports Broadcom NetXtremeII gigabit Ethernet cards.
diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 8466d35..54e9c06 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -46,12 +46,11 @@
 #include <linux/crc32.h>
 #include <linux/prefetch.h>
 #include <linux/cache.h>
-#include <linux/zlib.h>
+#include <linux/firmware.h>
 #include <linux/log2.h>
=20
 #include "bnx2.h"
 #include "bnx2_fw.h"
-#include "bnx2_fw2.h"
=20
 #define FW_BUF_SIZE		0x10000
=20
@@ -59,6 +58,8 @@
 #define PFX DRV_MODULE_NAME	": "
 #define DRV_MODULE_VERSION	"1.9.2"
 #define DRV_MODULE_RELDATE	"Feb 11, 2009"
+#define FW_FILE_06		"bnx2-06-4.6.16.fw"
+#define FW_FILE_09		"bnx2-09-4.6.15.fw"
=20
 #define RUN_AT(x) (jiffies + (x))
=20
@@ -72,6 +73,8 @@ MODULE_AUTHOR("Michael Chan <mchan@broadcom.com>");
 MODULE_DESCRIPTION("Broadcom NetXtreme II BCM5706/5708/5709/5716 Driver");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_MODULE_VERSION);
+MODULE_FIRMWARE(FW_FILE_06);
+MODULE_FIRMWARE(FW_FILE_09);
=20
 static int disable_msi =3D 0;
=20
@@ -3391,24 +3394,91 @@ bnx2_set_rx_mode(struct net_device *dev)
 	spin_unlock_bh(&bp->phy_lock);
 }
=20
-static void
-load_rv2p_fw(struct bnx2 *bp, __le32 *rv2p_code, u32 rv2p_code_len,
-	u32 rv2p_proc)
+static int
+check_fw_section(const struct firmware *fw,
+		 const struct bnx2_fw_file_section ...
From: Michael Chan
Date: Thursday, March 19, 2009 - 4:04 pm

Thanks Ben and Bastian.  I will review this and test it out.  I think I
will add some logic to check firmware versions to guarantee some level
of compatibility.



--

From: Bastian Blank
Date: Thursday, March 19, 2009 - 4:25 pm

The firmware version is coded into the filenames.

Bastian

-- 
Violence in reality is quite different from theory.
		-- Spock, "The Cloud Minders", stardate 5818.4
--

From: David Miller
Date: Friday, March 20, 2009 - 3:50 pm

From: Bastian Blank <waldi@debian.org>

Michael and co. please do not make such a big deal about
this.

This patch is perfectly fine as-is.
--

From: Michael Chan
Date: Monday, March 23, 2009 - 2:47 pm

Yes, the patch looks good.  I just noticed one problem while reviewing.
The following logic to let the firmware know about the host CPU's page
size is missing and I'll need to add it back.  Otherwise it will only
work on CPUs with 4K page size.

+static int
+load_rv2p_fw(struct bnx2 *bp, u32 rv2p_proc,
+            const struct bnx2_fw_file_section *fw_section)
+{
+       u32 rv2p_code_len, file_offset;
+       __be32 *rv2p_code;
        int i;
        u32 val;
 
-       if (rv2p_proc == RV2P_PROC2 && CHIP_NUM(bp) == CHIP_NUM_5709) {
-               val = le32_to_cpu(rv2p_code[XI_RV2P_PROC2_MAX_BD_PAGE_LOC]);
-               val &= ~XI_RV2P_PROC2_BD_PAGE_SIZE_MSK;
-               val |= XI_RV2P_PROC2_BD_PAGE_SIZE;
-               rv2p_code[XI_RV2P_PROC2_MAX_BD_PAGE_LOC] = cpu_to_le32(val);
-       }
+       rv2p_code_len = be32_to_cpu(fw_section->len);
+       file_offset = be32_to_cpu(fw_section->offset);
+
+       rv2p_code = (__be32 *)(bp->firmware->data + file_offset);


I'd also like to add the version information to the bnx2_fw_file_entry
struct instead of embedding it in the file name.  It is more flexible
this way as each section can have different versions and can be updated
separately.  So it will look something like:

struct bnx2_fw_file_entry {
	__be32 start_addr;
	__be32 version;
	struct bnx2_fw_file_section text;
	struct bnx2_fw_file_section data;
	struct bnx2_fw_file_section sbss;
	struct bnx2_fw_file_section bss;
	struct bnx2_fw_file_section rodata;
};


I'll generate an incremental patch on top of this one for review.
Thanks.

 


--

From: David Miller
Date: Monday, March 23, 2009 - 3:02 pm

From: "Michael Chan" <mchan@broadcom.com>



Thanks.
--

From: Bastian Blank
Date: Monday, March 23, 2009 - 3:29 pm

And how do you want to handle the compatibility? One firmware file have
to work with many kernels, which was always described as impossible.

Bastian

-- 
Women are more easily and more deeply terrified ... generating more
sheer horror than the male of the species.
		-- Spock, "Wolf in the Fold", stardate 3615.4
--

From: Michael Chan
Date: Monday, March 23, 2009 - 4:24 pm

I see your point.  May be I'll break up each firmware section into a
different file and the file name will be updated with each version.
This will allow different sections to be updated separately and older
kernels will still have access to the older firmware.


--

From: Rick Jones
Date: Monday, March 23, 2009 - 5:14 pm

Is that really necessary?  It is enough "fun" finding just the one firmware file 
as it is.

rick jones
--

From: Michael Chan
Date: Monday, March 23, 2009 - 6:11 pm

If all the firmware sections are in the same file, much of the firmware
file will be duplicated in a new file when we update just one section.


--

From: Rick Jones
Date: Monday, March 23, 2009 - 6:26 pm

So?  Perhaps I'm just experiencing distro pain which may not continue to exist or 
which may not matter to netdev, but when one is installing to a system that uses 
a core NIC which has firmware cast-out into "non-free" siberia, life is "fun" 
enough making sure one has the one firmware file let alone N of them.  If I now 
have to make sure I have all N firmware files, and they are to be updated 
separately, either that means I have to find N packages, or the distros are going 
to package them into one "uber" package that might as well be a single firmware 
file anyway.

rick jones

--

From: Michael Chan
Date: Monday, March 23, 2009 - 6:44 pm

I think we can assume that distros will package firmware files (make
firmware_install) properly and not require users to install these
firmware files on their own.


--

From: Ben Hutchings
Date: Monday, March 23, 2009 - 9:27 pm

[...]

If you're referring to Debian, that is exactly what Bastian and I are
working on.  Sourceless firmware is considered non-free, but it can
still be packaged, as the bnx2 firmware already has been.  We've also
had some discussions with David Woodhouse about semi-automatically
packaging all of the firmware that's now in the firmware subdirectory
and is clearly licensed.

Ben.

From: Bastian Blank
Date: Tuesday, March 24, 2009 - 12:42 am

What is the advantage of this? I always saw updates to more than one
firmware at once in the past. The question is not: is it possible, but:
it is necessary.

Bastian

-- 
Our way is peace.
		-- Septimus, the Son Worshiper, "Bread and Circuses",
		   stardate 4040.7.
--

From: Michael Chan
Date: Tuesday, March 24, 2009 - 8:27 am

Looking at the git history, about 3 out of 7 times, bnx2_fw.h was
partially updated.  About 2 out of 8 times bnx2_fw2.h was partially
updated.  The next planned update for net-next is a partial update.

Either way will work.  Using separate files will reduce the amount
of patch churn during partial updates.  That's the advantage.

--

From: Bastian Blank
Date: Monday, March 23, 2009 - 3:32 pm

Only if the firmwares are compatible enough. Can I pull the firmwares from a
2.6.21 kernel and ask a .29 to use them? One argument was always that
the firmware is coupled with the code.

Bastian

-- 
Worlds are conquered, galaxies destroyed -- but a woman is always a woman.
		-- Kirk, "The Conscience of the King", stardate 2818.9
--

From: Michael Chan
Date: Monday, March 23, 2009 - 4:28 pm

No, this won't work.  Please see new proposal to breakup firmware into
multiple files.


--

From: Michael Chan
Date: Wednesday, April 1, 2009 - 11:01 am

I've made some minor changes to Bastian and Ben's patch.  I've separated
the mips firmware from the non-mips firmware, added code to fix up the
non-mips firmware with the host CPU's page size, and other minor
changes.  If there are no issues, I'll send the entire patch with the
ihex files when net-next opens up.

Thanks again to Bastian and Ben.  Here's the new patch that excludes the
firmware ihex files for review.

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index ad446db..fed551b 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -46,19 +46,20 @@
 #include <linux/crc32.h>
 #include <linux/prefetch.h>
 #include <linux/cache.h>
-#include <linux/zlib.h>
+#include <linux/firmware.h>
 #include <linux/log2.h>
 
 #include "bnx2.h"
 #include "bnx2_fw.h"
-#include "bnx2_fw2.h"
-
-#define FW_BUF_SIZE		0x10000
 
 #define DRV_MODULE_NAME		"bnx2"
 #define PFX DRV_MODULE_NAME	": "
 #define DRV_MODULE_VERSION	"1.9.3"
 #define DRV_MODULE_RELDATE	"March 17, 2009"
+#define FW_MIPS_FILE_06		"bnx2-mips-06-4.6.16.fw"
+#define FW_RV2P_FILE_06		"bnx2-rv2p-06-4.6.16.fw"
+#define FW_MIPS_FILE_09		"bnx2-mips-09-4.6.15.fw"
+#define FW_RV2P_FILE_09		"bnx2-rv2p-09-4.6.15.fw"
 
 #define RUN_AT(x) (jiffies + (x))
 
@@ -72,6 +73,10 @@ MODULE_AUTHOR("Michael Chan <mchan@broadcom.com>");
 MODULE_DESCRIPTION("Broadcom NetXtreme II BCM5706/5708/5709/5716 Driver");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_MODULE_VERSION);
+MODULE_FIRMWARE(FW_MIPS_FILE_06);
+MODULE_FIRMWARE(FW_RV2P_FILE_06);
+MODULE_FIRMWARE(FW_MIPS_FILE_09);
+MODULE_FIRMWARE(FW_RV2P_FILE_09);
 
 static int disable_msi = 0;
 
@@ -3391,33 +3396,143 @@ bnx2_set_rx_mode(struct net_device *dev)
 	spin_unlock_bh(&bp->phy_lock);
 }
 
-static void
-load_rv2p_fw(struct bnx2 *bp, __le32 *rv2p_code, u32 rv2p_code_len,
-	u32 rv2p_proc)
+static int __devinit
+check_fw_section(const struct firmware *fw,
+		 const struct bnx2_fw_file_section *section,
+		 u32 alignment, bool non_empty)
+{
+	u32 offset = ...
From: David Miller
Date: Thursday, April 2, 2009 - 1:05 am

From: "Michael Chan" <mchan@broadcom.com>

I think I'm even willing to apply this for 2.6.30, let me know when
the final version is ready.
--

Previous thread: [PATCH net-next] sctp: Clean up TEST_FRAME hacks. by Vlad Yasevich on Thursday, March 19, 2009 - 2:43 pm. (2 messages)

Next thread: claims of by Mrs Joan Thomas on Thursday, March 19, 2009 - 4:03 pm. (1 message)