Re: [PATCH] typhoon: use request_firmware

[Jaswinder Singh <jaswinder@infradead.org>]

Hello Dave,

On Mon, 2008-07-28 at 21:38 -0400, David Dillow wrote:
> > +
> > +	/* firmware */
> > +	u8			*tp_fw_image;
> >  };
> 
> Now you keep one copy per NIC, which doubles the memory usage. Just use
> static struct firmware *typhoon_fw;
> 

OK, Fixed.

> > +static int typhoon_init_firmware(struct typhoon *tp)
> > +{
> > +	const struct firmware *fw;
> > +	const char fw_name[] = "3com/typhoon.bin";
> 
> If you use the #define and the global static, you can get rid of the
> above. Just use the define in place of fw_name below:
> 

OK, Fixed.

> > +	tp->tp_fw_image = vmalloc(fw->size);
> > +	if (!tp->tp_fw_image) {
> > +		err = -ENOMEM;
> > +		printk(KERN_ERR "%s: \"%s\" Failed %d\n",
> > +		       tp->name, fw_name, err);
> > +		goto out;
> > +	}
> 
> Then you can get rid of the double allocation and memcpy. Just put the
> call to release_firmware() in typhoon_cleanup().
> 

OK, Fixed.

> > +	err = typhoon_init_firmware(tp);
> > +	if (err)
> > +		goto out_irq;
> > +
> 
> This should move to either typhoon_init() or typhoon_init_one(). Putting
> it in typhoon_init() means we waste memory when the module is loaded if
> there is no typhoon NIC; putting it in typhoon_init_one() means it needs
> protection from threaded probing, should that ever be put back in. Given
> that most modern distros don't do probing by blinding loading modules,
> typhoon_init() seems a safe bet.
> 

Sorry Dave, I need tp->pdev so typhoon_init_one() seems better.
I hope you do not mind this.

> > @@ -2155,6 +2189,9 @@ typhoon_close(struct net_device *dev)
> 
> > +	if (tp->tp_fw_image)
> > +		vfree(tp->tp_fw_image);
> > +
> 
> This goes away and is replaced by a release_firmware() in
> typhoon_cleanup() if you do the above.
> 

OK, Fixed :)

Here is updated patch for typhoon.c :-

Subject: [PATCH] typhoon: use request_firmware

made following const as we treat firmware data as const:
        struct typhoon_file_header *fHdr
        struct typhoon_section_header *sHdr
        u8 *image_data

Signed-off-by: Jaswinder Singh <jaswinder@infradead.org>
---
 drivers/net/typhoon.c          |   33 +-
diff --git a/drivers/net/typhoon.c b/drivers/net/typhoon.c
index c0dd25b..a57941a 100644
--- a/drivers/net/typhoon.c
+++ b/drivers/net/typhoon.c
@@ -130,16 +130,18 @@ 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";
 
+#define FIRMWARE_NAME		"3com/typhoon.bin"
 MODULE_AUTHOR("David Dillow <dave@thedillows.org>");
 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 "
@@ -1347,14 +1349,28 @@ typhoon_init_rings(struct typhoon *tp)
 	tp->txHiRing.lastRead = 0;
 }
 
+static const struct firmware *typhoon_fw;
+
+static int typhoon_init_firmware(struct typhoon *tp)
+{
+	int err;
+
+	err = request_firmware(&typhoon_fw, FIRMWARE_NAME, &tp->pdev->dev);
+	if (err)
+		printk(KERN_ERR "%s: Failed to load firmware \"%s\"\n",
+		       tp->name, FIRMWARE_NAME);
+
+	return err;
+}
+
 static int
 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;
@@ -1369,7 +1385,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)) {
@@ -2445,6 +2461,10 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 */
 	tp->name = pci_name(pdev);
 
+	err = typhoon_init_firmware(tp);
+	if (err)
+		goto error_out_reset;
+
 	typhoon_init_interface(tp);
 	typhoon_init_rings(tp);
 
@@ -2625,6 +2645,9 @@ typhoon_init(void)
 static void __exit
 typhoon_cleanup(void)
 {
+	if (typhoon_fw)
+		release_firmware(typhoon_fw);
+
 	pci_unregister_driver(&typhoon_driver);
 }
 
You can check complete patch from :-
http://git.infradead.org/users/jaswinder/firm-jsr-2.6.git

Thank you,

Jaswinder Singh.