On Thu, Nov 01, 2007 at 03:49:09PM -0400, John W. Linville wrote:
> On Thu, Nov 01, 2007 at 03:17:16PM -0400, Luis R. Rodriguez wrote:
>
> > So I started reviewing the probes on each driver and came up with this
> > patch because Documenation/pci.txt has:
> >
> > "The device driver needs to call pci_request_region() to verify
> > no other device is already using the same address resource.
> > Conversely, drivers should call pci_release_region() AFTER
> > calling pci_disable_device(). The idea is to prevent two devices
> > colliding on the same address range"
>
> No idea off the top of my head if this relates to the problem or not...
>
> > --- a/drivers/net/wireless/ath5k/base.c
> > +++ b/drivers/net/wireless/ath5k/base.c
> > @@ -602,10 +602,10 @@ err_free:
> > ieee80211_free_hw(hw);
> > err_map:
> > pci_iounmap(pdev, mem);
> > -err_reg:
> > - pci_release_region(pdev, 0);
> > err_dis:
> > pci_disable_device(pdev);
> > +err_reg:
> > + pci_release_region(pdev, 0);
> > err:
> > return ret;
> > }
>
> If you do this, don't you need to change any "goto err_reg" to "goto
> err_dis" as well?
>
> > --- a/drivers/net/wireless/ipw2200.c
> > +++ b/drivers/net/wireless/ipw2200.c
> > @@ -11756,10 +11756,10 @@ static int ipw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > priv->workqueue = NULL;
> > out_iounmap:
> > iounmap(priv->hw_base);
> > - out_pci_release_regions:
> > - pci_release_regions(pdev);
> > out_pci_disable_device:
> > pci_disable_device(pdev);
> > + out_pci_release_regions:
> > + pci_release_regions(pdev);
> > pci_set_drvdata(pdev, NULL);
> > out_free_ieee80211:
> > free_ieee80211(priv->net_dev);
>
> Same as last comment, but for out_pci_release_regions and
> out_pci_disable_device.
Yeah, you're right, duh, here it is with some more changes. Anyway
this still doesn't fix it but it does address the documenation.
Any more ideas?
Changes to base.c
Changes-licensed-under: 3-clause-BSD
Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxx>
---
diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 15ae868..cea17ce 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -453,14 +453,15 @@ ath5k_pci_probe(struct pci_dev *pdev,
ret = pci_enable_device(pdev);
if (ret) {
dev_err(&pdev->dev, "can't enable device\n");
- goto err;
+ return ret;
}
/* XXX 32-bit addressing only */
ret = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
if (ret) {
dev_err(&pdev->dev, "32-bit DMA not available\n");
- goto err_dis;
+ pci_disable_device(pdev);
+ return ret;
}
/*
@@ -498,14 +499,15 @@ ath5k_pci_probe(struct pci_dev *pdev,
ret = pci_request_region(pdev, 0, "ath5k");
if (ret) {
dev_err(&pdev->dev, "cannot reserve PCI memory region\n");
- goto err_dis;
+ pci_disable_device(pdev);
+ return ret;
}
mem = pci_iomap(pdev, 0, 0);
if (!mem) {
dev_err(&pdev->dev, "cannot remap PCI memory region\n") ;
ret = -EIO;
- goto err_reg;
+ goto err_dis;
}
/*
@@ -602,11 +604,9 @@ err_free:
ieee80211_free_hw(hw);
err_map:
pci_iounmap(pdev, mem);
-err_reg:
- pci_release_region(pdev, 0);
err_dis:
pci_disable_device(pdev);
-err:
+ pci_release_region(pdev, 0);
return ret;
}
@@ -621,8 +621,8 @@ ath5k_pci_remove(struct pci_dev *pdev)
free_irq(pdev->irq, sc);
pci_disable_msi(pdev);
pci_iounmap(pdev, sc->iobase);
- pci_release_region(pdev, 0);
pci_disable_device(pdev);
+ pci_release_region(pdev, 0);
ieee80211_free_hw(hw);
}
diff --git a/drivers/net/wireless/ipw2200.c b/drivers/net/wireless/ipw2200.c
index 54f44e5..7f2ea6d 100644
--- a/drivers/net/wireless/ipw2200.c
+++ b/drivers/net/wireless/ipw2200.c
@@ -11611,8 +11611,7 @@ static int ipw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
net_dev = alloc_ieee80211(sizeof(struct ipw_priv));
if (net_dev == NULL) {
- err = -ENOMEM;
- goto out;
+ return -ENOMEM;
}
priv = ieee80211_priv(net_dev);
@@ -11628,8 +11627,8 @@ static int ipw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
mutex_init(&priv->mutex);
if (pci_enable_device(pdev)) {
- err = -ENODEV;
- goto out_free_ieee80211;
+ free_ieee80211(priv->net_dev);
+ return -ENODEV;
}
pci_set_master(pdev);
@@ -11639,14 +11638,14 @@ static int ipw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
err = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
if (err) {
printk(KERN_WARNING DRV_NAME ": No suitable DMA available.\n");
- goto out_pci_disable_device;
+ goto out_pci_disable_device_end;
}
pci_set_drvdata(pdev, priv);
err = pci_request_regions(pdev, DRV_NAME);
if (err)
- goto out_pci_disable_device;
+ goto out_pci_disable_device_end;
/* We disable the RETRY_TIMEOUT register (0x41) to keep
* PCI Tx retries from interfering with C3 CPU state */
@@ -11660,7 +11659,7 @@ static int ipw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
base = ioremap_nocache(pci_resource_start(pdev, 0), length);
if (!base) {
err = -ENODEV;
- goto out_pci_release_regions;
+ goto out_pci_disable_device;
}
priv->hw_base = base;
@@ -11756,14 +11755,15 @@ static int ipw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
priv->workqueue = NULL;
out_iounmap:
iounmap(priv->hw_base);
- out_pci_release_regions:
- pci_release_regions(pdev);
out_pci_disable_device:
pci_disable_device(pdev);
+ pci_release_regions(pdev);
pci_set_drvdata(pdev, NULL);
- out_free_ieee80211:
free_ieee80211(priv->net_dev);
- out:
+ return err;
+ out_pci_disable_device_end: /* We don't release regions here */
+ pci_disable_device(pdev);
+ free_ieee80211(priv->net_dev);
return err;
}
@@ -11824,8 +11824,8 @@ static void ipw_pci_remove(struct pci_dev *pdev)
free_irq(pdev->irq, priv);
iounmap(priv->hw_base);
- pci_release_regions(pdev);
pci_disable_device(pdev);
+ pci_release_regions(pdev);
pci_set_drvdata(pdev, NULL);
free_ieee80211(priv->net_dev);
free_firmware();
-
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
