Home » Mailing list archives » linux-netdev » 2009 » March » 2

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

Previous thread: unloading the ipv6 module by Daniel Lezcano on Thursday, February 26, 2009 - 6:22 am. (8 messages)

Next thread: probably bug in drr scheduler, 2.6.29-rc5 by Denys Fedoryschenko on Thursday, February 26, 2009 - 8:45 am. (7 messages)
[view original message]
From: Ben Hutchings
Subject: [PATCH v2] typhoon: Use request_firmware()
Date: Thursday, February 26, 2009 - 6:49 am

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 ...
[ message continues ]
[view original message]
From: David Miller
Subject: Re: [PATCH v2] typhoon: Use request_firmware()
Date: Friday, February 27, 2009 - 12:21 am

From: Ben Hutchings <ben@decadent.org.uk>

Applied.
--


[ message continues ]
[view original message]
From: David Dillow
Subject: Re: [PATCH v2] typhoon: Use request_firmware()
Date: Sunday, March 1, 2009 - 1:38 pm

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

--


[ message continues ]
[view original message]
From: Ben Hutchings
Subject: Re: [PATCH v2] typhoon: Use request_firmware()
Date: Sunday, March 1, 2009 - 6:02 pm

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.

You could well be right.

Ben.

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

[ message continues ]
[view original message]
From: David Miller
Subject: Re: [PATCH v2] typhoon: Use request_firmware()
Date: Sunday, March 1, 2009 - 6:44 pm

From: Ben Hutchings <ben@decadent.org.uk>

He is definitely right, this cannot work.
--


[ message continues ]
[view original message]
From: David Dillow
Subject: Re: [PATCH v2] typhoon: Use request_firmware()
Date: Sunday, March 1, 2009 - 8:16 pm

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...

--


[ message continues ]
[view original message]
From: David Miller
Subject: Re: [PATCH v2] typhoon: Use request_firmware()
Date: Sunday, March 1, 2009 - 9:11 pm

From: David Dillow <dave@thedillows.org>

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


[ message continues ]
[view original message]
From: David Miller
Subject: Re: [PATCH v2] typhoon: Use request_firmware()
Date: Sunday, March 1, 2009 - 9:29 pm

From: David Miller <davem@davemloft.net>

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 @@ ...
[ message continues ]
[view original message]
From: David Dillow
Subject: Re: [PATCH v2] typhoon: Use request_firmware()
Date: Monday, March 2, 2009 - 12:56 am

Like I said, I will be happy to test as soon as I can dig the hardware

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

Fix that and I think it will be OK.

--


[ message continues ]
[view original message]
From: David Miller
Subject: Re: [PATCH v2] typhoon: Use request_firmware()
Date: Monday, March 2, 2009 - 2:53 am

From: David Dillow <dave@thedillows.org>

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

--


[ message continues ]
[view original message]
From: David Dillow
Subject: Re: [PATCH v2] typhoon: Use request_firmware()
Date: Monday, March 2, 2009 - 10:44 pm

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 " (" ...
[ message continues ]
[view original message]
From: David Dillow
Subject: Re: [PATCH v3 checked] typhoon: Use request_firmware()
Date: Monday, March 2, 2009 - 10:49 pm

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 ...
[ message continues ]
[view original message]
From: David Miller
Subject: Re: [PATCH v3 checked] typhoon: Use request_firmware()
Date: Monday, March 2, 2009 - 11:15 pm

From: David Dillow <dave@thedillows.org>

Applied, thanks David.
--


[ message continues ]
Previous thread: unloading the ipv6 module by Daniel Lezcano on Thursday, February 26, 2009 - 6:22 am. (8 messages)

Next thread: probably bug in drr scheduler, 2.6.29-rc5 by Denys Fedoryschenko on Thursday, February 26, 2009 - 8:45 am. (7 messages)