Atm the EFI FB driver gets a runtime PM reference for the associated GFX PCI device during driver probing and releases it only when removing the driver.
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 driver getting unloaded, keeping the runtime PM reference acquired during driver 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.
Fixes: a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI D0") Cc: Kai-Heng Feng kai.heng.feng@canonical.com Signed-off-by: Imre Deak imre.deak@intel.com --- drivers/video/fbdev/efifb.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index 8ea8f079cde26..25cdea32b9633 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;
@@ -603,8 +607,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; }
On Mon, Aug 02, 2021 at 04:35:51PM +0300, Imre Deak wrote:
Atm the EFI FB driver gets a runtime PM reference for the associated GFX PCI device during driver probing and releases it only when removing the driver.
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 driver getting unloaded, keeping the runtime PM reference acquired during driver 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.
Fixes: a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI D0") Cc: Kai-Heng Feng kai.heng.feng@canonical.com Signed-off-by: Imre Deak imre.deak@intel.com
Patch looks good:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
But I've found a bunch of ordering issues here: - we should probably get the runtime pm reference _before_ we register the framebuffer. There's a race right now about there. - the sysfs_remove_groups and framebuffer_release should also be moved into the destroy callback. This is more a leak type of situation.
Cheers, Daniel
drivers/video/fbdev/efifb.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index 8ea8f079cde26..25cdea32b9633 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;
@@ -603,8 +607,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
On Thu, Aug 05, 2021 at 12:23:21AM +0200, Daniel Vetter wrote:
On Mon, Aug 02, 2021 at 04:35:51PM +0300, Imre Deak wrote:
Atm the EFI FB driver gets a runtime PM reference for the associated GFX PCI device during driver probing and releases it only when removing the driver.
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 driver getting unloaded, keeping the runtime PM reference acquired during driver 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.
Fixes: a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI D0") Cc: Kai-Heng Feng kai.heng.feng@canonical.com Signed-off-by: Imre Deak imre.deak@intel.com
Patch looks good:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
But I've found a bunch of ordering issues here:
- we should probably get the runtime pm reference _before_ we register the framebuffer. There's a race right now about there.
Yea, missed this will send a v2 moving it earlier.
- the sysfs_remove_groups and framebuffer_release should also be moved into the destroy callback. This is more a leak type of situation.
Those sysfs entries belong to the efifb platform device, showing the bootup screen_info.lfb_* info, not related to fb_info, so imo efifb_remove() is the correct place to remove those. But yes, freeing fb_info seems to belong to fb_destroy().
Atm, things will blow up when unbinding the efifb device after the efifb framebuffer was unregistered while removing it as a conflicting FB (since unregister_framebuffer() will be called twice), so that would need to be solved as well. Maybe remove_conflicting_pci_framebuffers() could unregister the platform device instead of only unregistering the framebuffer, similarly to drm_aperture_detach_firmware(), but haven't checked this in more detail.
Cheers, Daniel
drivers/video/fbdev/efifb.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index 8ea8f079cde26..25cdea32b9633 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;
@@ -603,8 +607,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
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Sat, Aug 07, 2021 at 06:21:10PM +0300, Imre Deak wrote:
On Thu, Aug 05, 2021 at 12:23:21AM +0200, Daniel Vetter wrote:
On Mon, Aug 02, 2021 at 04:35:51PM +0300, Imre Deak wrote:
Atm the EFI FB driver gets a runtime PM reference for the associated GFX PCI device during driver probing and releases it only when removing the driver.
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 driver getting unloaded, keeping the runtime PM reference acquired during driver 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.
Fixes: a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI D0") Cc: Kai-Heng Feng kai.heng.feng@canonical.com Signed-off-by: Imre Deak imre.deak@intel.com
Patch looks good:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
But I've found a bunch of ordering issues here:
- we should probably get the runtime pm reference _before_ we register the framebuffer. There's a race right now about there.
Yea, missed this will send a v2 moving it earlier.
- the sysfs_remove_groups and framebuffer_release should also be moved into the destroy callback. This is more a leak type of situation.
Those sysfs entries belong to the efifb platform device, showing the bootup screen_info.lfb_* info, not related to fb_info, so imo efifb_remove() is the correct place to remove those. But yes, freeing fb_info seems to belong to fb_destroy().
Ah ok. Might be good to put a comment down that this isn't tied to fb_info lifetime.
Atm, things will blow up when unbinding the efifb device after the efifb framebuffer was unregistered while removing it as a conflicting FB (since unregister_framebuffer() will be called twice), so that would need to be solved as well. Maybe remove_conflicting_pci_framebuffers() could unregister the platform device instead of only unregistering the framebuffer, similarly to drm_aperture_detach_firmware(), but haven't checked this in more detail.
Yeah either that, or a double-unregister check (plus correct refcount) in unregister_framebuffer. Ideally with a check so that only the double-unregstier from remove_conflicting_pci_framebuffers is caught, and not a driver that accidentally unregisters the fbdev twice.
Even better if this would be all devm_ wrapped so it's idiot proof.
I think generally I'd say "let's not invest in fbdev", but a) these hotremove/unload bugs have been hurting us since forever, and b) efifb seems to be bound to stay around for a very long time - the simpldrmfb stuff isn't really moving forward very fast.
Anyway, would be good to get this all sorted eventually. -Daniel
Cheers, Daniel
drivers/video/fbdev/efifb.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index 8ea8f079cde26..25cdea32b9633 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;
@@ -603,8 +607,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
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
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.feng@canonical.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch (v1) Signed-off-by: Imre Deak imre.deak@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; }
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.feng@canonical.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch (v1) Signed-off-by: Imre Deak imre.deak@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;
- if (efifb_pci_dev)
pm_runtime_get_sync(&efifb_pci_dev->dev);-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
On Tue, Aug 10, 2021 at 4:36 AM Imre Deak imre.deak@intel.com wrote:
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?
Acked-by: Alex Deucher alexander.deucher@amd.com
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.feng@canonical.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch (v1) Signed-off-by: Imre Deak imre.deak@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
On Tue, Aug 10, 2021 at 10:49 PM Alex Deucher alexdeucher@gmail.com wrote:
On Tue, Aug 10, 2021 at 4:36 AM Imre Deak imre.deak@intel.com wrote:
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?
Acked-by: Alex Deucher alexander.deucher@amd.com
Acked-by: Kai-Heng Feng kai.heng.feng@canonical.com
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.feng@canonical.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch (v1) Signed-off-by: Imre Deak imre.deak@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
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.feng@canonical.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch (v1) Signed-off-by: Imre Deak imre.deak@intel.com
Thanks for the reviews, pushed to drm-intel-next.
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;
- if (efifb_pci_dev)
pm_runtime_get_sync(&efifb_pci_dev->dev);-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
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Daniel Vetter -
Imre Deak -
Kai-Heng Feng