[PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()

[PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()

[Thomas Zimmermann]

Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
calling fbdev implementation. Avoids a possible stale mutex with
generic fbdev code.

As indicated by its name, drm_fb_helper_prepare() prepares struct
drm_fb_helper before setting up the fbdev support with a call to
drm_fb_helper_init(). In legacy fbdev emulation, this happens next
to each other. If successful, drm_fb_helper_fini() later tear down
the fbdev device and also unprepare via drm_fb_helper_unprepare().

Generic fbdev emulation prepares struct drm_fb_helper immediately
after allocating the instance. It only calls drm_fb_helper_init()
as part of processing a hotplug event. If the hotplug-handling fails,
it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
and the next hotplug event runs on stale data.

Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
into the fbdev implementations. Call it right before freeing the
fb-helper instance.

Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/armada/armada_fbdev.c      | 3 +++
 drivers/gpu/drm/drm_fb_helper.c            | 2 --
 drivers/gpu/drm/drm_fbdev_generic.c        | 2 ++
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c  | 3 ++-
 drivers/gpu/drm/gma500/framebuffer.c       | 2 ++
 drivers/gpu/drm/i915/display/intel_fbdev.c | 1 +
 drivers/gpu/drm/msm/msm_fbdev.c            | 2 ++
 drivers/gpu/drm/omapdrm/omap_fbdev.c       | 2 ++
 drivers/gpu/drm/radeon/radeon_fb.c         | 2 ++
 drivers/gpu/drm/tegra/fb.c                 | 1 +
 10 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_fbdev.c b/drivers/gpu/drm/armada/armada_fbdev.c
index 07e410c62b7a..0e44f53e9fa4 100644
--- a/drivers/gpu/drm/armada/armada_fbdev.c
+++ b/drivers/gpu/drm/armada/armada_fbdev.c
@@ -147,6 +147,7 @@ int armada_fbdev_init(struct drm_device *dev)
  err_fb_setup:
 	drm_fb_helper_fini(fbh);
  err_fb_helper:
+	drm_fb_helper_unprepare(fbh);
 	priv->fbdev = NULL;
 	return ret;
 }
@@ -164,6 +165,8 @@ void armada_fbdev_fini(struct drm_device *dev)
 		if (fbh->fb)
 			fbh->fb->funcs->destroy(fbh->fb);
 
+		drm_fb_helper_unprepare(fbh);
+
 		priv->fbdev = NULL;
 	}
 }
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 28c428e9c530..a39998047f8a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -590,8 +590,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	}
 	mutex_unlock(&kernel_fb_helper_lock);
 
-	drm_fb_helper_unprepare(fb_helper);
-
 	if (!fb_helper->client.funcs)
 		drm_client_release(&fb_helper->client);
 }
diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
index 365f80717fa1..4d6325e91565 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -65,6 +65,8 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
 
 	drm_client_framebuffer_delete(fb_helper->buffer);
 	drm_client_release(&fb_helper->client);
+
+	drm_fb_helper_unprepare(fb_helper);
 	kfree(fb_helper);
 }
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index b89e33af8da8..4929ffe5a09a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -183,8 +183,8 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
 
 err_setup:
 	drm_fb_helper_fini(helper);
-
 err_init:
+	drm_fb_helper_unprepare(helper);
 	private->fb_helper = NULL;
 	kfree(fbdev);
 
@@ -219,6 +219,7 @@ void exynos_drm_fbdev_fini(struct drm_device *dev)
 	fbdev = to_exynos_fbdev(private->fb_helper);
 
 	exynos_drm_fbdev_destroy(dev, private->fb_helper);
+	drm_fb_helper_unprepare(private->fb_helper);
 	kfree(fbdev);
 	private->fb_helper = NULL;
 }
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 1f04c07ee180..f471e0cb7298 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -427,6 +427,7 @@ int psb_fbdev_init(struct drm_device *dev)
 fini:
 	drm_fb_helper_fini(fb_helper);
 free:
+	drm_fb_helper_unprepare(fb_helper);
 	kfree(fb_helper);
 	return ret;
 }
@@ -439,6 +440,7 @@ static void psb_fbdev_fini(struct drm_device *dev)
 		return;
 
 	psb_fbdev_destroy(dev, dev_priv->fb_helper);
+	drm_fb_helper_unprepare(dev_priv->fb_helper);
 	kfree(dev_priv->fb_helper);
 	dev_priv->fb_helper = NULL;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 6113d7627d45..98029059f701 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -352,6 +352,7 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
 	if (ifbdev->fb)
 		drm_framebuffer_remove(&ifbdev->fb->base);
 
+	drm_fb_helper_unprepare(&ifbdev->helper);
 	kfree(ifbdev);
 }
 
diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index 915b213f3a5c..c804e5ba682a 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -170,6 +170,7 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev)
 fini:
 	drm_fb_helper_fini(helper);
 fail:
+	drm_fb_helper_unprepare(helper);
 	kfree(fbdev);
 	return NULL;
 }
@@ -196,6 +197,7 @@ void msm_fbdev_free(struct drm_device *dev)
 		drm_framebuffer_remove(fbdev->fb);
 	}
 
+	drm_fb_helper_unprepare(helper);
 	kfree(fbdev);
 
 	priv->fbdev = NULL;
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index fc5f52d567c6..84429728347f 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -256,6 +256,7 @@ void omap_fbdev_init(struct drm_device *dev)
 fini:
 	drm_fb_helper_fini(helper);
 fail:
+	drm_fb_helper_unprepare(helper);
 	kfree(fbdev);
 
 	dev_warn(dev->dev, "omap_fbdev_init failed\n");
@@ -286,6 +287,7 @@ void omap_fbdev_fini(struct drm_device *dev)
 	if (fbdev->fb)
 		drm_framebuffer_remove(fbdev->fb);
 
+	drm_fb_helper_unprepare(helper);
 	kfree(fbdev);
 
 	priv->fbdev = NULL;
diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index 6e5eed0e157c..c4807f0c43bc 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -367,6 +367,7 @@ int radeon_fbdev_init(struct radeon_device *rdev)
 fini:
 	drm_fb_helper_fini(&rfbdev->helper);
 free:
+	drm_fb_helper_unprepare(&rfbdev->helper);
 	kfree(rfbdev);
 	return ret;
 }
@@ -377,6 +378,7 @@ void radeon_fbdev_fini(struct radeon_device *rdev)
 		return;
 
 	radeon_fbdev_destroy(rdev->ddev, rdev->mode_info.rfbdev);
+	drm_fb_helper_unprepare(&rdev->mode_info.rfbdev->helper);
 	kfree(rdev->mode_info.rfbdev);
 	rdev->mode_info.rfbdev = NULL;
 }
diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
index 153c39c32c71..bfebe2786d61 100644
--- a/drivers/gpu/drm/tegra/fb.c
+++ b/drivers/gpu/drm/tegra/fb.c
@@ -315,6 +315,7 @@ static struct tegra_fbdev *tegra_fbdev_create(struct drm_device *drm)
 
 static void tegra_fbdev_free(struct tegra_fbdev *fbdev)
 {
+	drm_fb_helper_unprepare(&fbdev->base);
 	kfree(fbdev);
 }
 
-- 
2.39.1

Re: [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()

[Daniel Vetter]

On Thu, Feb 16, 2023 at 03:06:20PM +0100, Thomas Zimmermann wrote:
> Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
> calling fbdev implementation. Avoids a possible stale mutex with
> generic fbdev code.
> 
> As indicated by its name, drm_fb_helper_prepare() prepares struct
> drm_fb_helper before setting up the fbdev support with a call to
> drm_fb_helper_init(). In legacy fbdev emulation, this happens next
> to each other. If successful, drm_fb_helper_fini() later tear down
> the fbdev device and also unprepare via drm_fb_helper_unprepare().
> 
> Generic fbdev emulation prepares struct drm_fb_helper immediately
> after allocating the instance. It only calls drm_fb_helper_init()
> as part of processing a hotplug event. If the hotplug-handling fails,
> it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
> and the next hotplug event runs on stale data.
> 
> Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
> into the fbdev implementations. Call it right before freeing the
> fb-helper instance.
> 
> Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

This reminds me of an old patch I just recently stumbled over again:

https://lore.kernel.org/dri-devel/Y3St2VHJ7jEmcNFw@phenom.ffwll.local/

Should I resurrect that one maybe and send it out? I think that also ties
a bit into your story here.

> ---
>  drivers/gpu/drm/armada/armada_fbdev.c      | 3 +++
>  drivers/gpu/drm/drm_fb_helper.c            | 2 --
>  drivers/gpu/drm/drm_fbdev_generic.c        | 2 ++
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c  | 3 ++-
>  drivers/gpu/drm/gma500/framebuffer.c       | 2 ++
>  drivers/gpu/drm/i915/display/intel_fbdev.c | 1 +
>  drivers/gpu/drm/msm/msm_fbdev.c            | 2 ++
>  drivers/gpu/drm/omapdrm/omap_fbdev.c       | 2 ++
>  drivers/gpu/drm/radeon/radeon_fb.c         | 2 ++
>  drivers/gpu/drm/tegra/fb.c                 | 1 +
>  10 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/armada/armada_fbdev.c b/drivers/gpu/drm/armada/armada_fbdev.c
> index 07e410c62b7a..0e44f53e9fa4 100644
> --- a/drivers/gpu/drm/armada/armada_fbdev.c
> +++ b/drivers/gpu/drm/armada/armada_fbdev.c
> @@ -147,6 +147,7 @@ int armada_fbdev_init(struct drm_device *dev)
>   err_fb_setup:
>  	drm_fb_helper_fini(fbh);
>   err_fb_helper:
> +	drm_fb_helper_unprepare(fbh);
>  	priv->fbdev = NULL;
>  	return ret;
>  }
> @@ -164,6 +165,8 @@ void armada_fbdev_fini(struct drm_device *dev)
>  		if (fbh->fb)
>  			fbh->fb->funcs->destroy(fbh->fb);
>  
> +		drm_fb_helper_unprepare(fbh);
> +
>  		priv->fbdev = NULL;
>  	}
>  }
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 28c428e9c530..a39998047f8a 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -590,8 +590,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)

I think it would be good to update the kerneldoc of _init() and _fini()
here to mention each another like we usually do with these pairs. Same
with prepare/unprepare() although the latter rerfences _prepare() already.

>  	}
>  	mutex_unlock(&kernel_fb_helper_lock);
>  
> -	drm_fb_helper_unprepare(fb_helper);
> -
>  	if (!fb_helper->client.funcs)
>  		drm_client_release(&fb_helper->client);
>  }
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index 365f80717fa1..4d6325e91565 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -65,6 +65,8 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
>  
>  	drm_client_framebuffer_delete(fb_helper->buffer);
>  	drm_client_release(&fb_helper->client);
> +
> +	drm_fb_helper_unprepare(fb_helper);
>  	kfree(fb_helper);
>  }
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index b89e33af8da8..4929ffe5a09a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -183,8 +183,8 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
>  
>  err_setup:
>  	drm_fb_helper_fini(helper);
> -
>  err_init:
> +	drm_fb_helper_unprepare(helper);
>  	private->fb_helper = NULL;
>  	kfree(fbdev);
>  
> @@ -219,6 +219,7 @@ void exynos_drm_fbdev_fini(struct drm_device *dev)
>  	fbdev = to_exynos_fbdev(private->fb_helper);
>  
>  	exynos_drm_fbdev_destroy(dev, private->fb_helper);
> +	drm_fb_helper_unprepare(private->fb_helper);
>  	kfree(fbdev);
>  	private->fb_helper = NULL;
>  }
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 1f04c07ee180..f471e0cb7298 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -427,6 +427,7 @@ int psb_fbdev_init(struct drm_device *dev)
>  fini:
>  	drm_fb_helper_fini(fb_helper);
>  free:
> +	drm_fb_helper_unprepare(fb_helper);
>  	kfree(fb_helper);
>  	return ret;
>  }
> @@ -439,6 +440,7 @@ static void psb_fbdev_fini(struct drm_device *dev)
>  		return;
>  
>  	psb_fbdev_destroy(dev, dev_priv->fb_helper);
> +	drm_fb_helper_unprepare(dev_priv->fb_helper);
>  	kfree(dev_priv->fb_helper);
>  	dev_priv->fb_helper = NULL;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 6113d7627d45..98029059f701 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -352,6 +352,7 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
>  	if (ifbdev->fb)
>  		drm_framebuffer_remove(&ifbdev->fb->base);
>  
> +	drm_fb_helper_unprepare(&ifbdev->helper);
>  	kfree(ifbdev);
>  }
>  
> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> index 915b213f3a5c..c804e5ba682a 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -170,6 +170,7 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev)
>  fini:
>  	drm_fb_helper_fini(helper);
>  fail:
> +	drm_fb_helper_unprepare(helper);
>  	kfree(fbdev);
>  	return NULL;
>  }
> @@ -196,6 +197,7 @@ void msm_fbdev_free(struct drm_device *dev)
>  		drm_framebuffer_remove(fbdev->fb);
>  	}
>  
> +	drm_fb_helper_unprepare(helper);
>  	kfree(fbdev);
>  
>  	priv->fbdev = NULL;
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> index fc5f52d567c6..84429728347f 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -256,6 +256,7 @@ void omap_fbdev_init(struct drm_device *dev)
>  fini:
>  	drm_fb_helper_fini(helper);
>  fail:
> +	drm_fb_helper_unprepare(helper);
>  	kfree(fbdev);
>  
>  	dev_warn(dev->dev, "omap_fbdev_init failed\n");
> @@ -286,6 +287,7 @@ void omap_fbdev_fini(struct drm_device *dev)
>  	if (fbdev->fb)
>  		drm_framebuffer_remove(fbdev->fb);
>  
> +	drm_fb_helper_unprepare(helper);
>  	kfree(fbdev);
>  
>  	priv->fbdev = NULL;
> diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
> index 6e5eed0e157c..c4807f0c43bc 100644
> --- a/drivers/gpu/drm/radeon/radeon_fb.c
> +++ b/drivers/gpu/drm/radeon/radeon_fb.c
> @@ -367,6 +367,7 @@ int radeon_fbdev_init(struct radeon_device *rdev)
>  fini:
>  	drm_fb_helper_fini(&rfbdev->helper);
>  free:
> +	drm_fb_helper_unprepare(&rfbdev->helper);
>  	kfree(rfbdev);
>  	return ret;
>  }
> @@ -377,6 +378,7 @@ void radeon_fbdev_fini(struct radeon_device *rdev)
>  		return;
>  
>  	radeon_fbdev_destroy(rdev->ddev, rdev->mode_info.rfbdev);
> +	drm_fb_helper_unprepare(&rdev->mode_info.rfbdev->helper);
>  	kfree(rdev->mode_info.rfbdev);
>  	rdev->mode_info.rfbdev = NULL;
>  }
> diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
> index 153c39c32c71..bfebe2786d61 100644
> --- a/drivers/gpu/drm/tegra/fb.c
> +++ b/drivers/gpu/drm/tegra/fb.c
> @@ -315,6 +315,7 @@ static struct tegra_fbdev *tegra_fbdev_create(struct drm_device *drm)
>  
>  static void tegra_fbdev_free(struct tegra_fbdev *fbdev)
>  {
> +	drm_fb_helper_unprepare(&fbdev->base);

Ok this one tegra change was a bit tricky, drivers really should just use
drmm_/devm_ for everything :-)

With the kerneldoc augmented:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	kfree(fbdev);
>  }
>  
> -- 
> 2.39.1
> 

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

Re: [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 9823 bytes --]

Hi

Am 16.02.23 um 21:11 schrieb Daniel Vetter:
> On Thu, Feb 16, 2023 at 03:06:20PM +0100, Thomas Zimmermann wrote:
>> Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
>> calling fbdev implementation. Avoids a possible stale mutex with
>> generic fbdev code.
>>
>> As indicated by its name, drm_fb_helper_prepare() prepares struct
>> drm_fb_helper before setting up the fbdev support with a call to
>> drm_fb_helper_init(). In legacy fbdev emulation, this happens next
>> to each other. If successful, drm_fb_helper_fini() later tear down
>> the fbdev device and also unprepare via drm_fb_helper_unprepare().
>>
>> Generic fbdev emulation prepares struct drm_fb_helper immediately
>> after allocating the instance. It only calls drm_fb_helper_init()
>> as part of processing a hotplug event. If the hotplug-handling fails,
>> it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
>> and the next hotplug event runs on stale data.
>>
>> Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
>> into the fbdev implementations. Call it right before freeing the
>> fb-helper instance.
>>
>> Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: dri-devel@lists.freedesktop.org
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> This reminds me of an old patch I just recently stumbled over again:
> 
> https://lore.kernel.org/dri-devel/Y3St2VHJ7jEmcNFw@phenom.ffwll.local/
> 
> Should I resurrect that one maybe and send it out? I think that also ties
> a bit into your story here.

I don't think it will be necessary. I began to convert the existing 
fbdev emulation to make use of drm_client, which should resove a number 
of problems. I expect to post this after the various trees have merged 
the recent changes to fbdev helpers.

Best regards
Thomas

> 
>> ---
>>   drivers/gpu/drm/armada/armada_fbdev.c      | 3 +++
>>   drivers/gpu/drm/drm_fb_helper.c            | 2 --
>>   drivers/gpu/drm/drm_fbdev_generic.c        | 2 ++
>>   drivers/gpu/drm/exynos/exynos_drm_fbdev.c  | 3 ++-
>>   drivers/gpu/drm/gma500/framebuffer.c       | 2 ++
>>   drivers/gpu/drm/i915/display/intel_fbdev.c | 1 +
>>   drivers/gpu/drm/msm/msm_fbdev.c            | 2 ++
>>   drivers/gpu/drm/omapdrm/omap_fbdev.c       | 2 ++
>>   drivers/gpu/drm/radeon/radeon_fb.c         | 2 ++
>>   drivers/gpu/drm/tegra/fb.c                 | 1 +
>>   10 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/armada/armada_fbdev.c b/drivers/gpu/drm/armada/armada_fbdev.c
>> index 07e410c62b7a..0e44f53e9fa4 100644
>> --- a/drivers/gpu/drm/armada/armada_fbdev.c
>> +++ b/drivers/gpu/drm/armada/armada_fbdev.c
>> @@ -147,6 +147,7 @@ int armada_fbdev_init(struct drm_device *dev)
>>    err_fb_setup:
>>   	drm_fb_helper_fini(fbh);
>>    err_fb_helper:
>> +	drm_fb_helper_unprepare(fbh);
>>   	priv->fbdev = NULL;
>>   	return ret;
>>   }
>> @@ -164,6 +165,8 @@ void armada_fbdev_fini(struct drm_device *dev)
>>   		if (fbh->fb)
>>   			fbh->fb->funcs->destroy(fbh->fb);
>>   
>> +		drm_fb_helper_unprepare(fbh);
>> +
>>   		priv->fbdev = NULL;
>>   	}
>>   }
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 28c428e9c530..a39998047f8a 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -590,8 +590,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> 
> I think it would be good to update the kerneldoc of _init() and _fini()
> here to mention each another like we usually do with these pairs. Same
> with prepare/unprepare() although the latter rerfences _prepare() already.
> 
>>   	}
>>   	mutex_unlock(&kernel_fb_helper_lock);
>>   
>> -	drm_fb_helper_unprepare(fb_helper);
>> -
>>   	if (!fb_helper->client.funcs)
>>   		drm_client_release(&fb_helper->client);
>>   }
>> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
>> index 365f80717fa1..4d6325e91565 100644
>> --- a/drivers/gpu/drm/drm_fbdev_generic.c
>> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
>> @@ -65,6 +65,8 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
>>   
>>   	drm_client_framebuffer_delete(fb_helper->buffer);
>>   	drm_client_release(&fb_helper->client);
>> +
>> +	drm_fb_helper_unprepare(fb_helper);
>>   	kfree(fb_helper);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
>> index b89e33af8da8..4929ffe5a09a 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
>> @@ -183,8 +183,8 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
>>   
>>   err_setup:
>>   	drm_fb_helper_fini(helper);
>> -
>>   err_init:
>> +	drm_fb_helper_unprepare(helper);
>>   	private->fb_helper = NULL;
>>   	kfree(fbdev);
>>   
>> @@ -219,6 +219,7 @@ void exynos_drm_fbdev_fini(struct drm_device *dev)
>>   	fbdev = to_exynos_fbdev(private->fb_helper);
>>   
>>   	exynos_drm_fbdev_destroy(dev, private->fb_helper);
>> +	drm_fb_helper_unprepare(private->fb_helper);
>>   	kfree(fbdev);
>>   	private->fb_helper = NULL;
>>   }
>> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
>> index 1f04c07ee180..f471e0cb7298 100644
>> --- a/drivers/gpu/drm/gma500/framebuffer.c
>> +++ b/drivers/gpu/drm/gma500/framebuffer.c
>> @@ -427,6 +427,7 @@ int psb_fbdev_init(struct drm_device *dev)
>>   fini:
>>   	drm_fb_helper_fini(fb_helper);
>>   free:
>> +	drm_fb_helper_unprepare(fb_helper);
>>   	kfree(fb_helper);
>>   	return ret;
>>   }
>> @@ -439,6 +440,7 @@ static void psb_fbdev_fini(struct drm_device *dev)
>>   		return;
>>   
>>   	psb_fbdev_destroy(dev, dev_priv->fb_helper);
>> +	drm_fb_helper_unprepare(dev_priv->fb_helper);
>>   	kfree(dev_priv->fb_helper);
>>   	dev_priv->fb_helper = NULL;
>>   }
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> index 6113d7627d45..98029059f701 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> @@ -352,6 +352,7 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
>>   	if (ifbdev->fb)
>>   		drm_framebuffer_remove(&ifbdev->fb->base);
>>   
>> +	drm_fb_helper_unprepare(&ifbdev->helper);
>>   	kfree(ifbdev);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
>> index 915b213f3a5c..c804e5ba682a 100644
>> --- a/drivers/gpu/drm/msm/msm_fbdev.c
>> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
>> @@ -170,6 +170,7 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev)
>>   fini:
>>   	drm_fb_helper_fini(helper);
>>   fail:
>> +	drm_fb_helper_unprepare(helper);
>>   	kfree(fbdev);
>>   	return NULL;
>>   }
>> @@ -196,6 +197,7 @@ void msm_fbdev_free(struct drm_device *dev)
>>   		drm_framebuffer_remove(fbdev->fb);
>>   	}
>>   
>> +	drm_fb_helper_unprepare(helper);
>>   	kfree(fbdev);
>>   
>>   	priv->fbdev = NULL;
>> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> index fc5f52d567c6..84429728347f 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
>> @@ -256,6 +256,7 @@ void omap_fbdev_init(struct drm_device *dev)
>>   fini:
>>   	drm_fb_helper_fini(helper);
>>   fail:
>> +	drm_fb_helper_unprepare(helper);
>>   	kfree(fbdev);
>>   
>>   	dev_warn(dev->dev, "omap_fbdev_init failed\n");
>> @@ -286,6 +287,7 @@ void omap_fbdev_fini(struct drm_device *dev)
>>   	if (fbdev->fb)
>>   		drm_framebuffer_remove(fbdev->fb);
>>   
>> +	drm_fb_helper_unprepare(helper);
>>   	kfree(fbdev);
>>   
>>   	priv->fbdev = NULL;
>> diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
>> index 6e5eed0e157c..c4807f0c43bc 100644
>> --- a/drivers/gpu/drm/radeon/radeon_fb.c
>> +++ b/drivers/gpu/drm/radeon/radeon_fb.c
>> @@ -367,6 +367,7 @@ int radeon_fbdev_init(struct radeon_device *rdev)
>>   fini:
>>   	drm_fb_helper_fini(&rfbdev->helper);
>>   free:
>> +	drm_fb_helper_unprepare(&rfbdev->helper);
>>   	kfree(rfbdev);
>>   	return ret;
>>   }
>> @@ -377,6 +378,7 @@ void radeon_fbdev_fini(struct radeon_device *rdev)
>>   		return;
>>   
>>   	radeon_fbdev_destroy(rdev->ddev, rdev->mode_info.rfbdev);
>> +	drm_fb_helper_unprepare(&rdev->mode_info.rfbdev->helper);
>>   	kfree(rdev->mode_info.rfbdev);
>>   	rdev->mode_info.rfbdev = NULL;
>>   }
>> diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
>> index 153c39c32c71..bfebe2786d61 100644
>> --- a/drivers/gpu/drm/tegra/fb.c
>> +++ b/drivers/gpu/drm/tegra/fb.c
>> @@ -315,6 +315,7 @@ static struct tegra_fbdev *tegra_fbdev_create(struct drm_device *drm)
>>   
>>   static void tegra_fbdev_free(struct tegra_fbdev *fbdev)
>>   {
>> +	drm_fb_helper_unprepare(&fbdev->base);
> 
> Ok this one tegra change was a bit tricky, drivers really should just use
> drmm_/devm_ for everything :-)
> 
> With the kerneldoc augmented:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
>>   	kfree(fbdev);
>>   }
>>   
>> -- 
>> 2.39.1
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()

[Daniel Vetter]

On Fri, Feb 17, 2023 at 09:18:54AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 16.02.23 um 21:11 schrieb Daniel Vetter:
> > On Thu, Feb 16, 2023 at 03:06:20PM +0100, Thomas Zimmermann wrote:
> > > Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
> > > calling fbdev implementation. Avoids a possible stale mutex with
> > > generic fbdev code.
> > > 
> > > As indicated by its name, drm_fb_helper_prepare() prepares struct
> > > drm_fb_helper before setting up the fbdev support with a call to
> > > drm_fb_helper_init(). In legacy fbdev emulation, this happens next
> > > to each other. If successful, drm_fb_helper_fini() later tear down
> > > the fbdev device and also unprepare via drm_fb_helper_unprepare().
> > > 
> > > Generic fbdev emulation prepares struct drm_fb_helper immediately
> > > after allocating the instance. It only calls drm_fb_helper_init()
> > > as part of processing a hotplug event. If the hotplug-handling fails,
> > > it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
> > > and the next hotplug event runs on stale data.
> > > 
> > > Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
> > > into the fbdev implementations. Call it right before freeing the
> > > fb-helper instance.
> > > 
> > > Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: Javier Martinez Canillas <javierm@redhat.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Maxime Ripard <mripard@kernel.org>
> > > Cc: David Airlie <airlied@gmail.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: dri-devel@lists.freedesktop.org
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > This reminds me of an old patch I just recently stumbled over again:
> > 
> > https://lore.kernel.org/dri-devel/Y3St2VHJ7jEmcNFw@phenom.ffwll.local/
> > 
> > Should I resurrect that one maybe and send it out? I think that also ties
> > a bit into your story here.
> 
> I don't think it will be necessary. I began to convert the existing fbdev
> emulation to make use of drm_client, which should resove a number of
> problems. I expect to post this after the various trees have merged the
> recent changes to fbdev helpers.

The only version the patch is fixing is the client one, the old one is
unfixable (I think at least, hence just the comments). Note that the link
is pre-splitting, I do have a rebased version here.

I'll just send that out and head into vacations :-)
-Daniel

> 
> Best regards
> Thomas
> 
> > 
> > > ---
> > >   drivers/gpu/drm/armada/armada_fbdev.c      | 3 +++
> > >   drivers/gpu/drm/drm_fb_helper.c            | 2 --
> > >   drivers/gpu/drm/drm_fbdev_generic.c        | 2 ++
> > >   drivers/gpu/drm/exynos/exynos_drm_fbdev.c  | 3 ++-
> > >   drivers/gpu/drm/gma500/framebuffer.c       | 2 ++
> > >   drivers/gpu/drm/i915/display/intel_fbdev.c | 1 +
> > >   drivers/gpu/drm/msm/msm_fbdev.c            | 2 ++
> > >   drivers/gpu/drm/omapdrm/omap_fbdev.c       | 2 ++
> > >   drivers/gpu/drm/radeon/radeon_fb.c         | 2 ++
> > >   drivers/gpu/drm/tegra/fb.c                 | 1 +
> > >   10 files changed, 17 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/armada/armada_fbdev.c b/drivers/gpu/drm/armada/armada_fbdev.c
> > > index 07e410c62b7a..0e44f53e9fa4 100644
> > > --- a/drivers/gpu/drm/armada/armada_fbdev.c
> > > +++ b/drivers/gpu/drm/armada/armada_fbdev.c
> > > @@ -147,6 +147,7 @@ int armada_fbdev_init(struct drm_device *dev)
> > >    err_fb_setup:
> > >   	drm_fb_helper_fini(fbh);
> > >    err_fb_helper:
> > > +	drm_fb_helper_unprepare(fbh);
> > >   	priv->fbdev = NULL;
> > >   	return ret;
> > >   }
> > > @@ -164,6 +165,8 @@ void armada_fbdev_fini(struct drm_device *dev)
> > >   		if (fbh->fb)
> > >   			fbh->fb->funcs->destroy(fbh->fb);
> > > +		drm_fb_helper_unprepare(fbh);
> > > +
> > >   		priv->fbdev = NULL;
> > >   	}
> > >   }
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 28c428e9c530..a39998047f8a 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -590,8 +590,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
> > 
> > I think it would be good to update the kerneldoc of _init() and _fini()
> > here to mention each another like we usually do with these pairs. Same
> > with prepare/unprepare() although the latter rerfences _prepare() already.
> > 
> > >   	}
> > >   	mutex_unlock(&kernel_fb_helper_lock);
> > > -	drm_fb_helper_unprepare(fb_helper);
> > > -
> > >   	if (!fb_helper->client.funcs)
> > >   		drm_client_release(&fb_helper->client);
> > >   }
> > > diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> > > index 365f80717fa1..4d6325e91565 100644
> > > --- a/drivers/gpu/drm/drm_fbdev_generic.c
> > > +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> > > @@ -65,6 +65,8 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
> > >   	drm_client_framebuffer_delete(fb_helper->buffer);
> > >   	drm_client_release(&fb_helper->client);
> > > +
> > > +	drm_fb_helper_unprepare(fb_helper);
> > >   	kfree(fb_helper);
> > >   }
> > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> > > index b89e33af8da8..4929ffe5a09a 100644
> > > --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> > > +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> > > @@ -183,8 +183,8 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
> > >   err_setup:
> > >   	drm_fb_helper_fini(helper);
> > > -
> > >   err_init:
> > > +	drm_fb_helper_unprepare(helper);
> > >   	private->fb_helper = NULL;
> > >   	kfree(fbdev);
> > > @@ -219,6 +219,7 @@ void exynos_drm_fbdev_fini(struct drm_device *dev)
> > >   	fbdev = to_exynos_fbdev(private->fb_helper);
> > >   	exynos_drm_fbdev_destroy(dev, private->fb_helper);
> > > +	drm_fb_helper_unprepare(private->fb_helper);
> > >   	kfree(fbdev);
> > >   	private->fb_helper = NULL;
> > >   }
> > > diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> > > index 1f04c07ee180..f471e0cb7298 100644
> > > --- a/drivers/gpu/drm/gma500/framebuffer.c
> > > +++ b/drivers/gpu/drm/gma500/framebuffer.c
> > > @@ -427,6 +427,7 @@ int psb_fbdev_init(struct drm_device *dev)
> > >   fini:
> > >   	drm_fb_helper_fini(fb_helper);
> > >   free:
> > > +	drm_fb_helper_unprepare(fb_helper);
> > >   	kfree(fb_helper);
> > >   	return ret;
> > >   }
> > > @@ -439,6 +440,7 @@ static void psb_fbdev_fini(struct drm_device *dev)
> > >   		return;
> > >   	psb_fbdev_destroy(dev, dev_priv->fb_helper);
> > > +	drm_fb_helper_unprepare(dev_priv->fb_helper);
> > >   	kfree(dev_priv->fb_helper);
> > >   	dev_priv->fb_helper = NULL;
> > >   }
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > > index 6113d7627d45..98029059f701 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > > @@ -352,6 +352,7 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
> > >   	if (ifbdev->fb)
> > >   		drm_framebuffer_remove(&ifbdev->fb->base);
> > > +	drm_fb_helper_unprepare(&ifbdev->helper);
> > >   	kfree(ifbdev);
> > >   }
> > > diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> > > index 915b213f3a5c..c804e5ba682a 100644
> > > --- a/drivers/gpu/drm/msm/msm_fbdev.c
> > > +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> > > @@ -170,6 +170,7 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev)
> > >   fini:
> > >   	drm_fb_helper_fini(helper);
> > >   fail:
> > > +	drm_fb_helper_unprepare(helper);
> > >   	kfree(fbdev);
> > >   	return NULL;
> > >   }
> > > @@ -196,6 +197,7 @@ void msm_fbdev_free(struct drm_device *dev)
> > >   		drm_framebuffer_remove(fbdev->fb);
> > >   	}
> > > +	drm_fb_helper_unprepare(helper);
> > >   	kfree(fbdev);
> > >   	priv->fbdev = NULL;
> > > diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> > > index fc5f52d567c6..84429728347f 100644
> > > --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> > > +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> > > @@ -256,6 +256,7 @@ void omap_fbdev_init(struct drm_device *dev)
> > >   fini:
> > >   	drm_fb_helper_fini(helper);
> > >   fail:
> > > +	drm_fb_helper_unprepare(helper);
> > >   	kfree(fbdev);
> > >   	dev_warn(dev->dev, "omap_fbdev_init failed\n");
> > > @@ -286,6 +287,7 @@ void omap_fbdev_fini(struct drm_device *dev)
> > >   	if (fbdev->fb)
> > >   		drm_framebuffer_remove(fbdev->fb);
> > > +	drm_fb_helper_unprepare(helper);
> > >   	kfree(fbdev);
> > >   	priv->fbdev = NULL;
> > > diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
> > > index 6e5eed0e157c..c4807f0c43bc 100644
> > > --- a/drivers/gpu/drm/radeon/radeon_fb.c
> > > +++ b/drivers/gpu/drm/radeon/radeon_fb.c
> > > @@ -367,6 +367,7 @@ int radeon_fbdev_init(struct radeon_device *rdev)
> > >   fini:
> > >   	drm_fb_helper_fini(&rfbdev->helper);
> > >   free:
> > > +	drm_fb_helper_unprepare(&rfbdev->helper);
> > >   	kfree(rfbdev);
> > >   	return ret;
> > >   }
> > > @@ -377,6 +378,7 @@ void radeon_fbdev_fini(struct radeon_device *rdev)
> > >   		return;
> > >   	radeon_fbdev_destroy(rdev->ddev, rdev->mode_info.rfbdev);
> > > +	drm_fb_helper_unprepare(&rdev->mode_info.rfbdev->helper);
> > >   	kfree(rdev->mode_info.rfbdev);
> > >   	rdev->mode_info.rfbdev = NULL;
> > >   }
> > > diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
> > > index 153c39c32c71..bfebe2786d61 100644
> > > --- a/drivers/gpu/drm/tegra/fb.c
> > > +++ b/drivers/gpu/drm/tegra/fb.c
> > > @@ -315,6 +315,7 @@ static struct tegra_fbdev *tegra_fbdev_create(struct drm_device *drm)
> > >   static void tegra_fbdev_free(struct tegra_fbdev *fbdev)
> > >   {
> > > +	drm_fb_helper_unprepare(&fbdev->base);
> > 
> > Ok this one tegra change was a bit tricky, drivers really should just use
> > drmm_/devm_ for everything :-)
> > 
> > With the kerneldoc augmented:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > >   	kfree(fbdev);
> > >   }
> > > -- 
> > > 2.39.1
> > > 
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev




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

Re: [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
> calling fbdev implementation. Avoids a possible stale mutex with
> generic fbdev code.
>
> As indicated by its name, drm_fb_helper_prepare() prepares struct
> drm_fb_helper before setting up the fbdev support with a call to
> drm_fb_helper_init(). In legacy fbdev emulation, this happens next
> to each other. If successful, drm_fb_helper_fini() later tear down
> the fbdev device and also unprepare via drm_fb_helper_unprepare().
>
> Generic fbdev emulation prepares struct drm_fb_helper immediately
> after allocating the instance. It only calls drm_fb_helper_init()
> as part of processing a hotplug event. If the hotplug-handling fails,
> it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
> and the next hotplug event runs on stale data.
>
> Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
> into the fbdev implementations. Call it right before freeing the
> fb-helper instance.
>
> Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")

I think this should be Fixes: 032116bbe152 ("drm/fbdev-generic: Minimize
client unregistering") instead? Because commit 4825797c36da just added a
wrapper function for mutex_destroy(&fb_helper->lock), but it was commit
032116bbe152 that made drm_fbdev_cleanup() to call that helper function.

> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

The change itself looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2617 bytes --]

Hi

Am 21.02.23 um 11:27 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
>> Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
>> calling fbdev implementation. Avoids a possible stale mutex with
>> generic fbdev code.
>>
>> As indicated by its name, drm_fb_helper_prepare() prepares struct
>> drm_fb_helper before setting up the fbdev support with a call to
>> drm_fb_helper_init(). In legacy fbdev emulation, this happens next
>> to each other. If successful, drm_fb_helper_fini() later tear down
>> the fbdev device and also unprepare via drm_fb_helper_unprepare().
>>
>> Generic fbdev emulation prepares struct drm_fb_helper immediately
>> after allocating the instance. It only calls drm_fb_helper_init()
>> as part of processing a hotplug event. If the hotplug-handling fails,
>> it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
>> and the next hotplug event runs on stale data.
>>
>> Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
>> into the fbdev implementations. Call it right before freeing the
>> fb-helper instance.
>>
>> Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")
> 
> I think this should be Fixes: 032116bbe152 ("drm/fbdev-generic: Minimize
> client unregistering") instead? Because commit 4825797c36da just added a
> wrapper function for mutex_destroy(&fb_helper->lock), but it was commit
> 032116bbe152 that made drm_fbdev_cleanup() to call that helper function.

Good point. After looking through the recent fbdev commits, I'll use 
commit 643231b28380 ("drm/fbdev-generic: Minimize hotplug error 
handling") for the tag. This is the one that added the call to 
drm_fb_helper_fini() to the client's hotplug handler. And _fini() 
currently does the _unprepare(), when it shouldn't.

> 
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: dri-devel@lists.freedesktop.org
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> The change itself looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

Thanks a lot.

Best regards
Thomas


-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 21.02.23 um 11:27 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>>> Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
>>> calling fbdev implementation. Avoids a possible stale mutex with
>>> generic fbdev code.
>>>
>>> As indicated by its name, drm_fb_helper_prepare() prepares struct
>>> drm_fb_helper before setting up the fbdev support with a call to
>>> drm_fb_helper_init(). In legacy fbdev emulation, this happens next
>>> to each other. If successful, drm_fb_helper_fini() later tear down
>>> the fbdev device and also unprepare via drm_fb_helper_unprepare().
>>>
>>> Generic fbdev emulation prepares struct drm_fb_helper immediately
>>> after allocating the instance. It only calls drm_fb_helper_init()
>>> as part of processing a hotplug event. If the hotplug-handling fails,
>>> it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
>>> and the next hotplug event runs on stale data.
>>>
>>> Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
>>> into the fbdev implementations. Call it right before freeing the
>>> fb-helper instance.
>>>
>>> Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")
>> 
>> I think this should be Fixes: 032116bbe152 ("drm/fbdev-generic: Minimize
>> client unregistering") instead? Because commit 4825797c36da just added a
>> wrapper function for mutex_destroy(&fb_helper->lock), but it was commit
>> 032116bbe152 that made drm_fbdev_cleanup() to call that helper function.
>
> Good point. After looking through the recent fbdev commits, I'll use 
> commit 643231b28380 ("drm/fbdev-generic: Minimize hotplug error 
> handling") for the tag. This is the one that added the call to 
> drm_fb_helper_fini() to the client's hotplug handler. And _fini() 
> currently does the _unprepare(), when it shouldn't.
>

Ah, much better indeed.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat