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(-)
A shadow copy of an option ROM is placed by the BIOS as a fixed address.
Set IORESOURCE_PCI_FIXED to indicate that we can't move the shadow copy.
This prevents warnings like the following when we assign resources:
BAR 6: [??? 0x00000000 flags 0x2] has bogus alignment
This warning is emitted by pdev_sort_resources(), which already ignores
IORESOURCE_PCI_FIXED resources.
Link: http://lkml.kernel.org/r/CA+55aFyVMfTBB0oz_yx8+eQOEJnzGtCsYSj9QuhEpdZ9BHdq5A@mail.gmail.com
Signed-off-by: Bjorn Helgaas <[email protected]>
---
arch/ia64/pci/fixup.c | 3 ++-
arch/x86/pci/fixup.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
index fc505d5..fd9ceff 100644
--- a/arch/ia64/pci/fixup.c
+++ b/arch/ia64/pci/fixup.c
@@ -61,7 +61,8 @@ static void pci_fixup_video(struct pci_dev *pdev)
if (!vga_default_device() || pdev == vga_default_device()) {
pci_read_config_word(pdev, PCI_COMMAND, &config);
if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
- pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
+ pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW |
+ IORESOURCE_PCI_FIXED;
dev_printk(KERN_DEBUG, &pdev->dev, "Video device with shadowed ROM\n");
}
}
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 0ae7e9f..dac027c 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -336,7 +336,8 @@ static void pci_fixup_video(struct pci_dev *pdev)
if (!vga_default_device() || pdev == vga_default_device()) {
pci_read_config_word(pdev, PCI_COMMAND, &config);
if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
- pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
+ pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW |
+ IORESOURCE_PCI_FIXED;
dev_printk(KERN_DEBUG, &pdev->dev, "Video device with shadowed ROM\n");
}
}
If we're using a RAM shadow copy instead of the ROM BAR, we don't need to
touch the ROM BAR enable bit.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/rom.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index 9eaca39..5da8d06 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -24,13 +24,17 @@
*/
int pci_enable_rom(struct pci_dev *pdev)
{
- struct resource *res = pdev->resource + PCI_ROM_RESOURCE;
+ struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
struct pci_bus_region region;
u32 rom_addr;
if (!res->flags)
return -1;
+ /* Nothing to enable if we're using a shadow copy in RAM */
+ if (res->flags & IORESOURCE_ROM_SHADOW)
+ return 0;
+
pcibios_resource_to_bus(pdev->bus, ®ion, res);
pci_read_config_dword(pdev, pdev->rom_base_reg, &rom_addr);
rom_addr &= ~PCI_ROM_ADDRESS_MASK;
@@ -49,7 +53,12 @@ EXPORT_SYMBOL_GPL(pci_enable_rom);
*/
void pci_disable_rom(struct pci_dev *pdev)
{
+ struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
u32 rom_addr;
+
+ if (res->flags & IORESOURCE_ROM_SHADOW)
+ return;
+
pci_read_config_dword(pdev, pdev->rom_base_reg, &rom_addr);
rom_addr &= ~PCI_ROM_ADDRESS_ENABLE;
pci_write_config_dword(pdev, pdev->rom_base_reg, rom_addr);
@@ -154,7 +163,6 @@ void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
if (!rom) {
/* restore enable if ioremap fails */
if (!(res->flags & (IORESOURCE_ROM_ENABLE |
- IORESOURCE_ROM_SHADOW |
IORESOURCE_ROM_COPY)))
pci_disable_rom(pdev);
return NULL;
@@ -186,8 +194,8 @@ void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom)
iounmap(rom);
- /* Disable again before continuing, leave enabled if pci=rom */
- if (!(res->flags & (IORESOURCE_ROM_ENABLE | IORESOURCE_ROM_SHADOW)))
+ /* Disable again before continuing */
+ if (!(res->flags & IORESOURCE_ROM_ENABLE))
pci_disable_rom(pdev);
}
EXPORT_SYMBOL(pci_unmap_rom);
IORESOURCE_ROM_SHADOW means there is a copy of a device's option ROM in
RAM. The existence of such a copy and its location are arch-specific.
Previously the IORESOURCE_ROM_SHADOW flag was set in arch code, but the
0xC0000-0xDFFFF location was hard-coded into the PCI core.
If we're using a shadow copy in RAM, disable the ROM BAR and release the
address space it was consuming. Move the location information from the PCI
core to the arch code that sets IORESOURCE_ROM_SHADOW. Save the location
of the RAM copy in the struct resource for PCI_ROM_RESOURCE.
After this change, pci_map_rom() will call pci_assign_resource() and
pci_enable_rom() for these IORESOURCE_ROM_SHADOW resources, which we did
not do before. This is safe because:
- pci_assign_resource() will do nothing because the resource is marked
IORESOURCE_PCI_FIXED, which means we can't move it, and
- pci_enable_rom() will not turn on the ROM BAR's enable bit because the
resource is marked IORESOURCE_ROM_SHADOW, which means it is in RAM
rather than in PCI memory space.
Storing the location in the struct resource means "lspci" will show the
shadow location, not the value from the ROM BAR.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
arch/ia64/pci/fixup.c | 22 ++++++++++++++++------
arch/x86/pci/fixup.c | 22 ++++++++++++++++------
drivers/pci/rom.c | 11 -----------
include/linux/ioport.h | 2 +-
4 files changed, 33 insertions(+), 24 deletions(-)
diff --git a/arch/ia64/pci/fixup.c b/arch/ia64/pci/fixup.c
index fd9ceff..41caa99 100644
--- a/arch/ia64/pci/fixup.c
+++ b/arch/ia64/pci/fixup.c
@@ -17,14 +17,14 @@
*
* The standard boot ROM sequence for an x86 machine uses the BIOS
* to select an initial video card for boot display. This boot video
- * card will have it's BIOS copied to C0000 in system RAM.
+ * card will have its BIOS copied to 0xC0000 in system RAM.
* IORESOURCE_ROM_SHADOW is used to associate the boot video
* card with this copy. On laptops this copy has to be used since
* the main ROM may be compressed or combined with another image.
* See pci_map_rom() for use of this flag. Before marking the device
* with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
- * by either arch cde or vga-arbitration, if so only apply the fixup to this
- * already determined primary video card.
+ * by either arch code or vga-arbitration; if so only apply the fixup to this
+ * already-determined primary video card.
*/
static void pci_fixup_video(struct pci_dev *pdev)
@@ -32,6 +32,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
struct pci_dev *bridge;
struct pci_bus *bus;
u16 config;
+ struct resource *res;
if ((strcmp(ia64_platform_name, "dig") != 0)
&& (strcmp(ia64_platform_name, "hpzx1") != 0))
@@ -61,9 +62,18 @@ static void pci_fixup_video(struct pci_dev *pdev)
if (!vga_default_device() || pdev == vga_default_device()) {
pci_read_config_word(pdev, PCI_COMMAND, &config);
if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
- pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW |
- IORESOURCE_PCI_FIXED;
- dev_printk(KERN_DEBUG, &pdev->dev, "Video device with shadowed ROM\n");
+ res = &pdev->resource[PCI_ROM_RESOURCE];
+
+ pci_disable_rom(pdev);
+ if (res->parent)
+ release_resource(res);
+
+ res->start = 0xC0000;
+ res->end = res->start + 0x20000 - 1;
+ res->flags = IORESOURCE_MEM | IORESOURCE_ROM_SHADOW |
+ IORESOURCE_PCI_FIXED;
+ dev_info(&pdev->dev, "Video device with shadowed ROM at %pR\n",
+ res);
}
}
}
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index dac027c..b7de192 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -297,14 +297,14 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_MCH_PC1, pcie_r
*
* The standard boot ROM sequence for an x86 machine uses the BIOS
* to select an initial video card for boot display. This boot video
- * card will have it's BIOS copied to C0000 in system RAM.
+ * card will have its BIOS copied to 0xC0000 in system RAM.
* IORESOURCE_ROM_SHADOW is used to associate the boot video
* card with this copy. On laptops this copy has to be used since
* the main ROM may be compressed or combined with another image.
* See pci_map_rom() for use of this flag. Before marking the device
* with IORESOURCE_ROM_SHADOW check if a vga_default_device is already set
- * by either arch cde or vga-arbitration, if so only apply the fixup to this
- * already determined primary video card.
+ * by either arch code or vga-arbitration; if so only apply the fixup to this
+ * already-determined primary video card.
*/
static void pci_fixup_video(struct pci_dev *pdev)
@@ -312,6 +312,7 @@ static void pci_fixup_video(struct pci_dev *pdev)
struct pci_dev *bridge;
struct pci_bus *bus;
u16 config;
+ struct resource *res;
/* Is VGA routed to us? */
bus = pdev->bus;
@@ -336,9 +337,18 @@ static void pci_fixup_video(struct pci_dev *pdev)
if (!vga_default_device() || pdev == vga_default_device()) {
pci_read_config_word(pdev, PCI_COMMAND, &config);
if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
- pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW |
- IORESOURCE_PCI_FIXED;
- dev_printk(KERN_DEBUG, &pdev->dev, "Video device with shadowed ROM\n");
+ res = &pdev->resource[PCI_ROM_RESOURCE];
+
+ pci_disable_rom(pdev);
+ if (res->parent)
+ release_resource(res);
+
+ res->start = 0xC0000;
+ res->end = res->start + 0x20000 - 1;
+ res->flags = IORESOURCE_MEM | IORESOURCE_ROM_SHADOW |
+ IORESOURCE_PCI_FIXED;
+ dev_info(&pdev->dev, "Video device with shadowed ROM at %pR\n",
+ res);
}
}
}
diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index 5da8d06..80e82b1 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -128,16 +128,6 @@ void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
loff_t start;
void __iomem *rom;
- /*
- * IORESOURCE_ROM_SHADOW set on x86, x86_64 and IA64 supports legacy
- * memory map if the VGA enable bit of the Bridge Control register is
- * set for embedded VGA.
- */
- if (res->flags & IORESOURCE_ROM_SHADOW) {
- /* primary video rom always starts here */
- start = (loff_t)0xC0000;
- *size = 0x20000; /* cover C000:0 through E000:0 */
- } else {
if (res->flags &
(IORESOURCE_ROM_COPY | IORESOURCE_ROM_BIOS_COPY)) {
*size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
@@ -157,7 +147,6 @@ void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
if (pci_enable_rom(pdev))
return NULL;
}
- }
rom = ioremap(start, *size);
if (!rom) {
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 24bea08..2cf1667 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -98,7 +98,7 @@ 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) /* ROM is copy at C000:0 */
+#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 */
Remove unnecessary indentation in pci_map_rom(). This is logically part of
the previous patch; I split it out to make the critical changes in that
patch more obvious. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/rom.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index 80e82b1..2a07f34 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -128,25 +128,24 @@ 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);
- } else {
- /* assign the ROM an address if it doesn't have one */
- if (res->parent == NULL &&
- pci_assign_resource(pdev, PCI_ROM_RESOURCE))
- return NULL;
- start = pci_resource_start(pdev, PCI_ROM_RESOURCE);
- *size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
- if (*size == 0)
- return NULL;
-
- /* Enable ROM space decodes */
- if (pci_enable_rom(pdev))
- return NULL;
- }
+ 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;
+
+ start = pci_resource_start(pdev, PCI_ROM_RESOURCE);
+ *size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+ if (*size == 0)
+ return NULL;
+
+ /* Enable ROM space decodes */
+ if (pci_enable_rom(pdev))
+ return NULL;
rom = ioremap(start, *size);
if (!rom) {
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 <[email protected]>
---
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;
}
}
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 <[email protected]>
---
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,
Loongson 3 used the IORESOURCE_ROM_COPY flag for its ROM resource. There
are two problems with this:
- When IORESOURCE_ROM_COPY is set, pci_map_rom() assumes the resource
contains virtual addresses, so it doesn't ioremap the resource. This
implies loongson_sysconf.vgabios_addr is a virtual address. That's a
problem because resources should contain CPU *physical* addresses not
virtual addresses.
- When IORESOURCE_ROM_COPY is set, pci_cleanup_rom() calls kfree() on the
resource. We did not kmalloc() the loongson_sysconf.vgabios_addr area,
so it is incorrect to kfree() it.
If we're using a shadow copy in RAM for the Loongson 3 VGA BIOS area,
disable the ROM BAR and release the address space it was consuming.
Use IORESOURCE_ROM_SHADOW instead of IORESOURCE_ROM_COPY. This means the
struct resource contains CPU physical addresses, and pci_map_rom() will
ioremap() it as needed.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
arch/mips/pci/fixup-loongson3.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/mips/pci/fixup-loongson3.c b/arch/mips/pci/fixup-loongson3.c
index b66b1eb..c015ca1 100644
--- a/arch/mips/pci/fixup-loongson3.c
+++ b/arch/mips/pci/fixup-loongson3.c
@@ -48,9 +48,14 @@ static void pci_fixup_radeon(struct pci_dev *pdev)
if (!loongson_sysconf.vgabios_addr)
return;
- res->start = loongson_sysconf.vgabios_addr;
+ pci_disable_rom(pdev);
+ if (res->parent)
+ release_resource(res);
+
+ res->start = virt_to_phys(loongson_sysconf.vgabios_addr);
res->end = res->start + 256*1024 - 1;
- res->flags |= IORESOURCE_ROM_COPY;
+ res->flags = IORESOURCE_MEM | IORESOURCE_ROM_SHADOW |
+ IORESOURCE_PCI_FIXED;
dev_info(&pdev->dev, "BAR %d: assigned %pR for Radeon ROM\n",
PCI_ROM_RESOURCE, res);
The value of pdev->rom_attr is the definitive indicator of the fact that
we're created a sysfs attribute. Check that rather than rom_size, which is
only used incidentally when deciding whether to create a sysfs attribute.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/pci-sysfs.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 95d9e7b..a4cfa52 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1409,7 +1409,7 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
return 0;
err_rom_file:
- if (rom_size) {
+ if (pdev->rom_attr) {
sysfs_remove_bin_file(&pdev->dev.kobj, pdev->rom_attr);
kfree(pdev->rom_attr);
pdev->rom_attr = NULL;
@@ -1447,8 +1447,6 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
*/
void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
{
- int rom_size = 0;
-
if (!sysfs_initialized)
return;
@@ -1461,18 +1459,13 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
pci_remove_resource_files(pdev);
- if (pci_resource_len(pdev, PCI_ROM_RESOURCE))
- rom_size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
- else if (pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW)
- rom_size = 0x20000;
-
- if (rom_size && pdev->rom_attr) {
+ if (pdev->rom_attr) {
sysfs_remove_bin_file(&pdev->dev.kobj, pdev->rom_attr);
kfree(pdev->rom_attr);
+ pdev->rom_attr = NULL;
}
pci_remove_firmware_label_files(pdev);
-
}
static int __init pci_sysfs_init(void)
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 <[email protected]>
---
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 */
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 <[email protected]>
---
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;
/*
Use a temporary struct resource pointer to avoid needless repetition of
"dev->resource[idx]". No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
arch/ia64/sn/kernel/io_acpi_init.c | 10 +++++----
arch/ia64/sn/kernel/io_init.c | 39 ++++++++++++++++--------------------
2 files changed, 22 insertions(+), 27 deletions(-)
diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c
index 0640739..815c291 100644
--- a/arch/ia64/sn/kernel/io_acpi_init.c
+++ b/arch/ia64/sn/kernel/io_acpi_init.c
@@ -429,6 +429,7 @@ sn_acpi_slot_fixup(struct pci_dev *dev)
void __iomem *addr;
struct pcidev_info *pcidev_info = NULL;
struct sn_irq_info *sn_irq_info = NULL;
+ struct resource *res;
size_t image_size, size;
if (sn_acpi_get_pcidev_info(dev, &pcidev_info, &sn_irq_info)) {
@@ -446,14 +447,13 @@ sn_acpi_slot_fixup(struct pci_dev *dev)
addr = ioremap(pcidev_info->pdi_pio_mapped_addr[PCI_ROM_RESOURCE],
size);
image_size = pci_get_rom_size(dev, addr, size);
- dev->resource[PCI_ROM_RESOURCE].start = (unsigned long) addr;
- dev->resource[PCI_ROM_RESOURCE].end =
- (unsigned long) addr + image_size - 1;
- dev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_BIOS_COPY;
+ 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;
}
sn_pci_fixup_slot(dev, pcidev_info, sn_irq_info);
}
-
EXPORT_SYMBOL(sn_acpi_slot_fixup);
diff --git a/arch/ia64/sn/kernel/io_init.c b/arch/ia64/sn/kernel/io_init.c
index 1be65eb..40c0263 100644
--- a/arch/ia64/sn/kernel/io_init.c
+++ b/arch/ia64/sn/kernel/io_init.c
@@ -150,7 +150,8 @@ void
sn_io_slot_fixup(struct pci_dev *dev)
{
int idx;
- unsigned long addr, end, size, start;
+ struct resource *res;
+ unsigned long addr, size;
struct pcidev_info *pcidev_info;
struct sn_irq_info *sn_irq_info;
int status;
@@ -175,33 +176,31 @@ sn_io_slot_fixup(struct pci_dev *dev)
/* Copy over PIO Mapped Addresses */
for (idx = 0; idx <= PCI_ROM_RESOURCE; idx++) {
-
- if (!pcidev_info->pdi_pio_mapped_addr[idx]) {
+ if (!pcidev_info->pdi_pio_mapped_addr[idx])
continue;
- }
- start = dev->resource[idx].start;
- end = dev->resource[idx].end;
- size = end - start;
- if (size == 0) {
+ res = &dev->resource[idx];
+
+ size = res->end - res->start;
+ if (size == 0)
continue;
- }
+
addr = pcidev_info->pdi_pio_mapped_addr[idx];
addr = ((addr << 4) >> 4) | __IA64_UNCACHED_OFFSET;
- dev->resource[idx].start = addr;
- dev->resource[idx].end = addr + size;
+ res->start = addr;
+ res->end = addr + size;
/*
* if it's already in the device structure, remove it before
* inserting
*/
- if (dev->resource[idx].parent && dev->resource[idx].parent->child)
- release_resource(&dev->resource[idx]);
+ if (res->parent && res->parent->child)
+ release_resource(res);
- if (dev->resource[idx].flags & IORESOURCE_IO)
- insert_resource(&ioport_resource, &dev->resource[idx]);
+ if (res->flags & IORESOURCE_IO)
+ insert_resource(&ioport_resource, res);
else
- insert_resource(&iomem_resource, &dev->resource[idx]);
+ insert_resource(&iomem_resource, res);
/*
* If ROM, set the actual ROM image size, and mark as
* shadowed in PROM.
@@ -213,17 +212,13 @@ sn_io_slot_fixup(struct pci_dev *dev)
rom = ioremap(pci_resource_start(dev, PCI_ROM_RESOURCE),
size + 1);
image_size = pci_get_rom_size(dev, rom, size + 1);
- dev->resource[PCI_ROM_RESOURCE].end =
- dev->resource[PCI_ROM_RESOURCE].start +
- image_size - 1;
- dev->resource[PCI_ROM_RESOURCE].flags |=
- IORESOURCE_ROM_BIOS_COPY;
+ res->end = res->start + image_size - 1;
+ res->flags |= IORESOURCE_ROM_BIOS_COPY;
}
}
sn_pci_fixup_slot(dev, pcidev_info, sn_irq_info);
}
-
EXPORT_SYMBOL(sn_io_slot_fixup);
/*
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 <[email protected]>
---
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) {
On Thu, Mar 3, 2016 at 8:53 AM, Bjorn Helgaas <[email protected]> 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
On Thu, Mar 03, 2016 at 10:53:50AM -0600, Bjorn Helgaas wrote:
> 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(-)
I applied this series to pci/resource for v4.6.
On Tue, Mar 8, 2016 at 9:45 AM, Bjorn Helgaas <[email protected]> wrote:
> On Thu, Mar 03, 2016 at 10:53:50AM -0600, Bjorn Helgaas wrote:
>> 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(-)
>
> I applied this series to pci/resource for v4.6.
This gets rid of all the warnings for me until I try to read my i915
device's rom using sysfs. Then I get:
i915 0000:00:02.0: Invalid PCI ROM header signature: expecting 0xaa55,
got 0xffff
So I suspect that something is still subtly wrong -- I'd imagine that
this should either work or the intialization code should detect that
there is no usable ROM and not expose it.
(To be clear, there's no regression here.)
On Fri, Mar 11, 2016 at 01:16:09PM -0800, Andy Lutomirski wrote:
> On Tue, Mar 8, 2016 at 9:45 AM, Bjorn Helgaas <[email protected]> wrote:
> > On Thu, Mar 03, 2016 at 10:53:50AM -0600, Bjorn Helgaas wrote:
> >> 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(-)
> >
> > I applied this series to pci/resource for v4.6.
>
> This gets rid of all the warnings for me until I try to read my i915
> device's rom using sysfs. Then I get:
>
> i915 0000:00:02.0: Invalid PCI ROM header signature: expecting 0xaa55,
> got 0xffff
>
> So I suspect that something is still subtly wrong -- I'd imagine that
> this should either work or the intialization code should detect that
> there is no usable ROM and not expose it.
>
> (To be clear, there's no regression here.)
Hmmm. Thanks for testing this. As you say, I think this is the way
it's always been, but it does seem non-intuitive.
That "Invalid PCI ROM header signature" warning comes from
pci_get_rom_size(). We don't call that at enumeration-time; we only
call it later when somebody tries to read the ROM via sysfs:
pci_bus_add_device
pci_fixup_device(pci_fixup_final)
pci_fixup_video # final fixup
res->flags = MEM | SHADOW | PCI_FIXED
pci_create_sysfs_dev_files
if (SHADOW)
rom_size = 0x20000 # oops, I should have fixed this too
if (rom_size)
attr->read = pci_read_rom
sysfs_create_bin_file # sysfs "rom" file
pci_read_rom # sysfs "rom" read function
pci_map_rom
ioremap
pci_get_rom_size
dev_err("Invalid PCI ROM header signature")
memcpy_fromio
pci_unmap_rom
I think this is the same for regular ROMs on the device as it is for
the magic VGA shadow ROM. In both cases, we create the sysfs "rom"
file regardless of what the ROM contains.
I guess we could try to read the ROM at enumeration time and look for
a valid signature. I've considered doing that anyway so we could
cache the ROM contents and avoid permanently allocating MMIO space for
it, since many BIOSes don't allocate space, and Linux isn't really smart
enough to do a good job of it itself.
I don't know why the PCI core cares about the ROM signature anyway,
since it doesn't use the content. Maybe we should get rid of
pci_get_rom_size() and allow you to read whatever is there, like
we do for other BARs. I suppose there's some history behind limiting
the size, but I don't know what it is.
Bjorn
On Fri, Mar 11, 2016 at 3:29 PM, Bjorn Helgaas <[email protected]> wrote:
> On Fri, Mar 11, 2016 at 01:16:09PM -0800, Andy Lutomirski wrote:
>> On Tue, Mar 8, 2016 at 9:45 AM, Bjorn Helgaas <[email protected]> wrote:
>> > On Thu, Mar 03, 2016 at 10:53:50AM -0600, Bjorn Helgaas wrote:
>> >> 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(-)
>> >
>> > I applied this series to pci/resource for v4.6.
>>
>> This gets rid of all the warnings for me until I try to read my i915
>> device's rom using sysfs. Then I get:
>>
>> i915 0000:00:02.0: Invalid PCI ROM header signature: expecting 0xaa55,
>> got 0xffff
>>
>> So I suspect that something is still subtly wrong -- I'd imagine that
>> this should either work or the intialization code should detect that
>> there is no usable ROM and not expose it.
>>
>> (To be clear, there's no regression here.)
>
> Hmmm. Thanks for testing this. As you say, I think this is the way
> it's always been, but it does seem non-intuitive.
>
> That "Invalid PCI ROM header signature" warning comes from
> pci_get_rom_size(). We don't call that at enumeration-time; we only
> call it later when somebody tries to read the ROM via sysfs:
>
> pci_bus_add_device
> pci_fixup_device(pci_fixup_final)
> pci_fixup_video # final fixup
> res->flags = MEM | SHADOW | PCI_FIXED
> pci_create_sysfs_dev_files
> if (SHADOW)
> rom_size = 0x20000 # oops, I should have fixed this too
> if (rom_size)
> attr->read = pci_read_rom
> sysfs_create_bin_file # sysfs "rom" file
>
> pci_read_rom # sysfs "rom" read function
> pci_map_rom
> ioremap
> pci_get_rom_size
> dev_err("Invalid PCI ROM header signature")
> memcpy_fromio
> pci_unmap_rom
>
> I think this is the same for regular ROMs on the device as it is for
> the magic VGA shadow ROM. In both cases, we create the sysfs "rom"
> file regardless of what the ROM contains.
>
> I guess we could try to read the ROM at enumeration time and look for
> a valid signature. I've considered doing that anyway so we could
> cache the ROM contents and avoid permanently allocating MMIO space for
> it, since many BIOSes don't allocate space, and Linux isn't really smart
> enough to do a good job of it itself.
>
> I don't know why the PCI core cares about the ROM signature anyway,
> since it doesn't use the content. Maybe we should get rid of
> pci_get_rom_size() and allow you to read whatever is there, like
> we do for other BARs. I suppose there's some history behind limiting
> the size, but I don't know what it is.
FWIW, if I disable all the checks in pci_get_rom_size, I learn that my
video ROM consists entirely of 0xff bytes. Maybe there just isn't a
ROM shadow on my laptop.
--Andy
On Fri, Mar 11, 2016 at 4:49 PM, Andy Lutomirski <[email protected]> wrote:
>
> FWIW, if I disable all the checks in pci_get_rom_size, I learn that my
> video ROM consists entirely of 0xff bytes. Maybe there just isn't a
> ROM shadow on my laptop.
I think most laptops end up having the graphics ROM be part of the
regular system flash, and there is no actual rom associated with the
PCI device that is the GPU itself.
The actual GPU ROM tends to be associated with plug-in cards, not
soldered-down chips in a laptop where they don't want extra flash
chips.
Linus
On Fri, Mar 11, 2016 at 8:09 PM, Linus Torvalds
<[email protected]> wrote:
> On Fri, Mar 11, 2016 at 4:49 PM, Andy Lutomirski <[email protected]> wrote:
>>
>> FWIW, if I disable all the checks in pci_get_rom_size, I learn that my
>> video ROM consists entirely of 0xff bytes. Maybe there just isn't a
>> ROM shadow on my laptop.
>
> I think most laptops end up having the graphics ROM be part of the
> regular system flash, and there is no actual rom associated with the
> PCI device that is the GPU itself.
>
> The actual GPU ROM tends to be associated with plug-in cards, not
> soldered-down chips in a laptop where they don't want extra flash
> chips.
Right; on (at least AMD) mobile dGPUs and systems with APUs, the vbios
"rom" is part of the sbios image and is set up by the sbios when it
runs. The driver either gets it from the legacy vga location or some
platform specific method such as ACPI.
Alex
On Thu, Mar 03, 2016 at 10:53:50AM -0600, Bjorn Helgaas wrote:
> The purpose of this series is to:
> ...
> - Move arch-specific shadow ROM location knowledge, e.g.,
> 0xC0000-0xDFFFF, from PCI core to arch code.
> ...
> 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
I propose to add the patch below at this point in the series.
> 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
commit ac0c302a919ba7b68dbf274babdc08c83df6f532
Author: Bjorn Helgaas <[email protected]>
Date: Sat Mar 12 05:48:08 2016 -0600
PCI: Remove arch-specific IORESOURCE_ROM_SHADOW size from sysfs
When pci_create_sysfs_dev_files() created the "rom" sysfs file, it set the
sysfs file size to the actual size of a ROM BAR, or if there was no ROM BAR
but the platform provided a shadow copy in RAM, to 0x20000. 0x20000 is an
arch-specific length that should not be baked into the PCI core.
Every place that sets IORESOURCE_ROM_SHADOW also sets the size of the
PCI_ROM_RESOURCE, so use the resource length always.
Signed-off-by: Bjorn Helgaas <[email protected]>
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 95d9e7b..51d4dad 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1356,7 +1356,7 @@ error:
int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
{
int retval;
- int rom_size = 0;
+ int rom_size;
struct bin_attribute *attr;
if (!sysfs_initialized)
@@ -1373,12 +1373,8 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
if (retval)
goto err_config_file;
- if (pci_resource_len(pdev, PCI_ROM_RESOURCE))
- rom_size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
- else if (pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW)
- rom_size = 0x20000;
-
/* If the device has a ROM, try to expose it in sysfs. */
+ rom_size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
if (rom_size) {
attr = kzalloc(sizeof(*attr), GFP_ATOMIC);
if (!attr) {