| Message ID | A24693684029E5489D1D202277BE89444B781B6F@dlee02.ent.ti.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Tomi Valkeinen |
| Headers | show |
On Tue, 2009-09-29 at 16:14 +0200, ext Aguirre Rodriguez, Sergio Alberto wrote: > From: Sergio Aguirre <saaguirre@ti.com> > > Acquiring mutex before framebuffer registration doesn't make sense, > As there's no danger of external access to the memory related fields. What problem does this patch solve? It makes the code more complex. Tomi > > NOTE: PLEASE REVIEW! I'M NOT AN EXPERT ON THIS. > > Signed-off-by: Sergio Aguirre <saaguirre@ti.com> > --- > drivers/video/omap/omapfb_main.c | 22 ++++++++++++++-------- > 1 files changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/video/omap/omapfb_main.c b/drivers/video/omap/omapfb_main.c > index 125e605..0d0c8c8 100644 > --- a/drivers/video/omap/omapfb_main.c > +++ b/drivers/video/omap/omapfb_main.c > @@ -393,7 +393,7 @@ static void omapfb_sync(struct fb_info *fbi) > * Set fb_info.fix fields and also updates fbdev. > * When calling this fb_info.var must be set up already. > */ > -static void set_fb_fix(struct fb_info *fbi) > +static void set_fb_fix(struct fb_info *fbi, int from_init) > { > struct fb_fix_screeninfo *fix = &fbi->fix; > struct fb_var_screeninfo *var = &fbi->var; > @@ -403,10 +403,16 @@ static void set_fb_fix(struct fb_info *fbi) > > rg = &plane->fbdev->mem_desc.region[plane->idx]; > fbi->screen_base = rg->vaddr; > - mutex_lock(&fbi->mm_lock); > - fix->smem_start = rg->paddr; > - fix->smem_len = rg->size; > - mutex_unlock(&fbi->mm_lock); > + > + if (!from_init) { > + mutex_lock(&fbi->mm_lock); > + fix->smem_start = rg->paddr; > + fix->smem_len = rg->size; > + mutex_unlock(&fbi->mm_lock); > + } else { > + fix->smem_start = rg->paddr; > + fix->smem_len = rg->size; > + } > > fix->type = FB_TYPE_PACKED_PIXELS; > bpp = var->bits_per_pixel; > @@ -704,7 +710,7 @@ static int omapfb_set_par(struct fb_info *fbi) > int r = 0; > > omapfb_rqueue_lock(fbdev); > - set_fb_fix(fbi); > + set_fb_fix(fbi, 0); > r = ctrl_change_mode(fbi); > omapfb_rqueue_unlock(fbdev); > > @@ -904,7 +910,7 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi) > if (old_size != size) { > if (size) { > memcpy(&fbi->var, new_var, sizeof(fbi->var)); > - set_fb_fix(fbi); > + set_fb_fix(fbi, 0); > } else { > /* > * Set these explicitly to indicate that the > @@ -1504,7 +1510,7 @@ static int fbinfo_init(struct omapfb_device *fbdev, struct fb_info *info) > var->bits_per_pixel = fbdev->panel->bpp; > > set_fb_var(info, var); > - set_fb_fix(info); > + set_fb_fix(info, 1); > > r = fb_alloc_cmap(&info->cmap, 16, 0); > if (r != 0) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tomi Valkeinen [tomi.valkeinen@nokia.com] Sent: Tuesday, September 29, 2009 9:22 AM > On Tue, 2009-09-29 at 16:14 +0200, ext Aguirre Rodriguez, Sergio Alberto > wrote: > > From: Sergio Aguirre <saaguirre@ti.com> > > > > Acquiring mutex before framebuffer registration doesn't make sense, > > As there's no danger of external access to the memory related fields. > > What problem does this patch solve? It makes the code more complex. > Tomi, Thanks for your time. The problem was that, during platform driver registration, this sequence was executed: -> omapfb_probe -> omapfb_do_probe -> planes_init -> fbinfo_init -> set_fb_fix ... -> register_framebuffer And then, inside that function, an attempt of acquiring a mutex failed, because it wasn't initialized before trying it: mutex_lock(&fbi->mm_lock); It is actually initialized later in omapfb_do_probe in register_framebuffer call. So, how is the best to solve this then? BTW, this problem is found on current Linus tree, and in current l-o tree aswell, and i saw it on my 3430SDP VG5.0.1. Regards, Sergio > Tomi > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: linux-omap-owner@vger.kernel.org [linux-omap-owner@vger.kernel.org] On Behalf Of Aguirre Rodriguez, Sergio Alberto [saaguirre@ti.com] Sent: Tuesday, September 29, 2009 9:34 AM > From: Tomi Valkeinen [tomi.valkeinen@nokia.com] > Sent: Tuesday, September 29, 2009 9:22 AM > > On Tue, 2009-09-29 at 16:14 +0200, ext Aguirre Rodriguez, Sergio Alberto > > wrote: > > > From: Sergio Aguirre <saaguirre@ti.com> > > > > > > Acquiring mutex before framebuffer registration doesn't make sense, > > > As there's no danger of external access to the memory related fields. > > > > What problem does this patch solve? It makes the code more complex. > > > > Tomi, > > Thanks for your time. > > The problem was that, during platform driver registration, > this sequence was executed: > > -> omapfb_probe > -> omapfb_do_probe > -> planes_init > -> fbinfo_init > -> set_fb_fix Forgot to mark that the attempt to use the mutex its done here in set_fb_fix function > ... > -> register_framebuffer > > And then, inside that function, an attempt of acquiring a > mutex failed, because it wasn't initialized before trying it: > > mutex_lock(&fbi->mm_lock); > > It is actually initialized later in omapfb_do_probe in register_framebuffer call. > > So, how is the best to solve this then? > > BTW, this problem is found on current Linus tree, and in > current l-o tree aswell, and i saw it on my 3430SDP VG5.0.1. BTW #2. This makes framebuffer work on my board. I can see Tux during bootup, and also i was able to run some basic framebuffer tests on my board without apparent issues. > > Regards, > Sergio > > > Tomi > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Aguirre Rodriguez, Sergio Alberto wrote: > And then, inside that function, an attempt of acquiring a > mutex failed, because it wasn't initialized before trying it: > > mutex_lock(&fbi->mm_lock); I would like to stress that this does cause the kernel to crash during boot up because this is mutex not initialised before it is used. > BTW, this problem is found on current Linus tree, and in > current l-o tree aswell, and i saw it on my 3430SDP VG5.0.1. I am seeing this too on the beagle-board and so it appears to impact a few omap3 boards. Cheers Jon-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2009-09-29 at 16:34 +0200, ext Aguirre Rodriguez, Sergio Alberto wrote: > From: Tomi Valkeinen [tomi.valkeinen@nokia.com] > Sent: Tuesday, September 29, 2009 9:22 AM > > On Tue, 2009-09-29 at 16:14 +0200, ext Aguirre Rodriguez, Sergio Alberto > > wrote: > > > From: Sergio Aguirre <saaguirre@ti.com> > > > > > > Acquiring mutex before framebuffer registration doesn't make sense, > > > As there's no danger of external access to the memory related fields. > > > > What problem does this patch solve? It makes the code more complex. > > > > Tomi, > > Thanks for your time. > > The problem was that, during platform driver registration, > this sequence was executed: > > -> omapfb_probe > -> omapfb_do_probe > -> planes_init > -> fbinfo_init > -> set_fb_fix > ... > -> register_framebuffer > > And then, inside that function, an attempt of acquiring a > mutex failed, because it wasn't initialized before trying it: > > mutex_lock(&fbi->mm_lock); > > It is actually initialized later in omapfb_do_probe in register_framebuffer call. > > So, how is the best to solve this then? Oh, I wasn't implying that there's something wrong with the fix, I just didn't know what it was fixing =). Looks like a valid fix to me. Tomi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tomi Valkeinen [tomi.valkeinen@nokia.com] Sent: Tuesday, September 29, 2009 9:53 AM > On Tue, 2009-09-29 at 16:34 +0200, ext Aguirre Rodriguez, Sergio Alberto > wrote: > > From: Tomi Valkeinen [tomi.valkeinen@nokia.com] > > Sent: Tuesday, September 29, 2009 9:22 AM > > > On Tue, 2009-09-29 at 16:14 +0200, ext Aguirre Rodriguez, Sergio Alberto > > > wrote: > > > > From: Sergio Aguirre <saaguirre@ti.com> > > > > > > > > Acquiring mutex before framebuffer registration doesn't make sense, > > > > As there's no danger of external access to the memory related fields. > > > > > > What problem does this patch solve? It makes the code more complex. > > > > > > > Tomi, > > > > Thanks for your time. > > > > The problem was that, during platform driver registration, > > this sequence was executed: > > > > -> omapfb_probe > > -> omapfb_do_probe > > -> planes_init > > -> fbinfo_init > > -> set_fb_fix > > ... > > -> register_framebuffer > > > > And then, inside that function, an attempt of acquiring a > > mutex failed, because it wasn't initialized before trying it: > > > > mutex_lock(&fbi->mm_lock); > > > > It is actually initialized later in omapfb_do_probe in register_framebuffer call. > > > > So, how is the best to solve this then? > > Oh, I wasn't implying that there's something wrong with the fix, I just > didn't know what it was fixing =). Ok, no prob :) Should I improve the patch description? Any suggestion coming from an expert ;) ? > > Looks like a valid fix to me. Great! :) Once you're ok with the description, I can add your "Acked-by:" and send to linux-fb-devel ML. Would that be ok? Regards, Sergio > > Tomi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From bba2d89ea45ec0359dec6fe21c1756ad3406e347 Mon Sep 17 00:00:00 2001 From: Sergio Aguirre <saaguirre@ti.com> Date: Tue, 29 Sep 2009 07:44:19 -0500 Subject: [PATCH] omapfb: Condition mutex acquisition Acquiring mutex before framebuffer registration doesn't make sense, As there's no danger of external access to the memory related fields. NOTE: PLEASE REVIEW! I'M NOT AN EXPERT ON THIS. Signed-off-by: Sergio Aguirre <saaguirre@ti.com> --- drivers/video/omap/omapfb_main.c | 22 ++++++++++++++-------- 1 files changed, 14 insertions(+), 8 deletions(-) diff --git a/drivers/video/omap/omapfb_main.c b/drivers/video/omap/omapfb_main.c index 125e605..0d0c8c8 100644 --- a/drivers/video/omap/omapfb_main.c +++ b/drivers/video/omap/omapfb_main.c @@ -393,7 +393,7 @@ static void omapfb_sync(struct fb_info *fbi) * Set fb_info.fix fields and also updates fbdev. * When calling this fb_info.var must be set up already. */ -static void set_fb_fix(struct fb_info *fbi) +static void set_fb_fix(struct fb_info *fbi, int from_init) { struct fb_fix_screeninfo *fix = &fbi->fix; struct fb_var_screeninfo *var = &fbi->var; @@ -403,10 +403,16 @@ static void set_fb_fix(struct fb_info *fbi) rg = &plane->fbdev->mem_desc.region[plane->idx]; fbi->screen_base = rg->vaddr; - mutex_lock(&fbi->mm_lock); - fix->smem_start = rg->paddr; - fix->smem_len = rg->size; - mutex_unlock(&fbi->mm_lock); + + if (!from_init) { + mutex_lock(&fbi->mm_lock); + fix->smem_start = rg->paddr; + fix->smem_len = rg->size; + mutex_unlock(&fbi->mm_lock); + } else { + fix->smem_start = rg->paddr; + fix->smem_len = rg->size; + } fix->type = FB_TYPE_PACKED_PIXELS; bpp = var->bits_per_pixel; @@ -704,7 +710,7 @@ static int omapfb_set_par(struct fb_info *fbi) int r = 0; omapfb_rqueue_lock(fbdev); - set_fb_fix(fbi); + set_fb_fix(fbi, 0); r = ctrl_change_mode(fbi); omapfb_rqueue_unlock(fbdev); @@ -904,7 +910,7 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi) if (old_size != size) { if (size) { memcpy(&fbi->var, new_var, sizeof(fbi->var)); - set_fb_fix(fbi); + set_fb_fix(fbi, 0); } else { /* * Set these explicitly to indicate that the @@ -1504,7 +1510,7 @@ static int fbinfo_init(struct omapfb_device *fbdev, struct fb_info *info) var->bits_per_pixel = fbdev->panel->bpp; set_fb_var(info, var); - set_fb_fix(info); + set_fb_fix(info, 1); r = fb_alloc_cmap(&info->cmap, 16, 0); if (r != 0) -- 1.6.0.4