Re: matroxfb: fix regression with uninitalized fb_info->mm_lock mutex (second head)

Re: matroxfb: fix regression with uninitalized fb_info->mm_lock mutex (second head)

[Linus Torvalds]

On Tue, 7 Jul 2009, Krzysztof Helt wrote:
> 
> Remove redundant locking by the mm_lock mutex before a second head of matrox 
> framebuffer is registered.

Why do you write misleading commentary like this.

> +/*
> + * This function is called before the register_framebuffer so
> + * no locking is needed.
> + */

Or this?

It's not about "needed". The locking is not only not needed, it would be 
BUGGY.

And it's not "redundant". That implies that it's done somewhere else. It's 
more than "not needed" - it would be actively buggy to lock things there.

I really don't like how you're approaching this. You're ignoring the real 
issues I ask you, you're writing misleading comments and commit messages, 
and the end result is fragile code. I still don't understand why you 
insist on initializing those things late, which is the primary problem 
here.

		Linus

matroxfb: fix regression with uninitalized fb_info->mm_lock mutex (second head)

[Krzysztof Helt]

From: Krzysztof Helt <krzysztof.h1@wp.pl>

Remove redundant locking by the mm_lock mutex before a second head of matrox 
framebuffer is registered.

This fixes a problem with uninitialized the fb_info->mm_lock mutex introduced 
by the commit 537a1bf059f " fbdev: add mutex for fb_mmap locking"

Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>

---

diff --git a/drivers/video/matrox/matroxfb_crtc2.c b/drivers/video/matrox/matroxfb_crtc2.c
index 909e10a..6caa369 100644
--- a/drivers/video/matrox/matroxfb_crtc2.c
+++ b/drivers/video/matrox/matroxfb_crtc2.c
@@ -289,16 +289,18 @@ static int matroxfb_dh_release(struct fb_info* info, int user) {
 #undef m2info
 }
 
+/*
+ * This function is called before the register_framebuffer so
+ * no locking is needed.
+ */
 static void matroxfb_dh_init_fix(struct matroxfb_dh_fb_info *m2info)
 {
 	struct fb_fix_screeninfo *fix = &m2info->fbcon.fix;
 
 	strcpy(fix->id, "MATROX DH");
 
-	mutex_lock(&m2info->fbcon.mm_lock);
 	fix->smem_start = m2info->video.base;
 	fix->smem_len = m2info->video.len_usable;
-	mutex_unlock(&m2info->fbcon.mm_lock);
 	fix->ypanstep = 1;
 	fix->ywrapstep = 0;
 	fix->xpanstep = 8;	/* TBD */

----------------------------------------------------------------------
Sprawdz promocje ubezpieczen komunikacyjnych w Ergo  Hestia
http://link.interia.pl/f222c