* Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it
2017-07-10 21:13 ` Ard Biesheuvel
(?)
@ 2017-07-11 9:43 ` Ard Biesheuvel
-1 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2017-07-11 9:43 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Peter Jones
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA, Matt Fleming,
Bartlomiej Zolnierkiewicz, Lorenzo Pieralisi, Ard Biesheuvel
On 10 July 2017 at 22:13, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On UEFI systems, the firmware may expose a Graphics Output Protocol (GOP)
> instance to which the efifb driver attempts to attach in order to provide
> a minimal, unaccelerated framebuffer. The GOP protocol itself is not very
> sophisticated, and only describes the offset and size of the framebuffer
> in memory, and the pixel format.
>
> If the GOP framebuffer is provided by a PCI device, it will have been
> configured and enabled by the UEFI firmware, and the GOP protocol will
> simply point into a live BAR region. However, the GOP protocol itself does
> not describe this relation, and so we have to take care not to reconfigure
> the BAR without taking efifb's dependency on it into account.
>
> Commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that covers
> the framebuffer") attempted to do so by claiming the BAR resource early
> on, which prevents the PCI resource allocation routines from changing it.
> However, it turns out that this only works if the PCI device is not
> behind any bridges, since the bridge resources need to be claimed first.
>
> So instead, allow the BAR to be moved, but make the efifb driver deal
> with that gracefully. So record the resource that covers the BAR early
> on, and if it turns out to have moved by the time we probe the efifb
> driver, update the framebuffer address accordingly.
>
> While this is less likely to occur on x86, given that the firmware's
> PCI resource allocation is more likely to be preserved, this is a
> worthwhile sanity check to have in place, and so let's remove the
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> v2: - use pr_info() not pr_warn() for non-error condition
>
> drivers/video/fbdev/efifb.c | 24 ++++++++++++--------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
Bjorn,
If this looks reasonable to you from the PCI side of things, I can
take this through the EFI tree for v4.14
--
Ard.
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ff01bed7112f..0dd7e5eb051f 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -146,6 +146,9 @@ ATTRIBUTE_GROUPS(efifb);
>
> static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
>
> +static struct resource *bar_resource;
> +static u64 bar_offset;
> +
> static int efifb_probe(struct platform_device *dev)
> {
> struct fb_info *info;
> @@ -200,6 +203,13 @@ static int efifb_probe(struct platform_device *dev)
> efifb_fix.smem_start |= ext_lfb_base;
> }
>
> + if (bar_resource &&
> + bar_resource->start + bar_offset != efifb_fix.smem_start) {
> +
> + pr_info("efifb: PCI BAR has moved, updating fb address\n");
> + efifb_fix.smem_start = bar_resource->start + bar_offset;
> + }
> +
> efifb_defined.bits_per_pixel = screen_info.lfb_depth;
> efifb_defined.xres = screen_info.lfb_width;
> efifb_defined.yres = screen_info.lfb_height;
> @@ -364,11 +374,11 @@ static struct platform_driver efifb_driver = {
>
> builtin_platform_driver(efifb_driver);
>
> -#if defined(CONFIG_PCI) && !defined(CONFIG_X86)
> +#if defined(CONFIG_PCI)
>
> static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
>
> -static void claim_efifb_bar(struct pci_dev *dev, int idx)
> +static void record_efifb_bar_resource(struct pci_dev *dev, int idx, u64 offset)
> {
> u16 word;
>
> @@ -383,12 +393,8 @@ static void claim_efifb_bar(struct pci_dev *dev, int idx)
> return;
> }
>
> - if (pci_claim_resource(dev, idx)) {
> - pci_dev_disabled = true;
> - dev_err(&dev->dev,
> - "BAR %d: failed to claim resource for efifb!\n", idx);
> - return;
> - }
> + bar_resource = &dev->resource[idx];
> + bar_offset = offset;
>
> dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
> }
> @@ -415,7 +421,7 @@ static void efifb_fixup_resources(struct pci_dev *dev)
> continue;
>
> if (res->start <= base && res->end >= base + size - 1) {
> - claim_efifb_bar(dev, i);
> + record_efifb_bar_resource(dev, i, base - res->start);
> break;
> }
> }
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it
@ 2017-07-11 9:43 ` Ard Biesheuvel
0 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2017-07-11 9:43 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Peter Jones
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA, Matt Fleming,
Bartlomiej Zolnierkiewicz, Lorenzo Pieralisi, Ard Biesheuvel
On 10 July 2017 at 22:13, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On UEFI systems, the firmware may expose a Graphics Output Protocol (GOP)
> instance to which the efifb driver attempts to attach in order to provide
> a minimal, unaccelerated framebuffer. The GOP protocol itself is not very
> sophisticated, and only describes the offset and size of the framebuffer
> in memory, and the pixel format.
>
> If the GOP framebuffer is provided by a PCI device, it will have been
> configured and enabled by the UEFI firmware, and the GOP protocol will
> simply point into a live BAR region. However, the GOP protocol itself does
> not describe this relation, and so we have to take care not to reconfigure
> the BAR without taking efifb's dependency on it into account.
>
> Commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that covers
> the framebuffer") attempted to do so by claiming the BAR resource early
> on, which prevents the PCI resource allocation routines from changing it.
> However, it turns out that this only works if the PCI device is not
> behind any bridges, since the bridge resources need to be claimed first.
>
> So instead, allow the BAR to be moved, but make the efifb driver deal
> with that gracefully. So record the resource that covers the BAR early
> on, and if it turns out to have moved by the time we probe the efifb
> driver, update the framebuffer address accordingly.
>
> While this is less likely to occur on x86, given that the firmware's
> PCI resource allocation is more likely to be preserved, this is a
> worthwhile sanity check to have in place, and so let's remove the
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: - use pr_info() not pr_warn() for non-error condition
>
> drivers/video/fbdev/efifb.c | 24 ++++++++++++--------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
Bjorn,
If this looks reasonable to you from the PCI side of things, I can
take this through the EFI tree for v4.14
--
Ard.
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ff01bed7112f..0dd7e5eb051f 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -146,6 +146,9 @@ ATTRIBUTE_GROUPS(efifb);
>
> static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
>
> +static struct resource *bar_resource;
> +static u64 bar_offset;
> +
> static int efifb_probe(struct platform_device *dev)
> {
> struct fb_info *info;
> @@ -200,6 +203,13 @@ static int efifb_probe(struct platform_device *dev)
> efifb_fix.smem_start |= ext_lfb_base;
> }
>
> + if (bar_resource &&
> + bar_resource->start + bar_offset != efifb_fix.smem_start) {
> +
> + pr_info("efifb: PCI BAR has moved, updating fb address\n");
> + efifb_fix.smem_start = bar_resource->start + bar_offset;
> + }
> +
> efifb_defined.bits_per_pixel = screen_info.lfb_depth;
> efifb_defined.xres = screen_info.lfb_width;
> efifb_defined.yres = screen_info.lfb_height;
> @@ -364,11 +374,11 @@ static struct platform_driver efifb_driver = {
>
> builtin_platform_driver(efifb_driver);
>
> -#if defined(CONFIG_PCI) && !defined(CONFIG_X86)
> +#if defined(CONFIG_PCI)
>
> static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
>
> -static void claim_efifb_bar(struct pci_dev *dev, int idx)
> +static void record_efifb_bar_resource(struct pci_dev *dev, int idx, u64 offset)
> {
> u16 word;
>
> @@ -383,12 +393,8 @@ static void claim_efifb_bar(struct pci_dev *dev, int idx)
> return;
> }
>
> - if (pci_claim_resource(dev, idx)) {
> - pci_dev_disabled = true;
> - dev_err(&dev->dev,
> - "BAR %d: failed to claim resource for efifb!\n", idx);
> - return;
> - }
> + bar_resource = &dev->resource[idx];
> + bar_offset = offset;
>
> dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
> }
> @@ -415,7 +421,7 @@ static void efifb_fixup_resources(struct pci_dev *dev)
> continue;
>
> if (res->start <= base && res->end >= base + size - 1) {
> - claim_efifb_bar(dev, i);
> + record_efifb_bar_resource(dev, i, base - res->start);
> break;
> }
> }
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it
@ 2017-07-11 9:43 ` Ard Biesheuvel
0 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2017-07-11 9:43 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas, Peter Jones
Cc: linux-fbdev, linux-efi, Matt Fleming, Bartlomiej Zolnierkiewicz,
Lorenzo Pieralisi, Ard Biesheuvel
On 10 July 2017 at 22:13, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On UEFI systems, the firmware may expose a Graphics Output Protocol (GOP)
> instance to which the efifb driver attempts to attach in order to provide
> a minimal, unaccelerated framebuffer. The GOP protocol itself is not very
> sophisticated, and only describes the offset and size of the framebuffer
> in memory, and the pixel format.
>
> If the GOP framebuffer is provided by a PCI device, it will have been
> configured and enabled by the UEFI firmware, and the GOP protocol will
> simply point into a live BAR region. However, the GOP protocol itself does
> not describe this relation, and so we have to take care not to reconfigure
> the BAR without taking efifb's dependency on it into account.
>
> Commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that covers
> the framebuffer") attempted to do so by claiming the BAR resource early
> on, which prevents the PCI resource allocation routines from changing it.
> However, it turns out that this only works if the PCI device is not
> behind any bridges, since the bridge resources need to be claimed first.
>
> So instead, allow the BAR to be moved, but make the efifb driver deal
> with that gracefully. So record the resource that covers the BAR early
> on, and if it turns out to have moved by the time we probe the efifb
> driver, update the framebuffer address accordingly.
>
> While this is less likely to occur on x86, given that the firmware's
> PCI resource allocation is more likely to be preserved, this is a
> worthwhile sanity check to have in place, and so let's remove the
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: - use pr_info() not pr_warn() for non-error condition
>
> drivers/video/fbdev/efifb.c | 24 ++++++++++++--------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
Bjorn,
If this looks reasonable to you from the PCI side of things, I can
take this through the EFI tree for v4.14
--
Ard.
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ff01bed7112f..0dd7e5eb051f 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -146,6 +146,9 @@ ATTRIBUTE_GROUPS(efifb);
>
> static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
>
> +static struct resource *bar_resource;
> +static u64 bar_offset;
> +
> static int efifb_probe(struct platform_device *dev)
> {
> struct fb_info *info;
> @@ -200,6 +203,13 @@ static int efifb_probe(struct platform_device *dev)
> efifb_fix.smem_start |= ext_lfb_base;
> }
>
> + if (bar_resource &&
> + bar_resource->start + bar_offset != efifb_fix.smem_start) {
> +
> + pr_info("efifb: PCI BAR has moved, updating fb address\n");
> + efifb_fix.smem_start = bar_resource->start + bar_offset;
> + }
> +
> efifb_defined.bits_per_pixel = screen_info.lfb_depth;
> efifb_defined.xres = screen_info.lfb_width;
> efifb_defined.yres = screen_info.lfb_height;
> @@ -364,11 +374,11 @@ static struct platform_driver efifb_driver = {
>
> builtin_platform_driver(efifb_driver);
>
> -#if defined(CONFIG_PCI) && !defined(CONFIG_X86)
> +#if defined(CONFIG_PCI)
>
> static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
>
> -static void claim_efifb_bar(struct pci_dev *dev, int idx)
> +static void record_efifb_bar_resource(struct pci_dev *dev, int idx, u64 offset)
> {
> u16 word;
>
> @@ -383,12 +393,8 @@ static void claim_efifb_bar(struct pci_dev *dev, int idx)
> return;
> }
>
> - if (pci_claim_resource(dev, idx)) {
> - pci_dev_disabled = true;
> - dev_err(&dev->dev,
> - "BAR %d: failed to claim resource for efifb!\n", idx);
> - return;
> - }
> + bar_resource = &dev->resource[idx];
> + bar_offset = offset;
>
> dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
> }
> @@ -415,7 +421,7 @@ static void efifb_fixup_resources(struct pci_dev *dev)
> continue;
>
> if (res->start <= base && res->end >= base + size - 1) {
> - claim_efifb_bar(dev, i);
> + record_efifb_bar_resource(dev, i, base - res->start);
> break;
> }
> }
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it
2017-07-10 21:13 ` Ard Biesheuvel
(?)
@ 2017-07-11 11:55 ` Peter Jones
-1 siblings, 0 replies; 18+ messages in thread
From: Peter Jones @ 2017-07-11 11:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-pci-u79uwXL29TY76Z2rM5mHXA,
bhelgaas-hpIqsD4AKlfQT0dZR+AlfA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
matt-mF/unelCI9GS6iBeEJttW/XRex20P6io,
b.zolnierkie-Sze3O3UU22JBDgjK7y7TUQ,
lorenzo.pieralisi-5wv7dgnIgG8
On Mon, Jul 10, 2017 at 10:13:05PM +0100, Ard Biesheuvel wrote:
> On UEFI systems, the firmware may expose a Graphics Output Protocol (GOP)
> instance to which the efifb driver attempts to attach in order to provide
> a minimal, unaccelerated framebuffer. The GOP protocol itself is not very
> sophisticated, and only describes the offset and size of the framebuffer
> in memory, and the pixel format.
>
> If the GOP framebuffer is provided by a PCI device, it will have been
> configured and enabled by the UEFI firmware, and the GOP protocol will
> simply point into a live BAR region. However, the GOP protocol itself does
> not describe this relation, and so we have to take care not to reconfigure
> the BAR without taking efifb's dependency on it into account.
>
> Commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that covers
> the framebuffer") attempted to do so by claiming the BAR resource early
> on, which prevents the PCI resource allocation routines from changing it.
> However, it turns out that this only works if the PCI device is not
> behind any bridges, since the bridge resources need to be claimed first.
>
> So instead, allow the BAR to be moved, but make the efifb driver deal
> with that gracefully. So record the resource that covers the BAR early
> on, and if it turns out to have moved by the time we probe the efifb
> driver, update the framebuffer address accordingly.
>
> While this is less likely to occur on x86, given that the firmware's
> PCI resource allocation is more likely to be preserved, this is a
> worthwhile sanity check to have in place, and so let's remove the
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> v2: - use pr_info() not pr_warn() for non-error condition
Well, that settled all of my concerns:
Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> drivers/video/fbdev/efifb.c | 24 ++++++++++++--------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ff01bed7112f..0dd7e5eb051f 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -146,6 +146,9 @@ ATTRIBUTE_GROUPS(efifb);
>
> static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
>
> +static struct resource *bar_resource;
> +static u64 bar_offset;
> +
> static int efifb_probe(struct platform_device *dev)
> {
> struct fb_info *info;
> @@ -200,6 +203,13 @@ static int efifb_probe(struct platform_device *dev)
> efifb_fix.smem_start |= ext_lfb_base;
> }
>
> + if (bar_resource &&
> + bar_resource->start + bar_offset != efifb_fix.smem_start) {
> +
> + pr_info("efifb: PCI BAR has moved, updating fb address\n");
> + efifb_fix.smem_start = bar_resource->start + bar_offset;
> + }
> +
> efifb_defined.bits_per_pixel = screen_info.lfb_depth;
> efifb_defined.xres = screen_info.lfb_width;
> efifb_defined.yres = screen_info.lfb_height;
> @@ -364,11 +374,11 @@ static struct platform_driver efifb_driver = {
>
> builtin_platform_driver(efifb_driver);
>
> -#if defined(CONFIG_PCI) && !defined(CONFIG_X86)
> +#if defined(CONFIG_PCI)
>
> static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
>
> -static void claim_efifb_bar(struct pci_dev *dev, int idx)
> +static void record_efifb_bar_resource(struct pci_dev *dev, int idx, u64 offset)
> {
> u16 word;
>
> @@ -383,12 +393,8 @@ static void claim_efifb_bar(struct pci_dev *dev, int idx)
> return;
> }
>
> - if (pci_claim_resource(dev, idx)) {
> - pci_dev_disabled = true;
> - dev_err(&dev->dev,
> - "BAR %d: failed to claim resource for efifb!\n", idx);
> - return;
> - }
> + bar_resource = &dev->resource[idx];
> + bar_offset = offset;
>
> dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
> }
> @@ -415,7 +421,7 @@ static void efifb_fixup_resources(struct pci_dev *dev)
> continue;
>
> if (res->start <= base && res->end >= base + size - 1) {
> - claim_efifb_bar(dev, i);
> + record_efifb_bar_resource(dev, i, base - res->start);
> break;
> }
> }
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Peter
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it
@ 2017-07-11 11:55 ` Peter Jones
0 siblings, 0 replies; 18+ messages in thread
From: Peter Jones @ 2017-07-11 11:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-pci-u79uwXL29TY76Z2rM5mHXA,
bhelgaas-hpIqsD4AKlfQT0dZR+AlfA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
matt-mF/unelCI9GS6iBeEJttW/XRex20P6io,
b.zolnierkie-Sze3O3UU22JBDgjK7y7TUQ,
lorenzo.pieralisi-5wv7dgnIgG8
On Mon, Jul 10, 2017 at 10:13:05PM +0100, Ard Biesheuvel wrote:
> On UEFI systems, the firmware may expose a Graphics Output Protocol (GOP)
> instance to which the efifb driver attempts to attach in order to provide
> a minimal, unaccelerated framebuffer. The GOP protocol itself is not very
> sophisticated, and only describes the offset and size of the framebuffer
> in memory, and the pixel format.
>
> If the GOP framebuffer is provided by a PCI device, it will have been
> configured and enabled by the UEFI firmware, and the GOP protocol will
> simply point into a live BAR region. However, the GOP protocol itself does
> not describe this relation, and so we have to take care not to reconfigure
> the BAR without taking efifb's dependency on it into account.
>
> Commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that covers
> the framebuffer") attempted to do so by claiming the BAR resource early
> on, which prevents the PCI resource allocation routines from changing it.
> However, it turns out that this only works if the PCI device is not
> behind any bridges, since the bridge resources need to be claimed first.
>
> So instead, allow the BAR to be moved, but make the efifb driver deal
> with that gracefully. So record the resource that covers the BAR early
> on, and if it turns out to have moved by the time we probe the efifb
> driver, update the framebuffer address accordingly.
>
> While this is less likely to occur on x86, given that the firmware's
> PCI resource allocation is more likely to be preserved, this is a
> worthwhile sanity check to have in place, and so let's remove the
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: - use pr_info() not pr_warn() for non-error condition
Well, that settled all of my concerns:
Signed-off-by: Peter Jones <pjones@redhat.com>
>
> drivers/video/fbdev/efifb.c | 24 ++++++++++++--------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ff01bed7112f..0dd7e5eb051f 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -146,6 +146,9 @@ ATTRIBUTE_GROUPS(efifb);
>
> static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
>
> +static struct resource *bar_resource;
> +static u64 bar_offset;
> +
> static int efifb_probe(struct platform_device *dev)
> {
> struct fb_info *info;
> @@ -200,6 +203,13 @@ static int efifb_probe(struct platform_device *dev)
> efifb_fix.smem_start |= ext_lfb_base;
> }
>
> + if (bar_resource &&
> + bar_resource->start + bar_offset != efifb_fix.smem_start) {
> +
> + pr_info("efifb: PCI BAR has moved, updating fb address\n");
> + efifb_fix.smem_start = bar_resource->start + bar_offset;
> + }
> +
> efifb_defined.bits_per_pixel = screen_info.lfb_depth;
> efifb_defined.xres = screen_info.lfb_width;
> efifb_defined.yres = screen_info.lfb_height;
> @@ -364,11 +374,11 @@ static struct platform_driver efifb_driver = {
>
> builtin_platform_driver(efifb_driver);
>
> -#if defined(CONFIG_PCI) && !defined(CONFIG_X86)
> +#if defined(CONFIG_PCI)
>
> static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
>
> -static void claim_efifb_bar(struct pci_dev *dev, int idx)
> +static void record_efifb_bar_resource(struct pci_dev *dev, int idx, u64 offset)
> {
> u16 word;
>
> @@ -383,12 +393,8 @@ static void claim_efifb_bar(struct pci_dev *dev, int idx)
> return;
> }
>
> - if (pci_claim_resource(dev, idx)) {
> - pci_dev_disabled = true;
> - dev_err(&dev->dev,
> - "BAR %d: failed to claim resource for efifb!\n", idx);
> - return;
> - }
> + bar_resource = &dev->resource[idx];
> + bar_offset = offset;
>
> dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
> }
> @@ -415,7 +421,7 @@ static void efifb_fixup_resources(struct pci_dev *dev)
> continue;
>
> if (res->start <= base && res->end >= base + size - 1) {
> - claim_efifb_bar(dev, i);
> + record_efifb_bar_resource(dev, i, base - res->start);
> break;
> }
> }
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Peter
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it
@ 2017-07-11 11:55 ` Peter Jones
0 siblings, 0 replies; 18+ messages in thread
From: Peter Jones @ 2017-07-11 11:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-pci, bhelgaas, linux-fbdev, linux-efi, matt, b.zolnierkie,
lorenzo.pieralisi
On Mon, Jul 10, 2017 at 10:13:05PM +0100, Ard Biesheuvel wrote:
> On UEFI systems, the firmware may expose a Graphics Output Protocol (GOP)
> instance to which the efifb driver attempts to attach in order to provide
> a minimal, unaccelerated framebuffer. The GOP protocol itself is not very
> sophisticated, and only describes the offset and size of the framebuffer
> in memory, and the pixel format.
>
> If the GOP framebuffer is provided by a PCI device, it will have been
> configured and enabled by the UEFI firmware, and the GOP protocol will
> simply point into a live BAR region. However, the GOP protocol itself does
> not describe this relation, and so we have to take care not to reconfigure
> the BAR without taking efifb's dependency on it into account.
>
> Commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that covers
> the framebuffer") attempted to do so by claiming the BAR resource early
> on, which prevents the PCI resource allocation routines from changing it.
> However, it turns out that this only works if the PCI device is not
> behind any bridges, since the bridge resources need to be claimed first.
>
> So instead, allow the BAR to be moved, but make the efifb driver deal
> with that gracefully. So record the resource that covers the BAR early
> on, and if it turns out to have moved by the time we probe the efifb
> driver, update the framebuffer address accordingly.
>
> While this is less likely to occur on x86, given that the firmware's
> PCI resource allocation is more likely to be preserved, this is a
> worthwhile sanity check to have in place, and so let's remove the
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: - use pr_info() not pr_warn() for non-error condition
Well, that settled all of my concerns:
Signed-off-by: Peter Jones <pjones@redhat.com>
>
> drivers/video/fbdev/efifb.c | 24 ++++++++++++--------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ff01bed7112f..0dd7e5eb051f 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -146,6 +146,9 @@ ATTRIBUTE_GROUPS(efifb);
>
> static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
>
> +static struct resource *bar_resource;
> +static u64 bar_offset;
> +
> static int efifb_probe(struct platform_device *dev)
> {
> struct fb_info *info;
> @@ -200,6 +203,13 @@ static int efifb_probe(struct platform_device *dev)
> efifb_fix.smem_start |= ext_lfb_base;
> }
>
> + if (bar_resource &&
> + bar_resource->start + bar_offset != efifb_fix.smem_start) {
> +
> + pr_info("efifb: PCI BAR has moved, updating fb address\n");
> + efifb_fix.smem_start = bar_resource->start + bar_offset;
> + }
> +
> efifb_defined.bits_per_pixel = screen_info.lfb_depth;
> efifb_defined.xres = screen_info.lfb_width;
> efifb_defined.yres = screen_info.lfb_height;
> @@ -364,11 +374,11 @@ static struct platform_driver efifb_driver = {
>
> builtin_platform_driver(efifb_driver);
>
> -#if defined(CONFIG_PCI) && !defined(CONFIG_X86)
> +#if defined(CONFIG_PCI)
>
> static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
>
> -static void claim_efifb_bar(struct pci_dev *dev, int idx)
> +static void record_efifb_bar_resource(struct pci_dev *dev, int idx, u64 offset)
> {
> u16 word;
>
> @@ -383,12 +393,8 @@ static void claim_efifb_bar(struct pci_dev *dev, int idx)
> return;
> }
>
> - if (pci_claim_resource(dev, idx)) {
> - pci_dev_disabled = true;
> - dev_err(&dev->dev,
> - "BAR %d: failed to claim resource for efifb!\n", idx);
> - return;
> - }
> + bar_resource = &dev->resource[idx];
> + bar_offset = offset;
>
> dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
> }
> @@ -415,7 +421,7 @@ static void efifb_fixup_resources(struct pci_dev *dev)
> continue;
>
> if (res->start <= base && res->end >= base + size - 1) {
> - claim_efifb_bar(dev, i);
> + record_efifb_bar_resource(dev, i, base - res->start);
> break;
> }
> }
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Peter
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it
2017-07-11 11:55 ` Peter Jones
@ 2017-07-11 12:12 ` Ard Biesheuvel
-1 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2017-07-11 12:12 UTC (permalink / raw)
To: Peter Jones
Cc: linux-pci, Bjorn Helgaas, linux-fbdev, linux-efi, Matt Fleming,
Bartlomiej Zolnierkiewicz, Lorenzo Pieralisi
On 11 July 2017 at 12:55, Peter Jones <pjones@redhat.com> wrote:
> On Mon, Jul 10, 2017 at 10:13:05PM +0100, Ard Biesheuvel wrote:
>> On UEFI systems, the firmware may expose a Graphics Output Protocol (GOP)
>> instance to which the efifb driver attempts to attach in order to provide
>> a minimal, unaccelerated framebuffer. The GOP protocol itself is not very
>> sophisticated, and only describes the offset and size of the framebuffer
>> in memory, and the pixel format.
>>
>> If the GOP framebuffer is provided by a PCI device, it will have been
>> configured and enabled by the UEFI firmware, and the GOP protocol will
>> simply point into a live BAR region. However, the GOP protocol itself does
>> not describe this relation, and so we have to take care not to reconfigure
>> the BAR without taking efifb's dependency on it into account.
>>
>> Commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that covers
>> the framebuffer") attempted to do so by claiming the BAR resource early
>> on, which prevents the PCI resource allocation routines from changing it.
>> However, it turns out that this only works if the PCI device is not
>> behind any bridges, since the bridge resources need to be claimed first.
>>
>> So instead, allow the BAR to be moved, but make the efifb driver deal
>> with that gracefully. So record the resource that covers the BAR early
>> on, and if it turns out to have moved by the time we probe the efifb
>> driver, update the framebuffer address accordingly.
>>
>> While this is less likely to occur on x86, given that the firmware's
>> PCI resource allocation is more likely to be preserved, this is a
>> worthwhile sanity check to have in place, and so let's remove the
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> v2: - use pr_info() not pr_warn() for non-error condition
>
> Well, that settled all of my concerns:
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
>
Thanks Peter. I will take that as a Reviewed-by
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it
@ 2017-07-11 12:12 ` Ard Biesheuvel
0 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2017-07-11 12:12 UTC (permalink / raw)
To: Peter Jones
Cc: linux-pci, Bjorn Helgaas, linux-fbdev, linux-efi, Matt Fleming,
Bartlomiej Zolnierkiewicz, Lorenzo Pieralisi
On 11 July 2017 at 12:55, Peter Jones <pjones@redhat.com> wrote:
> On Mon, Jul 10, 2017 at 10:13:05PM +0100, Ard Biesheuvel wrote:
>> On UEFI systems, the firmware may expose a Graphics Output Protocol (GOP)
>> instance to which the efifb driver attempts to attach in order to provide
>> a minimal, unaccelerated framebuffer. The GOP protocol itself is not very
>> sophisticated, and only describes the offset and size of the framebuffer
>> in memory, and the pixel format.
>>
>> If the GOP framebuffer is provided by a PCI device, it will have been
>> configured and enabled by the UEFI firmware, and the GOP protocol will
>> simply point into a live BAR region. However, the GOP protocol itself does
>> not describe this relation, and so we have to take care not to reconfigure
>> the BAR without taking efifb's dependency on it into account.
>>
>> Commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that covers
>> the framebuffer") attempted to do so by claiming the BAR resource early
>> on, which prevents the PCI resource allocation routines from changing it.
>> However, it turns out that this only works if the PCI device is not
>> behind any bridges, since the bridge resources need to be claimed first.
>>
>> So instead, allow the BAR to be moved, but make the efifb driver deal
>> with that gracefully. So record the resource that covers the BAR early
>> on, and if it turns out to have moved by the time we probe the efifb
>> driver, update the framebuffer address accordingly.
>>
>> While this is less likely to occur on x86, given that the firmware's
>> PCI resource allocation is more likely to be preserved, this is a
>> worthwhile sanity check to have in place, and so let's remove the
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> v2: - use pr_info() not pr_warn() for non-error condition
>
> Well, that settled all of my concerns:
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
>
Thanks Peter. I will take that as a Reviewed-by
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it
2017-07-10 21:13 ` Ard Biesheuvel
(?)
@ 2017-07-12 19:54 ` Bjorn Helgaas
-1 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2017-07-12 19:54 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-pci-u79uwXL29TY76Z2rM5mHXA,
bhelgaas-hpIqsD4AKlfQT0dZR+AlfA, pjones-H+wXaHxf7aLQT0dZR+AlfA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
matt-mF/unelCI9GS6iBeEJttW/XRex20P6io,
b.zolnierkie-Sze3O3UU22JBDgjK7y7TUQ,
lorenzo.pieralisi-5wv7dgnIgG8
On Mon, Jul 10, 2017 at 10:13:05PM +0100, Ard Biesheuvel wrote:
> On UEFI systems, the firmware may expose a Graphics Output Protocol (GOP)
> instance to which the efifb driver attempts to attach in order to provide
> a minimal, unaccelerated framebuffer. The GOP protocol itself is not very
> sophisticated, and only describes the offset and size of the framebuffer
> in memory, and the pixel format.
>
> If the GOP framebuffer is provided by a PCI device, it will have been
> configured and enabled by the UEFI firmware, and the GOP protocol will
> simply point into a live BAR region. However, the GOP protocol itself does
> not describe this relation, and so we have to take care not to reconfigure
> the BAR without taking efifb's dependency on it into account.
>
> Commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that covers
> the framebuffer") attempted to do so by claiming the BAR resource early
> on, which prevents the PCI resource allocation routines from changing it.
> However, it turns out that this only works if the PCI device is not
> behind any bridges, since the bridge resources need to be claimed first.
>
> So instead, allow the BAR to be moved, but make the efifb driver deal
> with that gracefully. So record the resource that covers the BAR early
> on, and if it turns out to have moved by the time we probe the efifb
> driver, update the framebuffer address accordingly.
>
> While this is less likely to occur on x86, given that the firmware's
> PCI resource allocation is more likely to be preserved, this is a
> worthwhile sanity check to have in place, and so let's remove the
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
I like this a lot!
Acked-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Would you be willing to save the pci_dev as well, so the pr_info() could be
made a dev_info()?
Or maybe, since I notice everything else from efifb_probe() is a pr_info()
(although we do get a platform_device), it would be better to use a
pr_info() but just include the related PCI device by using pci_name()?
> ---
> v2: - use pr_info() not pr_warn() for non-error condition
>
> drivers/video/fbdev/efifb.c | 24 ++++++++++++--------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ff01bed7112f..0dd7e5eb051f 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -146,6 +146,9 @@ ATTRIBUTE_GROUPS(efifb);
>
> static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
>
> +static struct resource *bar_resource;
> +static u64 bar_offset;
> +
> static int efifb_probe(struct platform_device *dev)
> {
> struct fb_info *info;
> @@ -200,6 +203,13 @@ static int efifb_probe(struct platform_device *dev)
> efifb_fix.smem_start |= ext_lfb_base;
> }
>
> + if (bar_resource &&
> + bar_resource->start + bar_offset != efifb_fix.smem_start) {
> +
> + pr_info("efifb: PCI BAR has moved, updating fb address\n");
> + efifb_fix.smem_start = bar_resource->start + bar_offset;
> + }
> +
> efifb_defined.bits_per_pixel = screen_info.lfb_depth;
> efifb_defined.xres = screen_info.lfb_width;
> efifb_defined.yres = screen_info.lfb_height;
> @@ -364,11 +374,11 @@ static struct platform_driver efifb_driver = {
>
> builtin_platform_driver(efifb_driver);
>
> -#if defined(CONFIG_PCI) && !defined(CONFIG_X86)
> +#if defined(CONFIG_PCI)
>
> static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
>
> -static void claim_efifb_bar(struct pci_dev *dev, int idx)
> +static void record_efifb_bar_resource(struct pci_dev *dev, int idx, u64 offset)
> {
> u16 word;
>
> @@ -383,12 +393,8 @@ static void claim_efifb_bar(struct pci_dev *dev, int idx)
> return;
> }
>
> - if (pci_claim_resource(dev, idx)) {
> - pci_dev_disabled = true;
> - dev_err(&dev->dev,
> - "BAR %d: failed to claim resource for efifb!\n", idx);
> - return;
> - }
> + bar_resource = &dev->resource[idx];
> + bar_offset = offset;
>
> dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
> }
> @@ -415,7 +421,7 @@ static void efifb_fixup_resources(struct pci_dev *dev)
> continue;
>
> if (res->start <= base && res->end >= base + size - 1) {
> - claim_efifb_bar(dev, i);
> + record_efifb_bar_resource(dev, i, base - res->start);
> break;
> }
> }
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it
@ 2017-07-12 19:54 ` Bjorn Helgaas
0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2017-07-12 19:54 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-pci-u79uwXL29TY76Z2rM5mHXA,
bhelgaas-hpIqsD4AKlfQT0dZR+AlfA, pjones-H+wXaHxf7aLQT0dZR+AlfA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA,
matt-mF/unelCI9GS6iBeEJttW/XRex20P6io,
b.zolnierkie-Sze3O3UU22JBDgjK7y7TUQ,
lorenzo.pieralisi-5wv7dgnIgG8
On Mon, Jul 10, 2017 at 10:13:05PM +0100, Ard Biesheuvel wrote:
> On UEFI systems, the firmware may expose a Graphics Output Protocol (GOP)
> instance to which the efifb driver attempts to attach in order to provide
> a minimal, unaccelerated framebuffer. The GOP protocol itself is not very
> sophisticated, and only describes the offset and size of the framebuffer
> in memory, and the pixel format.
>
> If the GOP framebuffer is provided by a PCI device, it will have been
> configured and enabled by the UEFI firmware, and the GOP protocol will
> simply point into a live BAR region. However, the GOP protocol itself does
> not describe this relation, and so we have to take care not to reconfigure
> the BAR without taking efifb's dependency on it into account.
>
> Commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that covers
> the framebuffer") attempted to do so by claiming the BAR resource early
> on, which prevents the PCI resource allocation routines from changing it.
> However, it turns out that this only works if the PCI device is not
> behind any bridges, since the bridge resources need to be claimed first.
>
> So instead, allow the BAR to be moved, but make the efifb driver deal
> with that gracefully. So record the resource that covers the BAR early
> on, and if it turns out to have moved by the time we probe the efifb
> driver, update the framebuffer address accordingly.
>
> While this is less likely to occur on x86, given that the firmware's
> PCI resource allocation is more likely to be preserved, this is a
> worthwhile sanity check to have in place, and so let's remove the
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
I like this a lot!
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Would you be willing to save the pci_dev as well, so the pr_info() could be
made a dev_info()?
Or maybe, since I notice everything else from efifb_probe() is a pr_info()
(although we do get a platform_device), it would be better to use a
pr_info() but just include the related PCI device by using pci_name()?
> ---
> v2: - use pr_info() not pr_warn() for non-error condition
>
> drivers/video/fbdev/efifb.c | 24 ++++++++++++--------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ff01bed7112f..0dd7e5eb051f 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -146,6 +146,9 @@ ATTRIBUTE_GROUPS(efifb);
>
> static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
>
> +static struct resource *bar_resource;
> +static u64 bar_offset;
> +
> static int efifb_probe(struct platform_device *dev)
> {
> struct fb_info *info;
> @@ -200,6 +203,13 @@ static int efifb_probe(struct platform_device *dev)
> efifb_fix.smem_start |= ext_lfb_base;
> }
>
> + if (bar_resource &&
> + bar_resource->start + bar_offset != efifb_fix.smem_start) {
> +
> + pr_info("efifb: PCI BAR has moved, updating fb address\n");
> + efifb_fix.smem_start = bar_resource->start + bar_offset;
> + }
> +
> efifb_defined.bits_per_pixel = screen_info.lfb_depth;
> efifb_defined.xres = screen_info.lfb_width;
> efifb_defined.yres = screen_info.lfb_height;
> @@ -364,11 +374,11 @@ static struct platform_driver efifb_driver = {
>
> builtin_platform_driver(efifb_driver);
>
> -#if defined(CONFIG_PCI) && !defined(CONFIG_X86)
> +#if defined(CONFIG_PCI)
>
> static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
>
> -static void claim_efifb_bar(struct pci_dev *dev, int idx)
> +static void record_efifb_bar_resource(struct pci_dev *dev, int idx, u64 offset)
> {
> u16 word;
>
> @@ -383,12 +393,8 @@ static void claim_efifb_bar(struct pci_dev *dev, int idx)
> return;
> }
>
> - if (pci_claim_resource(dev, idx)) {
> - pci_dev_disabled = true;
> - dev_err(&dev->dev,
> - "BAR %d: failed to claim resource for efifb!\n", idx);
> - return;
> - }
> + bar_resource = &dev->resource[idx];
> + bar_offset = offset;
>
> dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
> }
> @@ -415,7 +421,7 @@ static void efifb_fixup_resources(struct pci_dev *dev)
> continue;
>
> if (res->start <= base && res->end >= base + size - 1) {
> - claim_efifb_bar(dev, i);
> + record_efifb_bar_resource(dev, i, base - res->start);
> break;
> }
> }
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it
@ 2017-07-12 19:54 ` Bjorn Helgaas
0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2017-07-12 19:54 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-pci, bhelgaas, pjones, linux-fbdev, linux-efi, matt,
b.zolnierkie, lorenzo.pieralisi
On Mon, Jul 10, 2017 at 10:13:05PM +0100, Ard Biesheuvel wrote:
> On UEFI systems, the firmware may expose a Graphics Output Protocol (GOP)
> instance to which the efifb driver attempts to attach in order to provide
> a minimal, unaccelerated framebuffer. The GOP protocol itself is not very
> sophisticated, and only describes the offset and size of the framebuffer
> in memory, and the pixel format.
>
> If the GOP framebuffer is provided by a PCI device, it will have been
> configured and enabled by the UEFI firmware, and the GOP protocol will
> simply point into a live BAR region. However, the GOP protocol itself does
> not describe this relation, and so we have to take care not to reconfigure
> the BAR without taking efifb's dependency on it into account.
>
> Commit 55d728a40d36 ("efi/fb: Avoid reconfiguration of BAR that covers
> the framebuffer") attempted to do so by claiming the BAR resource early
> on, which prevents the PCI resource allocation routines from changing it.
> However, it turns out that this only works if the PCI device is not
> behind any bridges, since the bridge resources need to be claimed first.
>
> So instead, allow the BAR to be moved, but make the efifb driver deal
> with that gracefully. So record the resource that covers the BAR early
> on, and if it turns out to have moved by the time we probe the efifb
> driver, update the framebuffer address accordingly.
>
> While this is less likely to occur on x86, given that the firmware's
> PCI resource allocation is more likely to be preserved, this is a
> worthwhile sanity check to have in place, and so let's remove the
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
I like this a lot!
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Would you be willing to save the pci_dev as well, so the pr_info() could be
made a dev_info()?
Or maybe, since I notice everything else from efifb_probe() is a pr_info()
(although we do get a platform_device), it would be better to use a
pr_info() but just include the related PCI device by using pci_name()?
> ---
> v2: - use pr_info() not pr_warn() for non-error condition
>
> drivers/video/fbdev/efifb.c | 24 ++++++++++++--------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index ff01bed7112f..0dd7e5eb051f 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -146,6 +146,9 @@ ATTRIBUTE_GROUPS(efifb);
>
> static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */
>
> +static struct resource *bar_resource;
> +static u64 bar_offset;
> +
> static int efifb_probe(struct platform_device *dev)
> {
> struct fb_info *info;
> @@ -200,6 +203,13 @@ static int efifb_probe(struct platform_device *dev)
> efifb_fix.smem_start |= ext_lfb_base;
> }
>
> + if (bar_resource &&
> + bar_resource->start + bar_offset != efifb_fix.smem_start) {
> +
> + pr_info("efifb: PCI BAR has moved, updating fb address\n");
> + efifb_fix.smem_start = bar_resource->start + bar_offset;
> + }
> +
> efifb_defined.bits_per_pixel = screen_info.lfb_depth;
> efifb_defined.xres = screen_info.lfb_width;
> efifb_defined.yres = screen_info.lfb_height;
> @@ -364,11 +374,11 @@ static struct platform_driver efifb_driver = {
>
> builtin_platform_driver(efifb_driver);
>
> -#if defined(CONFIG_PCI) && !defined(CONFIG_X86)
> +#if defined(CONFIG_PCI)
>
> static bool pci_bar_found; /* did we find a BAR matching the efifb base? */
>
> -static void claim_efifb_bar(struct pci_dev *dev, int idx)
> +static void record_efifb_bar_resource(struct pci_dev *dev, int idx, u64 offset)
> {
> u16 word;
>
> @@ -383,12 +393,8 @@ static void claim_efifb_bar(struct pci_dev *dev, int idx)
> return;
> }
>
> - if (pci_claim_resource(dev, idx)) {
> - pci_dev_disabled = true;
> - dev_err(&dev->dev,
> - "BAR %d: failed to claim resource for efifb!\n", idx);
> - return;
> - }
> + bar_resource = &dev->resource[idx];
> + bar_offset = offset;
>
> dev_info(&dev->dev, "BAR %d: assigned to efifb\n", idx);
> }
> @@ -415,7 +421,7 @@ static void efifb_fixup_resources(struct pci_dev *dev)
> continue;
>
> if (res->start <= base && res->end >= base + size - 1) {
> - claim_efifb_bar(dev, i);
> + record_efifb_bar_resource(dev, i, base - res->start);
> break;
> }
> }
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread