2008-11-14 18:41:59

by Christian Lamparter

[permalink] [raw]
Subject: [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]>
---
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);


2008-11-16 11:20:36

by Christian Lamparter

[permalink] [raw]
Subject: [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!
---
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;

2008-11-15 00:42:30

by Christian Lamparter

[permalink] [raw]
Subject: [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]>
---
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 */

2008-11-14 23:54:33

by Christian Lamparter

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

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

2008-11-15 00:35:13

by Luis R. Rodriguez

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

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

2008-11-16 15:36:52

by Johannes Berg

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

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


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-11-15 18:30:22

by Christian Lamparter

[permalink] [raw]
Subject: [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...
---
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;

2008-11-14 22:00:33

by Johannes Berg

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

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


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-11-15 00:02:37

by Johannes Berg

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

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


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-11-16 15:35:08

by Larry Finger

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

Christian Lamparter wrote:
> 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