[PATCH] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy

[PATCH] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy

[Imre Deak]

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;
 }
-- 
2.27.0

Re: [Intel-gfx] [PATCH] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy

[Imre Deak]

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

[PATCH v2] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy

[Imre Deak]

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

Re: [Intel-gfx] [PATCH] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH v2] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy

[Imre Deak]

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;
>  	}
>  	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
> 

Re: [Intel-gfx] [PATCH v2] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy

[Alex Deucher]

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
> >

Re: [Intel-gfx] [PATCH v2] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy

[Kai-Heng Feng]

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
> > >

Re: [Intel-gfx] [PATCH v2] fbdev/efifb: Release PCI device's runtime PM ref during FB destroy

[Imre Deak]

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;
>  	}
>  	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
>