ACK: [PATCH][SRU Artful] drivers/fbdev/efifb: Allow BAR to be moved instead of claiming it

[Stefan Bader]

On 23.03.2018 21:06, dann frazier wrote:
> From: Ard Biesheuvel <ard.biesheuvel at 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 at linaro.org>
> Reviewed-by: Peter Jones <pjones at redhat.com>
> Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie at samsung.com>
> Acked-by: Bjorn Helgaas <bhelgaas at google.com>
> Cc: Linus Torvalds <torvalds at linux-foundation.org>
> Cc: Matt Fleming <matt at codeblueprint.co.uk>
> Cc: Peter Zijlstra <peterz at infradead.org>
> Cc: Thomas Gleixner <tglx at linutronix.de>
> Cc: linux-efi at vger.kernel.org
> Link: http://lkml.kernel.org/r/20170818194947.19347-8-ard.biesheuvel@linaro.org
> Signed-off-by: Ingo Molnar <mingo at kernel.org>
> (cherry picked from commit dcf8f5ce31656534efada252f6a563c09b295983)
> Signed-off-by: dann frazier <dann.frazier at canonical.com>
Acked-by: Stefan Bader <stefan.bader at 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;
>  		}
>  	}
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180328/f4993ae4/attachment.sig>