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... [2] http://lkml.kernel.org/r/CALCETrV+RwNPzxyL8UVNsrAGu-6cCzD_Cc9PFJT2NCTJPLZZiw... [3] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/reso...
---
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... Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- 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"); } }
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) {
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 bhelgaas@google.com --- 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 bhelgaas@google.com --- 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 bhelgaas@google.com --- 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) {
Use a temporary struct resource pointer to avoid needless repetition of "dev->resource[idx]". No functional change intended.
Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- 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);
/*
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;
/*
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; } }
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,
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 bhelgaas@google.com --- 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 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 */
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 bhelgaas@google.com --- 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)
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
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... [2] http://lkml.kernel.org/r/CALCETrV+RwNPzxyL8UVNsrAGu-6cCzD_Cc9PFJT2NCTJPLZZiw... [3] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/reso...
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 helgaas@kernel.org 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... [2] http://lkml.kernel.org/r/CALCETrV+RwNPzxyL8UVNsrAGu-6cCzD_Cc9PFJT2NCTJPLZZiw... [3] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/reso...
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 helgaas@kernel.org 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... [2] http://lkml.kernel.org/r/CALCETrV+RwNPzxyL8UVNsrAGu-6cCzD_Cc9PFJT2NCTJPLZZiw... [3] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/reso...
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 helgaas@kernel.org 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 helgaas@kernel.org 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... [2] http://lkml.kernel.org/r/CALCETrV+RwNPzxyL8UVNsrAGu-6cCzD_Cc9PFJT2NCTJPLZZiw... [3] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/reso...
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 luto@amacapital.net 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 torvalds@linux-foundation.org wrote:
On Fri, Mar 11, 2016 at 4:49 PM, Andy Lutomirski luto@amacapital.net 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 bhelgaas@google.com 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 bhelgaas@google.com
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) {
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Andy Lutomirski -
Bjorn Helgaas -
Bjorn Helgaas -
Linus Torvalds