On 13/08/12 00:53, David Herrmann wrote:
> This opens the framebuffer upon registration so we can use it for
> drawing-operations. On unregistration we close it again.
>
> While opening/closing or accessing the fb in any other way, we must hold
> the fb-mutex. However, since the notifiers are often called with the mutex
> already held, we cannot lock it _after_ taking the
> fblog_registration_lock. Therefore, we require the caller to make sure the
> fb-mutex is held.
>
> Signed-off-by: David Herrmann <dh.herrm...@googlemail.com>
Some comments below,
~Ryan
> ---
> drivers/video/console/fblog.c | 94
> ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 93 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
> index 279f4d8..1c526c5 100644
> --- a/drivers/video/console/fblog.c
> +++ b/drivers/video/console/fblog.c
> @@ -32,12 +32,14 @@
>
> enum fblog_flags {
> FBLOG_KILLED,
> + FBLOG_OPEN,
> };
>
> struct fblog_fb {
> unsigned long flags;
> struct fb_info *info;
> struct device dev;
> + struct mutex lock;
> };
>
> static DEFINE_MUTEX(fblog_registration_lock);
> @@ -46,6 +48,74 @@ static struct fblog_fb *fblog_fbs[FB_MAX];
> #define to_fblog_dev(_d) container_of(_d, struct fblog_fb, dev)
>
> /*
> + * 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;
}
> + ret = -EIO;
> + goto out_unref;
> + }
> +
> + set_bit(FBLOG_OPEN, &fb->flags);
> + mutex_unlock(&fb->lock);
> + return 0;
> +
> +out_unref:
> + module_put(fb->info->fbops->owner);
> +out_killed:
> + set_bit(FBLOG_KILLED, &fb->flags);
> +unlock:
> + mutex_unlock(&fb->lock);
> + return ret;
> +}
> +
> +static void fblog_close(struct fblog_fb *fb, bool kill_dev)
> +{
> + mutex_lock(&fb->lock);
> +
> + if (test_bit(FBLOG_OPEN, &fb->flags)) {
> + if (fb->info->fbops->fb_release)
> + fb->info->fbops->fb_release(fb->info, 0);
> + module_put(fb->info->fbops->owner);
> + clear_bit(FBLOG_OPEN, &fb->flags);
> + }
> +
> + if (kill_dev)
> + set_bit(FBLOG_KILLED, &fb->flags);
> +
> + mutex_unlock(&fb->lock);
> +}
> +
> +/*
> * 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?
> }
> @@ -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.
>
> static void fblog_register(struct fb_info *info, bool force)
> @@ -134,6 +208,7 @@ static int fblog_event(struct notifier_block *self,
> unsigned long action,
> {
> struct fb_event *event = data;
> struct fb_info *info = event->info;
> + struct fblog_fb *fb;
>
> switch(action) {
> case FB_EVENT_FB_REGISTERED:
> @@ -145,8 +220,21 @@ static int fblog_event(struct notifier_block *self,
> unsigned long action,
> case FB_EVENT_FB_UNREGISTERED:
> /* This is called when a low-level system driver unregisters a
> * framebuffer. The registration lock is held but the console
> - * lock might not be held. */
> + * lock might not be held. The fb-lock is not held, either! */
> + mutex_lock(&info->lock);
> fblog_unregister(info);
> + mutex_unlock(&info->lock);
> + break;
> + case FB_EVENT_FB_UNBIND:
> + /* Called directly before unregistering an FB. The FB is still
> + * valid here and the registration lock is held but the console
> + * lock might not be held (really?). */
> + mutex_lock(&fblog_registration_lock);
> + fb = fblog_fbs[info->node];
> + mutex_unlock(&fblog_registration_lock);
> +
> + if (fb)
> + fblog_close(fb, true);
> break;
> }
>
> @@ -163,7 +251,9 @@ static void fblog_scan(void)
> if (!info || IS_ERR(info))
> continue;
>
> + mutex_lock(&info->lock);
> fblog_register(info, false);
> + mutex_unlock(&info->lock);
>
> /* There is a very subtle race-condition. Even though we might
> * own a reference to the fb, it may still get unregistered
> @@ -224,7 +314,9 @@ static void __exit fblog_exit(void)
> if (!info || IS_ERR(info))
> continue;
>
> + mutex_lock(&info->lock);
> fblog_unregister(info);
> + mutex_unlock(&info->lock);
> put_fb_info(info);
> }
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
