Re: [PATCH] fb: convert lock/unlock_kernel() into local fb mutex
From: <hidden>
Date: 2008-08-05 10:07:40
On Tue, 5 Aug 2008 07:37:55 +0200 Krzysztof Helt [off-list ref] wrote:quoted
From: Krzysztof Helt <redacted> Change lock_kernel()/unlock_kernel() to local fb mutex. Each frame buffer instance has its own mutex. The one line try_to_load() function is unrolled to request_module() in two places for readability. Signed-off-by: Krzysztof Helt <redacted> --- The only caveat is that I had to put request_module() call outside this new mutex while it was inside the lock_kernel() (inside fb_open()).Why did you have to do that?
This has to be done if the per fb mutex (fb_info->lock) is used as the request_module is called before fb_info pointer exists. I tried to avoid one big mutex for the fb layer (which would not be worse than lock_kernel now). Alternative solution is to provide one global mutex inside the fb layer to mutex fb_open and fb_close functions and another mutex per each fb (fb_info->lock) for all other locking (so ioctls on different fb instances/cards can be concurrent).
The request_module() in fb_ioctl() is inside the lock...
Yes. This is some mistake I suppose as the fb_info pointer is used before the switch then it is checked against null inside the switch. Either the request_module is never called there (dead code) or a core dump happens sometimes. As I see it now, the whole fbmem locking is somehow not right as IMO the registered_fb[idx] access should be locked to avoid situation that someone is adding/removing the fb and another function grabs fb_info pointer from the registered_fb[].
quoted
If the request_module() should be called only inside this mutex please drop the patch.No, request_module() doesn't need lock_kernel(). The only possible problem I can see is the sole remaining lock_kernel() in drivers/video/console/vgacon.c. The fbcon.c code used to be serialised against vgacon_do_font_op() via lock_kernel, but with this change vgacon_do_font_op() can now run concurrently with all the code which you've put inside fb_info.mutex_lock. Probably not a problem, but please check that sometime.
I will look into it and either resend this patch or another one. Should I cc some other list or person next time? Kind regards, Krzysztof ---------------------------------------------------------------------- Wymien zeszyty na notebooka! Sprawdz >>> http://link.interia.pl/f1eab ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/