Skip to contentSkip to conversation

p54: protect against sudden firmware file changes

PATCH In review Christian Lamparter 4 participants 10 messages Nov 14, 2008 – Nov 16, 2008 Networking drivers (wireless)

lore
Share & tools
lore

Christian Lamparter

author
#0 lore raw

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

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 <[email protected]>

---

drivers/net/wireless/p54/p54common.c +26 -6
diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
@@ -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;
 }
drivers/net/wireless/p54/p54.h +1 -0
diff -Nurp a/drivers/net/wireless/p54/p54.h b/drivers/net/wireless/p54/p54.h
@@ -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);
maintainer
#1 lore raw
Quoted 3 lines · 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.

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

johannes

author
#2 lore raw
Quoted 7 lines · On Friday 14 November 2008 23:00:28 Johannes Berg wrote:
Quoted 3 lines · 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.

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

maintainer
#3 lore raw
Quoted 6 lines · Christian Lamparter
Quoted 5 lines · On Friday 14 November 2008 23:00:28 Johannes Berg wrote:
Quoted 3 lines · 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.

Quoted 8 lines
Quoted 2 lines

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

author
#4 lore raw

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

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

Signed-off-by Christian Lamparter <[email protected]>

---

Quoted 5 lines · On Saturday 15 November 2008 01:02:30 Johannes Berg wrote:
Quoted 4 lines · Christian Lamparter
Quoted 2 lines · 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?

Quoted 8 lines
Quoted 5 lines

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

drivers/net/wireless/p54/p54common.c +12 -7
diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
@@ -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;
 }
drivers/net/wireless/p54/p54pci.c +21 -20

92 lines not on this page · raw message

drivers/net/wireless/p54/p54pci.h +2 -0
diff -Nurp a/drivers/net/wireless/p54/p54pci.h b/drivers/net/wireless/p54/p54pci.h
@@ -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 */
author
#5 lore raw

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

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

Signed-off-by Christian Lamparter <[email protected]>

---
v1 removed a bit too much. it was very chatty about the firmware capabilities...
---

drivers/net/wireless/p54/p54common.c +12 -4
diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
@@ -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;
 }
drivers/net/wireless/p54/p54pci.c +21 -20

92 lines not on this page · raw message

drivers/net/wireless/p54/p54pci.h +1 -1
diff -Nurp a/drivers/net/wireless/p54/p54pci.h b/drivers/net/wireless/p54/p54pci.h
@@ -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;
author
#6 lore raw

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

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

Signed-off-by Christian Lamparter <[email protected]>

---
v1 removed a bit too much. it was very chatty about the firmware capabilities... v2 forget release_firmware. d'oh!
---

drivers/net/wireless/p54/p54common.c +12 -4
diff -Nurp a/drivers/net/wireless/p54/p54common.c b/drivers/net/wireless/p54/p54common.c
@@ -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;
 }
drivers/net/wireless/p54/p54pci.c +22 -20

99 lines not on this page · raw message

drivers/net/wireless/p54/p54pci.h +1 -1
diff -Nurp a/drivers/net/wireless/p54/p54pci.h b/drivers/net/wireless/p54/p54pci.h
@@ -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;
#7 lore raw

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

Quoted 7 lines · Christian Lamparter

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

Signed-off-by Christian Lamparter <[email protected]>

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

maintainer
#8 lore raw

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

Quoted 3 lines · Larry Finger

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

#9 lore raw
In reply to Christian Lamparter
Quoted 6 lines · Christian Lamparter
Quoted 5 lines · On Friday 14 November 2008 23:00:28 Johannes Berg wrote:
Quoted 3 lines · 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.

Quoted 6 lines
Quoted 2 lines

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