[Linux-fbdev-devel] fix fb_info->lock and mm->mmap_sem problem again

[Krzysztof H. <krz...@po...>]

Hi,

I have run into the lockdep problem between the fb_info->lock and mm->mmap_sem again.
I run the Mplayer with fbdev output device. The latest git tree.

I looked through the fb_info->lock usage and I have some questions.

The main issue is that fb_mmap acquires the fb_info->lock. I wonder
what is protected there. The part of the fb_mmap which uses the lock
is below:

...
    if (fb->fb_mmap) {
        int res;
        mutex_lock(&info->lock);
        res = fb->fb_mmap(info, vma);
        mutex_unlock(&info->lock);
        return res;
    }

    mutex_lock(&info->lock);

    /* frame buffer memory */
    start = info->fix.smem_start;
    len = PAGE_ALIGN((start & ~PAGE_MASK) + info->fix.smem_len);
    if (off >= len) {
        /* memory mapped io */
        off -= len;
        if (info->var.accel_flags) {
            mutex_unlock(&info->lock);
            return -EINVAL;
        }
        start = info->fix.mmio_start;
        len = PAGE_ALIGN((start & ~PAGE_MASK) + info->fix.mmio_len);
    }
    mutex_unlock(&info->lock);
...

The lock protects two things:
1. Read of the info->fix.smem_start and info->fix.smem_len.
2. The fbops->fb_mmap call.

The first point seems redundant as I have checked and
the smem_start and smem_len are only set in the
probe and release functions. They are always set
before the register_framebuffer() is called and
after the unregister_framebuffer() call.

If the lock is pushed down into the second point it
becomes the same issue as the first point.

I tracked the usage of the fb_info->lock in the fb
layer and it is used for various purposes:

1. It is used in the fb_open and fb_release functions
as an open() and close() synchronization .

2. It is used in various functions to guard an entry to
the fb_notifier_call_chain() (suspend, blank, register_framebuffer,
unregister_framebuffer).

3. It is used in the fb_ioctl to protect against concurrent
calls to the fb layer.

I understand the third purpose. I am not sure if the
first and the third purpose need to use the same mutex
(ie. if the ioctl and open/close needs to be protected 
against each other - maybe a higher layer does it anyway).

The second purpose (protection of the fb_notifier_call_chain)
is quite a mistery to me. The fb_notifier_call_chain() is just
the blocking_notifier_call_chain() which is already synchronized
with a rw semaphore.

A pure mystery to me is what the fb_info->lock does in the fb_mmap.
What is there trying to be protected?

IMHO, there should be no fb_info->lock usage in the fb_mmap.
If someone needs this lock in driver's fb_mmap (fbops->fb_mmap)
he should lock it by himself possibly releasing the mmap_sem
before and acquiring it after.

Another solution is to divide the fb_info->lock into two
or more smaller mutexes (e.g. mmap_lock so an notifier
or color map change does not block the fb_mmap function).

I want to know what should be protected inside the fb layer.
E.g. changing fb_info->var and reading/using its values should
be synchronized. I am going to prepare few patches to fix
issues I found during code inspection (fb_info->lock
not initialized early enough, some driver's fb_mmap()
functions are just copies of the common fb_mmap, possible
races, etc).

Please forgive me if the above questions are plain stupid.
Krzysztof

----------------------------------------------------------------------
Gotowka na koncie. Otworz konto direct i wez podwojny limit. 	 
http://link.interia.pl/f2145