[BUG] drivers/video/sis: deadlock introduced by "fbdev: add mutex for fb_mmap locking"

  On Sun, Jul 05, 2009 at 07:19:33AM -0700, Linus Torvalds wrote:
> 
> 
> On Sun, 5 Jul 2009, Wu Zhangjin wrote:
> > 
> > then it works! so, I guess there is a deadlock introduced by the above
> > commit.
> 
> Hmm. Perhaps more likely, the 'mm_lock' mutex hasn't even been initialized 
> yet.  We appear to have had that problem with matroxfb and sm501fb, and it 
> may be more common than that. See commit f50bf2b2.
> 
> That said, I do agree that the mm_lock seems to be causing more problems 
> than it actually fixes, and maybe we should revert it. Krzysztof?
> 
Looking at this a bit closer, just moving the mutex initialization in to
framebuffer_alloc() should basically fix most of these, at least it
certainly does for sm501fb and for this sis case as well. So, here's a
patch to do that.

As an aside note, matroxfb is the odd one out, as it doesn't use
framebuffer_alloc() directly for whatever reason. This actually raises an
additional issue, in that framebuffer_alloc() is already where other
mutexes are initialized, which will simply never happen on matroxfb
(suggesting that at least the FB_BACKLIGHT and matroxfb combination will
blow up, although perhaps that's not a valid combination).

Signed-off-by: Paul Mundt <lethal@linux-sh.org>

---

 drivers/video/fbmem.c   |    1 -
 drivers/video/fbsysfs.c |    2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Sun, 5 Jul 2009, Paul Mundt wrote:
>  			break;
>  	fb_info->node = i;
>  	mutex_init(&fb_info->lock);
> -	mutex_init(&fb_info->mm_lock);

Why not "lock" as well?

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Sun, Jul 05, 2009 at 07:56:56AM -0700, Linus Torvalds wrote:
> 
> 
> On Sun, 5 Jul 2009, Paul Mundt wrote:
> >  			break;
> >  	fb_info->node = i;
> >  	mutex_init(&fb_info->lock);
> > -	mutex_init(&fb_info->mm_lock);
> 
> Why not "lock" as well?
> 
I had that initially, but matroxfb will break if we do that, and
presently nothing cares about trying to take ->lock that early on.

->mm_lock was a special case as the lock/unlock pairs were sprinkled
around well before initialization, while in the ->lock case all of the
lock/unlock pairs are handled internally by the fbmem code (at least a
quick grep does not show any drivers using it on their own).
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Mon, 6 Jul 2009, Paul Mundt wrote:
> >
> > Why not "lock" as well?
> 
> I had that initially, but matroxfb will break if we do that, and
> presently nothing cares about trying to take ->lock that early on.

I really would rather have consistency than some odd rules like that.

In particular - if matroxfb is different and needs its own lock 
initialization because it doesn't use the common allocation routine, then 
please make _that_ consistent too. Rather than have it special-case just 
one lock that it needs to initialize separately, make it clear that since 
it does its own allocations it needs to initialize _everything_ 
separately.

Otherwise anybody grepping for things will always be confused, since 
depending on random factors they'll notice the initializations in one 
place or the other, and then do the wrong thing - exactly like mm_lock was 
not correctly initialized this time around.

In other words: it's actually _better_ to make the matroxfb pain _more_ 
obvious rather than less.

And maybe we can deprecate the driver, throw it out entirely, or have 
somebody actually fix it?

			Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html