Support device unplug by taking a ref on drm_device during probe, drop
it in fbops.destroy = drm_fb_helper_fb_destroy().
Use drm_dev_is_unplugged() to block futile operations.

drm_fb_helper_unregister_fbi() can now be called on device removal and
drm_fb_helper_fini() in drm_driver.release.

It turns out that fbdev has the necessary ref counting, so
unregister_framebuffer() together with fb_ops->destroy handles unplug
with open fd's. The ref counting doesn't apply to the fb_info structure
itself, but to use of the fbdev framebuffer.

Analysis of entry points after unregister_framebuffer():
- fbcon has been unbound (through notifier)
- sysfs files removed

First we look at possible operations on open fbdev file handles:

static const struct file_operations fb_fops = {
        .read =         fb_read,
        .write =        fb_write,
        .unlocked_ioctl = fb_ioctl,
        .compat_ioctl = fb_compat_ioctl,
        .mmap =         fb_mmap,
        .open =         fb_open,
        .release =      fb_release,
        .get_unmapped_area = get_fb_unmapped_area,
        .fsync =        fb_deferred_io_fsync,
};

Protected by file_fb_info() (-ENODEV):
fb_read() -> fb_ops.fb_read = drm_fb_helper_sys_read()
fb_write() -> fb_ops.fb_write = drm_fb_helper_sys_write()
fb_ioctl() -> fb_ops.fb_ioctl = drm_fb_helper_ioctl()
fb_compat_ioctl() -> fb_ops.fb_compat_ioctl
fb_mmap() -> fb_ops.fb_mmap

Safe:
fb_release() -> fb_ops.fb_release
get_fb_unmapped_area() : info->screen_base
fb_deferred_io_fsync() : if (info->fbdefio) schedule info->deferred_work

Active mmap's will need the backing buffer to be present.
If deferred IO is used, mmap writes will via a worker generate calls to
drm_fb_helper_deferred_io() which in turn via a worker calls into
drm_fb_helper_dirty_work().

The remaining struct fb_ops operations are safe since they're either
called through fb_ioctl(), fbcon or sysfs.

Next we look at other call paths:

drm_fb_helper_set_suspend{_unlocked}() and
drm_fb_helper_resume_worker():
Calls into fb_set_suspend(), but that's fine since it just uses the
fbdev notifier.

drm_fb_helper_restore_fbdev_mode_unlocked():
Called from drm_driver.last_close.

drm_fb_helper_force_kernel_mode():
Triggered by sysrq.

drm_fb_helper_hotplug_event():
Called by drm_kms_helper_hotplug_event().

Based on this analysis the following functions gets a check:
- drm_fb_helper_restore_fbdev_mode_unlocked()
- drm_fb_helper_force_kernel_mode()
- drm_fb_helper_hotplug_event()
- drm_fb_helper_dirty_work()

Signed-off-by: Noralf Trønnes <nor...@tronnes.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 53 +++++++++++++++++++++++++++++++++++++----
 include/drm/drm_fb_helper.h     |  7 ++++++
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 6a31d13..74c1053 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -498,7 +498,7 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct 
drm_fb_helper *fb_helper)
        bool do_delayed;
        int ret;
 
-       if (!drm_fbdev_emulation)
+       if (!drm_fbdev_emulation || drm_dev_is_unplugged(fb_helper->dev))
                return -ENODEV;
 
        if (READ_ONCE(fb_helper->deferred_setup))
@@ -563,6 +563,9 @@ static bool drm_fb_helper_force_kernel_mode(void)
        list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) {
                struct drm_device *dev = helper->dev;
 
+               if (drm_dev_is_unplugged(dev))
+                       continue;
+
                if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
                        continue;
 
@@ -735,6 +738,9 @@ static void drm_fb_helper_dirty_work(struct work_struct 
*work)
        struct drm_clip_rect clip_copy;
        unsigned long flags;
 
+       if (drm_dev_is_unplugged(helper->dev))
+               return;
+
        spin_lock_irqsave(&helper->dirty_lock, flags);
        clip_copy = *clip;
        clip->x1 = clip->y1 = ~0;
@@ -886,13 +892,24 @@ EXPORT_SYMBOL(drm_fb_helper_alloc_fbi);
  * @fb_helper: driver-allocated fbdev helper
  *
  * A wrapper around unregister_framebuffer, to release the fb_info
- * framebuffer device. This must be called before releasing all resources for
- * @fb_helper by calling drm_fb_helper_fini().
+ * framebuffer device. Unless drm_fb_helper_fb_destroy() set by
+ * DRM_FB_HELPER_DEFAULT_OPS() is used, the ref taken on &drm_device in
+ * drm_fb_helper_initial_config() is dropped. This function must be called
+ * before releasing all resources for @fb_helper by calling
+ * drm_fb_helper_fini().
  */
 void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper)
 {
-       if (fb_helper && fb_helper->fbdev)
-               unregister_framebuffer(fb_helper->fbdev);
+       struct fb_info *info;
+
+       if (!fb_helper || !fb_helper->fbdev)
+               return;
+
+       info = fb_helper->fbdev;
+       unregister_framebuffer(info);
+       if (!(info->fbops &&
+             info->fbops->fb_destroy == drm_fb_helper_fb_destroy))
+               drm_dev_unref(fb_helper->dev);
 }
 EXPORT_SYMBOL(drm_fb_helper_unregister_fbi);
 
@@ -1002,6 +1019,24 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
 EXPORT_SYMBOL(drm_fb_helper_deferred_io);
 
 /**
+ * drm_fb_helper_fb_destroy - implementation for &fb_ops.fb_destroy
+ * @info: fbdev registered by the helper
+ *
+ * Drop ref taken on &drm_device in drm_fb_helper_initial_config().
+ *
+ * &fb_ops.fb_destroy is called during unregister_framebuffer() or the last
+ * fb_release() which ever comes last.
+ */
+void drm_fb_helper_fb_destroy(struct fb_info *info)
+{
+       struct drm_fb_helper *fb_helper = info->par;
+
+       DRM_DEBUG("\n");
+       drm_dev_unref(fb_helper->dev);
+}
+EXPORT_SYMBOL(drm_fb_helper_fb_destroy);
+
+/**
  * drm_fb_helper_sys_read - wrapper around fb_sys_read
  * @info: fb_info struct pointer
  * @buf: userspace buffer to read from framebuffer memory
@@ -2496,6 +2531,8 @@ __drm_fb_helper_initial_config_and_unlock(struct 
drm_fb_helper *fb_helper,
        if (ret < 0)
                return ret;
 
+       drm_dev_ref(dev);
+
        dev_info(dev->dev, "fb%d: %s frame buffer device\n",
                 info->node, info->fix.id);
 
@@ -2527,6 +2564,9 @@ __drm_fb_helper_initial_config_and_unlock(struct 
drm_fb_helper *fb_helper,
  * drm_fb_helper_fill_fix() are provided as helpers to setup simple default
  * values for the fbdev info structure.
  *
+ * A ref is taken on &drm_device if the framebuffer is registered. This ref is
+ * dropped in drm_fb_helper_unregister_fbi() or drm_fb_helper_fb_destroy().
+ *
  * HANG DEBUGGING:
  *
  * When you have fbcon support built-in or already loaded, this function will 
do
@@ -2593,6 +2633,9 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper 
*fb_helper)
        if (!drm_fbdev_emulation)
                return 0;
 
+       if (drm_dev_is_unplugged(fb_helper->dev))
+               return -ENODEV;
+
        mutex_lock(&fb_helper->lock);
        if (fb_helper->deferred_setup) {
                err = __drm_fb_helper_initial_config_and_unlock(fb_helper,
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 33fe959..dd78eb3 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -232,6 +232,7 @@ struct drm_fb_helper {
  * functions. To be used in struct fb_ops of drm drivers.
  */
 #define DRM_FB_HELPER_DEFAULT_OPS \
+       .fb_destroy     = drm_fb_helper_fb_destroy, \
        .fb_check_var   = drm_fb_helper_check_var, \
        .fb_set_par     = drm_fb_helper_set_par, \
        .fb_setcmap     = drm_fb_helper_setcmap, \
@@ -268,6 +269,8 @@ void drm_fb_helper_unlink_fbi(struct drm_fb_helper 
*fb_helper);
 void drm_fb_helper_deferred_io(struct fb_info *info,
                               struct list_head *pagelist);
 
+void drm_fb_helper_fb_destroy(struct fb_info *info);
+
 ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
                               size_t count, loff_t *ppos);
 ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
@@ -398,6 +401,10 @@ static inline void drm_fb_helper_deferred_io(struct 
fb_info *info,
 {
 }
 
+static inline void drm_fb_helper_fb_destroy(struct fb_info *info)
+{
+}
+
 static inline ssize_t drm_fb_helper_sys_read(struct fb_info *info,
                                             char __user *buf, size_t count,
                                             loff_t *ppos)
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to