[PATCH 1/2] simplefb: fix unmapping fb during destruction
Hi Tomi Could we get this in -next before the merge-window starts? Stephen already ack'ed it. Thanks David On Wed, Oct 2, 2013 at 6:23 PM, David Herrmann wrote: > Hi > > On Wed, Oct 2, 2013 at 6:16 PM, Stephen Warren > wrote: >> On 10/02/2013 08:58 AM, David Herrmann wrote: >>> Unfortunately, fbdev does not create its own "struct device" for >>> framebuffers. Instead, it attaches to the device of the parent layer. This >>> has the side-effect that devm_* managed resources are not cleaned up on >>> framebuffer-destruction but rather during destruction of the >>> parent-device. In case of fbdev this might be too late, though. >>> remove_conflicting_framebuffer() may remove fbdev devices but keep the >>> parent device as it is. >>> >>> Therefore, we now use plain ioremap() and unmap the framebuffer in the >>> fb_destroy() callback. Note that we must not free the device here as this >>> might race with the parent-device removal. Instead, we rely on >>> unregister_framebuffer() as barrier and we're safe. >> >> So, once the .fb_destroy callback has been executed, there's no other >> callback to resurrect it? The framebuffer itself is still registered >> until the device's remove, yet after .fb_destroy, the memory is >> unmapped, which would be dangerous if the FB can be re-started. > > fbdev lifetime tracking is weird.. ->fb_destroy() is called by > unregister_framebuffer() _after_ it got unlinked. So no, the > framebuffer is gone and cannot be resurrected. However, the > unregistered/dead fbdev object itself stays around until you call > framebuffer_release(). We cannot call it from fb_destroy(), though as > the platform_data in your platform device still points to the fbdev > device. Therefore we keep it and wait for the platform-driver to be > removed which then again calls unregister_framebuffer() (which will > immediately return as the fbdev device is not registered) and then you > can finally call framebuffer_release(). > > Note that even though there's fb_get_info() and fb_put_info(), both > are not exported and never used. So there is *no* fbdev ref-counting > (which would be horribly broken anyway) and the driver is the sole > owner of the fbdev object. > >> If that's not an issue, this patch seems fine, so >> Acked-by: Stephen Warren > > Thanks! > >>> I know that simplefb was supposed to stay "as simple as possible" but I >>> really >>> think this series is the last set of fixes I have. Unfortunately >>> framebuffer DRM >>> handover is mandatory so we cannot ignore it in simplefb. >> >> I don't think this patch adds any significant complexity, so I'm not >> worried at least. > > Good to hear! > > Thanks > David
[PATCH 1/2] simplefb: fix unmapping fb during destruction
Unfortunately, fbdev does not create its own struct device for
framebuffers. Instead, it attaches to the device of the parent layer. This
has the side-effect that devm_* managed resources are not cleaned up on
framebuffer-destruction but rather during destruction of the
parent-device. In case of fbdev this might be too late, though.
remove_conflicting_framebuffer() may remove fbdev devices but keep the
parent device as it is.
Therefore, we now use plain ioremap() and unmap the framebuffer in the
fb_destroy() callback. Note that we must not free the device here as this
might race with the parent-device removal. Instead, we rely on
unregister_framebuffer() as barrier and we're safe.
Reported-by: Tom Gundersen t...@jklm.no
Signed-off-by: David Herrmann dh.herrm...@gmail.com
---
Hi
I know that simplefb was supposed to stay as simple as possible but I really
think this series is the last set of fixes I have. Unfortunately framebuffer DRM
handover is mandatory so we cannot ignore it in simplefb.
Both patches are not critical at all and are targeted at 3.13-rc1.
Thanks
David
drivers/video/simplefb.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index 8d78106..74b016c 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -66,8 +66,15 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int
green, u_int blue,
return 0;
}
+static void simplefb_destroy(struct fb_info *info)
+{
+ if (info-screen_base)
+ iounmap(info-screen_base);
+}
+
static struct fb_ops simplefb_ops = {
.owner = THIS_MODULE,
+ .fb_destroy = simplefb_destroy,
.fb_setcolreg = simplefb_setcolreg,
.fb_fillrect= cfb_fillrect,
.fb_copyarea= cfb_copyarea,
@@ -212,8 +219,8 @@ static int simplefb_probe(struct platform_device *pdev)
info-fbops = simplefb_ops;
info-flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE;
- info-screen_base = devm_ioremap(pdev-dev, info-fix.smem_start,
-info-fix.smem_len);
+ info-screen_base = ioremap(info-fix.smem_start,
+ info-fix.smem_len);
if (!info-screen_base) {
framebuffer_release(info);
return -ENODEV;
@@ -223,6 +230,7 @@ static int simplefb_probe(struct platform_device *pdev)
ret = register_framebuffer(info);
if (ret 0) {
dev_err(pdev-dev, Unable to register simplefb: %d\n, ret);
+ iounmap(info-screen_base);
framebuffer_release(info);
return ret;
}
--
1.8.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] simplefb: fix unmapping fb during destruction
On 10/02/2013 08:58 AM, David Herrmann wrote: Unfortunately, fbdev does not create its own struct device for framebuffers. Instead, it attaches to the device of the parent layer. This has the side-effect that devm_* managed resources are not cleaned up on framebuffer-destruction but rather during destruction of the parent-device. In case of fbdev this might be too late, though. remove_conflicting_framebuffer() may remove fbdev devices but keep the parent device as it is. Therefore, we now use plain ioremap() and unmap the framebuffer in the fb_destroy() callback. Note that we must not free the device here as this might race with the parent-device removal. Instead, we rely on unregister_framebuffer() as barrier and we're safe. So, once the .fb_destroy callback has been executed, there's no other callback to resurrect it? The framebuffer itself is still registered until the device's remove, yet after .fb_destroy, the memory is unmapped, which would be dangerous if the FB can be re-started. If that's not an issue, this patch seems fine, so Acked-by: Stephen Warren swar...@nvidia.com I know that simplefb was supposed to stay as simple as possible but I really think this series is the last set of fixes I have. Unfortunately framebuffer DRM handover is mandatory so we cannot ignore it in simplefb. I don't think this patch adds any significant complexity, so I'm not worried at least. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] simplefb: fix unmapping fb during destruction
Hi On Wed, Oct 2, 2013 at 6:16 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 10/02/2013 08:58 AM, David Herrmann wrote: Unfortunately, fbdev does not create its own struct device for framebuffers. Instead, it attaches to the device of the parent layer. This has the side-effect that devm_* managed resources are not cleaned up on framebuffer-destruction but rather during destruction of the parent-device. In case of fbdev this might be too late, though. remove_conflicting_framebuffer() may remove fbdev devices but keep the parent device as it is. Therefore, we now use plain ioremap() and unmap the framebuffer in the fb_destroy() callback. Note that we must not free the device here as this might race with the parent-device removal. Instead, we rely on unregister_framebuffer() as barrier and we're safe. So, once the .fb_destroy callback has been executed, there's no other callback to resurrect it? The framebuffer itself is still registered until the device's remove, yet after .fb_destroy, the memory is unmapped, which would be dangerous if the FB can be re-started. fbdev lifetime tracking is weird.. -fb_destroy() is called by unregister_framebuffer() _after_ it got unlinked. So no, the framebuffer is gone and cannot be resurrected. However, the unregistered/dead fbdev object itself stays around until you call framebuffer_release(). We cannot call it from fb_destroy(), though as the platform_data in your platform device still points to the fbdev device. Therefore we keep it and wait for the platform-driver to be removed which then again calls unregister_framebuffer() (which will immediately return as the fbdev device is not registered) and then you can finally call framebuffer_release(). Note that even though there's fb_get_info() and fb_put_info(), both are not exported and never used. So there is *no* fbdev ref-counting (which would be horribly broken anyway) and the driver is the sole owner of the fbdev object. If that's not an issue, this patch seems fine, so Acked-by: Stephen Warren swar...@nvidia.com Thanks! I know that simplefb was supposed to stay as simple as possible but I really think this series is the last set of fixes I have. Unfortunately framebuffer DRM handover is mandatory so we cannot ignore it in simplefb. I don't think this patch adds any significant complexity, so I'm not worried at least. Good to hear! Thanks David ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
