[PATCH 3/4] p54: protect against sudden firmware file changes

[PATCH 3/4] p54: protect against sudden firmware file changes

[Christian Lamparter]

Thanks to "p54: introduce new names for device firmwares"
(325ca16910a380c...) people are now migrating away from the buggy firmwares. 
Of course this is a very good idea, but no one told them to unload the driver first.

Signed-off-by: Christian Lamparter <chunkeey@web.de>
---
diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
--- a/drivers/net/wireless/p54/p54common.c	2008-11-14 18:08:22.000000000 +0100
+++ b/drivers/net/wireless/p54/p54common.c	2008-11-14 18:12:15.000000000 +0100
@@ -19,6 +19,7 @@
 #include <linux/init.h>
 #include <linux/firmware.h>
 #include <linux/etherdevice.h>
+#include <linux/crc32.h>
 
 #include <net/mac80211.h>
 
@@ -132,12 +133,22 @@ int p54_parse_firmware(struct ieee80211_
 	struct bootrec *bootrec;
 	u32 *data = (u32 *)fw->data;
 	u32 *end_data = (u32 *)fw->data + (fw->size >> 2);
+	u32 crc = crc32_le(~0, (void *)data, fw->size >> 2);
 	u8 *fw_version = NULL;
 	size_t len;
 	int i;
 
-	if (priv->rx_start)
-		return 0;
+	if (priv->fw_crc) {
+		if (priv->fw_crc == crc)
+			return 0;
+
+		printk(KERN_ERR "%s: DO NOT (ex)change firmware files while "
+				"the driver is loaded. This device is now "
+				"disabled. Please reload the driver, "
+				"if you want it back!",
+				wiphy_name(dev->wiphy));
+		return -EINVAL;
+	}
 
 	while (data < end_data && *data)
 		data++;
@@ -227,6 +238,7 @@ int p54_parse_firmware(struct ieee80211_
 		priv->tx_stats[7].limit = 2;		/* AC_BK */
 		dev->queues = 4;
 	}
+	priv->fw_crc = crc;
 
 	return 0;
 }
@@ -1525,16 +1537,24 @@ static int p54_start(struct ieee80211_hw
 
 	mutex_lock(&priv->conf_mutex);
 	err = priv->open(dev);
-	if (!err)
-		priv->mode = NL80211_IFTYPE_MONITOR;
+	if (err)
+		goto out;
 	P54_SET_QUEUE(priv->qos_params[0], 0x0002, 0x0003, 0x0007, 47);
 	P54_SET_QUEUE(priv->qos_params[1], 0x0002, 0x0007, 0x000f, 94);
 	P54_SET_QUEUE(priv->qos_params[2], 0x0003, 0x000f, 0x03ff, 0);
 	P54_SET_QUEUE(priv->qos_params[3], 0x0007, 0x000f, 0x03ff, 0);
 	err = p54_set_edcf(dev);
-	if (!err)
-		err = p54_init_stats(dev);
+	if (err)
+		goto out;
+	err = p54_init_stats(dev);
+	if (err)
+		goto out;
+	err = p54_setup_mac(dev, P54_FILTER_TYPE_NONE, NULL);
+	if (err)
+		goto out;
+	priv->mode = NL80211_IFTYPE_MONITOR;
 
+out:
 	mutex_unlock(&priv->conf_mutex);
 	return err;
 }
diff -Nurp a/drivers/net/wireless/p54/p54.h b/drivers/net/wireless/p54/p54.h
--- a/drivers/net/wireless/p54/p54.h	2008-11-14 00:14:59.000000000 +0100
+++ b/drivers/net/wireless/p54/p54.h	2008-11-05 23:31:28.000000000 +0100
@@ -73,6 +73,7 @@ struct p54_edcf_queue_param {
 struct p54_common {
 	u32 rx_start;
 	u32 rx_end;
+	u32 fw_crc;
 	struct sk_buff_head tx_queue;
 	void (*tx)(struct ieee80211_hw *dev, struct sk_buff *skb,
 		   int free_on_tx);

Re: [PATCH 3/4] p54: protect against sudden firmware file changes

[Johannes Berg]

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

On Fri, 2008-11-14 at 19:41 +0100, Christian Lamparter wrote:
> Thanks to "p54: introduce new names for device firmwares"
> (325ca16910a380c...) people are now migrating away from the buggy firmwares. 
> Of course this is a very good idea, but no one told them to unload the driver first.

that seems weird. Doesn't the driver pretty much have to cache the
firmware anyway for suspend/resume?

johannes

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

Re: [PATCH 3/4] p54: protect against sudden firmware file changes

[Christian Lamparter]

On Friday 14 November 2008 23:00:28 Johannes Berg wrote:
> On Fri, 2008-11-14 at 19:41 +0100, Christian Lamparter wrote:
> > Thanks to "p54: introduce new names for device firmwares"
> > (325ca16910a380c...) people are now migrating away from the buggy firmwares. 
> > Of course this is a very good idea, but no one told them to unload the driver first.
> 
> that seems weird. Doesn't the driver pretty much have to cache the
> firmware anyway for suspend/resume?

well, that's odd. No one complained about this bug yet?!
Even better, this bug/quirk is even present in the initial p54pci version that made it
into the kernel. In fact it looks like its even present in the fullmac driver! Luis?

I guess, I have to leave it that way to provide "bug-to-bug" compatibility! ;-)

Regards,
	Chr

Re: [PATCH 3/4] p54: protect against sudden firmware file changes

[Johannes Berg]

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

On Sat, 2008-11-15 at 00:54 +0100, Christian Lamparter wrote:
> On Friday 14 November 2008 23:00:28 Johannes Berg wrote:
> > On Fri, 2008-11-14 at 19:41 +0100, Christian Lamparter wrote:
> > > Thanks to "p54: introduce new names for device firmwares"
> > > (325ca16910a380c...) people are now migrating away from the buggy firmwares. 
> > > Of course this is a very good idea, but no one told them to unload the driver first.
> > 
> > that seems weird. Doesn't the driver pretty much have to cache the
> > firmware anyway for suspend/resume?
> 
> well, that's odd. No one complained about this bug yet?!
> Even better, this bug/quirk is even present in the initial p54pci version that made it
> into the kernel. In fact it looks like its even present in the fullmac driver! Luis?
> 
> I guess, I have to leave it that way to provide "bug-to-bug" compatibility! ;-)

Maybe it doesn't upload the firmware at resume? But that'd be odd.

johannes

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

[PATCH 3/4] p54pci: cache firmware for suspend/resume

[Christian Lamparter]

Johannes pointed out that the driver has cache the firmware for
suspend/resume cycles. 

Signed-off-by: Christian Lamparter <chunkeey@web.de>
---
On Saturday 15 November 2008 01:02:30 Johannes Berg wrote:
> On Sat, 2008-11-15 at 00:54 +0100, Christian Lamparter wrote:
> > On Friday 14 November 2008 23:00:28 Johannes Berg wrote:
> > > that seems weird. Doesn't the driver pretty much have to cache the
> > > firmware anyway for suspend/resume?
> > 
> > well, that's odd. No one complained about this bug yet?!
> > Even better, this bug/quirk is even present in the initial p54pci version that made it
> > into the kernel. In fact it looks like its even present in the fullmac driver! Luis?
> > 
> > I guess, I have to leave it that way to provide "bug-to-bug" compatibility! ;-)
> 
> Maybe it doesn't upload the firmware at resume? But that'd be odd.
> 
Well, after a suspend/resume any wifi-link is "probably" gone and all userspace
tools are trying to reactive them. and of course "this" time, this will work!

But yeah, odd... so what about this patch instead? sure we "waste" now
about 20-32kb but that's nothing compared to firmware images from other vendors.
---
diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
--- a/drivers/net/wireless/p54/p54common.c	2008-11-14 18:08:22.000000000 +0100
+++ b/drivers/net/wireless/p54/p54common.c	2008-11-15 01:29:38.000000000 +0100
@@ -136,9 +136,6 @@ int p54_parse_firmware(struct ieee80211_
 	size_t len;
 	int i;
 
-	if (priv->rx_start)
-		return 0;
-
 	while (data < end_data && *data)
 		data++;
 
@@ -1525,16 +1522,24 @@ static int p54_start(struct ieee80211_hw
 
 	mutex_lock(&priv->conf_mutex);
 	err = priv->open(dev);
-	if (!err)
-		priv->mode = NL80211_IFTYPE_MONITOR;
+	if (err)
+		goto out;
 	P54_SET_QUEUE(priv->qos_params[0], 0x0002, 0x0003, 0x0007, 47);
 	P54_SET_QUEUE(priv->qos_params[1], 0x0002, 0x0007, 0x000f, 94);
 	P54_SET_QUEUE(priv->qos_params[2], 0x0003, 0x000f, 0x03ff, 0);
 	P54_SET_QUEUE(priv->qos_params[3], 0x0007, 0x000f, 0x03ff, 0);
 	err = p54_set_edcf(dev);
-	if (!err)
-		err = p54_init_stats(dev);
+	if (err)
+		goto out;
+	err = p54_init_stats(dev);
+	if (err)
+		goto out;
+	err = p54_setup_mac(dev, P54_FILTER_TYPE_NONE, NULL);
+	if (err)
+		goto out;
+	priv->mode = NL80211_IFTYPE_MONITOR;
 
+out:
 	mutex_unlock(&priv->conf_mutex);
 	return err;
 }
diff -Nurp a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c
--- a/drivers/net/wireless/p54/p54pci.c	2008-11-14 00:14:59.000000000 +0100
+++ b/drivers/net/wireless/p54/p54pci.c	2008-11-15 01:33:58.000000000 +0100
@@ -47,7 +47,6 @@ MODULE_DEVICE_TABLE(pci, p54p_table);
 static int p54p_upload_firmware(struct ieee80211_hw *dev)
 {
 	struct p54p_priv *priv = dev->priv;
-	const struct firmware *fw_entry = NULL;
 	__le32 reg;
 	int err;
 	__le32 *data;
@@ -73,23 +72,15 @@ static int p54p_upload_firmware(struct i
 	P54P_WRITE(ctrl_stat, reg);
 	wmb();
 
-	err = request_firmware(&fw_entry, "isl3886pci", &priv->pdev->dev);
-	if (err) {
-		printk(KERN_ERR "%s (p54pci): cannot find firmware "
-		       "(isl3886pci)\n", pci_name(priv->pdev));
-		err = request_firmware(&fw_entry, "isl3886", &priv->pdev->dev);
-		if (err)
-			return err;
-	}
+	/* wait for the firmware to reset properly */
+	mdelay(10);
 
-	err = p54_parse_firmware(dev, fw_entry);
-	if (err) {
-		release_firmware(fw_entry);
+	err = p54_parse_firmware(dev, priv->firmware);
+	if (err)
 		return err;
-	}
 
-	data = (__le32 *) fw_entry->data;
-	remains = fw_entry->size;
+	data = (__le32 *) priv->firmware->data;
+	remains = priv->firmware->size;
 	device_addr = ISL38XX_DEV_FIRMWARE_ADDR;
 	while (remains) {
 		u32 i = 0;
@@ -107,8 +98,6 @@ static int p54p_upload_firmware(struct i
 		P54P_READ(int_enable);
 	}
 
-	release_firmware(fw_entry);
-
 	reg = P54P_READ(ctrl_stat);
 	reg &= cpu_to_le32(~ISL38XX_CTRL_STAT_CLKRUN);
 	reg &= cpu_to_le32(~ISL38XX_CTRL_STAT_RESET);
@@ -501,15 +490,14 @@ static int __devinit p54p_probe(struct p
 	if (mem_len < sizeof(struct p54p_csr)) {
 		printk(KERN_ERR "%s (p54pci): Too short PCI resources\n",
 		       pci_name(pdev));
-		pci_disable_device(pdev);
-		return err;
+		goto err_disable_dev;
 	}
 
 	err = pci_request_regions(pdev, "p54pci");
 	if (err) {
 		printk(KERN_ERR "%s (p54pci): Cannot obtain PCI resources\n",
 		       pci_name(pdev));
-		return err;
+		goto err_disable_dev;
 	}
 
 	if (pci_set_dma_mask(pdev, DMA_32BIT_MASK) ||
@@ -562,6 +550,17 @@ static int __devinit p54p_probe(struct p
 	spin_lock_init(&priv->lock);
 	tasklet_init(&priv->rx_tasklet, p54p_rx_tasklet, (unsigned long)dev);
 
+	err = request_firmware(&priv->firmware, "isl3886pci",
+			       &priv->pdev->dev);
+	if (err) {
+		printk(KERN_ERR "%s (p54pci): cannot find firmware "
+			"(isl3886pci)\n", pci_name(priv->pdev));
+		err = request_firmware(&priv->firmware, "isl3886",
+				       &priv->pdev->dev);
+		if (err)
+			goto err_free_common;
+	}
+
 	err = p54p_open(dev);
 	if (err)
 		goto err_free_common;
@@ -580,6 +579,7 @@ static int __devinit p54p_probe(struct p
 	return 0;
 
  err_free_common:
+	release_firmware(priv->firmware);
 	p54_free_common(dev);
 	pci_free_consistent(pdev, sizeof(*priv->ring_control),
 			    priv->ring_control, priv->ring_control_dma);
@@ -593,6 +593,7 @@ static int __devinit p54p_probe(struct p
 
  err_free_reg:
 	pci_release_regions(pdev);
+ err_disable_dev:
 	pci_disable_device(pdev);
 	return err;
 }
diff -Nurp a/drivers/net/wireless/p54/p54pci.h b/drivers/net/wireless/p54/p54pci.h
--- a/drivers/net/wireless/p54/p54pci.h	2008-11-14 00:14:59.000000000 +0100
+++ b/drivers/net/wireless/p54/p54pci.h	2008-11-15 01:04:29.000000000 +0100
@@ -104,6 +104,8 @@ struct p54p_priv {
 	void *tx_buf_data[32];
 	void *tx_buf_mgmt[4];
 	struct completion boot_comp;
+
+	const struct firmware *firmware;
 };
 
 #endif /* P54USB_H */

[PATCH 3/4] p54pci: cache firmware for suspend/resume v2

[Christian Lamparter]

Johannes pointed out that the driver has cache the firmware for
suspend/resume cycles. 
 
Signed-off-by: Christian Lamparter <chunkeey@web.de>
---
v1 removed a bit too much. it was very chatty about the firmware capabilities...
---
diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
--- a/drivers/net/wireless/p54/p54common.c	2008-11-14 18:08:22.000000000 +0100
+++ b/drivers/net/wireless/p54/p54common.c	2008-11-15 18:57:30.000000000 +0100
@@ -1525,16 +1525,24 @@ static int p54_start(struct ieee80211_hw
 
 	mutex_lock(&priv->conf_mutex);
 	err = priv->open(dev);
-	if (!err)
-		priv->mode = NL80211_IFTYPE_MONITOR;
+	if (err)
+		goto out;
 	P54_SET_QUEUE(priv->qos_params[0], 0x0002, 0x0003, 0x0007, 47);
 	P54_SET_QUEUE(priv->qos_params[1], 0x0002, 0x0007, 0x000f, 94);
 	P54_SET_QUEUE(priv->qos_params[2], 0x0003, 0x000f, 0x03ff, 0);
 	P54_SET_QUEUE(priv->qos_params[3], 0x0007, 0x000f, 0x03ff, 0);
 	err = p54_set_edcf(dev);
-	if (!err)
-		err = p54_init_stats(dev);
+	if (err)
+		goto out;
+	err = p54_init_stats(dev);
+	if (err)
+		goto out;
+	err = p54_setup_mac(dev, P54_FILTER_TYPE_NONE, NULL);
+	if (err)
+		goto out;
+	priv->mode = NL80211_IFTYPE_MONITOR;
 
+out:
 	mutex_unlock(&priv->conf_mutex);
 	return err;
 }
diff -Nurp a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c
--- a/drivers/net/wireless/p54/p54pci.c	2008-11-14 00:14:59.000000000 +0100
+++ b/drivers/net/wireless/p54/p54pci.c	2008-11-15 01:33:58.000000000 +0100
@@ -47,7 +47,6 @@ MODULE_DEVICE_TABLE(pci, p54p_table);
 static int p54p_upload_firmware(struct ieee80211_hw *dev)
 {
 	struct p54p_priv *priv = dev->priv;
-	const struct firmware *fw_entry = NULL;
 	__le32 reg;
 	int err;
 	__le32 *data;
@@ -73,23 +72,15 @@ static int p54p_upload_firmware(struct i
 	P54P_WRITE(ctrl_stat, reg);
 	wmb();
 
-	err = request_firmware(&fw_entry, "isl3886pci", &priv->pdev->dev);
-	if (err) {
-		printk(KERN_ERR "%s (p54pci): cannot find firmware "
-		       "(isl3886pci)\n", pci_name(priv->pdev));
-		err = request_firmware(&fw_entry, "isl3886", &priv->pdev->dev);
-		if (err)
-			return err;
-	}
+	/* wait for the firmware to reset properly */
+	mdelay(10);
 
-	err = p54_parse_firmware(dev, fw_entry);
-	if (err) {
-		release_firmware(fw_entry);
+	err = p54_parse_firmware(dev, priv->firmware);
+	if (err)
 		return err;
-	}
 
-	data = (__le32 *) fw_entry->data;
-	remains = fw_entry->size;
+	data = (__le32 *) priv->firmware->data;
+	remains = priv->firmware->size;
 	device_addr = ISL38XX_DEV_FIRMWARE_ADDR;
 	while (remains) {
 		u32 i = 0;
@@ -107,8 +98,6 @@ static int p54p_upload_firmware(struct i
 		P54P_READ(int_enable);
 	}
 
-	release_firmware(fw_entry);
-
 	reg = P54P_READ(ctrl_stat);
 	reg &= cpu_to_le32(~ISL38XX_CTRL_STAT_CLKRUN);
 	reg &= cpu_to_le32(~ISL38XX_CTRL_STAT_RESET);
@@ -501,15 +490,14 @@ static int __devinit p54p_probe(struct p
 	if (mem_len < sizeof(struct p54p_csr)) {
 		printk(KERN_ERR "%s (p54pci): Too short PCI resources\n",
 		       pci_name(pdev));
-		pci_disable_device(pdev);
-		return err;
+		goto err_disable_dev;
 	}
 
 	err = pci_request_regions(pdev, "p54pci");
 	if (err) {
 		printk(KERN_ERR "%s (p54pci): Cannot obtain PCI resources\n",
 		       pci_name(pdev));
-		return err;
+		goto err_disable_dev;
 	}
 
 	if (pci_set_dma_mask(pdev, DMA_32BIT_MASK) ||
@@ -562,6 +550,17 @@ static int __devinit p54p_probe(struct p
 	spin_lock_init(&priv->lock);
 	tasklet_init(&priv->rx_tasklet, p54p_rx_tasklet, (unsigned long)dev);
 
+	err = request_firmware(&priv->firmware, "isl3886pci",
+			       &priv->pdev->dev);
+	if (err) {
+		printk(KERN_ERR "%s (p54pci): cannot find firmware "
+			"(isl3886pci)\n", pci_name(priv->pdev));
+		err = request_firmware(&priv->firmware, "isl3886",
+				       &priv->pdev->dev);
+		if (err)
+			goto err_free_common;
+	}
+
 	err = p54p_open(dev);
 	if (err)
 		goto err_free_common;
@@ -580,6 +579,7 @@ static int __devinit p54p_probe(struct p
 	return 0;
 
  err_free_common:
+	release_firmware(priv->firmware);
 	p54_free_common(dev);
 	pci_free_consistent(pdev, sizeof(*priv->ring_control),
 			    priv->ring_control, priv->ring_control_dma);
@@ -593,6 +593,7 @@ static int __devinit p54p_probe(struct p
 
  err_free_reg:
 	pci_release_regions(pdev);
+ err_disable_dev:
 	pci_disable_device(pdev);
 	return err;
 }
diff -Nurp a/drivers/net/wireless/p54/p54pci.h b/drivers/net/wireless/p54/p54pci.h
--- a/drivers/net/wireless/p54/p54pci.h	2008-11-14 00:14:59.000000000 +0100
+++ b/drivers/net/wireless/p54/p54pci.h	2008-11-15 18:58:41.000000000 +0100
@@ -93,7 +93,7 @@ struct p54p_priv {
 	struct pci_dev *pdev;
 	struct p54p_csr __iomem *map;
 	struct tasklet_struct rx_tasklet;
-
+	const struct firmware *firmware;
 	spinlock_t lock;
 	struct p54p_ring_control *ring_control;
 	dma_addr_t ring_control_dma;

[PATCH 3/4] p54pci: cache firmware for suspend/resume v2.1

[Christian Lamparter]

Johannes pointed out that the driver has cache the firmware for
suspend/resume cycles. 
 
Signed-off-by: Christian Lamparter <chunkeey@web.de>
---
v1 removed a bit too much. it was very chatty about the firmware capabilities...
v2 forget release_firmware. d'oh!
---
diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
--- a/drivers/net/wireless/p54/p54common.c	2008-11-14 18:08:22.000000000 +0100
+++ b/drivers/net/wireless/p54/p54common.c	2008-11-15 18:57:30.000000000 +0100
@@ -1525,16 +1525,24 @@ static int p54_start(struct ieee80211_hw
 
 	mutex_lock(&priv->conf_mutex);
 	err = priv->open(dev);
-	if (!err)
-		priv->mode = NL80211_IFTYPE_MONITOR;
+	if (err)
+		goto out;
 	P54_SET_QUEUE(priv->qos_params[0], 0x0002, 0x0003, 0x0007, 47);
 	P54_SET_QUEUE(priv->qos_params[1], 0x0002, 0x0007, 0x000f, 94);
 	P54_SET_QUEUE(priv->qos_params[2], 0x0003, 0x000f, 0x03ff, 0);
 	P54_SET_QUEUE(priv->qos_params[3], 0x0007, 0x000f, 0x03ff, 0);
 	err = p54_set_edcf(dev);
-	if (!err)
-		err = p54_init_stats(dev);
+	if (err)
+		goto out;
+	err = p54_init_stats(dev);
+	if (err)
+		goto out;
+	err = p54_setup_mac(dev, P54_FILTER_TYPE_NONE, NULL);
+	if (err)
+		goto out;
+	priv->mode = NL80211_IFTYPE_MONITOR;
 
+out:
 	mutex_unlock(&priv->conf_mutex);
 	return err;
 }
diff -Nurp a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c
--- a/drivers/net/wireless/p54/p54pci.c	2008-11-14 00:14:59.000000000 +0100
+++ b/drivers/net/wireless/p54/p54pci.c	2008-11-16 12:09:44.000000000 +0100
@@ -47,7 +47,6 @@ MODULE_DEVICE_TABLE(pci, p54p_table);
 static int p54p_upload_firmware(struct ieee80211_hw *dev)
 {
 	struct p54p_priv *priv = dev->priv;
-	const struct firmware *fw_entry = NULL;
 	__le32 reg;
 	int err;
 	__le32 *data;
@@ -73,23 +72,15 @@ static int p54p_upload_firmware(struct i
 	P54P_WRITE(ctrl_stat, reg);
 	wmb();
 
-	err = request_firmware(&fw_entry, "isl3886pci", &priv->pdev->dev);
-	if (err) {
-		printk(KERN_ERR "%s (p54pci): cannot find firmware "
-		       "(isl3886pci)\n", pci_name(priv->pdev));
-		err = request_firmware(&fw_entry, "isl3886", &priv->pdev->dev);
-		if (err)
-			return err;
-	}
+	/* wait for the firmware to reset properly */
+	mdelay(10);
 
-	err = p54_parse_firmware(dev, fw_entry);
-	if (err) {
-		release_firmware(fw_entry);
+	err = p54_parse_firmware(dev, priv->firmware);
+	if (err)
 		return err;
-	}
 
-	data = (__le32 *) fw_entry->data;
-	remains = fw_entry->size;
+	data = (__le32 *) priv->firmware->data;
+	remains = priv->firmware->size;
 	device_addr = ISL38XX_DEV_FIRMWARE_ADDR;
 	while (remains) {
 		u32 i = 0;
@@ -107,8 +98,6 @@ static int p54p_upload_firmware(struct i
 		P54P_READ(int_enable);
 	}
 
-	release_firmware(fw_entry);
-
 	reg = P54P_READ(ctrl_stat);
 	reg &= cpu_to_le32(~ISL38XX_CTRL_STAT_CLKRUN);
 	reg &= cpu_to_le32(~ISL38XX_CTRL_STAT_RESET);
@@ -501,15 +490,14 @@ static int __devinit p54p_probe(struct p
 	if (mem_len < sizeof(struct p54p_csr)) {
 		printk(KERN_ERR "%s (p54pci): Too short PCI resources\n",
 		       pci_name(pdev));
-		pci_disable_device(pdev);
-		return err;
+		goto err_disable_dev;
 	}
 
 	err = pci_request_regions(pdev, "p54pci");
 	if (err) {
 		printk(KERN_ERR "%s (p54pci): Cannot obtain PCI resources\n",
 		       pci_name(pdev));
-		return err;
+		goto err_disable_dev;
 	}
 
 	if (pci_set_dma_mask(pdev, DMA_32BIT_MASK) ||
@@ -562,6 +550,17 @@ static int __devinit p54p_probe(struct p
 	spin_lock_init(&priv->lock);
 	tasklet_init(&priv->rx_tasklet, p54p_rx_tasklet, (unsigned long)dev);
 
+	err = request_firmware(&priv->firmware, "isl3886pci",
+			       &priv->pdev->dev);
+	if (err) {
+		printk(KERN_ERR "%s (p54pci): cannot find firmware "
+			"(isl3886pci)\n", pci_name(priv->pdev));
+		err = request_firmware(&priv->firmware, "isl3886",
+				       &priv->pdev->dev);
+		if (err)
+			goto err_free_common;
+	}
+
 	err = p54p_open(dev);
 	if (err)
 		goto err_free_common;
@@ -580,6 +579,7 @@ static int __devinit p54p_probe(struct p
 	return 0;
 
  err_free_common:
+	release_firmware(priv->firmware);
 	p54_free_common(dev);
 	pci_free_consistent(pdev, sizeof(*priv->ring_control),
 			    priv->ring_control, priv->ring_control_dma);
@@ -593,6 +593,7 @@ static int __devinit p54p_probe(struct p
 
  err_free_reg:
 	pci_release_regions(pdev);
+ err_disable_dev:
 	pci_disable_device(pdev);
 	return err;
 }
@@ -607,6 +608,7 @@ static void __devexit p54p_remove(struct
 
 	ieee80211_unregister_hw(dev);
 	priv = dev->priv;
+	release_firmware(priv->firmware);
 	pci_free_consistent(pdev, sizeof(*priv->ring_control),
 			    priv->ring_control, priv->ring_control_dma);
 	p54_free_common(dev);
diff -Nurp a/drivers/net/wireless/p54/p54pci.h b/drivers/net/wireless/p54/p54pci.h
--- a/drivers/net/wireless/p54/p54pci.h	2008-11-14 00:14:59.000000000 +0100
+++ b/drivers/net/wireless/p54/p54pci.h	2008-11-15 18:58:41.000000000 +0100
@@ -93,7 +93,7 @@ struct p54p_priv {
 	struct pci_dev *pdev;
 	struct p54p_csr __iomem *map;
 	struct tasklet_struct rx_tasklet;
-
+	const struct firmware *firmware;
 	spinlock_t lock;
 	struct p54p_ring_control *ring_control;
 	dma_addr_t ring_control_dma;

Re: [PATCH 3/4] p54pci: cache firmware for suspend/resume v2.1

[Larry Finger]

Christian Lamparter wrote:
> Johannes pointed out that the driver has cache the firmware for
> suspend/resume cycles. 
>  
> Signed-off-by: Christian Lamparter <chunkeey@web.de>
> ---
> v1 removed a bit too much. it was very chatty about the firmware capabilities...
> v2 forget release_firmware. d'oh!

Christian,

When you post updated versions, please indicate them in the subject line, as in
"[PATCH 3/4 V3] ...." or a similar indication. When several versions have the
same subject, it gets hard to keep them separate.

Thanks,

Larry

Re: [PATCH 3/4] p54pci: cache firmware for suspend/resume v2.1

[Johannes Berg]

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

On Sun, 2008-11-16 at 09:35 -0600, Larry Finger wrote:

> When you post updated versions, please indicate them in the subject line, as in
> "[PATCH 3/4 V3] ...." or a similar indication. When several versions have the
> same subject, it gets hard to keep them separate.

He did add it at the end, but I agree that putting it into the [PATCH]
brackets is better since then it won't end up in the git tree changelog.

johannes

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

Re: [PATCH 3/4] p54: protect against sudden firmware file changes

[Luis R. Rodriguez]

On Fri, Nov 14, 2008 at 03:54:29PM -0800, Christian Lamparter wrote:
> On Friday 14 November 2008 23:00:28 Johannes Berg wrote:
> > On Fri, 2008-11-14 at 19:41 +0100, Christian Lamparter wrote:
> > > Thanks to "p54: introduce new names for device firmwares"
> > > (325ca16910a380c...) people are now migrating away from the buggy firmwares.
> > > Of course this is a very good idea, but no one told them to unload the driver first.
> >
> > that seems weird. Doesn't the driver pretty much have to cache the
> > firmware anyway for suspend/resume?
> 
> well, that's odd. No one complained about this bug yet?!
> Even better, this bug/quirk is even present in the initial p54pci version that made it
> into the kernel. In fact it looks like its even present in the fullmac driver! Luis?

Umm, hey there's a shiny new driver called p54pci, I hear its maintainer
is really good with it and actually keeps it up to date and everything.

  Luis