Hi Kai-Heng, Alex,
could you add your ack if the fix looks ok and you're ok if I push it to
the i915 tree?
Thanks,
Imre
On Mon, Aug 09, 2021 at 04:31:46PM +0300, Imre Deak wrote:
> Atm the EFI FB platform driver gets a runtime PM reference for the
> associated GFX PCI device during probing the EFI FB platform device and
> releases it only when the platform device gets unbound.
>
> When fbcon switches to the FB provided by the PCI device's driver (for
> instance i915/drmfb), the EFI FB will get only unregistered without the
> EFI FB platform device getting unbound, keeping the runtime PM reference
> acquired during the platform device probing. This reference will prevent
> the PCI driver from runtime suspending the device.
>
> Fix this by releasing the RPM reference from the EFI FB's destroy hook,
> called when the FB gets unregistered.
>
> While at it assert that pm_runtime_get_sync() didn't fail.
>
> v2:
> - Move pm_runtime_get_sync() before register_framebuffer() to avoid its
> race wrt. efifb_destroy()->pm_runtime_put(). (Daniel)
> - Assert that pm_runtime_get_sync() didn't fail.
> - Clarify commit message wrt. platform/PCI device/driver and driver
> removal vs. device unbinding.
>
> Fixes: a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI
> D0")
> Cc: Kai-Heng Feng <kai.heng.f...@canonical.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch> (v1)
> Signed-off-by: Imre Deak <imre.d...@intel.com>
> ---
> drivers/video/fbdev/efifb.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 8ea8f079cde26..edca3703b9640 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -47,6 +47,8 @@ static bool use_bgrt = true;
> static bool request_mem_succeeded = false;
> static u64 mem_flags = EFI_MEMORY_WC | EFI_MEMORY_UC;
>
> +static struct pci_dev *efifb_pci_dev; /* dev with BAR covering the
> efifb */
> +
> static struct fb_var_screeninfo efifb_defined = {
> .activate = FB_ACTIVATE_NOW,
> .height = -1,
> @@ -243,6 +245,9 @@ static inline void efifb_show_boot_graphics(struct
> fb_info *info) {}
>
> static void efifb_destroy(struct fb_info *info)
> {
> + if (efifb_pci_dev)
> + pm_runtime_put(&efifb_pci_dev->dev);
> +
> if (info->screen_base) {
> if (mem_flags & (EFI_MEMORY_UC | EFI_MEMORY_WC))
> iounmap(info->screen_base);
> @@ -333,7 +338,6 @@ 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;
>
> @@ -569,17 +573,22 @@ static int efifb_probe(struct platform_device *dev)
> pr_err("efifb: cannot allocate colormap\n");
> goto err_groups;
> }
> +
> + if (efifb_pci_dev)
> + WARN_ON(pm_runtime_get_sync(&efifb_pci_dev->dev) < 0);
> +
> err = register_framebuffer(info);
> if (err < 0) {
> pr_err("efifb: cannot register framebuffer\n");
> - goto err_fb_dealoc;
> + goto err_put_rpm_ref;
> }
> fb_info(info, "%s frame buffer device\n", info->fix.id);
> - if (efifb_pci_dev)
> - pm_runtime_get_sync(&efifb_pci_dev->dev);
> return 0;
>
> -err_fb_dealoc:
> +err_put_rpm_ref:
> + if (efifb_pci_dev)
> + pm_runtime_put(&efifb_pci_dev->dev);
> +
> fb_dealloc_cmap(&info->cmap);
> err_groups:
> sysfs_remove_groups(&dev->dev.kobj, efifb_groups);
> @@ -603,8 +612,6 @@ static int efifb_remove(struct platform_device *pdev)
> unregister_framebuffer(info);
> sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
> framebuffer_release(info);
> - if (efifb_pci_dev)
> - pm_runtime_put(&efifb_pci_dev->dev);
>
> return 0;
> }
> --
> 2.27.0
>
