[PATCH] RFC: move ath5k to use pci_request_selected_regions()

[PATCH] RFC: move ath5k to use pci_request_selected_regions()

[Luis R. Rodriguez]

Documentation/pci.txt states:

"If your PCI device driver doesn't need I/O port resources assigned to
I/O Port BARs, you should use pci_enable_device_bars() instead of
pci_enable_device() in order not to enable I/O port regions for the
corresponding devices. In addition, you should use
pci_request_selected_regions() and pci_release_selected_regions()
instead of pci_request_regions()/pci_release_regions() in order not to
request/release I/O port regions for the corresponding devices."

So shall we? This also re-arranges the pci_release_selected_regions()
as per the documenation.

Changes to base.c
Changes-licensed-under: 3-clause-BSD

Signed-off-by: Luis R. Rodriguez <mcgrof@gmail.com>
---

diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 15ae868..a87a47f 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -445,11 +445,14 @@ ath5k_pci_probe(struct pci_dev *pdev,
 		const struct pci_device_id *id)
 {
 	void __iomem *mem;
-	struct ath5k_softc *sc;
+	struct ath5k_softc *sc = NULL;
 	struct ieee80211_hw *hw;
 	int ret;
+	int bars;
 	u8 csz;
 
+	bars = pci_select_bars(pdev, IORESOURCE_MEM);
+
 	ret = pci_enable_device(pdev);
 	if (ret) {
 		dev_err(&pdev->dev, "can't enable device\n");
@@ -495,7 +498,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	 */
 	pci_write_config_byte(pdev, 0x41, 0);
 
-	ret = pci_request_region(pdev, 0, "ath5k");
+	ret = pci_request_selected_regions(pdev, bars, "ath5k");
 	if (ret) {
 		dev_err(&pdev->dev, "cannot reserve PCI memory region\n");
 		goto err_dis;
@@ -528,6 +531,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	sc = hw->priv;
 	sc->hw = hw;
 	sc->pdev = pdev;
+	sc->bars = bars;
 
 	/*
 	 * Mark the device as detached to avoid processing
@@ -602,10 +606,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_selected_regions(pdev, sc->bars);
 err:
 	return ret;
 }
@@ -621,8 +625,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_selected_regions(pdev, sc->bars);
 	ieee80211_free_hw(hw);
 }
 
diff --git a/drivers/net/wireless/ath5k/base.h b/drivers/net/wireless/ath5k/base.h
index c13e54b..913d0ab 100644
--- a/drivers/net/wireless/ath5k/base.h
+++ b/drivers/net/wireless/ath5k/base.h
@@ -89,6 +89,7 @@ struct ath5k_txq {
  * associated with an instance of a device */
 struct ath5k_softc {
 	struct pci_dev		*pdev;		/* for dma mapping */
+	int			bars;
 	void __iomem		*iobase;	/* address of the device */
 	struct mutex		lock;		/* dev-level lock */
 	struct ieee80211_tx_queue_stats tx_stats;

Re: [PATCH] RFC: move ath5k to use pci_request_selected_regions()

[Jeff Garzik]

Luis R. Rodriguez wrote:
> Documentation/pci.txt states:
> 
> "If your PCI device driver doesn't need I/O port resources assigned to
> I/O Port BARs, you should use pci_enable_device_bars() instead of
> pci_enable_device() in order not to enable I/O port regions for the
> corresponding devices. In addition, you should use
> pci_request_selected_regions() and pci_release_selected_regions()
> instead of pci_request_regions()/pci_release_regions() in order not to
> request/release I/O port regions for the corresponding devices."
> 
> So shall we? This also re-arranges the pci_release_selected_regions()
> as per the documenation.
> 
> Changes to base.c
> Changes-licensed-under: 3-clause-BSD
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@gmail.com>

IMO the documentation is a bit bogus:

pci_request_regions() ensures that nobody else will touch -any- of your 
resources.

I see no logical reason to enable sharing of I/O regions with another 
driver, which would be the net effect of avoiding their request/release 
by using pci_request_selected_regions()

pci_enable_device() and pci_request_regions() are just fine; the strange 
and lesser-used APIs should be avoided unless you _need_ to use them.

	Jeff

Re: [PATCH] RFC: move ath5k to use pci_request_selected_regions()

[Luis R. Rodriguez]

On Thu, Nov 01, 2007 at 03:28:00PM -0400, Jeff Garzik wrote:
> Luis R. Rodriguez wrote:
>> Documentation/pci.txt states:
>> "If your PCI device driver doesn't need I/O port resources assigned to
>> I/O Port BARs, you should use pci_enable_device_bars() instead of
>> pci_enable_device() in order not to enable I/O port regions for the
>> corresponding devices. In addition, you should use
>> pci_request_selected_regions() and pci_release_selected_regions()
>> instead of pci_request_regions()/pci_release_regions() in order not to
>> request/release I/O port regions for the corresponding devices."
>> So shall we? This also re-arranges the pci_release_selected_regions()
>> as per the documenation.
>> Changes to base.c
>> Changes-licensed-under: 3-clause-BSD
>> Signed-off-by: Luis R. Rodriguez <mcgrof@gmail.com>
>
> IMO the documentation is a bit bogus:
>
> pci_request_regions() ensures that nobody else will touch -any- of your 
> resources.
>
> I see no logical reason to enable sharing of I/O regions with another 
> driver, which would be the net effect of avoiding their request/release by 
> using pci_request_selected_regions()

The documentation doesn't make emphasis in any way that 
pci_request_selected_regions() could be used to enable sharing of I/O
regions with another driver but I can see now how that makes sense. 
The documenation gives the impression it wants you to avoid unnecessary
enabling of I/O regions which the driver simply does not need access to.

> pci_enable_device() and pci_request_regions() are just fine; the strange 
> and lesser-used APIs should be avoided unless you _need_ to use them.

Gotcha, then how about this clarification:

**
If your PCI device driver doesn't need I/O port resources assigned to
I/O Port BARs, you *can* use pci_enable_device_bars() instead of
pci_enable_device() in order *not* to enable I/O port regions for the
corresponding device, however, you should avoid this unless completely
necessary as requesting all regions provides no overhead. You may want
to use this, for example, if you want to share some PCI regions of the
device with another driver. If used, you should then use
pci_request_selected_regions() and pci_release_selected_regions()
instead of pci_request_regions()/pci_release_regions() in order *not*
to request/release I/O port regions for the corresponding device.
**

Signed-off-by: Luis R. Rodriguez <mcgrof@gmail.com>
---

diff --git a/Documentation/pci.txt b/Documentation/pci.txt
index 7754f5a..987bdd4 100644
--- a/Documentation/pci.txt
+++ b/Documentation/pci.txt
@@ -626,12 +626,15 @@ assigned. The "PCI Local Bus Specification Revision 3.0" discusses
 this on p.44, "IMPLEMENTATION NOTE".
 
 If your PCI device driver doesn't need I/O port resources assigned to
-I/O Port BARs, you should use pci_enable_device_bars() instead of
-pci_enable_device() in order not to enable I/O port regions for the
-corresponding devices. In addition, you should use
+I/O Port BARs, you *can* use pci_enable_device_bars() instead of
+pci_enable_device() in order *not* to enable I/O port regions for the
+corresponding device, however, you should avoid this unless completely
+necessary as requesting all regions provides no overhead. You may want
+to use this, for example, if you want to share some PCI regions of the device
+with another driver. If used, you should then use
 pci_request_selected_regions() and pci_release_selected_regions()
-instead of pci_request_regions()/pci_release_regions() in order not to
-request/release I/O port regions for the corresponding devices.
+instead of pci_request_regions()/pci_release_regions() in order *not* to
+request/release I/O port regions for the corresponding device.
 
 [1] Some systems support 64KB I/O port space per PCI segment.
 [2] Some PCI-to-PCI bridges support optional 1KB aligned I/O base.