Skip to contentSkip to conversation

PCI: Convert maintaining FW-assigned BIOS BAR values to a list of temporary entries

PATCH In review Myron Stowe 3 participants 6 messages Nov 10, 2011 – Nov 14, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover PCI: Convert maintaining FW-assigned BIOS BAR values to a list of temporary entries

In commit 58c84eda075 functionality was introduced that tries to reinstate the original, FW-assigned, BAR addresses of a PCI device when normal resource assignment attempts fail. To keep track of the BIOS BAR addresses, struct pci_dev was augmented with an array to hold the BAR addresses of the PCI device: 'resource_size_t fw_addr[DEVICE_COUNT_RESOURCE]'.

The reinstatement of BAR addresses is an uncommon event leaving the 'fw_addr' array unused normally. As the use of struct pci_dev is so prevalent, this seems wasteful.

This patch series introduces a stand alone data structure and corresponding set of interfacing routines, converting the underlying aspects of the existing maintanence scheme to a list of temporary BIOS BAR value entries instead.

---

Myron Stowe (3):
x86/PCI: Convert maintaining FW-assigned BIOS BAR values to use a list
PCI: Maintain a list of FW-assigned BIOS BAR values
PCI: Fix starting basis for resource requests

 arch/x86/pci/i386.c     |    2 -
 drivers/pci/setup-res.c |  100 ++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/pci.h     |    2 -
 3 files changed, 92 insertions(+), 12 deletions(-)
#1 lore raw

PATCH 1/3 PCI: Fix starting basis for resource requests

From: Myron Stowe <[email protected]>

pci_revert_fw_address() is used to reinstate a PCI device's original FW-assigned BIOS BAR value(s) if normal resource assignment fails.

When attempting to reinstate an address, the point within the resource tree from which to attempt the new resource request should be the parent resource corresponding to the device, not the base of the resource tree (ioport_resource or iomem_resource). For PCI devices this would typically be the resource corresponding to the upstream PCI host bridge or P2P bridge aperture.

This patch sets the point within the resource tree to attempt a new resource assignment request to the PCI device's parent resource and only if that fails does it fall back to the base ioport_resource or iomem_resource.

---

 drivers/pci/setup-res.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)
drivers/pci/setup-res.c +8 -5
index 51a9095..ad8c4ae 100644
@@ -164,15 +164,18 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev,
 	resource_size_t start, end;
 	int ret = 0;
 
-	if (res->flags & IORESOURCE_IO)
-		root = &ioport_resource;
-	else
-		root = &iomem_resource;
-
 	start = res->start;
 	end = res->end;
 	res->start = dev->fw_addr[resno];
 	res->end = res->start + size - 1;
+
+	root = pci_find_parent_resource(dev, res);
+	if (!root)
+		if (res->flags & IORESOURCE_IO)
+			root = &ioport_resource;
+		else
+			root = &iomem_resource;
+
 	dev_info(&dev->dev, "BAR %d: trying firmware assignment %pR\n",
 		 resno, res);
 	conflict = request_resource_conflict(root, res);
#2 lore raw

PATCH 2/3 PCI: Maintain a list of FW-assigned BIOS BAR values

In reply to Myron Stowe

From: Myron Stowe <[email protected]>

Commit 58c84eda075 introduced functionality to try and reinstate the original BIOS BAR addresses of a PCI device when normal resource assignment attempts fail. To keep track of the BIOS BAR addresses, struct pci_dev was augmented with an array to hold the BAR addresses of the PCI device: 'resource_size_t fw_addr[DEVICE_COUNT_RESOURCE]'.

The reinstatement of BAR addresses is an uncommon event leaving the 'fw_addr' array unused normally. As the use of struct pci_dev is so prevalent, this seems wasteful.

This patch introduces a stand alone data structure and corresponding set of interfacing routines for maintaining a list of FW-assigned BIOS BAR value temporary entries.

---

 drivers/pci/setup-res.c |   73 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |    1 +
 2 files changed, 74 insertions(+), 0 deletions(-)
drivers/pci/setup-res.c +73 -0
index ad8c4ae..ab45c11 100644
@@ -26,6 +26,79 @@
 #include "pci.h"
 
 
+/*
+ * This list of dynamic mappings is for temporarly maintaining
+ * original BIOS BAR addresses for possbile reinstatement.
+ */
+struct pci_fwaddrmap {
+	struct list_head list;
+	struct pci_dev *dev;
+	resource_size_t fw_addr[DEVICE_COUNT_RESOURCE];
+	unsigned long refcount;
+};
+
+static LIST_HEAD(pci_fwaddrmappings);
+static DEFINE_SPINLOCK(pci_fwaddrmap_lock);
+
+/* Must be called with 'pci_fwaddrmap_lock' lock held. */
+static struct pci_fwaddrmap *pci_fwaddrmap_lookup(struct pci_dev *dev)
+{
+	struct pci_fwaddrmap *map;
+
+	list_for_each_entry(map, &pci_fwaddrmappings, list)
+		if (map->dev == dev)
+			return map;
+
+	return NULL;
+}
+
+void pci_save_fw_addr(struct pci_dev *dev, int idx, resource_size_t fw_addr)
+{
+	unsigned long flags;
+	struct pci_fwaddrmap *map;
+
+	spin_lock_irqsave(&pci_fwaddrmap_lock, flags);
+	map = pci_fwaddrmap_lookup(dev);
+	if (!map) {
+		spin_unlock_irqrestore(&pci_fwaddrmap_lock, flags);
+		map = kzalloc(sizeof(*map), GFP_KERNEL);
+		if (!map)
+			return;
+
+		INIT_LIST_HEAD(&map->list);
+		map->dev = dev;
+		map->fw_addr[idx] = fw_addr;
+
+		spin_lock_irqsave(&pci_fwaddrmap_lock, flags);
+		list_add_tail(&map->list, &pci_fwaddrmappings);
+	} else {
+		map->fw_addr[idx] = fw_addr;
+	}
+	map->refcount++;
+	spin_unlock_irqrestore(&pci_fwaddrmap_lock, flags);
+}
+
+static resource_size_t pci_retrieve_fw_addr(struct pci_dev *dev, int idx)
+{
+	unsigned long flags;
+	resource_size_t fw_addr = 0;
+	struct pci_fwaddrmap *map;
+
+	spin_lock_irqsave(&pci_fwaddrmap_lock, flags);
+	map = pci_fwaddrmap_lookup(dev);
+	if (map) {
+		fw_addr = map->fw_addr[idx];
+
+		if (!--map->refcount) {
+			list_del(&map->list);
+			kfree(map);
+		}
+	}
+	spin_unlock_irqrestore(&pci_fwaddrmap_lock, flags);
+
+	return fw_addr;
+}
+
 void pci_update_resource(struct pci_dev *dev, int resno)
 {
 	struct pci_bus_region region;
include/linux/pci.h +1 -0
index 84225c7..14f2492 100644
@@ -889,6 +889,7 @@ ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void
 int pci_vpd_truncate(struct pci_dev *dev, size_t size);
 
 /* Helper functions for low-level code (drivers/pci/setup-[bus,res].c) */
+void pci_save_fw_addr(struct pci_dev *dev, int idx, resource_size_t fw_addr);
 void pci_bus_assign_resources(const struct pci_bus *bus);
 void pci_bus_size_bridges(struct pci_bus *bus);
 int pci_claim_resource(struct pci_dev *, int);
#3 lore raw

Re: [PATCH 2/3] PCI: Maintain a list of FW-assigned BIOS BAR values

Quoted 34 lines · On Wed, 09 Nov 2011 17:43:51 -0700 Myron Stowe <[email protected]> wrote:

From: Myron Stowe <[email protected]>

Commit 58c84eda075 introduced functionality to try and reinstate the original BIOS BAR addresses of a PCI device when normal resource assignment attempts fail. To keep track of the BIOS BAR addresses, struct pci_dev was augmented with an array to hold the BAR addresses of the PCI device: 'resource_size_t fw_addr[DEVICE_COUNT_RESOURCE]'.

The reinstatement of BAR addresses is an uncommon event leaving the 'fw_addr' array unused normally. As the use of struct pci_dev is so prevalent, this seems wasteful.

This patch introduces a stand alone data structure and corresponding set of interfacing routines for maintaining a list of FW-assigned BIOS BAR value temporary entries.

---

 drivers/pci/setup-res.c |   73 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |    1 +
 2 files changed, 74 insertions(+), 0 deletions(-)
drivers/pci/setup-res.c +3 -0
index ad8c4ae..ab45c11 100644
@@ -26,6 +26,79 @@
 #include "pci.h"
 
 
+/*
+ * This list of dynamic mappings is for temporarly maintaining
+ * original BIOS BAR addresses for possbile reinstatement.

drivers/pci/setup-res.c line 41

Quoted 10 lines
drivers/pci/setup-res.c +10 -0
+ */
+struct pci_fwaddrmap {
+	struct list_head list;
+	struct pci_dev *dev;
+	resource_size_t fw_addr[DEVICE_COUNT_RESOURCE];
+	unsigned long refcount;
+};
+
+static LIST_HEAD(pci_fwaddrmappings);
+static DEFINE_SPINLOCK(pci_fwaddrmap_lock);

Is the refcounting really needed? Can't we just free the whole list at some point to simplify things a bit?

Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
#4 lore raw

Re: [PATCH 2/3] PCI: Maintain a list of FW-assigned BIOS BAR values

Quoted 34 lines · Jesse Barnes
Quoted 31 lines · On Wed, 09 Nov 2011 17:43:51 -0700 Myron Stowe <[email protected]> wrote:

From: Myron Stowe <[email protected]>

Commit 58c84eda075 introduced functionality to try and reinstate the original BIOS BAR addresses of a PCI device when normal resource assignment attempts fail.  To keep track of the BIOS BAR addresses, struct pci_dev was augmented with an array to hold the BAR addresses of the PCI device: 'resource_size_t fw_addr[DEVICE_COUNT_RESOURCE]'.

The reinstatement of BAR addresses is an uncommon event leaving the 'fw_addr' array unused normally.  As the use of struct pci_dev is so prevalent, this seems wasteful.

This patch introduces a stand alone data structure and corresponding set of interfacing routines for maintaining a list of FW-assigned BIOS BAR value temporary entries.

---

 drivers/pci/setup-res.c |   73 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h     |    1 +

 2 files changed, 74 insertions(+), 0 deletions(-)
drivers/pci/setup-res.c +0 -0
index ad8c4ae..ab45c11 100644
@@ -26,6 +26,79 @@
  #include "pci.h"
 
 
Quoted 16 lines

drivers/pci/setup-res.c line 41

Quoted 13 lines
drivers/pci/setup-res.c +13 -0
+/*
+ * This list of dynamic mappings is for temporarly maintaining
+ * original BIOS BAR addresses for possbile reinstatement.
+ */
+struct pci_fwaddrmap {
+     struct list_head list;
+     struct pci_dev *dev;
+     resource_size_t fw_addr[DEVICE_COUNT_RESOURCE];
+     unsigned long refcount;
+};
+
+static LIST_HEAD(pci_fwaddrmappings);
+static DEFINE_SPINLOCK(pci_fwaddrmap_lock);

Is the refcounting really needed?  Can't we just free the whole list at some point to simplify things a bit?

No, refcounting is not absolutely necessary, it's just the approach I came up with initially. I'll look into just freeing the entire list once we are done with it.

Myron

Quoted 4 lines

--
Jesse Barnes, Intel Open Source Technology Center

#5 lore raw

PATCH 3/3 x86/PCI: Convert maintaining FW-assigned BIOS BAR values to use a list

In reply to Myron Stowe

From: Myron Stowe <[email protected]>

This patch converts the underlying maintenance aspects of FW-assigned BIOS BAR values from a statically allocated array within struct pci_dev to a list of temporary, stand alone, entries.

---

 arch/x86/pci/i386.c     |    2 +-
 drivers/pci/setup-res.c |   14 +++++++++-----
 include/linux/pci.h     |    1 -
 3 files changed, 10 insertions(+), 7 deletions(-)
arch/x86/pci/i386.c +1 -1
index 494f2e7..99ba108 100644
@@ -181,7 +181,7 @@ static void __init pcibios_allocate_resources(int pass)
 					idx, r, disabled, pass);
 				if (pci_claim_resource(dev, idx) < 0) {
 					/* We'll assign a new address later */
-					dev->fw_addr[idx] = r->start;
+					pci_save_fw_addr(dev, idx, r->start);
 					r->end -= r->start;
 					r->start = 0;
 				}
drivers/pci/setup-res.c +9 -5
index ab45c11..7e4711e 100644
@@ -231,7 +231,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 }
 
 static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev, 
-		int resno, resource_size_t size)
+		int resno, resource_size_t fw_addr, resource_size_t size)
 {
 	struct resource *root, *conflict;
 	resource_size_t start, end;
@@ -239,7 +239,7 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev,
 
 	start = res->start;
 	end = res->end;
-	res->start = dev->fw_addr[resno];
+	res->start = fw_addr;
 	res->end = res->start + size - 1;
 
 	root = pci_find_parent_resource(dev, res);
@@ -322,7 +322,7 @@ int pci_reassign_resource(struct pci_dev *dev, int resno, resource_size_t addsiz
 int pci_assign_resource(struct pci_dev *dev, int resno)
 {
 	struct resource *res = dev->resource + resno;
-	resource_size_t align, size;
+	resource_size_t align, fw_addr, size;
 	struct pci_bus *bus;
 	int ret;
 
@@ -342,8 +342,12 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	 * where firmware left it.  That at least has a chance of
 	 * working, which is better than just leaving it disabled.
 	 */
-	if (ret < 0 && dev->fw_addr[resno])
-		ret = pci_revert_fw_address(res, dev, resno, size);
+	if (ret < 0) {
+		fw_addr = pci_retrieve_fw_addr(dev, resno);
+		if (fw_addr)
+			ret = pci_revert_fw_address(res, dev, resno, fw_addr,
+						    size);
+	}
 
 	if (!ret) {
 		res->flags &= ~IORESOURCE_STARTALIGN;
include/linux/pci.h +0 -1
index 14f2492..5a5d2fc 100644
@@ -299,7 +299,6 @@ struct pci_dev {
 	 */
 	unsigned int	irq;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
-	resource_size_t	fw_addr[DEVICE_COUNT_RESOURCE]; /* FW-assigned addr */
 
 	/* These fields are used by common fixups */
 	unsigned int	transparent:1;	/* Transparent PCI bridge */