On 2013-02-22 03:03, Mantas Mikulėnas wrote:
> On 2013-02-22 01:54, Dave Airlie wrote:
>>>
>>> | radeon 0000:01:00.0: No connectors reported connected with modes
>>> | [drm] Cannot find any crtc or sizes - going 1024x768
>>>
>>> The connector is definitely connected, since this is a laptop with a
>>> built-in screen...
>>>
>>
>> Can you get the log with drm.debug=6 from both boots as well?
>
> Attached.
The log is also at http://nullroute.eu.org/tmp/2013/dmesg-drm-debug.txt
Not to be annoying, but I hope this can be fixed until 3.9...
(I just tested v3.9-rc1-278-g8343bce, and it still does not detect any
displays. And if I understood it correctly, "nomodeset" is going to go
away?)
--
Mantas Mikulėnas <[email protected]>
This is apparently still outstanding, and Mantas hadn't cc'd the
people involved with the commit itself.
Background: with UEFI, commit f9a37be0f02a ("x86: Use PCI setup data")
apparently results in a black screen for Mantas. The commit reverts
fairly easily (there's been a trivial change to the function since due
to dev->rom now being a proper phys_add_t), and considering that the
commit doesn't explain what the f*ck it is needed for, or what it
would help, I'm inclined to do just that.
Trusting firmware-provided values over the things we can find
ourselves is known to be fundamentally crap, so what the hell is the
point of that commit in the first place? The likelihood that firmware
messes up is pretty damn high. Why would we take idiotic "here's the
PCI ROM" data from firmware in the first place? What did this fix? We
know what it broke..
Doing things like blindly trusting the firmware data without even
validating it is a really really bad idea. The commit actually looks
seriously broken in other ways too, like blindly doing phys_to_virt()
on that, and then trusting the result
Mantas, mind changing that "pcibios_add_device()" function so that
instead of setting dev->rom/romlen, it just prints out the values
(including the device address)? Plase also make it print out the
"data->len" field in addition to the rom->xyz fields..
Linus
On Sat, Mar 9, 2013 at 1:42 PM, Mantas Mikulėnas <[email protected]> wrote:
> On 2013-02-22 03:03, Mantas Mikulėnas wrote:
>> On 2013-02-22 01:54, Dave Airlie wrote:
>>>>
>>>> | radeon 0000:01:00.0: No connectors reported connected with modes
>>>> | [drm] Cannot find any crtc or sizes - going 1024x768
>>>>
>>>> The connector is definitely connected, since this is a laptop with a
>>>> built-in screen...
>>>>
>>>
>>> Can you get the log with drm.debug=6 from both boots as well?
>>
>> Attached.
>
> The log is also at http://nullroute.eu.org/tmp/2013/dmesg-drm-debug.txt
>
> Not to be annoying, but I hope this can be fixed until 3.9...
>
> (I just tested v3.9-rc1-278-g8343bce, and it still does not detect any
> displays. And if I understood it correctly, "nomodeset" is going to go
> away?)
>
> --
> Mantas Mikulėnas <[email protected]>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Tue, Mar 19, 2013 at 10:09 AM, Linus Torvalds
<[email protected]> wrote:
>
> Doing things like blindly trusting the firmware data without even
> validating it is a really really bad idea. The commit actually looks
> seriously broken in other ways too, like blindly doing phys_to_virt()
> on that, and then trusting the result
Ok, looks like the only thing filling it in is eboot.c, and I guess it
relies on the EFI memory allocations having been mapped. Which they
hopefully have been.
Still, even that seems somewhat debatable. eboot.c does a plain
memcpy() on the pci->romimage returned by
EfiPciIoAttributeOperationGet. And I can *guarantee* that that doesn't
work on some PCI chips that end up sharing the decoder for the ROM and
the graphics aperture or other device oddities. Afaik, some Radeons do
that, for example.
So whoever wrote that eboot thing seems to assume that the world is a
lot simpler and saner than it actually is, and that everybody
magically got things right. Which they never do. The code was
presumably tested on just a couple of machines.
The problem (well, at least *one* problem) is that pci_map_rom()
actually knows about some of these issues, but if dev->rom and
dev->romlen have been set, it trusts them unconditionally. So we'd
either need to fix that, or we need to be really *really* sure that we
only set dev->rom to guaranteed-correct buffers.
At least the radeon code seems to verify that the ROM image starts
with 0x55/0xaa, but I'm guessing we can't do that in general, even if
it is the traditional PC rom pattern.
We only have a few users of "pci_map_rom()", I'm wondering if we can
move the "dev->rom/romsize" cases into the callers. Then the callers
could decide if they want to trust that "pseudo-shadowed" ROM image
(which would test that 55/aa pattern for example), or whether they
want to try to map the actual physical ROM.
Linus
On 2013-03-19 19:09, Linus Torvalds wrote:
> This is apparently still outstanding, and Mantas hadn't cc'd the
> people involved with the commit itself.
I completely forgot about that – sorry.
> Mantas, mind changing that "pcibios_add_device()" function so that
> instead of setting dev->rom/romlen, it just prints out the values
> (including the device address)? Plase also make it print out the
> "data->len" field in addition to the rom->xyz fields..
Which variable is the device address stored in? I'm mostly clueless
about the kernel beyond adding printk's, unfortunately.
But I tried printing out the other values, and this is what I have so far:
For 01:00.0 VGA compatible controller [0300]: Advanced Micro Devices
[AMD] nee ATI Robson CE [AMD Radeon HD 6300 Series] [1002:68e4]
pci 0000:01:00.0: [1002:68e4] type 00 class 0x030000
pci 0000:01:00.0: reg 10: [mem 0xc0000000-0xcfffffff 64bit pref]
pci 0000:01:00.0: reg 18: [mem 0xd0020000-0xd003ffff 64bit]
pci 0000:01:00.0: reg 20: [io 0xd000-0xd0ff]
pci 0000:01:00.0: reg 30: [mem 0xd0000000-0xd001ffff pref]
pci 0000:01:00.0: supports D1 D2
(dbg) -------------------------------------
(dbg) old dev->rom: 0x0000000000000000, dev->romlen: 0
(dbg) new dev->rom: 0x00000000029ad058, dev->romlen: 61952
(dbg) pa_data: 0x00000000029ad018 + offset: 64
(dbg) data->type: 3, data->len: 62000
(dbg) rom->segment: 0, rom->bus: 1
(dbg) rom->device: 0, rom->function: 0
(dbg) rom->vendor: 1002, rom->devid: 68e4 <-- radeon
(dbg) -------------------------------------
And for 05:00.5 Ethernet controller [0200]: JMicron Technology Corp.
JMC250 PCI Express Gigabit Ethernet Controller [197b:0250] (rev 03) –
haven't checked if it's affected too, but including anyway.
pci 0000:05:00.5: [197b:0250] type 00 class 0x020000
pci 0000:05:00.5: reg 10: [mem 0xd0200000-0xd0203fff]
pci 0000:05:00.5: reg 18: [io 0x9100-0x917f]
pci 0000:05:00.5: reg 1c: [io 0x9000-0x90ff]
pci 0000:05:00.5: PME# supported from D0 D3hot D3cold
(dbg) -------------------------------------
(dbg) old dev->rom: 0x0000000000000000, dev->romlen: 0
(dbg) new dev->rom: 0x00000000029bd058, dev->romlen: 40960
(dbg) pa_data: 0x00000000029bd018 + offset: 64
(dbg) data->type: 3, data->len: 41008
(dbg) rom->segment: 0, rom->bus: 5
(dbg) rom->device: 0, rom->function: 5
(dbg) rom->vendor: 197b, rom->devid: 250
(dbg) -------------------------------------
pci 0000:05:00.5: System wakeup disabled by ACPI
--
Mantas Mikulėnas <[email protected]>
On Tue, Mar 19, 2013 at 10:09:22AM -0700, Linus Torvalds wrote:
> Trusting firmware-provided values over the things we can find
> ourselves is known to be fundamentally crap, so what the hell is the
> point of that commit in the first place? The likelihood that firmware
> messes up is pretty damn high. Why would we take idiotic "here's the
> PCI ROM" data from firmware in the first place? What did this fix? We
> know what it broke..
Because it's the only way to get the PCI ROM in some cases, like on
pretty much all Apples with Radeons. Only using it if we have no other
options probably makes sense, though. Something like this (entirely
untested)?
diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index ab886b7..a746dd9 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -100,6 +100,27 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size)
return min((size_t)(image - rom), size);
}
+static loff_t pci_find_rom(struct pci_dev *pdev, size_t *size)
+{
+ struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
+ loff_t start;
+
+ /* assign the ROM an address if it doesn't have one */
+ if (res->parent == NULL && pci_assign_resource(pdev,PCI_ROM_RESOURCE))
+ return 0;
+ start = pci_resource_start(pdev, PCI_ROM_RESOURCE);
+ *size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+
+ if (*size == 0)
+ return 0;
+
+ /* Enable ROM space decodes */
+ if (pci_enable_rom(pdev))
+ return 0;
+
+ return start;
+}
+
/**
* pci_map_rom - map a PCI ROM to kernel space
* @pdev: pointer to pci device struct
@@ -114,21 +135,15 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size)
void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
{
struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
- loff_t start;
+ loff_t start = 0;
void __iomem *rom;
/*
- * Some devices may provide ROMs via a source other than the BAR
- */
- if (pdev->rom && pdev->romlen) {
- *size = pdev->romlen;
- return phys_to_virt(pdev->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.
*/
- } else if (res->flags & IORESOURCE_ROM_SHADOW) {
+ if (res->flags & IORESOURCE_ROM_SHADOW) {
/* primary video rom always starts here */
start = (loff_t)0xC0000;
*size = 0x20000; /* cover C000:0 through E000:0 */
@@ -139,21 +154,21 @@ void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
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;
+ start = pci_find_rom(pdev, size);
}
}
+ /*
+ * Some devices may provide ROMs via a source other than the BAR
+ */
+ if (!start && pdev->rom && pdev->romlen) {
+ *size = pdev->romlen;
+ return phys_to_virt(pdev->rom);
+ }
+
+ if (!start)
+ return NULL;
+
rom = ioremap(start, *size);
if (!rom) {
/* restore enable if ioremap fails */
--
Matthew Garrett | [email protected]
On Tue, Mar 19, 2013 at 12:59 PM, Matthew Garrett <[email protected]> wrote:
>
> Because it's the only way to get the PCI ROM in some cases, like on
> pretty much all Apples with Radeons. Only using it if we have no other
> options probably makes sense, though. Something like this (entirely
> untested)?
This looks reasonable. Mantas?
Trusting the firmware-provided image when we can't find the actual HW
image is quite reasonable. It's the "trust firmware unconditionally"
part that gets my goat.
Linus
On 2013-03-19 22:20, Linus Torvalds wrote:
> On Tue, Mar 19, 2013 at 12:59 PM, Matthew Garrett <[email protected]> wrote:
>>
>> Because it's the only way to get the PCI ROM in some cases, like on
>> pretty much all Apples with Radeons. Only using it if we have no other
>> options probably makes sense, though. Something like this (entirely
>> untested)?
>
> This looks reasonable. Mantas?
It compiles, boots, and even makes the graphics card work again.
So it looks good to me.
--
Mantas Mikulėnas <[email protected]>
Mantas Mikulėnas reported that his graphics hardware failed to initialise
after commit f9a37be0f02a. The aim of this commit was to ensure that ROM
images were available on some Apple systems that don't expose the GPU ROM
via any other source. In this case, UEFI appears to have provided a broken
ROM image that we were using even though there was a perfectly valid ROM
available via other sources. The simplest way to handle this seems to be
to just re-order pci_map_rom() and leave any firmare-supplied ROM to last.
Signed-off-by: Matthew Garrett <[email protected]>
Tested-by: Mantas Mikulėnas <[email protected]>
---
drivers/pci/rom.c | 55 +++++++++++++++++++++++++++++++++++--------------------
1 file changed, 35 insertions(+), 20 deletions(-)
diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index ab886b7..b41ac77 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -100,6 +100,27 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size)
return min((size_t)(image - rom), size);
}
+static loff_t pci_find_rom(struct pci_dev *pdev, size_t *size)
+{
+ struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
+ loff_t start;
+
+ /* assign the ROM an address if it doesn't have one */
+ if (res->parent == NULL && pci_assign_resource(pdev, PCI_ROM_RESOURCE))
+ return 0;
+ start = pci_resource_start(pdev, PCI_ROM_RESOURCE);
+ *size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+
+ if (*size == 0)
+ return 0;
+
+ /* Enable ROM space decodes */
+ if (pci_enable_rom(pdev))
+ return 0;
+
+ return start;
+}
+
/**
* pci_map_rom - map a PCI ROM to kernel space
* @pdev: pointer to pci device struct
@@ -114,21 +135,15 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size)
void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
{
struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
- loff_t start;
+ loff_t start = 0;
void __iomem *rom;
/*
- * Some devices may provide ROMs via a source other than the BAR
- */
- if (pdev->rom && pdev->romlen) {
- *size = pdev->romlen;
- return phys_to_virt(pdev->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.
*/
- } else if (res->flags & IORESOURCE_ROM_SHADOW) {
+ if (res->flags & IORESOURCE_ROM_SHADOW) {
/* primary video rom always starts here */
start = (loff_t)0xC0000;
*size = 0x20000; /* cover C000:0 through E000:0 */
@@ -139,21 +154,21 @@ void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
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;
+ start = pci_find_rom(pdev, size);
}
}
+ /*
+ * Some devices may provide ROMs via a source other than the BAR
+ */
+ if (!start && pdev->rom && pdev->romlen) {
+ *size = pdev->romlen;
+ return phys_to_virt(pdev->rom);
+ }
+
+ if (!start)
+ return NULL;
+
rom = ioremap(start, *size);
if (!rom) {
/* restore enable if ioremap fails */
--
1.8.1.2
>>> Because it's the only way to get the PCI ROM in some cases, like on
>>> pretty much all Apples with Radeons. Only using it if we have no other
>>> options probably makes sense, though. Something like this (entirely
>>> untested)?
>>
>> This looks reasonable. Mantas?
>
> It compiles, boots, and even makes the graphics card work again.
> So it looks good to me.
https://bugzilla.redhat.com/show_bug.cgi?id=927451
popped up sounds like a regression caused by this.
Dave.
On Tue, Mar 26, 2013 at 09:52:14AM +1000, Dave Airlie wrote:
> >>> Because it's the only way to get the PCI ROM in some cases, like on
> >>> pretty much all Apples with Radeons. Only using it if we have no other
> >>> options probably makes sense, though. Something like this (entirely
> >>> untested)?
> >>
> >> This looks reasonable. Mantas?
> >
> > It compiles, boots, and even makes the graphics card work again.
> > So it looks good to me.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=927451
>
> popped up sounds like a regression caused by this.
Sigh. I guess we need to figure out where it thinks it's getting that
image from. The alternative is basically to go back to what Linus
suggested, remove this from pci_map_rom() and add an explicit lookup to
the video drivers.
--
Matthew Garrett | [email protected]
Some platforms only provide their PCI ROM via a platform-specific interface.
Fall back to attempting that if all other sources fail.
Signed-off-by: Matthew Garrett <[email protected]>
---
drivers/gpu/drm/radeon/radeon_bios.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
index b801591..fa3c56f 100644
--- a/drivers/gpu/drm/radeon/radeon_bios.c
+++ b/drivers/gpu/drm/radeon/radeon_bios.c
@@ -99,6 +99,29 @@ static bool radeon_read_bios(struct radeon_device *rdev)
return true;
}
+static bool radeon_read_platform_bios(struct radeon_device *rdev)
+{
+ uint8_t __iomem *bios;
+ size_t size;
+
+ rdev->bios = NULL;
+
+ bios = pci_platform_rom(rdev->pdev, &size);
+ if (!bios) {
+ return false;
+ }
+
+ if (size == 0 || bios[0] != 0x55 || bios[1] != 0xaa) {
+ return false;
+ }
+ rdev->bios = kmemdup(bios, size, GFP_KERNEL);
+ if (rdev->bios == NULL) {
+ return false;
+ }
+
+ return true;
+}
+
#ifdef CONFIG_ACPI
/* ATRM is used to get the BIOS on the discrete cards in
* dual-gpu systems.
@@ -620,6 +643,9 @@ bool radeon_get_bios(struct radeon_device *rdev)
if (r == false) {
r = radeon_read_disabled_bios(rdev);
}
+ if (r == false) {
+ r = radeon_read_platform_bios(rdev);
+ }
if (r == false || rdev->bios == NULL) {
DRM_ERROR("Unable to locate a BIOS ROM\n");
rdev->bios = NULL;
--
1.8.1.2
It turns out that some UEFI systems provide apparently an apparently valid
PCI ROM BAR that turns out to contain garbage, so the attempt in f4eb5ff05
to prefer the ROM from the BAR actually breaks a different set of machines.
As Linus pointed out, the graphics drivers are probably in the best
position to make this judgement, so this basically reverts f4eb5ff05 and
f9a37be0f and adds a new helper function. Followup patches will add support
to nouveau and radeon for probing this ROM source if they can't find a ROM
from some other source.
Signed-off-by: Matthew Garrett <[email protected]>
---
drivers/pci/rom.c | 67 +++++++++++++++++++++++++----------------------------
include/linux/pci.h | 1 +
2 files changed, 32 insertions(+), 36 deletions(-)
diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
index b41ac77..c5d0a08 100644
--- a/drivers/pci/rom.c
+++ b/drivers/pci/rom.c
@@ -100,27 +100,6 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size)
return min((size_t)(image - rom), size);
}
-static loff_t pci_find_rom(struct pci_dev *pdev, size_t *size)
-{
- struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
- loff_t start;
-
- /* assign the ROM an address if it doesn't have one */
- if (res->parent == NULL && pci_assign_resource(pdev, PCI_ROM_RESOURCE))
- return 0;
- start = pci_resource_start(pdev, PCI_ROM_RESOURCE);
- *size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
-
- if (*size == 0)
- return 0;
-
- /* Enable ROM space decodes */
- if (pci_enable_rom(pdev))
- return 0;
-
- return start;
-}
-
/**
* pci_map_rom - map a PCI ROM to kernel space
* @pdev: pointer to pci device struct
@@ -135,7 +114,7 @@ static loff_t pci_find_rom(struct pci_dev *pdev, size_t *size)
void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
{
struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
- loff_t start = 0;
+ loff_t start;
void __iomem *rom;
/*
@@ -154,21 +133,21 @@ void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
return (void __iomem *)(unsigned long)
pci_resource_start(pdev, PCI_ROM_RESOURCE);
} else {
- start = pci_find_rom(pdev, size);
- }
- }
+ /* 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;
- /*
- * Some devices may provide ROMs via a source other than the BAR
- */
- if (!start && pdev->rom && pdev->romlen) {
- *size = pdev->romlen;
- return phys_to_virt(pdev->rom);
+ /* Enable ROM space decodes */
+ if (pci_enable_rom(pdev))
+ return NULL;
+ }
}
- if (!start)
- return NULL;
-
rom = ioremap(start, *size);
if (!rom) {
/* restore enable if ioremap fails */
@@ -202,8 +181,7 @@ void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom)
if (res->flags & (IORESOURCE_ROM_COPY | IORESOURCE_ROM_BIOS_COPY))
return;
- if (!pdev->rom || !pdev->romlen)
- iounmap(rom);
+ iounmap(rom);
/* Disable again before continuing, leave enabled if pci=rom */
if (!(res->flags & (IORESOURCE_ROM_ENABLE | IORESOURCE_ROM_SHADOW)))
@@ -227,7 +205,24 @@ void pci_cleanup_rom(struct pci_dev *pdev)
}
}
+/**
+ * pci_platform_rom - provides a pointer to any ROM image provided by the
+ * platform
+ * @pdev: pointer to pci device struct
+ * @size: pointer to receive size of pci window over ROM
+ */
+void __iomem *pci_platform_rom(struct pci_dev *pdev, size_t *size)
+{
+ if (pdev->rom && pdev->romlen) {
+ *size = pdev->romlen;
+ return phys_to_virt((phys_addr_t)pdev->rom);
+ }
+
+ return NULL;
+}
+
EXPORT_SYMBOL(pci_map_rom);
EXPORT_SYMBOL(pci_unmap_rom);
EXPORT_SYMBOL_GPL(pci_enable_rom);
EXPORT_SYMBOL_GPL(pci_disable_rom);
+EXPORT_SYMBOL(pci_platform_rom);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2461033a..710067f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -916,6 +916,7 @@ void pci_disable_rom(struct pci_dev *pdev);
void __iomem __must_check *pci_map_rom(struct pci_dev *pdev, size_t *size);
void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom);
size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size);
+void __iomem __must_check *pci_platform_rom(struct pci_dev *pdev, size_t *size);
/* Power management related routines */
int pci_save_state(struct pci_dev *dev);
--
1.8.1.2
Some platforms only provide their PCI ROM via a platform-specific interface.
Fall back to attempting that if all other sources fail.
Signed-off-by: Matthew Garrett <[email protected]>
---
drivers/gpu/drm/nouveau/core/subdev/bios/base.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/core/subdev/bios/base.c b/drivers/gpu/drm/nouveau/core/subdev/bios/base.c
index e816f06..0e2c1a4 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/bios/base.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/bios/base.c
@@ -248,6 +248,22 @@ nouveau_bios_shadow_pci(struct nouveau_bios *bios)
}
}
+static void
+nouveau_bios_shadow_platform(struct nouveau_bios *bios)
+{
+ struct pci_dev *pdev = nv_device(bios)->pdev;
+ size_t size;
+
+ void __iomem *rom = pci_platform_rom(pdev, &size);
+ if (rom && size) {
+ bios->data = kmalloc(size, GFP_KERNEL);
+ if (bios->data) {
+ memcpy_fromio(bios->data, rom, size);
+ bios->size = size;
+ }
+ }
+}
+
static int
nouveau_bios_score(struct nouveau_bios *bios, const bool writeable)
{
@@ -288,6 +304,7 @@ nouveau_bios_shadow(struct nouveau_bios *bios)
{ "PROM", nouveau_bios_shadow_prom, false, 0, 0, NULL },
{ "ACPI", nouveau_bios_shadow_acpi, true, 0, 0, NULL },
{ "PCIROM", nouveau_bios_shadow_pci, true, 0, 0, NULL },
+ { "PLATFORM", nouveau_bios_shadow_platform, true, 0, 0, NULL },
{}
};
struct methods *mthd, *best;
--
1.8.1.2
On Tue, Mar 26, 2013 at 3:25 PM, Matthew Garrett
<[email protected]> wrote:
> It turns out that some UEFI systems provide apparently an apparently valid
> PCI ROM BAR that turns out to contain garbage, so the attempt in f4eb5ff05
> to prefer the ROM from the BAR actually breaks a different set of machines.
> As Linus pointed out, the graphics drivers are probably in the best
> position to make this judgement, so this basically reverts f4eb5ff05 and
> f9a37be0f and adds a new helper function. Followup patches will add support
> to nouveau and radeon for probing this ROM source if they can't find a ROM
> from some other source.
I've been on vacation and didn't follow this closely, but it seems
like this fixes a regression and should be merged before v3.9, right?
Can you include a reference to a bugzilla or the mailing list
discussion in the changelog? I can just fold it in if you want.
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> drivers/pci/rom.c | 67 +++++++++++++++++++++++++----------------------------
> include/linux/pci.h | 1 +
> 2 files changed, 32 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
> index b41ac77..c5d0a08 100644
> --- a/drivers/pci/rom.c
> +++ b/drivers/pci/rom.c
> @@ -100,27 +100,6 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size)
> return min((size_t)(image - rom), size);
> }
>
> -static loff_t pci_find_rom(struct pci_dev *pdev, size_t *size)
> -{
> - struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
> - loff_t start;
> -
> - /* assign the ROM an address if it doesn't have one */
> - if (res->parent == NULL && pci_assign_resource(pdev, PCI_ROM_RESOURCE))
> - return 0;
> - start = pci_resource_start(pdev, PCI_ROM_RESOURCE);
> - *size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
> -
> - if (*size == 0)
> - return 0;
> -
> - /* Enable ROM space decodes */
> - if (pci_enable_rom(pdev))
> - return 0;
> -
> - return start;
> -}
> -
> /**
> * pci_map_rom - map a PCI ROM to kernel space
> * @pdev: pointer to pci device struct
> @@ -135,7 +114,7 @@ static loff_t pci_find_rom(struct pci_dev *pdev, size_t *size)
> void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
> {
> struct resource *res = &pdev->resource[PCI_ROM_RESOURCE];
> - loff_t start = 0;
> + loff_t start;
> void __iomem *rom;
>
> /*
> @@ -154,21 +133,21 @@ void __iomem *pci_map_rom(struct pci_dev *pdev, size_t *size)
> return (void __iomem *)(unsigned long)
> pci_resource_start(pdev, PCI_ROM_RESOURCE);
> } else {
> - start = pci_find_rom(pdev, size);
> - }
> - }
> + /* 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;
>
> - /*
> - * Some devices may provide ROMs via a source other than the BAR
> - */
> - if (!start && pdev->rom && pdev->romlen) {
> - *size = pdev->romlen;
> - return phys_to_virt(pdev->rom);
> + /* Enable ROM space decodes */
> + if (pci_enable_rom(pdev))
> + return NULL;
> + }
> }
>
> - if (!start)
> - return NULL;
> -
> rom = ioremap(start, *size);
> if (!rom) {
> /* restore enable if ioremap fails */
> @@ -202,8 +181,7 @@ void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom)
> if (res->flags & (IORESOURCE_ROM_COPY | IORESOURCE_ROM_BIOS_COPY))
> return;
>
> - if (!pdev->rom || !pdev->romlen)
> - iounmap(rom);
> + iounmap(rom);
>
> /* Disable again before continuing, leave enabled if pci=rom */
> if (!(res->flags & (IORESOURCE_ROM_ENABLE | IORESOURCE_ROM_SHADOW)))
> @@ -227,7 +205,24 @@ void pci_cleanup_rom(struct pci_dev *pdev)
> }
> }
>
> +/**
> + * pci_platform_rom - provides a pointer to any ROM image provided by the
> + * platform
> + * @pdev: pointer to pci device struct
> + * @size: pointer to receive size of pci window over ROM
> + */
> +void __iomem *pci_platform_rom(struct pci_dev *pdev, size_t *size)
> +{
> + if (pdev->rom && pdev->romlen) {
> + *size = pdev->romlen;
> + return phys_to_virt((phys_addr_t)pdev->rom);
> + }
> +
> + return NULL;
> +}
> +
> EXPORT_SYMBOL(pci_map_rom);
> EXPORT_SYMBOL(pci_unmap_rom);
> EXPORT_SYMBOL_GPL(pci_enable_rom);
> EXPORT_SYMBOL_GPL(pci_disable_rom);
> +EXPORT_SYMBOL(pci_platform_rom);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2461033a..710067f 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -916,6 +916,7 @@ void pci_disable_rom(struct pci_dev *pdev);
> void __iomem __must_check *pci_map_rom(struct pci_dev *pdev, size_t *size);
> void pci_unmap_rom(struct pci_dev *pdev, void __iomem *rom);
> size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size);
> +void __iomem __must_check *pci_platform_rom(struct pci_dev *pdev, size_t *size);
>
> /* Power management related routines */
> int pci_save_state(struct pci_dev *dev);
> --
> 1.8.1.2
>
On Tue, 2013-03-26 at 16:53 -0600, Bjorn Helgaas wrote:
> I've been on vacation and didn't follow this closely, but it seems
> like this fixes a regression and should be merged before v3.9, right?
> Can you include a reference to a bugzilla or the mailing list
> discussion in the changelog? I can just fold it in if you want.
https://bugzilla.redhat.com/show_bug.cgi?id=927451
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
[+cc Chris, reporter of Fedora issue]
On Mon, Mar 25, 2013 at 5:55 PM, Matthew Garrett <[email protected]> wrote:
> On Tue, Mar 26, 2013 at 09:52:14AM +1000, Dave Airlie wrote:
>> >>> Because it's the only way to get the PCI ROM in some cases, like on
>> >>> pretty much all Apples with Radeons. Only using it if we have no other
>> >>> options probably makes sense, though. Something like this (entirely
>> >>> untested)?
>> >>
>> >> This looks reasonable. Mantas?
>> >
>> > It compiles, boots, and even makes the graphics card work again.
>> > So it looks good to me.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=927451
>>
>> popped up sounds like a regression caused by this.
That RedHat bugzilla is from kernel-3.9.0-0.rc4.git0.1.fc19.x86_64.
Does that kernel include Matthew's patch from Mar 19? I'm wondering
if that report is for the same problem Mantas saw and possibly would
be *fixed* by the Mar 19 patch, or if it is for a problem with the Mar
19 patch itself.
I would check this myself, but I don't know how to figure out what's
in kernel-3.9.0-0.rc4.git0.1.fc19.x86_64.
Mantas confirmed that the Mar 19 problem fixed *his* issue, but I
haven't seen any reports from either Mantas or Chris (the RedHat
bugzilla reporter) for the new Mar 26 patches.
> Sigh. I guess we need to figure out where it thinks it's getting that
> image from. The alternative is basically to go back to what Linus
> suggested, remove this from pci_map_rom() and add an explicit lookup to
> the video drivers.
>
> --
> Matthew Garrett | [email protected]
>
> That RedHat bugzilla is from kernel-3.9.0-0.rc4.git0.1.fc19.x86_64.
> Does that kernel include Matthew's patch from Mar 19? I'm wondering
> if that report is for the same problem Mantas saw and possibly would
> be *fixed* by the Mar 19 patch, or if it is for a problem with the Mar
> 19 patch itself.
>
> I would check this myself, but I don't know how to figure out what's
> in kernel-3.9.0-0.rc4.git0.1.fc19.x86_64.
Its 3.9-rc4 + a git snapshot one day after,
>From what I can see 3.9-rc4 contains this patch from Mar 19.
Dave.
On Wed, Mar 27, 2013 at 2:02 PM, Dave Airlie <[email protected]> wrote:
>>
>> That RedHat bugzilla is from kernel-3.9.0-0.rc4.git0.1.fc19.x86_64.
>> Does that kernel include Matthew's patch from Mar 19? I'm wondering
>> if that report is for the same problem Mantas saw and possibly would
>> be *fixed* by the Mar 19 patch, or if it is for a problem with the Mar
>> 19 patch itself.
>>
>> I would check this myself, but I don't know how to figure out what's
>> in kernel-3.9.0-0.rc4.git0.1.fc19.x86_64.
>
> Its 3.9-rc4 + a git snapshot one day after,
>
> From what I can see 3.9-rc4 contains this patch from Mar 19.
Yep, looks like it to me, too. Apparently the Mar 19 patch
(547b52463) worked for Mantas but not for Chris.
So I'll wait for testing reports from Mantas and Chris before pushing
the Mar 26 patches. Linus can take them earlier if he wants, of
course :)
Bjorn
[+cc Mantas, Chris]
On Tue, Mar 26, 2013 at 4:55 PM, Matthew Garrett
<[email protected]> wrote:
> On Tue, 2013-03-26 at 16:53 -0600, Bjorn Helgaas wrote:
>
>> I've been on vacation and didn't follow this closely, but it seems
>> like this fixes a regression and should be merged before v3.9, right?
>> Can you include a reference to a bugzilla or the mailing list
>> discussion in the changelog? I can just fold it in if you want.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=927451
Thanks, I added that URL and queued up these patches. I'll wait to
push them until we have confirmation from Mantas [1] and Chris [2]
that this fixes it for both.
[1] http://marc.info/?l=linux-kernel&m=136148818405871
[2] https://bugzilla.redhat.com/show_bug.cgi?id=927451
[+cc linux-pci]
On Wed, Mar 27, 2013 at 2:33 PM, Bjorn Helgaas <[email protected]> wrote:
> [+cc Mantas, Chris]
>
> On Tue, Mar 26, 2013 at 4:55 PM, Matthew Garrett
> <[email protected]> wrote:
>> On Tue, 2013-03-26 at 16:53 -0600, Bjorn Helgaas wrote:
>>
>>> I've been on vacation and didn't follow this closely, but it seems
>>> like this fixes a regression and should be merged before v3.9, right?
>>> Can you include a reference to a bugzilla or the mailing list
>>> discussion in the changelog? I can just fold it in if you want.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=927451
>
> Thanks, I added that URL and queued up these patches. I'll wait to
> push them until we have confirmation from Mantas [1] and Chris [2]
> that this fixes it for both.
>
> [1] http://marc.info/?l=linux-kernel&m=136148818405871
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=927451
On 2013-03-27 22:33, Bjorn Helgaas wrote:
> On Tue, Mar 26, 2013 at 4:55 PM, Matthew Garrett
> <[email protected]> wrote:
>> On Tue, 2013-03-26 at 16:53 -0600, Bjorn Helgaas wrote:
>>
>>> I've been on vacation and didn't follow this closely, but it seems
>>> like this fixes a regression and should be merged before v3.9, right?
>>> Can you include a reference to a bugzilla or the mailing list
>>> discussion in the changelog? I can just fold it in if you want.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=927451
>
> Thanks, I added that URL and queued up these patches. I'll wait to
> push them until we have confirmation from Mantas [1] and Chris [2]
> that this fixes it for both.
Tested the three patches on top of v3.9-rc4-134-gb175293, works fine.
[the first patch refers to reverting commit f4eb5ff05, which doesn't
seem to exist; should probably have been 547b52463]
--
Mantas Mikulėnas <[email protected]>
On Thu, Mar 28, 2013 at 2:48 AM, Mantas Mikulėnas <[email protected]> wrote:
> On 2013-03-27 22:33, Bjorn Helgaas wrote:
>> On Tue, Mar 26, 2013 at 4:55 PM, Matthew Garrett
>> <[email protected]> wrote:
>>> On Tue, 2013-03-26 at 16:53 -0600, Bjorn Helgaas wrote:
>>>
>>>> I've been on vacation and didn't follow this closely, but it seems
>>>> like this fixes a regression and should be merged before v3.9, right?
>>>> Can you include a reference to a bugzilla or the mailing list
>>>> discussion in the changelog? I can just fold it in if you want.
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=927451
>>
>> Thanks, I added that URL and queued up these patches. I'll wait to
>> push them until we have confirmation from Mantas [1] and Chris [2]
>> that this fixes it for both.
>
> Tested the three patches on top of v3.9-rc4-134-gb175293, works fine.
>
>
> [the first patch refers to reverting commit f4eb5ff05, which doesn't
> seem to exist; should probably have been 547b52463]
Both Chris and Mantas have tested these patches. Mantas reports success.
Chris still has problems (see
https://bugzilla.redhat.com/show_bug.cgi?id=927451), but I don't know
whether they are related to these patches or something else.
Matthew or Dave, can you take a look at the bugzilla so we can make
some progress with these?
Bjorn
On Mon, 2013-04-01 at 11:39 -0600, Bjorn Helgaas wrote:
> Chris still has problems (see
> https://bugzilla.redhat.com/show_bug.cgi?id=927451), but I don't know
> whether they are related to these patches or something else.
I think they're unrelated. The log he posts using this patch gives the
correct output - the ROM image comes from the platform method rather
than from PCI. I think Ben probably needs to look at that.
--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, Apr 1, 2013 at 11:47 AM, Matthew Garrett
<[email protected]> wrote:
> On Mon, 2013-04-01 at 11:39 -0600, Bjorn Helgaas wrote:
>
>> Chris still has problems (see
>> https://bugzilla.redhat.com/show_bug.cgi?id=927451), but I don't know
>> whether they are related to these patches or something else.
>
> I think they're unrelated. The log he posts using this patch gives the
> correct output - the ROM image comes from the platform method rather
> than from PCI. I think Ben probably needs to look at that.
OK, I added these three patches to my for-linus branch, headed for v3.9.
On Fri, Apr 5, 2013 at 2:31 PM, Chris Murphy <[email protected]> wrote:
>
> On Apr 2, 2013, at 2:10 PM, Bjorn Helgaas <[email protected]> wrote:
>
>> On Mon, Apr 1, 2013 at 11:47 AM, Matthew Garrett
>> <[email protected]> wrote:
>>> On Mon, 2013-04-01 at 11:39 -0600, Bjorn Helgaas wrote:
>>>
>>>> Chris still has problems (see
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=927451), but I don't know
>>>> whether they are related to these patches or something else.
>>>
>>> I think they're unrelated. The log he posts using this patch gives the
>>> correct output - the ROM image comes from the platform method rather
>>> than from PCI. I think Ben probably needs to look at that.
>>
>> OK, I added these three patches to my for-linus branch, headed for v3.9.
>
> Are they in 3.9.0-0.rc5.git2.1.f19? I'm seeing a regression from 3.8.5 with the radeon driver not finding BIOS ROM as well.
> https://bugzilla.redhat.com/show_bug.cgi?id=949083
No. I haven't asked Linus to pull my branch yet (was just thinking it
was time to do that, coincidentally :))
Bjorn
On Apr 2, 2013, at 2:10 PM, Bjorn Helgaas <[email protected]> wrote:
> On Mon, Apr 1, 2013 at 11:47 AM, Matthew Garrett
> <[email protected]> wrote:
>> On Mon, 2013-04-01 at 11:39 -0600, Bjorn Helgaas wrote:
>>
>>> Chris still has problems (see
>>> https://bugzilla.redhat.com/show_bug.cgi?id=927451), but I don't know
>>> whether they are related to these patches or something else.
>>
>> I think they're unrelated. The log he posts using this patch gives the
>> correct output - the ROM image comes from the platform method rather
>> than from PCI. I think Ben probably needs to look at that.
>
> OK, I added these three patches to my for-linus branch, headed for v3.9.
Are they in 3.9.0-0.rc5.git2.1.f19? I'm seeing a regression from 3.8.5 with the radeon driver not finding BIOS ROM as well.
https://bugzilla.redhat.com/show_bug.cgi?id=949083
On Apr 5, 2013, at 2:35 PM, Bjorn Helgaas <[email protected]> wrote:
> On Fri, Apr 5, 2013 at 2:31 PM, Chris Murphy <[email protected]> wrote:
>>
>>
>> Are they in 3.9.0-0.rc5.git2.1.f19? I'm seeing a regression from 3.8.5 with the radeon driver not finding BIOS ROM as well.
>> https://bugzilla.redhat.com/show_bug.cgi?id=949083
>
> No. I haven't asked Linus to pull my branch yet (was just thinking it
> was time to do that, coincidentally :))
The patch appears to fix Bug 949083 radeon issue as well. I've updated the bug report.-