diff mbox

[1/2] simplefb: fix unmapping fb during destruction

Message ID 1380725919-1961-1-git-send-email-dh.herrmann@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Herrmann Oct. 2, 2013, 2:58 p.m. UTC
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 <teg@jklm.no>
Signed-off-by: David Herrmann <dh.herrmann@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(-)

Comments

Stephen Warren Oct. 2, 2013, 4:16 p.m. UTC | #1
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 <swarren@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Herrmann Oct. 2, 2013, 4:23 p.m. UTC | #2
Hi

On Wed, Oct 2, 2013 at 6:16 PM, Stephen Warren <swarren@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 <swarren@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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Herrmann Oct. 30, 2013, 7:48 a.m. UTC | #3
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 <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Oct 2, 2013 at 6:16 PM, Stephen Warren <swarren@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 <swarren@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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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