[PATCH v2] typhoon: Use request_firmware()

[PATCH v2] typhoon: Use request_firmware()

[Ben Hutchings]

Based on a patch by Jaswinder Singh <jaswinder@infradead.org>.

Compile-tested only.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
This should address the issues raised in
<http://thread.gmane.org/gmane.linux.kernel/712328>, which I had not
previously read.  I have also simplified the DMA-mapping as hinted by
the existing comment in typhoon_download_firmware().

This should be sent without MIME encoding this time.

This copy sent *without* the firmware format changes so it's under
netdev's size limit.

Ben.

 drivers/net/typhoon-firmware.h | 3778 ----------------------------------------
 drivers/net/typhoon.c          |  131 +-
 firmware/3com/typhoon.bin.ihex | 2819 ++++++++++++++++++++++++++++++
 firmware/Makefile              |    1 +
 firmware/WHENCE                |   42 +
 5 files changed, 2936 insertions(+), 3835 deletions(-)
 delete mode 100644 drivers/net/typhoon-firmware.h
 create mode 100644 firmware/3com/typhoon.bin.ihex

diff --git a/drivers/net/typhoon.c b/drivers/net/typhoon.c
index a8e5651..cd3283f 100644
--- a/drivers/net/typhoon.c
+++ b/drivers/net/typhoon.c
@@ -129,16 +129,18 @@ static const int multicast_filter_limit = 32;
 #include <asm/uaccess.h>
 #include <linux/in6.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 "
@@ -1344,45 +1346,61 @@ typhoon_init_rings(struct typhoon *tp)
 	tp->txHiRing.lastRead = 0;
 }
 
+static const struct firmware *typhoon_fw;
+
+static int
+typhoon_request_firmware(struct typhoon *tp)
+{
+	int err;
+
+	if (typhoon_fw)
+		return 0;
+
+	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;
+	}
+
+	if (typhoon_fw->size < sizeof(struct typhoon_file_header) ||
+	    memcmp(typhoon_fw->data, "TYPHOON", 8)) {
+		printk(KERN_ERR "%s: Invalid firmware image\n",
+		       tp->name);
+		release_firmware(typhoon_fw);
+		typhoon_fw = NULL;
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 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;
-	void *dpage;
-	dma_addr_t dpage_dma;
+	const struct typhoon_file_header *fHdr;
+	const struct typhoon_section_header *sHdr;
+	const u8 *image_data;
+	dma_addr_t image_dma;
 	__sum16 csum;
 	u32 irqEnabled;
 	u32 irqMasked;
 	u32 numSections;
 	u32 section_len;
-	u32 len;
 	u32 load_addr;
 	u32 hmac;
 	int i;
 	int err;
 
-	err = -EINVAL;
-	fHdr = (struct typhoon_file_header *) typhoon_firmware_image;
-	image_data = (u8 *) fHdr;
-
-	if(memcmp(fHdr->tag, "TYPHOON", 8)) {
-		printk(KERN_ERR "%s: Invalid firmware image!\n", tp->name);
-		goto err_out;
-	}
+	image_data = typhoon_fw->data;
+	fHdr = (struct typhoon_file_header *) image_data;
 
-	/* Cannot just map the firmware image using pci_map_single() as
-	 * the firmware is part of the kernel/module image, so we allocate
-	 * some consistent memory to copy the sections into, as it is simpler,
-	 * and short-lived. If we ever split out and require a userland
-	 * firmware loader, then we can revisit this.
-	 */
 	err = -ENOMEM;
-	dpage = pci_alloc_consistent(pdev, PAGE_SIZE, &dpage_dma);
-	if(!dpage) {
+	image_dma = pci_map_single(pdev, (u8 *) typhoon_fw->data,
+				   typhoon_fw->size, PCI_DMA_TODEVICE);
+	if (pci_dma_mapping_error(pdev, image_dma)) {
 		printk(KERN_ERR "%s: no DMA mem for firmware\n", tp->name);
 		goto err_out;
 	}
@@ -1430,41 +1448,34 @@ typhoon_download_firmware(struct typhoon *tp)
 		load_addr = le32_to_cpu(sHdr->startAddr);
 		section_len = le32_to_cpu(sHdr->len);
 
-		while(section_len) {
-			len = min_t(u32, section_len, PAGE_SIZE);
+		if (typhoon_wait_interrupt(ioaddr) < 0 ||
+		    ioread32(ioaddr + TYPHOON_REG_STATUS) !=
+		    TYPHOON_STATUS_WAITING_FOR_SEGMENT) {
+			printk(KERN_ERR "%s: segment ready timeout\n",
+			       tp->name);
+			goto err_out_irq;
+		}
 
-			if(typhoon_wait_interrupt(ioaddr) < 0 ||
-			   ioread32(ioaddr + TYPHOON_REG_STATUS) !=
-			   TYPHOON_STATUS_WAITING_FOR_SEGMENT) {
-				printk(KERN_ERR "%s: segment ready timeout\n",
-				       tp->name);
-				goto err_out_irq;
-			}
+		/* Do an pseudo IPv4 checksum on the data -- first
+		 * need to convert each u16 to cpu order before
+		 * summing. Fortunately, due to the properties of
+		 * the checksum, we can do this once, at the end.
+		 */
+		csum = csum_fold(csum_partial(image_data, section_len, 0));
+
+		iowrite32(section_len, ioaddr + TYPHOON_REG_BOOT_LENGTH);
+		iowrite32(le16_to_cpu((__force __le16)csum),
+			  ioaddr + TYPHOON_REG_BOOT_CHECKSUM);
+		iowrite32(load_addr,
+			  ioaddr + TYPHOON_REG_BOOT_DEST_ADDR);
+		iowrite32(0, ioaddr + TYPHOON_REG_BOOT_DATA_HI);
+		iowrite32(image_dma + (image_data - typhoon_fw->data),
+			  ioaddr + TYPHOON_REG_BOOT_DATA_LO);
+		typhoon_post_pci_writes(ioaddr);
+		iowrite32(TYPHOON_BOOTCMD_SEG_AVAILABLE,
+			  ioaddr + TYPHOON_REG_COMMAND);
 
-			/* Do an pseudo IPv4 checksum on the data -- first
-			 * need to convert each u16 to cpu order before
-			 * summing. Fortunately, due to the properties of
-			 * the checksum, we can do this once, at the end.
-			 */
-			csum = csum_fold(csum_partial_copy_nocheck(image_data,
-								  dpage, len,
-								  0));
-
-			iowrite32(len, ioaddr + TYPHOON_REG_BOOT_LENGTH);
-			iowrite32(le16_to_cpu((__force __le16)csum),
-					ioaddr + TYPHOON_REG_BOOT_CHECKSUM);
-			iowrite32(load_addr,
-					ioaddr + TYPHOON_REG_BOOT_DEST_ADDR);
-			iowrite32(0, ioaddr + TYPHOON_REG_BOOT_DATA_HI);
-			iowrite32(dpage_dma, ioaddr + TYPHOON_REG_BOOT_DATA_LO);
-			typhoon_post_pci_writes(ioaddr);
-			iowrite32(TYPHOON_BOOTCMD_SEG_AVAILABLE,
-			       ioaddr + TYPHOON_REG_COMMAND);
-
-			image_data += len;
-			load_addr += len;
-			section_len -= len;
-		}
+		image_data += section_len;
 	}
 
 	if(typhoon_wait_interrupt(ioaddr) < 0 ||
@@ -1488,7 +1499,7 @@ err_out_irq:
 	iowrite32(irqMasked, ioaddr + TYPHOON_REG_INTR_MASK);
 	iowrite32(irqEnabled, ioaddr + TYPHOON_REG_INTR_ENABLE);
 
-	pci_free_consistent(pdev, PAGE_SIZE, dpage, dpage_dma);
+	pci_unmap_single(pdev, image_dma,  typhoon_fw->size, PCI_DMA_TODEVICE);
 
 err_out:
 	return err;
@@ -2086,6 +2097,10 @@ typhoon_open(struct net_device *dev)
 	struct typhoon *tp = netdev_priv(dev);
 	int err;
 
+	err = typhoon_request_firmware(tp);
+	if (err)
+		goto out;
+
 	err = typhoon_wakeup(tp, WaitSleep);
 	if(err < 0) {
 		printk(KERN_ERR "%s: unable to wakeup device\n", dev->name);
@@ -2624,6 +2639,8 @@ typhoon_init(void)
 static void __exit
 typhoon_cleanup(void)
 {
+	if (typhoon_fw)
+		release_firmware(typhoon_fw);
 	pci_unregister_driver(&typhoon_driver);
 }
 
diff --git a/firmware/Makefile b/firmware/Makefile
index e1585f5..aa2e02d 100644
--- a/firmware/Makefile
+++ b/firmware/Makefile
@@ -52,6 +52,7 @@ fw-shipped-$(CONFIG_SND_YMFPCI) += yamaha/ds1_ctrl.fw yamaha/ds1_dsp.fw \
 fw-shipped-$(CONFIG_TEHUTI) += tehuti/bdx.bin
 fw-shipped-$(CONFIG_TIGON3) += tigon/tg3.bin tigon/tg3_tso.bin \
 			       tigon/tg3_tso5.bin
+fw-shipped-$(CONFIG_TYPHOON) += 3com/typhoon.bin
 fw-shipped-$(CONFIG_USB_DABUSB) += dabusb/firmware.fw dabusb/bitstream.bin
 fw-shipped-$(CONFIG_USB_EMI26) += emi26/loader.fw emi26/firmware.fw \
 				  emi26/bitstream.fw
diff --git a/firmware/WHENCE b/firmware/WHENCE
index e56aa48..ea4fc2e 100644
--- a/firmware/WHENCE
+++ b/firmware/WHENCE
@@ -451,3 +451,45 @@ Licence:
 Found in hex form in kernel source.
 
 --------------------------------------------------------------------------
+
+Driver: TYPHOON - 3cr990 series Typhoon
+
+File: 3com/typhoon.bin
+
+Licence:
+/*
+ * Copyright 1999-2004 3Com Corporation.  All Rights Reserved.
+ *
+ * Redistribution and use in source and binary forms of the 3c990img.h
+ * microcode software are permitted provided that the following conditions
+ * are met:
+ * 1. Redistribution of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistribution in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. The name of 3Com may not be used to endorse or promote products
+ *    derived from this software without specific prior written permission
+ *
+ * THIS SOFTWARE IS PROVIDED BY 3COM ``AS IS'' AND ANY EXPRESS OR
+ * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+ * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * USER ACKNOWLEDGES AND AGREES THAT PURCHASE OR USE OF THE 3c990img.h
+ * MICROCODE SOFTWARE WILL NOT CREATE OR GIVE GROUNDS FOR A LICENSE BY
+ * IMPLICATION, ESTOPPEL, OR OTHERWISE IN ANY INTELLECTUAL PROPERTY RIGHTS
+ * (PATENT, COPYRIGHT, TRADE SECRET, MASK WORK, OR OTHER PROPRIETARY RIGHT)
+ * EMBODIED IN ANY OTHER 3COM HARDWARE OR SOFTWARE EITHER SOLELY OR IN
+ * COMBINATION WITH THE 3c990img.h MICROCODE SOFTWARE
+ */
+
+Found in hex form in kernel source.
+
+--------------------------------------------------------------------------
-- 
1.6.1.3

Re: [PATCH v2] typhoon: Use request_firmware()

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Thu, 26 Feb 2009 13:49:51 +0000

> Based on a patch by Jaswinder Singh <jaswinder@infradead.org>.
> 
> Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Applied.

Re: [PATCH v2] typhoon: Use request_firmware()

[David Dillow]

On Thu, 2009-02-26 at 13:49 +0000, Ben Hutchings wrote:
> Based on a patch by Jaswinder Singh <jaswinder@infradead.org>.
> 
> Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

I suppose it's my fault for not getting around to this sooner, but I've
been waiting for the fallout to settle with tg3 and follow the leader.

Anyways, is it too much to ask for you to CC the maintainer? Hell, you
added a line right above my email address, so it wouldn't have been hard
for you to find...

I suppose I better test this sooner rather than later, because I think
you've broken it. The firmware gets vmalloc'd, which means it is not
necessarily physically contiguous, and I don't think pci_map_single() is
going to like that since the firmware is always larger than a page. But
I may be wrong on this.

Dave

Re: [PATCH v2] typhoon: Use request_firmware()

[Ben Hutchings]

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

On Sun, 2009-03-01 at 15:38 -0500, David Dillow wrote:
> On Thu, 2009-02-26 at 13:49 +0000, Ben Hutchings wrote:
> > Based on a patch by Jaswinder Singh <jaswinder@infradead.org>.
> > 
> > Compile-tested only.
> > 
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> 
> I suppose it's my fault for not getting around to this sooner, but I've
> been waiting for the fallout to settle with tg3 and follow the leader.
> 
> Anyways, is it too much to ask for you to CC the maintainer? Hell, you
> added a line right above my email address, so it wouldn't have been hard
> for you to find...

Sorry about that - I looked for a maintainer but there was no listing
for "typhoon" in MAINTAINERS nor any recent change to the driver that
was not part of a general API change.  Email addresses in source files
tend to be outdated, so I didn't look for them.  I see now that you have
an entry under "3CR990" in MAINTAINERS, but that isn't the driver name.
Please could you change that so that it's easier for people to find you?

> I suppose I better test this sooner rather than later, because I think
> you've broken it. The firmware gets vmalloc'd, which means it is not
> necessarily physically contiguous, and I don't think pci_map_single() is
> going to like that since the firmware is always larger than a page. But
> I may be wrong on this.

You could well be right.

Ben.

-- 
Ben Hutchings
In a hierarchy, every employee tends to rise to his level of incompetence.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH v2] typhoon: Use request_firmware()

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Mon, 02 Mar 2009 01:02:54 +0000

> On Sun, 2009-03-01 at 15:38 -0500, David Dillow wrote:
> > I suppose I better test this sooner rather than later, because I think
> > you've broken it. The firmware gets vmalloc'd, which means it is not
> > necessarily physically contiguous, and I don't think pci_map_single() is
> > going to like that since the firmware is always larger than a page. But
> > I may be wrong on this.
> 
> You could well be right.

He is definitely right, this cannot work.

Re: [PATCH v2] typhoon: Use request_firmware()

[David Dillow]

On Sun, 2009-03-01 at 17:44 -0800, David Miller wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> Date: Mon, 02 Mar 2009 01:02:54 +0000
> 
> > On Sun, 2009-03-01 at 15:38 -0500, David Dillow wrote:
> > > I suppose I better test this sooner rather than later, because I think
> > > you've broken it. The firmware gets vmalloc'd, which means it is not
> > > necessarily physically contiguous, and I don't think pci_map_single() is
> > > going to like that since the firmware is always larger than a page. But
> > > I may be wrong on this.
> > 
> > You could well be right.
> 
> He is definitely right, this cannot work.

Do you want to revert, or do you a patch to fix it? Revert will be
quicker as I have to dig out hardware to test...

Re: [PATCH v2] typhoon: Use request_firmware()

[David Miller]

From: David Dillow <dave@thedillows.org>
Date: Sun, 01 Mar 2009 22:16:47 -0500

> On Sun, 2009-03-01 at 17:44 -0800, David Miller wrote:
> > From: Ben Hutchings <ben@decadent.org.uk>
> > Date: Mon, 02 Mar 2009 01:02:54 +0000
> > 
> > > On Sun, 2009-03-01 at 15:38 -0500, David Dillow wrote:
> > > > I suppose I better test this sooner rather than later, because I think
> > > > you've broken it. The firmware gets vmalloc'd, which means it is not
> > > > necessarily physically contiguous, and I don't think pci_map_single() is
> > > > going to like that since the firmware is always larger than a page. But
> > > > I may be wrong on this.
> > > 
> > > You could well be right.
> > 
> > He is definitely right, this cannot work.
> 
> Do you want to revert, or do you a patch to fix it? Revert will be
> quicker as I have to dig out hardware to test...

Give me a fix so I don't have to revert.

Re: [PATCH v2] typhoon: Use request_firmware()

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Sun, 01 Mar 2009 20:11:23 -0800 (PST)

> Give me a fix so I don't have to revert.

Nevermind, I checked in a fix myself.

I cared enough to fix this, maybe one of you will care enough
to test it.

typhoon: Need non-vmalloc memory to DMA firmware to the card.

request_firmware() gives vmalloc'd memory, which is not suitable
for pci_map_single() and friends.

Use a kmalloc()'d copy of the firmware for this DMA operation.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 drivers/net/typhoon.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/typhoon.c b/drivers/net/typhoon.c
index cd3283f..ec2541c 100644
--- a/drivers/net/typhoon.c
+++ b/drivers/net/typhoon.c
@@ -1347,6 +1347,7 @@ typhoon_init_rings(struct typhoon *tp)
 }
 
 static const struct firmware *typhoon_fw;
+static u8 *typhoon_fw_image;
 
 static int
 typhoon_request_firmware(struct typhoon *tp)
@@ -1367,12 +1368,22 @@ typhoon_request_firmware(struct typhoon *tp)
 	    memcmp(typhoon_fw->data, "TYPHOON", 8)) {
 		printk(KERN_ERR "%s: Invalid firmware image\n",
 		       tp->name);
-		release_firmware(typhoon_fw);
-		typhoon_fw = NULL;
-		return -EINVAL;
+		err = -EINVAL;
+		goto out_err;
+	}
+
+	typhoon_fw_image = kmalloc(typhoon_fw->size, GFP_KERNEL);
+	if (!typhoon_fw_image) {
+		err = -ENOMEM;
+		goto out_err;
 	}
 
 	return 0;
+
+out_err:
+	release_firmware(typhoon_fw);
+	typhoon_fw = NULL;
+	return err;
 }
 
 static int
@@ -1394,11 +1405,11 @@ typhoon_download_firmware(struct typhoon *tp)
 	int i;
 	int err;
 
-	image_data = typhoon_fw->data;
+	image_data = typhoon_fw_image;
 	fHdr = (struct typhoon_file_header *) image_data;
 
 	err = -ENOMEM;
-	image_dma = pci_map_single(pdev, (u8 *) typhoon_fw->data,
+	image_dma = pci_map_single(pdev, (u8 *) image_data,
 				   typhoon_fw->size, PCI_DMA_TODEVICE);
 	if (pci_dma_mapping_error(pdev, image_dma)) {
 		printk(KERN_ERR "%s: no DMA mem for firmware\n", tp->name);
@@ -1469,7 +1480,7 @@ typhoon_download_firmware(struct typhoon *tp)
 		iowrite32(load_addr,
 			  ioaddr + TYPHOON_REG_BOOT_DEST_ADDR);
 		iowrite32(0, ioaddr + TYPHOON_REG_BOOT_DATA_HI);
-		iowrite32(image_dma + (image_data - typhoon_fw->data),
+		iowrite32(image_dma + (image_data - typhoon_fw_image),
 			  ioaddr + TYPHOON_REG_BOOT_DATA_LO);
 		typhoon_post_pci_writes(ioaddr);
 		iowrite32(TYPHOON_BOOTCMD_SEG_AVAILABLE,
@@ -2639,8 +2650,10 @@ typhoon_init(void)
 static void __exit
 typhoon_cleanup(void)
 {
-	if (typhoon_fw)
+	if (typhoon_fw) {
+		kfree(typhoon_fw_image);
 		release_firmware(typhoon_fw);
+	}
 	pci_unregister_driver(&typhoon_driver);
 }
 
-- 
1.6.1.2.253.ga34a

Re: [PATCH v2] typhoon: Use request_firmware()

[David Dillow]

On Sun, 2009-03-01 at 20:29 -0800, David Miller wrote:
> I cared enough to fix this, maybe one of you will care enough
> to test it.

Like I said, I will be happy to test as soon as I can dig the hardware
out. In any event, this is broken.

>  static int
>  typhoon_request_firmware(struct typhoon *tp)
> @@ -1367,12 +1368,22 @@ typhoon_request_firmware(struct typhoon *tp)
>  	    memcmp(typhoon_fw->data, "TYPHOON", 8)) {
>  		printk(KERN_ERR "%s: Invalid firmware image\n",
>  		       tp->name);
> -		release_firmware(typhoon_fw);
> -		typhoon_fw = NULL;
> -		return -EINVAL;
> +		err = -EINVAL;
> +		goto out_err;
> +	}
> +
> +	typhoon_fw_image = kmalloc(typhoon_fw->size, GFP_KERNEL);
> +	if (!typhoon_fw_image) {
> +		err = -ENOMEM;
> +		goto out_err;
>  	}

You never copied the image into the kmalloc'd memory, so you upload
garbage.

Fix that and I think it will be OK.

Re: [PATCH v2] typhoon: Use request_firmware()

[David Miller]

From: David Dillow <dave@thedillows.org>
Date: Mon, 02 Mar 2009 02:56:19 -0500

> On Sun, 2009-03-01 at 20:29 -0800, David Miller wrote:
> > +
> > +	typhoon_fw_image = kmalloc(typhoon_fw->size, GFP_KERNEL);
> > +	if (!typhoon_fw_image) {
> > +		err = -ENOMEM;
> > +		goto out_err;
> >  	}
> 
> You never copied the image into the kmalloc'd memory, so you upload
> garbage.
> 
> Fix that and I think it will be OK.

Good catch, I've checked in the following fix:

typhoon: Add missing firmware copy.

Noticed by David Dillow.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 drivers/net/typhoon.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/typhoon.c b/drivers/net/typhoon.c
index ec2541c..9bba787 100644
--- a/drivers/net/typhoon.c
+++ b/drivers/net/typhoon.c
@@ -1377,6 +1377,7 @@ typhoon_request_firmware(struct typhoon *tp)
 		err = -ENOMEM;
 		goto out_err;
 	}
+	memcpy(typhoon_fw_image, typhoon_fw->data, typhoon_fw->size);
 
 	return 0;
 
-- 
1.6.1.2.253.ga34a

Re: [PATCH v2] typhoon: Use request_firmware()

[David Dillow]

typhoon: repair firmware loading

The conversion to avoid using pci_alloc_consistent() broke the firmware
load process, as well as added an order-4 kmalloc and doubled the memory
usage of the firmware image. Go back to loading a page at a time.

Also, since the user can now give us utter garbage for firmware, do a
cursory validation so we don't try to load just anything.

Signed-off-by: David Dillow <dave@thedillows.org>
---
 MAINTAINERS           |    2 
 drivers/net/typhoon.c |  137 +++++++++++++++++++++++++-----------------
 2 files changed, 84 insertions(+), 55 deletions(-)

I tested the version in net-next-2.6 and it wouldn't load the firmware,
so here's my fix. It has been tested to work, and to reject images that
fail it's cursory tests.

Also, I added typhoon to the existing MAINTAINERS entry, since it is
apparently too much trouble to Cc: the email address in the source file.

diff --git a/MAINTAINERS b/MAINTAINERS
index e74a133..617a397 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -97,7 +97,7 @@ M:	klassert@mathematik.tu-chemnitz.de
 L:	netdev@vger.kernel.org
 S:	Maintained
 
-3CR990 NETWORK DRIVER
+3CR990 (typhoon) NETWORK DRIVER
 P:	David Dillow
 M:	dave@thedillows.org
 L:	netdev@vger.kernel.org
diff --git a/drivers/net/typhoon.c b/drivers/net/typhoon.c
index 9bba787..9dcd897 100644
--- a/drivers/net/typhoon.c
+++ b/drivers/net/typhoon.c
@@ -100,10 +100,11 @@ 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.5.9"
+#define DRV_MODULE_RELDATE	"Mar 2, 2009"
 #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>
@@ -136,7 +137,6 @@ static const int multicast_filter_limit = 32;
 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");
@@ -1347,11 +1347,16 @@ typhoon_init_rings(struct typhoon *tp)
 }
 
 static const struct firmware *typhoon_fw;
-static u8 *typhoon_fw_image;
 
 static int
 typhoon_request_firmware(struct typhoon *tp)
 {
+	const struct typhoon_file_header *fHdr;
+	const struct typhoon_section_header *sHdr;
+	const u8 *image_data;
+	u32 numSections;
+	u32 section_len; 
+	u32 remaining; 
 	int err;
 
 	if (typhoon_fw)
@@ -1360,31 +1365,45 @@ typhoon_request_firmware(struct typhoon *tp)
 	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);
+				tp->name, FIRMWARE_NAME);
 		return err;
 	}
 
-	if (typhoon_fw->size < sizeof(struct typhoon_file_header) ||
-	    memcmp(typhoon_fw->data, "TYPHOON", 8)) {
-		printk(KERN_ERR "%s: Invalid firmware image\n",
-		       tp->name);
-		err = -EINVAL;
-		goto out_err;
-	}
+	image_data = (u8 *) typhoon_fw->data;
+	remaining = typhoon_fw->size;
+	if (remaining < sizeof(struct typhoon_file_header))
+		goto invalid_fw;
 
-	typhoon_fw_image = kmalloc(typhoon_fw->size, GFP_KERNEL);
-	if (!typhoon_fw_image) {
-		err = -ENOMEM;
-		goto out_err;
+	fHdr = (struct typhoon_file_header *) image_data;
+	if (memcmp(fHdr->tag, "TYPHOON", 8))
+		goto invalid_fw;
+
+	numSections = le32_to_cpu(fHdr->numSections);
+	image_data += sizeof(struct typhoon_file_header);
+	remaining -= sizeof(struct typhoon_file_header);
+
+	while (numSections--) {
+		if (remaining < sizeof(struct typhoon_section_header))
+			goto invalid_fw;
+
+		sHdr = (struct typhoon_section_header *) image_data;
+		image_data += sizeof(struct typhoon_section_header);
+		section_len = le32_to_cpu(sHdr->len);
+
+		if (remaining < section_len)
+			goto invalid_fw;
+
+		image_data += section_len;
+		remaining -= section_len;
 	}
-	memcpy(typhoon_fw_image, typhoon_fw->data, typhoon_fw->size);
 
 	return 0;
 
-out_err:
+invalid_fw:
+	printk(KERN_ERR "%s: Invalid firmware image\n", tp->name);
 	release_firmware(typhoon_fw);
 	typhoon_fw = NULL;
-	return err;
+	return -EINVAL;
 }
 
 static int
@@ -1395,24 +1414,29 @@ typhoon_download_firmware(struct typhoon *tp)
 	const struct typhoon_file_header *fHdr;
 	const struct typhoon_section_header *sHdr;
 	const u8 *image_data;
-	dma_addr_t image_dma;
+	void *dpage;
+	dma_addr_t dpage_dma;
 	__sum16 csum;
 	u32 irqEnabled;
 	u32 irqMasked;
 	u32 numSections;
 	u32 section_len;
+	u32 len;
 	u32 load_addr;
 	u32 hmac;
 	int i;
 	int err;
 
-	image_data = typhoon_fw_image;
+	image_data = (u8 *) typhoon_fw->data;
 	fHdr = (struct typhoon_file_header *) image_data;
 
+	/* Cannot just map the firmware image using pci_map_single() as
+	 * the firmware is vmalloc()'d and may not be physically contiguous,
+	 * so we allocate some consistent memory to copy the sections into.
+	 */
 	err = -ENOMEM;
-	image_dma = pci_map_single(pdev, (u8 *) image_data,
-				   typhoon_fw->size, PCI_DMA_TODEVICE);
-	if (pci_dma_mapping_error(pdev, image_dma)) {
+	dpage = pci_alloc_consistent(pdev, PAGE_SIZE, &dpage_dma);
+	if(!dpage) {
 		printk(KERN_ERR "%s: no DMA mem for firmware\n", tp->name);
 		goto err_out;
 	}
@@ -1460,34 +1484,41 @@ typhoon_download_firmware(struct typhoon *tp)
 		load_addr = le32_to_cpu(sHdr->startAddr);
 		section_len = le32_to_cpu(sHdr->len);
 
-		if (typhoon_wait_interrupt(ioaddr) < 0 ||
-		    ioread32(ioaddr + TYPHOON_REG_STATUS) !=
-		    TYPHOON_STATUS_WAITING_FOR_SEGMENT) {
-			printk(KERN_ERR "%s: segment ready timeout\n",
-			       tp->name);
-			goto err_out_irq;
-		}
+		while(section_len) {
+			len = min_t(u32, section_len, PAGE_SIZE);
 
-		/* Do an pseudo IPv4 checksum on the data -- first
-		 * need to convert each u16 to cpu order before
-		 * summing. Fortunately, due to the properties of
-		 * the checksum, we can do this once, at the end.
-		 */
-		csum = csum_fold(csum_partial(image_data, section_len, 0));
-
-		iowrite32(section_len, ioaddr + TYPHOON_REG_BOOT_LENGTH);
-		iowrite32(le16_to_cpu((__force __le16)csum),
-			  ioaddr + TYPHOON_REG_BOOT_CHECKSUM);
-		iowrite32(load_addr,
-			  ioaddr + TYPHOON_REG_BOOT_DEST_ADDR);
-		iowrite32(0, ioaddr + TYPHOON_REG_BOOT_DATA_HI);
-		iowrite32(image_dma + (image_data - typhoon_fw_image),
-			  ioaddr + TYPHOON_REG_BOOT_DATA_LO);
-		typhoon_post_pci_writes(ioaddr);
-		iowrite32(TYPHOON_BOOTCMD_SEG_AVAILABLE,
-			  ioaddr + TYPHOON_REG_COMMAND);
+			if(typhoon_wait_interrupt(ioaddr) < 0 ||
+			   ioread32(ioaddr + TYPHOON_REG_STATUS) !=
+			   TYPHOON_STATUS_WAITING_FOR_SEGMENT) {
+				printk(KERN_ERR "%s: segment ready timeout\n",
+				       tp->name);
+				goto err_out_irq;
+			}
 
-		image_data += section_len;
+			/* Do an pseudo IPv4 checksum on the data -- first
+			 * need to convert each u16 to cpu order before
+			 * summing. Fortunately, due to the properties of
+			 * the checksum, we can do this once, at the end.
+			 */
+			csum = csum_fold(csum_partial_copy_nocheck(image_data,
+								  dpage, len,
+								  0));
+
+			iowrite32(len, ioaddr + TYPHOON_REG_BOOT_LENGTH);
+			iowrite32(le16_to_cpu((__force __le16)csum),
+					ioaddr + TYPHOON_REG_BOOT_CHECKSUM);
+			iowrite32(load_addr,
+					ioaddr + TYPHOON_REG_BOOT_DEST_ADDR);
+			iowrite32(0, ioaddr + TYPHOON_REG_BOOT_DATA_HI);
+			iowrite32(dpage_dma, ioaddr + TYPHOON_REG_BOOT_DATA_LO);
+			typhoon_post_pci_writes(ioaddr);
+			iowrite32(TYPHOON_BOOTCMD_SEG_AVAILABLE,
+			       ioaddr + TYPHOON_REG_COMMAND);
+
+			image_data += len;
+			load_addr += len;
+			section_len -= len;
+		}
 	}
 
 	if(typhoon_wait_interrupt(ioaddr) < 0 ||
@@ -1511,7 +1542,7 @@ err_out_irq:
 	iowrite32(irqMasked, ioaddr + TYPHOON_REG_INTR_MASK);
 	iowrite32(irqEnabled, ioaddr + TYPHOON_REG_INTR_ENABLE);
 
-	pci_unmap_single(pdev, image_dma,  typhoon_fw->size, PCI_DMA_TODEVICE);
+	pci_free_consistent(pdev, PAGE_SIZE, dpage, dpage_dma);
 
 err_out:
 	return err;
@@ -2651,10 +2682,8 @@ typhoon_init(void)
 static void __exit
 typhoon_cleanup(void)
 {
-	if (typhoon_fw) {
-		kfree(typhoon_fw_image);
+	if (typhoon_fw)
 		release_firmware(typhoon_fw);
-	}
 	pci_unregister_driver(&typhoon_driver);
 }
 

Re: [PATCH v3 checked] typhoon: Use request_firmware()

[David Dillow]

typhoon: repair firmware loading

The conversion to avoid using pci_alloc_consistent() broke the firmware
load process, as well as added an order-4 kmalloc and doubled the memory
usage of the firmware image. Go back to loading a page at a time.

Also, since the user can now give us utter garbage for firmware, do a
cursory validation so we don't try to load just anything.

Signed-off-by: David Dillow <dave@thedillows.org>
---
 MAINTAINERS           |    2 
 drivers/net/typhoon.c |  137 +++++++++++++++++++++++++-----------------
 2 files changed, 84 insertions(+), 55 deletions(-)

I forgot to run checkpatch on it, and of course I would have some
trailing whitespace. I've left the non-space between if and the (, as
that is the style of the original code.

diff --git a/drivers/net/typhoon.c b/drivers/net/typhoon.c
index 9bba787..9dcd897 100644
--- a/drivers/net/typhoon.c
+++ b/drivers/net/typhoon.c
@@ -100,10 +100,11 @@ 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.5.9"
+#define DRV_MODULE_RELDATE	"Mar 2, 2009"
 #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>
@@ -136,7 +137,6 @@ static const int multicast_filter_limit = 32;
 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");
@@ -1347,11 +1347,16 @@ typhoon_init_rings(struct typhoon *tp)
 }
 
 static const struct firmware *typhoon_fw;
-static u8 *typhoon_fw_image;
 
 static int
 typhoon_request_firmware(struct typhoon *tp)
 {
+	const struct typhoon_file_header *fHdr;
+	const struct typhoon_section_header *sHdr;
+	const u8 *image_data;
+	u32 numSections;
+	u32 section_len;
+	u32 remaining;
 	int err;
 
 	if (typhoon_fw)
@@ -1360,31 +1365,45 @@ typhoon_request_firmware(struct typhoon *tp)
 	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);
+				tp->name, FIRMWARE_NAME);
 		return err;
 	}
 
-	if (typhoon_fw->size < sizeof(struct typhoon_file_header) ||
-	    memcmp(typhoon_fw->data, "TYPHOON", 8)) {
-		printk(KERN_ERR "%s: Invalid firmware image\n",
-		       tp->name);
-		err = -EINVAL;
-		goto out_err;
-	}
+	image_data = (u8 *) typhoon_fw->data;
+	remaining = typhoon_fw->size;
+	if (remaining < sizeof(struct typhoon_file_header))
+		goto invalid_fw;
 
-	typhoon_fw_image = kmalloc(typhoon_fw->size, GFP_KERNEL);
-	if (!typhoon_fw_image) {
-		err = -ENOMEM;
-		goto out_err;
+	fHdr = (struct typhoon_file_header *) image_data;
+	if (memcmp(fHdr->tag, "TYPHOON", 8))
+		goto invalid_fw;
+
+	numSections = le32_to_cpu(fHdr->numSections);
+	image_data += sizeof(struct typhoon_file_header);
+	remaining -= sizeof(struct typhoon_file_header);
+
+	while (numSections--) {
+		if (remaining < sizeof(struct typhoon_section_header))
+			goto invalid_fw;
+
+		sHdr = (struct typhoon_section_header *) image_data;
+		image_data += sizeof(struct typhoon_section_header);
+		section_len = le32_to_cpu(sHdr->len);
+
+		if (remaining < section_len)
+			goto invalid_fw;
+
+		image_data += section_len;
+		remaining -= section_len;
 	}
-	memcpy(typhoon_fw_image, typhoon_fw->data, typhoon_fw->size);
 
 	return 0;
 
-out_err:
+invalid_fw:
+	printk(KERN_ERR "%s: Invalid firmware image\n", tp->name);
 	release_firmware(typhoon_fw);
 	typhoon_fw = NULL;
-	return err;
+	return -EINVAL;
 }
 
 static int
@@ -1395,24 +1414,29 @@ typhoon_download_firmware(struct typhoon *tp)
 	const struct typhoon_file_header *fHdr;
 	const struct typhoon_section_header *sHdr;
 	const u8 *image_data;
-	dma_addr_t image_dma;
+	void *dpage;
+	dma_addr_t dpage_dma;
 	__sum16 csum;
 	u32 irqEnabled;
 	u32 irqMasked;
 	u32 numSections;
 	u32 section_len;
+	u32 len;
 	u32 load_addr;
 	u32 hmac;
 	int i;
 	int err;
 
-	image_data = typhoon_fw_image;
+	image_data = (u8 *) typhoon_fw->data;
 	fHdr = (struct typhoon_file_header *) image_data;
 
+	/* Cannot just map the firmware image using pci_map_single() as
+	 * the firmware is vmalloc()'d and may not be physically contiguous,
+	 * so we allocate some consistent memory to copy the sections into.
+	 */
 	err = -ENOMEM;
-	image_dma = pci_map_single(pdev, (u8 *) image_data,
-				   typhoon_fw->size, PCI_DMA_TODEVICE);
-	if (pci_dma_mapping_error(pdev, image_dma)) {
+	dpage = pci_alloc_consistent(pdev, PAGE_SIZE, &dpage_dma);
+	if(!dpage) {
 		printk(KERN_ERR "%s: no DMA mem for firmware\n", tp->name);
 		goto err_out;
 	}
@@ -1460,34 +1484,41 @@ typhoon_download_firmware(struct typhoon *tp)
 		load_addr = le32_to_cpu(sHdr->startAddr);
 		section_len = le32_to_cpu(sHdr->len);
 
-		if (typhoon_wait_interrupt(ioaddr) < 0 ||
-		    ioread32(ioaddr + TYPHOON_REG_STATUS) !=
-		    TYPHOON_STATUS_WAITING_FOR_SEGMENT) {
-			printk(KERN_ERR "%s: segment ready timeout\n",
-			       tp->name);
-			goto err_out_irq;
-		}
+		while(section_len) {
+			len = min_t(u32, section_len, PAGE_SIZE);
 
-		/* Do an pseudo IPv4 checksum on the data -- first
-		 * need to convert each u16 to cpu order before
-		 * summing. Fortunately, due to the properties of
-		 * the checksum, we can do this once, at the end.
-		 */
-		csum = csum_fold(csum_partial(image_data, section_len, 0));
-
-		iowrite32(section_len, ioaddr + TYPHOON_REG_BOOT_LENGTH);
-		iowrite32(le16_to_cpu((__force __le16)csum),
-			  ioaddr + TYPHOON_REG_BOOT_CHECKSUM);
-		iowrite32(load_addr,
-			  ioaddr + TYPHOON_REG_BOOT_DEST_ADDR);
-		iowrite32(0, ioaddr + TYPHOON_REG_BOOT_DATA_HI);
-		iowrite32(image_dma + (image_data - typhoon_fw_image),
-			  ioaddr + TYPHOON_REG_BOOT_DATA_LO);
-		typhoon_post_pci_writes(ioaddr);
-		iowrite32(TYPHOON_BOOTCMD_SEG_AVAILABLE,
-			  ioaddr + TYPHOON_REG_COMMAND);
+			if(typhoon_wait_interrupt(ioaddr) < 0 ||
+			   ioread32(ioaddr + TYPHOON_REG_STATUS) !=
+			   TYPHOON_STATUS_WAITING_FOR_SEGMENT) {
+				printk(KERN_ERR "%s: segment ready timeout\n",
+				       tp->name);
+				goto err_out_irq;
+			}
 
-		image_data += section_len;
+			/* Do an pseudo IPv4 checksum on the data -- first
+			 * need to convert each u16 to cpu order before
+			 * summing. Fortunately, due to the properties of
+			 * the checksum, we can do this once, at the end.
+			 */
+			csum = csum_fold(csum_partial_copy_nocheck(image_data,
+								  dpage, len,
+								  0));
+
+			iowrite32(len, ioaddr + TYPHOON_REG_BOOT_LENGTH);
+			iowrite32(le16_to_cpu((__force __le16)csum),
+					ioaddr + TYPHOON_REG_BOOT_CHECKSUM);
+			iowrite32(load_addr,
+					ioaddr + TYPHOON_REG_BOOT_DEST_ADDR);
+			iowrite32(0, ioaddr + TYPHOON_REG_BOOT_DATA_HI);
+			iowrite32(dpage_dma, ioaddr + TYPHOON_REG_BOOT_DATA_LO);
+			typhoon_post_pci_writes(ioaddr);
+			iowrite32(TYPHOON_BOOTCMD_SEG_AVAILABLE,
+			       ioaddr + TYPHOON_REG_COMMAND);
+
+			image_data += len;
+			load_addr += len;
+			section_len -= len;
+		}
 	}
 
 	if(typhoon_wait_interrupt(ioaddr) < 0 ||
@@ -1511,7 +1542,7 @@ err_out_irq:
 	iowrite32(irqMasked, ioaddr + TYPHOON_REG_INTR_MASK);
 	iowrite32(irqEnabled, ioaddr + TYPHOON_REG_INTR_ENABLE);
 
-	pci_unmap_single(pdev, image_dma,  typhoon_fw->size, PCI_DMA_TODEVICE);
+	pci_free_consistent(pdev, PAGE_SIZE, dpage, dpage_dma);
 
 err_out:
 	return err;
@@ -2651,10 +2682,8 @@ typhoon_init(void)
 static void __exit
 typhoon_cleanup(void)
 {
-	if (typhoon_fw) {
-		kfree(typhoon_fw_image);
+	if (typhoon_fw)
 		release_firmware(typhoon_fw);
-	}
 	pci_unregister_driver(&typhoon_driver);
 }
 

Re: [PATCH v3 checked] typhoon: Use request_firmware()

[David Miller]

From: David Dillow <dave@thedillows.org>
Date: Tue, 03 Mar 2009 00:49:16 -0500

> typhoon: repair firmware loading
> 
> The conversion to avoid using pci_alloc_consistent() broke the firmware
> load process, as well as added an order-4 kmalloc and doubled the memory
> usage of the firmware image. Go back to loading a page at a time.
> 
> Also, since the user can now give us utter garbage for firmware, do a
> cursory validation so we don't try to load just anything.
> 
> Signed-off-by: David Dillow <dave@thedillows.org>

Applied, thanks David.

Re: [PATCH v2] typhoon: Use request_firmware()

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Thu, 26 Feb 2009 04:26:21 +0000

> Based on a patch by Jaswinder Singh <jaswinder@infradead.org>.
> 
> Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Also applied, thanks.