| Message ID | 20210802133551.1904964-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | fbdev/efifb: Release PCI device's runtime PM ref during FB destroy | expand |
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
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; }
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(-)