by David Herrmann

[permalink] [raw]

Subject: Re: [PATCH 07/11] fblog: allow selecting fbs via sysfs and module-parameters

Hi Ryan

On Mon, Aug 13, 2012 at 2:04 AM, Ryan Mallon <[email protected]> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> +static ssize_t fblog_dev_active_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> +
>> + return snprintf(buf, PAGE_SIZE, "%d\n",
>> + !!test_bit(FBLOG_OPEN, &fb->flags));
>
> Nitpick. sprintf is okay here, %d is rarely longer than PAGE_SIZE :-).
>
>> +}
>> +
>> +static ssize_t fblog_dev_active_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf,
>> + size_t count)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> + unsigned long num;
>> + int ret = 0;
>> +
>> + num = simple_strtoul(buf, NULL, 10);
>
> kstrtoul is preferred these days I think, it also catches errors.
>
>> +
>> + mutex_lock(&fb->info->lock);
>> + if (num)
>> + ret = fblog_open(fb);
>> + else
>> + fblog_close(fb, false);
>> + mutex_unlock(&fb->info->lock);
>> +
>> + return ret ? ret : count;
>
> Nitpick, you can use gcc's shortcut form of the ? operator here:
>
> return ret ?: count;
>
>> @@ -186,7 +228,20 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> return;
>> }
>>
>> - fblog_open(fb);
>> + ret = device_create_file(&fb->dev, &dev_attr_active);
>> + if (ret) {
>> + pr_err("fblog: cannot create sysfs entry");
>
> Shouldn't need the "fblog: " prefix, since you have pr_fmt defined.
>
>> + /* do not open fb if we cannot create control file */
>> + do_open = false;
>> + }
>> +
>> + if (!activate_on_hotplug)
>> + do_open = false;
>> + if (main_only && info->node != 0)
>> + do_open = false;
>> +
>> + if (do_open)
>> + fblog_open(fb);
>> }
>>
>> static void fblog_register(struct fb_info *info, bool force)

All four fixes make sense, thanks!
David

2012-08-15 00:17:13

by Ryan Mallon

[permalink] [raw]

Subject: Re: [PATCH 05/11] fblog: register one fblog object per framebuffer

On 14/08/12 21:01, David Herrmann wrote:
> Hi Ryan
>
> On Mon, Aug 13, 2012 at 1:54 AM, Ryan Mallon <[email protected]> wrote:
>> On 13/08/12 00:53, David Herrmann wrote:
>>> drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 195 insertions(+)
>>>
>>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>>> index fb39737..279f4d8 100644
>>> --- a/drivers/video/console/fblog.c
>>> +++ b/drivers/video/console/fblog.c
>>> @@ -23,15 +23,210 @@
>>> * all fblog instances before running other graphics applications.
>>> */
>>>
>>> +#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/fb.h>
>>> #include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +
>>> +enum fblog_flags {
>>> + FBLOG_KILLED,
>>> +};
>>> +
>>> +struct fblog_fb {
>>> + unsigned long flags;
>>
>> Are more flags added in later patches? If not, why not just have:
>>
>> bool is_killed;
>>
>> ?
>
> Yes, more are added in patch-6 and patch-8 which includes FBLOG_OPEN,
> FBLOG_SUSPENDED, FBLOG_BLANKED.
>
>>> +static void fblog_release(struct device *dev)
>>> +{
>>> + struct fblog_fb *fb = to_fblog_dev(dev);
>>> +
>>> + kfree(fb);
>>> + module_put(THIS_MODULE);
>>> +}
>>> +
>>> +static void fblog_do_unregister(struct fb_info *info)
>>> +{
>>> + struct fblog_fb *fb;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (!fb || fb->info != info)
>>> + return;
>>> +
>>> + fblog_fbs[info->node] = NULL;
>>> +
>>> + device_del(&fb->dev);
>>> + put_device(&fb->dev);
>>
>> device_unregister?
>
> Right, I will replace it.
>
>>> +}
>>> +
>>> +static void fblog_do_register(struct fb_info *info, bool force)
>>> +{
>>> + struct fblog_fb *fb;
>>> + int ret;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (fb && fb->info != info) {
>>> + if (!force)
>>> + return;
>>> +
>>> + fblog_do_unregister(fb->info);
>>> + }
>>> +
>>> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>>> + if (!fb)
>>> + return;
>>> +
>>> + fb->info = info;
>>> + __module_get(THIS_MODULE);
>>> + device_initialize(&fb->dev);
>>> + fb->dev.class = fb_class;
>>> + fb->dev.release = fblog_release;
>>> + dev_set_name(&fb->dev, "fblog%d", info->node);
>>> + fblog_fbs[info->node] = fb;
>>> +
>>> + ret = device_add(&fb->dev);
>>> + if (ret) {
>>> + fblog_fbs[info->node] = NULL;
>>> + set_bit(FBLOG_KILLED, &fb->flags);
>>> + put_device(&fb->dev);
>>
>> kfree(fb); ?
>
> No. See device_initialize() in ./drivers/base/core.c. After a call to
> device_initialize() the object is ref-counted so put_device() will
> invoke the fblog_release() callback which will call kfree(fb) itself.
>
>>> + return;
>>> + }
>>> +}
>>> +
>>> +static void fblog_register(struct fb_info *info, bool force)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_register(info, force);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>> +
>>> +static void fblog_unregister(struct fb_info *info)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_unregister(info);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>
>> This locking is needlessly heavy, and could easily pushed down into the
>> fb_do_(un)register functions. It would also help make it clear exactly
>> what the lock is protecting.
>
> I need to call fblog_do_unregister() from within fblog_do_register().
> I cannot release the locks while calling fblog_do_unregister() so I
> need the unlocked fblog_do_unregister() function. So the locking must
> be in a wrapper function.
>
> See below for an explanation of the locks.

I meant something like the below. It doesn't actually make the lock much
more fine-grained, but (IMHO) it does make it a bit more clear how the
lock is being used. I also don't think you need to split
device_initialize and device_add, which can make the code a bit simpler:

static void __fblog_unregister(struct fblog_fb *fb)
{
fblog_fbs[fb->info->node] = NULL;
device_unregister(&fb->dev);
}

static void fblog_unregister(struct fb_info *info)
{
struct fblog_fb *fb;

mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (!fb || fb->info != info) {
mutex_unlock(&fblog_registration_lock);
return;
}

__fblog_unregister(fb);
mutex_unlock(&fblog_registration_lock);
}

static int fblog_register(struct fb_info *info, bool force)
{
struct fblog_fb *fb;
int ret;

mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (fb && fb->info != info) {
if (!force) {
mutex_unlock(&fblog_registration_lock);
return -EEXIST;
}

__fblog_unregister(fb);
}

fb = kzalloc(sizeof(*fb), GFP_KERNEL);
if (!fb)
return;

fb->info = info;
__module_get(THIS_MODULE);
fb->dev.class = fb_class;
fb->dev.release = fblog_release;
dev_set_name(&fb->dev, "fblog%d", info->node);

ret = device_register(&fb->dev);
if (ret) {
mutex_unlock(&fblog_registeration_lock);
put_device(&fb->dev);
return ret;
}

fblog_fbs[info->node] = fb;
mutex_unlock(&fblog_registeration_lock);

return 0;
}

Functions which do:

foo() {
lock(some_lock);
do_foo();
unlock(some_lock);
}

can be a valid pattern for locked/unlocked versions (usually the
unlocked version do_foo will be called __foo). But other times it looks
lazy, where the lock is just serialising everthing and doesn't scale
well. Granted, in a case like this it probably doesn't matter, but it
still a good idea to try and make the locking as fine grained as
possible. It also helps when trying to determine what a lock is actually
protecting, since if do_foo is long, the lock may or may not be
protecting any number of things inside it.

>>> +static int fblog_event(struct notifier_block *self, unsigned long action,
>>> + void *data)
>>> +{
>>> + struct fb_event *event = data;
>>> + struct fb_info *info = event->info;
>>> +
>>> + switch(action) {
>>> + case FB_EVENT_FB_REGISTERED:
>>> + /* This is called when a low-level system driver registers a new
>>> + * framebuffer. The registration lock is held but the console
>>> + * lock might not be held when this is called. */
>>
>> Nitpick:
>>
>> /*
>> * The Linux kernel multi-line
>> * comment style looks like
>> * this.
>> */
>
> I was confused by a recent discussion on the LKML:
> http://comments.gmane.org/gmane.linux.kernel/1282421
> However, turns out they didn't add this to CodingStyle so I will adopt
> the old style again. Will be fixed in the next revision, thanks.
>
>>> + fblog_register(info, true);
>>> + break;
>>> + 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. */
>>> + fblog_unregister(info);
>>> + break;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void fblog_scan(void)
>>> +{
>>> + unsigned int i;
>>> + struct fb_info *info, *tmp;
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>
>> Nitpick:
>>
>> if (IS_ERR_OR_NULL(info))
>
> Didn't know of this macro, thanks.
>
>>> + continue;
>>> +
>>> + fblog_register(info, false);
>>
>> This function should really return a value to indicate if it failed.
>> There is no point continuing if it didn't register anything.
>
> Indeed.
>
>>> + /* There is a very subtle race-condition. Even though we might
>>> + * own a reference to the fb, it may still get unregistered
>>> + * between our call from get_fb_info() and fblog_register().
>>> + * Therefore, we simply check whether the same fb still is
>>> + * registered by calling get_fb_info() again. Only if they
>>> + * differ we know that it got unregistered, therefore, we
>>> + * call fblog_unregister() with the old pointer. */
>>> +
>>> + tmp = get_fb_info(i);
>>> + if (tmp && !IS_ERR(tmp))
>>> + put_fb_info(tmp);
>>> + if (tmp != info)
>>> + fblog_unregister(info);
>>
>> It would be better to fix this issue properly. Calling fblog_unregister
>> here also looks unsafe if the call to fblog_register above failed.
>
> fblog_unregister() does nothing if the passed "info" does not match
> the found "info" so this is safe. And as we are holding a reference to
> "info" here, the pointer is always valid and cannot be used by other
> FBs.
>
> Fixing this properly means changing the locking of fbdev. This can
> either be done by exporting the fbdev-registration-lock (which I want
> to avoid as locking should never be exported in an API) or by changing
> fbdev to provide an scan/enumeration function itself. However, in my
> opinion both would be uglier than using this race-condition-check
> here.
>
> The best way would be redesigning the fbdev in-kernel API. But that
> means checking that fbcon will not break and this is something I
> really don't want to touch.

Fair enough. It might be something to come back to once this gets merged.

>>> + /* Here we either called fblog_unregister() and therefore do not
>>> + * need any reference to the fb, or we can be sure that the FB
>>> + * is registered and FB_EVENT_FB_UNREGISTERED will be called
>>> + * before the last reference is dropped. Hence, we can drop our
>>> + * reference here. */
>>
>> This seems a slightly odd reasoning. Why would you not hold a reference
>> to something you are using?
>
> It's because get_fb_info() takes the fbdev-registration-lock so we
> cannot call it from fblog_event().

That could possibly be fixed by providing an unlocked __get_fb_info
function. Then the ref-counting could be done by calling __get_fb_info
in fblog_register and __put_fb_info in fblog_unregister, so that you
hold the ref-count for the lifetime of the fblog object which I think
makes a bit more sense.

> And fblog_event() calls
> fblog_register() for hotplugged framebuffers so we cannot get a refcnt
> for hotplugged framebuffers.
> Now I can either increase the fbdev-refcount manually without calling
> get_fb_info() in fblog_register() or I can simply drop the framebuffer
> when the fbdev-core notifies me that it is gone. I chose the latter
> one.
>
>>> + put_fb_info(info);
>>> + }
>>> +}
>>> +
>>> +static struct notifier_block fblog_notifier = {
>>> + .notifier_call = fblog_event,
>>> +};
>>>
>>> static int __init fblog_init(void)
>>> {
>>> + int ret;
>>> +
>>> + ret = fb_register_client(&fblog_notifier);
>>> + if (ret) {
>>> + pr_err("cannot register framebuffer notifier\n");
>>> + return ret;
>>> + }
>>> +
>>> + fblog_scan();
>>> +
>>> return 0;
>>> }
>>>
>>> static void __exit fblog_exit(void)
>>> {
>>> + unsigned int i;
>>> + struct fb_info *info;
>>> +
>>> + fb_unregister_client(&fblog_notifier);
>>> +
>>> + /* We scan through the whole registered_fb array here instead of
>>> + * fblog_fbs because we need to get the device lock _before_ the
>>> + * fblog-registration-lock. */
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>> + continue;
>>> +
>>> + fblog_unregister(info);
>>
>> Given the description of the get_fb_info/fblog_register race above, can
>> this unregister the wrong framebuffer?
>
> No. fblog_unregister() will do nothing if the "info" pointers do not
> match. Moreover, this unregisters _all_ framebuffers, so I don't
> understand how this can unregister the "wrong" framebuffer?
>
> But I just noticed a race here. If the fbdev core unregisters a
> framebuffer during my loop, I will not call fblog_unregister() on it,
> as it does not exist anymore. Therefore, I will not free it in my
> fblog_fbs array as the fblog_notifier has already been unregistered.
> So I need to set a global "exiting" flag and fblog_event() shall only
> handle the UNBIND/UNREGISTER events during exit. Then I simply move
> the fb_unregister_client() call below this loop.
>
>>> + put_fb_info(info);
>>> + }
>>> }
>>>
>>> module_init(fblog_init);
>>>
>>
>
> A short explanation for all locks:
> First of all, I spent hours getting this right. The easy way would be
> removing all locks and relying on the fbdev locking (fbcon does this).
> But obviously, that doesn't work as fbdev has no safe way of
> scanning/enumerating all existing devices. And this is needed as fblog
> can be compiled as a module.
> fbdev has a core registration-lock and each framebuffer has its own
> fb-lock. fblog_event() is sometimes called with these locks held,
> sometimes without any locks held (see the comments in this function)
> and I need to work around this.
> So fblog_event() as entry point for hotplugging is called with the
> fbdev-registration-lock held. And it calls fblog_(un)register() which
> uses its own locks. So when calling this from fblog_scan(), I need to
> make sure to guarantee that the locks are acquired in the same order
> as in fblog_event(), otherwise I might have deadlocks. But I have no
> outside access to the fbdev-registration-lock, therefore, the
> fblog-registration-lock is needed and fblog_scan() needs to check for
> those ugly races.

Right, I think providing unlocked versions of get/put_fb_info will help
fix part of this.

> As you mentioned that this locking is needlessly complex, I have to
> disagree.

Sorry, poor choice of words. I meant 'coarse-grained', not complex.
However, some documentation in the code explaining how the locking
works, and what the locking order is never goes amiss.

> I use one lock to protect the registration
> (fblog_registration_lock) and one lock to protect each registered
> framebuffer from concurrent access (struct fblog_fb->lock). This is
> the most common way to protect hotplugged devices and I don't see how
> this can be done with less locks? (these are the only locks that are
> added by fblog).

I was only referring to the 'heavy' usage of the registration lock by
just acquiring it for the whole register/unregister functions. I was
skimming through the code and was assuming that the actual concurrent
part would just be the addition/removal in the fblog_fbs array, and
therefore the lock was being held for much longer than it needed to be.
As shown above, it isn't as bad as I thought it was.

> Normally, this would be all locks I have to access. However, the
> fbdev-core wasn't designed as in-kernel API and thus has very
> inconsistent locking. And all entry points to fblog that are not
> through fbdev-notifier-callbacks (eg, fblog_scan) need to make sure to
> acquire the same locks as the fbdev-core to avoid races with the
> fbdev-core. As this is not possible, because the fbdev-locks are not
> exported, I need to carefully use fbdev-functions that guarantee that
> I have no races. And I think I found the only way to guarantee this.
> If anyone has other ideas, I would be glad to hear them.

Yeah, this makes sense. It would be good, as you say, to not export the
locks for fbmem. I think adding a couple of functions to fbmem.c for
doing unlocked access might help a lot though.

~Ryan