| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1217347786.2885.11.camel@jaswinder.satnam>
Date: Tue, 29 Jul 2008 21:39:46 +0530
From: Jaswinder Singh <jaswinder@...radead.org>
To: David Dillow <dave@...dillows.org>
Cc: LKML <linux-kernel@...r.kernel.org>, becker@...ld.com,
davidpmclean@...oo.com, Jeff Garzik <jeff@...zik.org>,
netdev <netdev@...r.kernel.org>,
David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH] typhoon: use request_firmware
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@...radead.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@...dillows.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists