# Re: \[PATCH\] fb: convert lock/unlock\_kernel() into local fb mutex

- From: \<hidden\>
- Date: 2008-08-05 10:07:40 
- List: linux-fbdev
- Page: https://ratatoskr.run/linux-fbdev/2008/08/13458579
- Thread: https://ratatoskr.run/linux-fbdev/2008/08/13458579/t.md

```text
> On Tue, 5 Aug 2008 07:37:55 +0200 Krzysztof Helt [ref]
> wrote:
> 
> > 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[].

> > 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=/
```