Hi Ryan On Mon, Aug 13, 2012 at 2:00 AM, Ryan Mallon <rmallon@xxxxxxxxx> wrote: > On 13/08/12 00:53, David Herrmann wrote: >> /* >> + * 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. >> +/* >> * 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. >> } >> @@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html