[PATCH -v2 0/3] PCI: Convert maintaining FW-assigned BIOS BAR values to a list of temporary entries

[PATCH -v2 0/3] PCI: Convert maintaining FW-assigned BIOS BAR values to a list of temporary entries

[Myron Stowe]

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

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

This patch series introduces a stand alone data structure and
corresponding interface routine that is used in converting the
underlying aspects of the existing maintanence scheme to a list of
temporary BIOS BAR value entries.

[v2]
 - Simplified list maintenance by adding a routine, and call to such,
   to delete the entire list once PCI enumeration is complete (as opposed
   to keeping reference counters and deleting entries as the reference
   counts go to 0).
 - Reinstatement of FW-assigned BAR addresses is currently only
   implemented for x86 so I relocated the stand alone data structure and
   maintenance routines within an x86 architecture specific file and
   introduced a '__weak' attribute generic interface routine -
   'pcibios_retrieve_fw_addr()' which returns a failure condition - into
   the PCI core that resolves the interface for all other architectures.

---

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


 arch/x86/pci/i386.c     |   83 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/setup-res.c |   36 ++++++++++++++++----
 include/linux/pci.h     |    2 +
 3 files changed, 112 insertions(+), 9 deletions(-)

-- 

[PATCH -v2 1/3] PCI: Fix starting basis for resource requests

[Myron Stowe]

From: Myron Stowe <mstowe@redhat.com>

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.

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
---

 drivers/pci/setup-res.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 51a9095..cdfed4f 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -164,15 +164,19 @@ 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);

[PATCH -v2 2/3] x86/PCI: Infrastructure to maintain a list of FW-assigned BIOS BAR values

[Myron Stowe]

From: Myron Stowe <mstowe@redhat.com>

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 under normal circumstances.  This functionality
is also currently architecture specific with an implementation limited
to x86.  As the use of struct pci_dev is so prevalent, having the
'fw_addr' array residing within such seems somewhat wasteful.

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

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
---

 arch/x86/pci/i386.c |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |    1 +
 2 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 494f2e7..bfb9d6a 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -38,6 +38,85 @@
 #include <asm/io_apic.h>
 
 
+/*
+ * This list of dynamic mappings is for temporarily maintaining
+ * original BIOS BAR addresses for possible reinstatement.
+ */
+struct pcibios_fwaddrmap {
+	struct list_head list;
+	struct pci_dev *dev;
+	resource_size_t fw_addr[DEVICE_COUNT_RESOURCE];
+};
+
+static LIST_HEAD(pcibios_fwaddrmappings);
+static DEFINE_SPINLOCK(pcibios_fwaddrmap_lock);
+
+/* Must be called with 'pcibios_fwaddrmap_lock' lock held. */
+static struct pcibios_fwaddrmap *pcibios_fwaddrmap_lookup(struct pci_dev *dev)
+{
+	struct pcibios_fwaddrmap *map;
+
+	list_for_each_entry(map, &pcibios_fwaddrmappings, list)
+		if (map->dev == dev)
+			return map;
+
+	return NULL;
+}
+
+static void
+pcibios_save_fw_addr(struct pci_dev *dev, int idx, resource_size_t fw_addr)
+{
+	unsigned long flags;
+	struct pcibios_fwaddrmap *map;
+
+	spin_lock_irqsave(&pcibios_fwaddrmap_lock, flags);
+	map = pcibios_fwaddrmap_lookup(dev);
+	if (!map) {
+		spin_unlock_irqrestore(&pcibios_fwaddrmap_lock, flags);
+		map = kzalloc(sizeof(*map), GFP_KERNEL);
+		if (!map)
+			return;
+
+		map->dev = pci_dev_get(dev);
+		map->fw_addr[idx] = fw_addr;
+		INIT_LIST_HEAD(&map->list);
+
+		spin_lock_irqsave(&pcibios_fwaddrmap_lock, flags);
+		list_add_tail(&map->list, &pcibios_fwaddrmappings);
+	} else
+		map->fw_addr[idx] = fw_addr;
+	spin_unlock_irqrestore(&pcibios_fwaddrmap_lock, flags);
+}
+
+resource_size_t pcibios_retrieve_fw_addr(struct pci_dev *dev, int idx)
+{
+	unsigned long flags;
+	struct pcibios_fwaddrmap *map;
+	resource_size_t fw_addr = 0;
+
+	spin_lock_irqsave(&pcibios_fwaddrmap_lock, flags);
+	map = pcibios_fwaddrmap_lookup(dev);
+	if (map)
+		fw_addr = map->fw_addr[idx];
+	spin_unlock_irqrestore(&pcibios_fwaddrmap_lock, flags);
+
+	return fw_addr;
+}
+
+static void pcibios_fw_addr_list_del(void)
+{
+	unsigned long flags;
+	struct pcibios_fwaddrmap *entry, *next;
+
+	spin_lock_irqsave(&pcibios_fwaddrmap_lock, flags);
+	list_for_each_entry_safe(entry, next, &pcibios_fwaddrmappings, list) {
+		list_del(&entry->list);
+		pci_dev_put(entry->dev);
+		kfree(entry);
+	}
+	spin_unlock_irqrestore(&pcibios_fwaddrmap_lock, flags);
+}
+
 static int
 skip_isa_ioresource_align(struct pci_dev *dev) {
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 84225c7..4545d23 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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) */
+resource_size_t pcibios_retrieve_fw_addr(struct pci_dev *dev, int idx);
 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);

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

[Myron Stowe]

From: Myron Stowe <mstowe@redhat.com>

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.

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
---

 arch/x86/pci/i386.c     |    4 +++-
 drivers/pci/setup-res.c |   24 +++++++++++++++++++++---
 include/linux/pci.h     |    1 -
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index bfb9d6a..f9bfd19 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -260,7 +260,8 @@ 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;
+					pcibios_save_fw_addr(dev,
+							idx, r->start);
 					r->end -= r->start;
 					r->start = 0;
 				}
@@ -306,6 +307,7 @@ static int __init pcibios_assign_resources(void)
 	}
 
 	pci_assign_unassigned_resources();
+	pcibios_fw_addr_list_del();
 
 	return 0;
 }
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index cdfed4f..b28750e 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -157,16 +157,34 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
 	return ret;
 }
 
+/*
+ * Generic function that returns a value indicating that the device's
+ * original BIOS BAR address was not saved and so is not available for
+ * reinstatement.
+ *
+ * Can be over-ridden by architecture specific code that implements
+ * reinstatement functionality rather than leaving it disabled when
+ * normal allocation attempts fail.
+ */
+resource_size_t __weak pcibios_retrieve_fw_addr(struct pci_dev *dev, int idx)
+{
+	return 0;
+}
+
 static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev, 
 		int resno, resource_size_t size)
 {
 	struct resource *root, *conflict;
-	resource_size_t start, end;
+	resource_size_t fw_addr, start, end;
 	int ret = 0;
 
+	fw_addr = pcibios_retrieve_fw_addr(dev, resno);
+	if (!fw_addr)
+		return 1;
+
 	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);
@@ -270,7 +288,7 @@ 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])
+	if (ret < 0)
 		ret = pci_revert_fw_address(res, dev, resno, size);
 
 	if (!ret) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4545d23..273539b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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 */

Re: [PATCH -v2 1/3] PCI: Fix starting basis for resource requests

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]

On Mon, 21 Nov 2011 11:54:07 -0700
Myron Stowe <myron.stowe@redhat.com> wrote:

> From: Myron Stowe <mstowe@redhat.com>
> 
> 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.
> 
> Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
> ---

Applied these 3 to my -next branch, thanks for your patience Myron.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]