RFC: move ath5k to use pci_request_selected_regions()

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

---

index 15ae868..a87a47f 100644
@@ -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);
 }
 

index c13e54b..913d0ab 100644
@@ -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;

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

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.

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

---

index 7754f5a..987bdd4 100644
@@ -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.