[PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Ard Biesheuvel]

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

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

[PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Ard Biesheuvel]

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

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

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[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
>

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[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
>

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[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
>

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Peter Jones]

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

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Peter Jones]

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

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Peter Jones]

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

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Ard Biesheuvel]

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

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Ard Biesheuvel]

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

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Bartlomiej Zolnierkiewicz]

On Monday, July 10, 2017 10:13:05 PM 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

It still cuts the patch description early..

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>From fbdev's side:

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

> ---
> v2: - use pr_info() not pr_warn() for non-error condition

Best regards,

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Bartlomiej Zolnierkiewicz]

On Monday, July 10, 2017 10:13:05 PM 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

It still cuts the patch description early..

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>From fbdev's side:

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

> ---
> v2: - use pr_info() not pr_warn() for non-error condition

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Bartlomiej Zolnierkiewicz]

On Monday, July 10, 2017 10:13:05 PM 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

It still cuts the patch description early..

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

From fbdev's side:

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

> ---
> v2: - use pr_info() not pr_warn() for non-error condition

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Ard Biesheuvel]

On 12 July 2017 at 11:00, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
> On Monday, July 10, 2017 10:13:05 PM 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
>
> It still cuts the patch description early..
>

Ah yes, thanks for pointing that out. I will fix it up before queuing the patch.

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> From fbdev's side:
>
> Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>

Cheers.

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Ard Biesheuvel]

On 12 July 2017 at 11:00, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
> On Monday, July 10, 2017 10:13:05 PM 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
>
> It still cuts the patch description early..
>

Ah yes, thanks for pointing that out. I will fix it up before queuing the patch.

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> From fbdev's side:
>
> Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>

Cheers.

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Bjorn Helgaas]

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
> 

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Bjorn Helgaas]

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
> 

Re: [PATCH v2] drivers/fbdev: efifb: allow BAR to be moved instead of claiming it

[Bjorn Helgaas]

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
>