[1/3] pci: Add PCI ROM helper for platform-provided ROM images
diff mbox series

Message ID 1364333156-4530-1-git-send-email-matthew.garrett@nebula.com
State New, archived
Headers show
Series
  • [1/3] pci: Add PCI ROM helper for platform-provided ROM images
Related show

Commit Message

Matthew Garrett March 26, 2013, 9:25 p.m. UTC
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 <matthew.garrett@nebula.com>
---
 drivers/pci/rom.c   | 67 +++++++++++++++++++++++++----------------------------
 include/linux/pci.h |  1 +
 2 files changed, 32 insertions(+), 36 deletions(-)

Comments

Bjorn Helgaas March 26, 2013, 10:53 p.m. UTC | #1
On Tue, Mar 26, 2013 at 3:25 PM, Matthew Garrett
<matthew.garrett@nebula.com> 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 <matthew.garrett@nebula.com>
> ---
>  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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Bjorn Helgaas March 27, 2013, 8:33 p.m. UTC | #2
[+cc Mantas, Chris]

On Tue, Mar 26, 2013 at 4:55 PM, Matthew Garrett
<matthew.garrett@nebula.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Bjorn Helgaas March 27, 2013, 8:34 p.m. UTC | #3
[+cc linux-pci]

On Wed, Mar 27, 2013 at 2:33 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Mantas, Chris]
>
> On Tue, Mar 26, 2013 at 4:55 PM, Matthew Garrett
> <matthew.garrett@nebula.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mantas Mikul─Śnas March 28, 2013, 8:48 a.m. UTC | #4
On 2013-03-27 22:33, Bjorn Helgaas wrote:
> On Tue, Mar 26, 2013 at 4:55 PM, Matthew Garrett
> <matthew.garrett@nebula.com> 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]
Bjorn Helgaas April 1, 2013, 5:39 p.m. UTC | #5
On Thu, Mar 28, 2013 at 2:48 AM, Mantas Mikul─Śnas <grawity@gmail.com> wrote:
> On 2013-03-27 22:33, Bjorn Helgaas wrote:
>> On Tue, Mar 26, 2013 at 4:55 PM, Matthew Garrett
>> <matthew.garrett@nebula.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Bjorn Helgaas April 2, 2013, 8:10 p.m. UTC | #6
On Mon, Apr 1, 2013 at 11:47 AM, Matthew Garrett
<matthew.garrett@nebula.com> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Chris Murphy April 5, 2013, 8:31 p.m. UTC | #7
On Apr 2, 2013, at 2:10 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Mon, Apr 1, 2013 at 11:47 AM, Matthew Garrett
> <matthew.garrett@nebula.com> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Bjorn Helgaas April 5, 2013, 8:35 p.m. UTC | #8
On Fri, Apr 5, 2013 at 2:31 PM, Chris Murphy <bugzilla@colorremedies.com> wrote:
>
> On Apr 2, 2013, at 2:10 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> On Mon, Apr 1, 2013 at 11:47 AM, Matthew Garrett
>> <matthew.garrett@nebula.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Chris Murphy April 5, 2013, 10:28 p.m. UTC | #9
On Apr 5, 2013, at 2:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Fri, Apr 5, 2013 at 2:31 PM, Chris Murphy <bugzilla@colorremedies.com> 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.--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

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);