Message ID | 20180323200613.GA2969@xps13.dannf |
---|---|
State | New |
Headers | show |
Series | [SRU,Artful] drivers/fbdev/efifb: Allow BAR to be moved instead of claiming it | expand |
On 23.03.2018 21:06, dann frazier wrote: > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > BugLink: https://bugs.launchpad.net/bugs/1758375 > > 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 > preprocessor conditional that makes it !X86 only. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Reviewed-by: Peter Jones <pjones@redhat.com> > Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Matt Fleming <matt@codeblueprint.co.uk> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: linux-efi@vger.kernel.org > Link: http://lkml.kernel.org/r/20170818194947.19347-8-ard.biesheuvel@linaro.org > Signed-off-by: Ingo Molnar <mingo@kernel.org> > (cherry picked from commit dcf8f5ce31656534efada252f6a563c09b295983) > Signed-off-by: dann frazier <dann.frazier@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > drivers/video/fbdev/efifb.c | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c > index 1e784adb89b1..3a010641f630 100644 > --- a/drivers/video/fbdev/efifb.c > +++ b/drivers/video/fbdev/efifb.c > @@ -149,6 +149,10 @@ ATTRIBUTE_GROUPS(efifb); > > static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */ > > +static struct pci_dev *efifb_pci_dev; /* dev with BAR covering the efifb */ > +static struct resource *bar_resource; > +static u64 bar_offset; > + > static int efifb_probe(struct platform_device *dev) > { > struct fb_info *info; > @@ -203,6 +207,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) { > + dev_info(&efifb_pci_dev->dev, > + "BAR has moved, updating efifb 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; > @@ -370,15 +381,13 @@ static struct platform_driver efifb_driver = { > > builtin_platform_driver(efifb_driver); > > -#if defined(CONFIG_PCI) && !defined(CONFIG_X86) > - > -static bool pci_bar_found; /* did we find a BAR matching the efifb base? */ > +#if defined(CONFIG_PCI) > > -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; > > - pci_bar_found = true; > + efifb_pci_dev = dev; > > pci_read_config_word(dev, PCI_COMMAND, &word); > if (!(word & PCI_COMMAND_MEMORY)) { > @@ -389,12 +398,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); > } > @@ -405,7 +410,7 @@ static void efifb_fixup_resources(struct pci_dev *dev) > u64 size = screen_info.lfb_size; > int i; > > - if (pci_bar_found || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) > + if (efifb_pci_dev || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) > return; > > if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > @@ -421,7 +426,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; > } > } >
On 03/23/18 21:06, dann frazier wrote: > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > BugLink: https://bugs.launchpad.net/bugs/1758375 > > 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 > preprocessor conditional that makes it !X86 only. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Reviewed-by: Peter Jones <pjones@redhat.com> > Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Matt Fleming <matt@codeblueprint.co.uk> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: linux-efi@vger.kernel.org > Link: http://lkml.kernel.org/r/20170818194947.19347-8-ard.biesheuvel@linaro.org > Signed-off-by: Ingo Molnar <mingo@kernel.org> > (cherry picked from commit dcf8f5ce31656534efada252f6a563c09b295983) > Signed-off-by: dann frazier <dann.frazier@canonical.com> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > drivers/video/fbdev/efifb.c | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c > index 1e784adb89b1..3a010641f630 100644 > --- a/drivers/video/fbdev/efifb.c > +++ b/drivers/video/fbdev/efifb.c > @@ -149,6 +149,10 @@ ATTRIBUTE_GROUPS(efifb); > > static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */ > > +static struct pci_dev *efifb_pci_dev; /* dev with BAR covering the efifb */ > +static struct resource *bar_resource; > +static u64 bar_offset; > + > static int efifb_probe(struct platform_device *dev) > { > struct fb_info *info; > @@ -203,6 +207,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) { > + dev_info(&efifb_pci_dev->dev, > + "BAR has moved, updating efifb 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; > @@ -370,15 +381,13 @@ static struct platform_driver efifb_driver = { > > builtin_platform_driver(efifb_driver); > > -#if defined(CONFIG_PCI) && !defined(CONFIG_X86) > - > -static bool pci_bar_found; /* did we find a BAR matching the efifb base? */ > +#if defined(CONFIG_PCI) > > -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; > > - pci_bar_found = true; > + efifb_pci_dev = dev; > > pci_read_config_word(dev, PCI_COMMAND, &word); > if (!(word & PCI_COMMAND_MEMORY)) { > @@ -389,12 +398,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); > } > @@ -405,7 +410,7 @@ static void efifb_fixup_resources(struct pci_dev *dev) > u64 size = screen_info.lfb_size; > int i; > > - if (pci_bar_found || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) > + if (efifb_pci_dev || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) > return; > > if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > @@ -421,7 +426,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; > } > } >
On 03/23/18 21:06, dann frazier wrote: > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > BugLink: https://bugs.launchpad.net/bugs/1758375 > > 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 > preprocessor conditional that makes it !X86 only. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Reviewed-by: Peter Jones <pjones@redhat.com> > Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Matt Fleming <matt@codeblueprint.co.uk> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: linux-efi@vger.kernel.org > Link: http://lkml.kernel.org/r/20170818194947.19347-8-ard.biesheuvel@linaro.org > Signed-off-by: Ingo Molnar <mingo@kernel.org> > (cherry picked from commit dcf8f5ce31656534efada252f6a563c09b295983) > Signed-off-by: dann frazier <dann.frazier@canonical.com> > --- > drivers/video/fbdev/efifb.c | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c > index 1e784adb89b1..3a010641f630 100644 > --- a/drivers/video/fbdev/efifb.c > +++ b/drivers/video/fbdev/efifb.c > @@ -149,6 +149,10 @@ ATTRIBUTE_GROUPS(efifb); > > static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */ > > +static struct pci_dev *efifb_pci_dev; /* dev with BAR covering the efifb */ > +static struct resource *bar_resource; > +static u64 bar_offset; > + > static int efifb_probe(struct platform_device *dev) > { > struct fb_info *info; > @@ -203,6 +207,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) { > + dev_info(&efifb_pci_dev->dev, > + "BAR has moved, updating efifb 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; > @@ -370,15 +381,13 @@ static struct platform_driver efifb_driver = { > > builtin_platform_driver(efifb_driver); > > -#if defined(CONFIG_PCI) && !defined(CONFIG_X86) > - > -static bool pci_bar_found; /* did we find a BAR matching the efifb base? */ > +#if defined(CONFIG_PCI) > > -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; > > - pci_bar_found = true; > + efifb_pci_dev = dev; > > pci_read_config_word(dev, PCI_COMMAND, &word); > if (!(word & PCI_COMMAND_MEMORY)) { > @@ -389,12 +398,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); > } > @@ -405,7 +410,7 @@ static void efifb_fixup_resources(struct pci_dev *dev) > u64 size = screen_info.lfb_size; > int i; > > - if (pci_bar_found || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) > + if (efifb_pci_dev || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) > return; > > if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) > @@ -421,7 +426,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; > } > } > Applied to artful/master-next branch. Thanks, Kleber
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index 1e784adb89b1..3a010641f630 100644 --- a/drivers/video/fbdev/efifb.c +++ b/drivers/video/fbdev/efifb.c @@ -149,6 +149,10 @@ ATTRIBUTE_GROUPS(efifb); static bool pci_dev_disabled; /* FB base matches BAR of a disabled device */ +static struct pci_dev *efifb_pci_dev; /* dev with BAR covering the efifb */ +static struct resource *bar_resource; +static u64 bar_offset; + static int efifb_probe(struct platform_device *dev) { struct fb_info *info; @@ -203,6 +207,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) { + dev_info(&efifb_pci_dev->dev, + "BAR has moved, updating efifb 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; @@ -370,15 +381,13 @@ static struct platform_driver efifb_driver = { builtin_platform_driver(efifb_driver); -#if defined(CONFIG_PCI) && !defined(CONFIG_X86) - -static bool pci_bar_found; /* did we find a BAR matching the efifb base? */ +#if defined(CONFIG_PCI) -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; - pci_bar_found = true; + efifb_pci_dev = dev; pci_read_config_word(dev, PCI_COMMAND, &word); if (!(word & PCI_COMMAND_MEMORY)) { @@ -389,12 +398,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); } @@ -405,7 +410,7 @@ static void efifb_fixup_resources(struct pci_dev *dev) u64 size = screen_info.lfb_size; int i; - if (pci_bar_found || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) + if (efifb_pci_dev || screen_info.orig_video_isVGA != VIDEO_TYPE_EFI) return; if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE) @@ -421,7 +426,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; } }