Re: [PATCH] typhoon: use request_firmware

[David Dillow]

On Wed, 2008-07-30 at 12:14 +0530, Jaswinder Singh wrote:
> On Wed, 2008-07-30 at 01:25 -0400, David Dillow wrote:
> > Also, if not adding a mutex, then this can be folded into
> > typhoon_init_one(), rather than living in a separate function.  
> 
> request_firmware is using mutex.

request_firmware()'s mutex is protecting against things inside of that
subsystem, but offers no protection between the test on typhoon_fw !=
NULL and the setting of it inside of the firmware loader. That requires
locking at a higher level.

Anyways, probing is single threaded and I seem to recall it being
unlikely to be made parallel due to the number issues exposed last time,
so a minimal comment is fine.

I finally got a few minutes to look at the patch as a whole and apply a
little polish. Here's my version, compile tested but it hasn't seen
actual hardware yet. Hopefully this weekend.

diff --git a/drivers/net/typhoon.c b/drivers/net/typhoon.c
index 8549f11..e9f7c5d 100644
--- a/drivers/net/typhoon.c
+++ b/drivers/net/typhoon.c
@@ -63,6 +63,10 @@ static unsigned int use_mmio = 2;
  */
 static const int multicast_filter_limit = 32;
 
+/* Pointer to the firmware loaded from userspace.
+ */
+static const struct firmware *typhoon_fw;
+
 /* Operational parameters that are set at compile time. */
 
 /* Keep the ring sizes a power of two for compile efficiency.
@@ -100,11 +104,13 @@ static const int multicast_filter_limit = 32;
 #define PKT_BUF_SZ		1536
 
 #define DRV_MODULE_NAME		"typhoon"
-#define DRV_MODULE_VERSION 	"1.5.8"
-#define DRV_MODULE_RELDATE	"06/11/09"
+#define DRV_MODULE_VERSION 	"1.6.0"
+#define DRV_MODULE_RELDATE	"31 Jul 2008"
 #define PFX			DRV_MODULE_NAME ": "
 #define ERR_PFX			KERN_ERR PFX
 
+#define FIRMWARE_NAME		"3com/typhoon.bin"
+
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
@@ -130,9 +136,9 @@ static const int multicast_filter_limit = 32;
 #include <linux/in6.h>
 #include <linux/version.h>
 #include <linux/dma-mapping.h>
+#include <linux/firmware.h>
 
 #include "typhoon.h"
-#include "typhoon-firmware.h"
 
 static char version[] __devinitdata =
     "typhoon.c: version " DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";
@@ -140,6 +146,7 @@ static char version[] __devinitdata =
 MODULE_AUTHOR("David Dillow <dave@xxxxxxxxxxxxxx>");
 MODULE_VERSION(DRV_MODULE_VERSION);
 MODULE_LICENSE("GPL");
+MODULE_FIRMWARE(FIRMWARE_NAME);
 MODULE_DESCRIPTION("3Com Typhoon Family (3C990, 3CR990, and variants)");
 MODULE_PARM_DESC(rx_copybreak, "Packets smaller than this are copied and "
 			       "the buffer given back to the NIC. Default "
@@ -1350,9 +1357,9 @@ typhoon_download_firmware(struct typhoon *tp)
 {
 	void __iomem *ioaddr = tp->ioaddr;
 	struct pci_dev *pdev = tp->pdev;
-	struct typhoon_file_header *fHdr;
-	struct typhoon_section_header *sHdr;
-	u8 *image_data;
+	const struct typhoon_file_header *fHdr;
+	const struct typhoon_section_header *sHdr;
+	const u8 *image_data;
 	void *dpage;
 	dma_addr_t dpage_dma;
 	__sum16 csum;
@@ -1367,7 +1374,7 @@ typhoon_download_firmware(struct typhoon *tp)
 	int err;
 
 	err = -EINVAL;
-	fHdr = (struct typhoon_file_header *) typhoon_firmware_image;
+	fHdr = (struct typhoon_file_header *) typhoon_fw->data;
 	image_data = (u8 *) fHdr;
 
 	if(memcmp(fHdr->tag, "TYPHOON", 8)) {
@@ -2317,6 +2324,19 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if(!did_version++)
 		printk(KERN_INFO "%s", version);
 
+	/* Only load the firmware once; if bus probing ever becomes
+	 * multi-threaded, we'll need a mutex here to prevent a race
+	 * condition that will leak memory.
+	 */
+	if(!typhoon_fw) {
+		err = request_firmware(&typhoon_fw, FIRMWARE_NAME, &pdev->dev);
+		if(err) {
+			printk(ERR_PFX "%s: Failed to load firmware \"%s\"\n",
+			       pci_name(pdev), FIRMWARE_NAME);
+			goto error_out;
+		}
+	}
+
 	dev = alloc_etherdev(sizeof(*tp));
 	if(dev == NULL) {
 		printk(ERR_PFX "%s: unable to alloc new net device\n",
@@ -2579,6 +2599,7 @@ error_out_disable:
 error_out_dev:
 	free_netdev(dev);
 error_out:
+	/* Module unload will free firmware if needed */
 	return err;
 }
 
@@ -2623,6 +2644,9 @@ static void __exit
 typhoon_cleanup(void)
 {
 	pci_unregister_driver(&typhoon_driver);
+
+	if(typhoon_fw)
+		release_firmware(typhoon_fw);
 }
 
 module_init(typhoon_init);



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/