Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1349372 > unrolled thread

[PATCH v1 00/12] PCI: Rework shadow ROM handling

Back to article view | Back to linux.kernel

Contents

  [PATCH v1 00/12] PCI: Rework shadow ROM handling Bjorn Helgaas <bhelgaas@google.com> - 2016-03-03 18:00 +0100
    [PATCH v1 07/12] ia64/PCI: Use ioremap() instead of open-coded  equivalent Bjorn Helgaas <bhelgaas@google.com> - 2016-03-03 18:00 +0100
    [PATCH v1 08/12] ia64/PCI: Keep CPU physical (not virtual)  addresses in shadow ROM resource Bjorn Helgaas <bhelgaas@google.com> - 2016-03-03 18:00 +0100
    [PATCH v1 11/12] PCI: Remove unused IORESOURCE_ROM_COPY and  IORESOURCE_ROM_BIOS_COPY Bjorn Helgaas <bhelgaas@google.com> - 2016-03-03 18:00 +0100
    [PATCH v1 09/12] MIPS: Loongson 3: Use temporary struct resource *  to avoid repetition Bjorn Helgaas <bhelgaas@google.com> - 2016-03-03 18:00 +0100
    [PATCH v1 02/12] PCI: Don't assign or reassign immutable resources Bjorn Helgaas <bhelgaas@google.com> - 2016-03-03 18:10 +0100
    Re: [PATCH v1 00/12] PCI: Rework shadow ROM handling Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-03 19:10 +0100

#1349372 — [PATCH v1 00/12] PCI: Rework shadow ROM handling

The purpose of this series is to:

  - Fix the "BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment"
    messages reported by Linus [1], Andy [2], and others.

  - Move arch-specific shadow ROM location knowledge, e.g.,
    0xC0000-0xDFFFF, from PCI core to arch code.

  - Fix the ia64 and MIPS Loongson 3 oddity of keeping virtual
    addresses in shadow ROM struct resource (resources should always
    contain *physical* addresses).

  - Remove now-unused IORESOURCE_ROM_COPY and IORESOURCE_ROM_BIOS_COPY
    flags.

This series is based on v4.5-rc1, and it's available on my
pci/resource git branch (along with a couple tiny unrelated patches)
at [3].

Bjorn


[1] http://lkml.kernel.org/r/CA+55aFyVMfTBB0oz_yx8+eQOEJnzGtCsYSj9QuhEpdZ9BHdq5A@mail.gmail.com
[2] http://lkml.kernel.org/r/CALCETrV+RwNPzxyL8UVNsrAGu-6cCzD_Cc9PFJT2NCTJPLZZiw@mail.gmail.com
[3] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/resource


---

Bjorn Helgaas (12):
      PCI: Mark shadow copy of VGA ROM as IORESOURCE_PCI_FIXED
      PCI: Don't assign or reassign immutable resources
      PCI: Don't enable/disable ROM BAR if we're using a RAM shadow copy
      PCI: Set ROM shadow location in arch code, not in PCI core
      PCI: Clean up pci_map_rom() whitespace
      ia64/PCI: Use temporary struct resource * to avoid repetition
      ia64/PCI: Use ioremap() instead of open-coded equivalent
      ia64/PCI: Keep CPU physical (not virtual) addresses in shadow ROM resource
      MIPS: Loongson 3: Use temporary struct resource * to avoid repetition
      MIPS: Loongson 3: Keep CPU physical (not virtual) addresses in shadow ROM resource
      PCI: Remove unused IORESOURCE_ROM_COPY and IORESOURCE_ROM_BIOS_COPY
      PCI: Simplify sysfs ROM cleanup


 arch/ia64/pci/fixup.c              |   21 +++++++--
 arch/ia64/sn/kernel/io_acpi_init.c |   22 ++++++----
 arch/ia64/sn/kernel/io_init.c      |   51 ++++++++--------------
 arch/mips/pci/fixup-loongson3.c    |   19 +++++---
 arch/x86/pci/fixup.c               |   21 +++++++--
 drivers/pci/pci-sysfs.c            |   13 +-----
 drivers/pci/remove.c               |    1 
 drivers/pci/rom.c                  |   83 +++++++++++-------------------------
 drivers/pci/setup-res.c            |    6 +++
 include/linux/ioport.h             |    4 --
 10 files changed, 111 insertions(+), 130 deletions(-)

[toc] | [next] | [standalone]

#1349375 — [PATCH v1 07/12] ia64/PCI: Use ioremap() instead of open-coded equivalent

Depositing __IA64_UNCACHED_OFFSET in the upper address bits is essentially
equivalent to ioremap(): it converts a CPU physical address to a virtual
address using the ia64 uncacheable identity map.

Call ioremap() instead of doing the phys-to-virt conversion manually with
__IA64_UNCACHED_OFFSET.

Note that this makes it obvious that (a) we're putting a virtual address in
a struct resource, and (b) we're passing a virtual address to ioremap()
below in the PCI_ROM_RESOURCE case.  These are both pre-existing problems
that I'll resolve next.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/ia64/sn/kernel/io_init.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/ia64/sn/kernel/io_init.c b/arch/ia64/sn/kernel/io_init.c
index 40c0263..0227e20 100644
--- a/arch/ia64/sn/kernel/io_init.c
+++ b/arch/ia64/sn/kernel/io_init.c
@@ -185,9 +185,8 @@ sn_io_slot_fixup(struct pci_dev *dev)
 		if (size == 0)
 			continue;
 
-		addr = pcidev_info->pdi_pio_mapped_addr[idx];
-		addr = ((addr << 4) >> 4) | __IA64_UNCACHED_OFFSET;
-		res->start = addr;
+		res->start = ioremap(pcidev_info->pdi_pio_mapped_addr[idx],
+				     size + 1);
 		res->end = addr + size;
 
 		/*

[toc] | [prev] | [next] | [standalone]

#1349376 — [PATCH v1 08/12] ia64/PCI: Keep CPU physical (not virtual) addresses in shadow ROM resource

A struct resource contains CPU physical addresses, not virtual addresses.
But sn_acpi_slot_fixup() and sn_io_slot_fixup() stored the virtual address
of a shadow ROM copy in the resource.  To compensate, pci_map_rom() had a
special case that returned the resource address directly rather than
calling ioremap() on it.

When we're using a shadow copy in RAM or PROM, disable the ROM BAR and
release the address space it was consuming.

Store the CPU physical (not virtual) address in the shadow ROM resource,
and mark the resource as IORESOURCE_ROM_SHADOW so we use the normal
pci_map_rom() path that ioremaps the copy.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/ia64/sn/kernel/io_acpi_init.c |   18 +++++++++++-------
 arch/ia64/sn/kernel/io_init.c      |   17 +++++------------
 2 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c
index 815c291..231234c 100644
--- a/arch/ia64/sn/kernel/io_acpi_init.c
+++ b/arch/ia64/sn/kernel/io_acpi_init.c
@@ -430,7 +430,7 @@ sn_acpi_slot_fixup(struct pci_dev *dev)
 	struct pcidev_info *pcidev_info = NULL;
 	struct sn_irq_info *sn_irq_info = NULL;
 	struct resource *res;
-	size_t image_size, size;
+	size_t size;
 
 	if (sn_acpi_get_pcidev_info(dev, &pcidev_info, &sn_irq_info)) {
 		panic("%s:  Failure obtaining pcidev_info for %s\n",
@@ -444,13 +444,17 @@ sn_acpi_slot_fixup(struct pci_dev *dev)
 		 * of the shadowed copy, and the actual length of the ROM image.
 		 */
 		size = pci_resource_len(dev, PCI_ROM_RESOURCE);
-		addr = ioremap(pcidev_info->pdi_pio_mapped_addr[PCI_ROM_RESOURCE],
-			       size);
-		image_size = pci_get_rom_size(dev, addr, size);
+
 		res = &dev->resource[PCI_ROM_RESOURCE];
-		res->start = (unsigned long) addr;
-		res->end = (unsigned long) addr + image_size - 1;
-		res->flags |= IORESOURCE_ROM_BIOS_COPY;
+
+		pci_disable_rom(dev);
+		if (res->parent)
+			release_resource(res);
+
+		res->start = pcidev_info->pdi_pio_mapped_addr[PCI_ROM_RESOURCE];
+		res->end = res->start + size - 1;
+		res->flags = IORESOURCE_MEM | IORESOURCE_ROM_SHADOW |
+			     IORESOURCE_PCI_FIXED;
 	}
 	sn_pci_fixup_slot(dev, pcidev_info, sn_irq_info);
 }
diff --git a/arch/ia64/sn/kernel/io_init.c b/arch/ia64/sn/kernel/io_init.c
index 0227e20..c15a41e 100644
--- a/arch/ia64/sn/kernel/io_init.c
+++ b/arch/ia64/sn/kernel/io_init.c
@@ -185,8 +185,7 @@ sn_io_slot_fixup(struct pci_dev *dev)
 		if (size == 0)
 			continue;
 
-		res->start = ioremap(pcidev_info->pdi_pio_mapped_addr[idx],
-				     size + 1);
+		res->start = pcidev_info->pdi_pio_mapped_addr[idx];
 		res->end = addr + size;
 
 		/*
@@ -201,18 +200,12 @@ sn_io_slot_fixup(struct pci_dev *dev)
 		else
 			insert_resource(&iomem_resource, res);
 		/*
-		 * If ROM, set the actual ROM image size, and mark as
-		 * shadowed in PROM.
+		 * If ROM, mark as shadowed in PROM.
 		 */
 		if (idx == PCI_ROM_RESOURCE) {
-			size_t image_size;
-			void __iomem *rom;
-
-			rom = ioremap(pci_resource_start(dev, PCI_ROM_RESOURCE),
-				      size + 1);
-			image_size = pci_get_rom_size(dev, rom, size + 1);
-			res->end = res->start + image_size - 1;
-			res->flags |= IORESOURCE_ROM_BIOS_COPY;
+			pci_disable_rom(dev);
+			res->flags = IORESOURCE_MEM | IORESOURCE_ROM_SHADOW |
+				     IORESOURCE_PCI_FIXED;
 		}
 	}
 

[toc] | [prev] | [next] | [standalone]

#1349379 — [PATCH v1 11/12] PCI: Remove unused IORESOURCE_ROM_COPY and IORESOURCE_ROM_BIOS_COPY

The IORESOURCE_ROM_COPY and IORESOURCE_ROM_BIOS_COPY bits are unused.
Remove them and code that depends on them.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/remove.c   |    1 -
 drivers/pci/rom.c      |   31 +------------------------------
 include/linux/ioport.h |    2 --
 3 files changed, 1 insertion(+), 33 deletions(-)

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 8a280e9..6b66329 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -7,7 +7,6 @@ static void pci_free_resources(struct pci_dev *dev)
 {
 	int i;
 
-	pci_cleanup_rom(dev);
 	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
 		struct resource *res = dev->resource + i;
 		if (res->parent)
diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index 2a07f34..06663d3 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -128,12 +128,6 @@ void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
 	loff_t start;
 	void __iomem *rom;
 
-	if (res->flags & (IORESOURCE_ROM_COPY | IORESOURCE_ROM_BIOS_COPY)) {
-		*size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
-		return (void __iomem *)(unsigned long)
-			pci_resource_start(pdev, PCI_ROM_RESOURCE);
-	}
-
 	/* assign the ROM an address if it doesn't have one */
 	if (res->parent == NULL && pci_assign_resource(pdev, PCI_ROM_RESOURCE))
 		return NULL;
@@ -150,8 +144,7 @@ void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
 	rom = ioremap(start, *size);
 	if (!rom) {
 		/* restore enable if ioremap fails */
-		if (!(res->flags & (IORESOURCE_ROM_ENABLE |
-				    IORESOURCE_ROM_COPY)))
+		if (!(res->flags & IORESOURCE_ROM_ENABLE))
 			pci_disable_rom(pdev);
 		return NULL;
 	}
@@ -177,9 +170,6 @@ void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom)
 {
 	struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
 
-	if (res->flags & (IORESOURCE_ROM_COPY | IORESOURCE_ROM_BIOS_COPY))
-		return;
-
 	iounmap(rom);
 
 	/* Disable again before continuing */
@@ -189,25 +179,6 @@ void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom)
 EXPORT_SYMBOL(pci_unmap_rom);
 
 /**
- * pci_cleanup_rom - free the ROM copy created by pci_map_rom_copy
- * @pdev: pointer to pci device struct
- *
- * Free the copied ROM if we allocated one.
- */
-void pci_cleanup_rom(struct pci_dev *pdev)
-{
-	struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
-
-	if (res->flags & IORESOURCE_ROM_COPY) {
-		kfree((void *)(unsigned long)res->start);
-		res->flags |= IORESOURCE_UNSET;
-		res->flags &= ~IORESOURCE_ROM_COPY;
-		res->start = 0;
-		res->end = 0;
-	}
-}
-
-/**
  * pci_platform_rom - provides a pointer to any ROM image provided by the
  * platform
  * @pdev: pointer to pci device struct
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 2cf1667..29a6deb 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -99,8 +99,6 @@ struct resource {
 /* PCI ROM control bits (IORESOURCE_BITS) */
 #define IORESOURCE_ROM_ENABLE		(1<<0)	/* ROM is enabled, same as PCI_ROM_ADDRESS_ENABLE */
 #define IORESOURCE_ROM_SHADOW		(1<<1)	/* Use RAM image, not ROM BAR */
-#define IORESOURCE_ROM_COPY		(1<<2)	/* ROM is alloc'd copy, resource field overlaid */
-#define IORESOURCE_ROM_BIOS_COPY	(1<<3)	/* ROM is BIOS copy, resource field overlaid */
 
 /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
 #define IORESOURCE_PCI_FIXED		(1<<4)	/* Do not move resource */

[toc] | [prev] | [next] | [standalone]

#1349380 — [PATCH v1 09/12] MIPS: Loongson 3: Use temporary struct resource * to avoid repetition

Use a temporary struct resource pointer to avoid needless repetition of
"pdev->resource[PCI_ROM_RESOURCE]".  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/mips/pci/fixup-loongson3.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/mips/pci/fixup-loongson3.c b/arch/mips/pci/fixup-loongson3.c
index d708ae4..b66b1eb 100644
--- a/arch/mips/pci/fixup-loongson3.c
+++ b/arch/mips/pci/fixup-loongson3.c
@@ -40,20 +40,20 @@ int __init pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 
 static void pci_fixup_radeon(struct pci_dev *pdev)
 {
-	if (pdev->resource[PCI_ROM_RESOURCE].start)
+	struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
+
+	if (res->start)
 		return;
 
 	if (!loongson_sysconf.vgabios_addr)
 		return;
 
-	pdev->resource[PCI_ROM_RESOURCE].start =
-		loongson_sysconf.vgabios_addr;
-	pdev->resource[PCI_ROM_RESOURCE].end   =
-		loongson_sysconf.vgabios_addr + 256*1024 - 1;
-	pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_COPY;
+	res->start = loongson_sysconf.vgabios_addr;
+	res->end   = res->start + 256*1024 - 1;
+	res->flags |= IORESOURCE_ROM_COPY;
 
 	dev_info(&pdev->dev, "BAR %d: assigned %pR for Radeon ROM\n",
-			PCI_ROM_RESOURCE, &pdev->resource[PCI_ROM_RESOURCE]);
+		 PCI_ROM_RESOURCE, res);
 }
 
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,

[toc] | [prev] | [next] | [standalone]

#1349412 — [PATCH v1 02/12] PCI: Don't assign or reassign immutable resources

IORESOURCE_PCI_FIXED means the resource can't be moved, so if it's set,
don't bother trying to assign or reassign the resource.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/setup-res.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 604011e..66c4d8f 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -276,6 +276,9 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
 	resource_size_t align, size;
 	int ret;
 
+	if (res->flags & IORESOURCE_PCI_FIXED)
+		return 0;
+
 	res->flags |= IORESOURCE_UNSET;
 	align = pci_resource_alignment(dev, res);
 	if (!align) {
@@ -321,6 +324,9 @@ int pci_reassign_resource(struct pci_dev *dev, int resno, resource_size_t addsiz
 	resource_size_t new_size;
 	int ret;
 
+	if (res->flags & IORESOURCE_PCI_FIXED)
+		return 0;
+
 	flags = res->flags;
 	res->flags |= IORESOURCE_UNSET;
 	if (!res->parent) {

[toc] | [prev] | [next] | [standalone]

#1349462

On Thu, Mar 3, 2016 at 8:53 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> The purpose of this series is to:
> [ .. ]

The patches look ok to me and seem to make sense.

Of course, let's see what they break. Hopefully nothing, but any time
the PCI resource code changes I get a bit worried. PTSD, I guess.

                   Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web