Hi Ryan

On Mon, Aug 13, 2012 at 2:00 AM, Ryan Mallon [off-list ref] wrote:

On 13/08/12 00:53, David Herrmann wrote:

quoted

 /*
+ * fblog_open/close()
+ * These functions manage access to the underlying framebuffer. While opened, we
+ * have a valid reference to the fb and can use it for drawing operations. When
+ * the fb is unregistered, we drop our reference and close the fb so it can get
+ * deleted properly. We also mark it as dead so no further fblog_open() call
+ * will succeed.
+ * Both functions must be called with the fb->info->lock mutex held! But make
+ * sure to lock it _before_ locking the fblog-registration-lock. Otherwise, we
+ * will dead-lock with fb-registration.
+ */
+
+static int fblog_open(struct fblog_fb *fb)
+{
+     int ret;
+
+     mutex_lock(&fb->lock);
+
+     if (test_bit(FBLOG_KILLED, &fb->flags)) {
+             ret = -ENODEV;
+             goto unlock;
+     }
+
+     if (test_bit(FBLOG_OPEN, &fb->flags)) {
+             ret = 0;
+             goto unlock;
+     }
+
+     if (!try_module_get(fb->info->fbops->owner)) {
+             ret = -ENODEV;
+             goto out_killed;
+     }
+
+     if (fb->info->fbops->fb_open && fb->info->fbops->fb_open(fb->info, 0)) {

Should propagate the error here:

        if (fb->info->fbops->fb_open) {
                ret = fb->info->fbops->fb_open(fb->info, 0);
                if (ret)
                        gotou out_unref;
        }

Nice catch, thanks.

quoted

+/*
  * fblog framebuffer list
  * The fblog_fbs[] array contains all currently registered framebuffers. If a
  * framebuffer is in that list, we always must make sure that we own a reference

@@ -77,6 +147,7 @@ static void fblog_do_unregister(struct fb_info *info)

      fblog_fbs[info->node] = NULL;

+     fblog_close(fb, true);
      device_del(&fb->dev);
      put_device(&fb->dev);

device_unregister?

Heh, you already mentioned this in the previous patch. I definitely
need to fix that.

quoted

 }

@@ -99,6 +170,7 @@ static void fblog_do_register(struct fb_info *info, bool force)
              return;

      fb->info = info;
+     mutex_init(&fb->lock);
      __module_get(THIS_MODULE);
      device_initialize(&fb->dev);
      fb->dev.class = fb_class;

@@ -113,6 +185,8 @@ static void fblog_do_register(struct fb_info *info, bool force)
              put_device(&fb->dev);
              return;
      }
+
+     fblog_open(fb);
 }

This function should really return an errno value.

I never used the return value in my code but as mentioned in the
previous patches, fblog_scan() should check them. So yes, I will add
this.

Thanks!
David