[PATCH] omapfb: Reorder Register_framebuffer call

[PATCH] omapfb: Reorder Register_framebuffer call

[Sergio Aguirre]

This fixes the issue in which mm_lock mutex was attempted to be
used without initializing previously.

Thanks to the testers!
 - OMAP3430 SDP (Anand Gadiyar)
 - OMAP3530 EVM (Vaibhav Hiremath)
 - LogicPD's OMAP boards (Peter Brada)
 - Beagleboard Rev. C2 (Eric Witcher)

Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
Tested-by: Vaibhav Hiremath <hvaibhav@ti.com>
Tested-by: Anand Gadiyar <gadiyar@ti.com>
Tested-by: Peter Barada <peterb@logicpd.com>
Tested-by: Eric Witcher <ewitcher@mindspring.com>
---
 drivers/video/omap/omapfb_main.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/video/omap/omapfb_main.c b/drivers/video/omap/omapfb_main.c
index 125e605..60f9482 100644
--- a/drivers/video/omap/omapfb_main.c
+++ b/drivers/video/omap/omapfb_main.c
@@ -1503,12 +1503,21 @@ static int fbinfo_init(struct omapfb_device *fbdev, struct fb_info *info)
 	var->rotate	  = def_rotate;
 	var->bits_per_pixel = fbdev->panel->bpp;
 
+	r = register_framebuffer(info);
+	if (r != 0) {
+		dev_err(fbdev->dev,
+			"registering framebuffer failed\n");
+		return r;
+	}
+
 	set_fb_var(info, var);
 	set_fb_fix(info);
 
 	r = fb_alloc_cmap(&info->cmap, 16, 0);
-	if (r != 0)
+	if (r != 0) {
 		dev_err(fbdev->dev, "unable to allocate color map memory\n");
+		unregister_framebuffer(info);
+	}
 
 	return r;
 }
@@ -1773,15 +1782,8 @@ static int omapfb_do_probe(struct platform_device *pdev,
 	init_state++;
 
 	vram = 0;
-	for (i = 0; i < fbdev->mem_desc.region_cnt; i++) {
-		r = register_framebuffer(fbdev->fb_info[i]);
-		if (r != 0) {
-			dev_err(fbdev->dev,
-				"registering framebuffer %d failed\n", i);
-			goto cleanup;
-		}
+	for (i = 0; i < fbdev->mem_desc.region_cnt; i++)
 		vram += fbdev->mem_desc.region[i].size;
-	}
 
 	fbdev->state = OMAPFB_ACTIVE;
 
-- 
1.6.3.2

RE: [PATCH] omapfb: Reorder Register_framebuffer call

[Aguirre Rodriguez, Sergio Alberto]

Hi,

This patch has been lying around for some weeks now, with just responses about successful testing (detailed in the description).

Is there anything holding it for merge? Or should I try sending it to other ML?

BTW, It applies cleanly on mainline aswell.

Regards,
Sergio

> -----Original Message-----
> From: Sergio Aguirre [mailto:sergio.a.aguirre@gmail.com] On Behalf Of
> Sergio Aguirre
> Sent: Saturday, September 12, 2009 11:34 AM
> To: Imre Deak
> Cc: linux-fbdev-devel@lists.sourceforge.net; linux-omap@vger.kernel.org;
> Aguirre Rodriguez, Sergio Alberto
> Subject: [PATCH] omapfb: Reorder Register_framebuffer call
> 
> This fixes the issue in which mm_lock mutex was attempted to be
> used without initializing previously.
> 
> Thanks to the testers!
>  - OMAP3430 SDP (Anand Gadiyar)
>  - OMAP3530 EVM (Vaibhav Hiremath)
>  - LogicPD's OMAP boards (Peter Brada)
>  - Beagleboard Rev. C2 (Eric Witcher)
> 
> Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> Tested-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Tested-by: Anand Gadiyar <gadiyar@ti.com>
> Tested-by: Peter Barada <peterb@logicpd.com>
> Tested-by: Eric Witcher <ewitcher@mindspring.com>
> ---
>  drivers/video/omap/omapfb_main.c |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/video/omap/omapfb_main.c
> b/drivers/video/omap/omapfb_main.c
> index 125e605..60f9482 100644
> --- a/drivers/video/omap/omapfb_main.c
> +++ b/drivers/video/omap/omapfb_main.c
> @@ -1503,12 +1503,21 @@ static int fbinfo_init(struct omapfb_device
> *fbdev, struct fb_info *info)
>  	var->rotate	  = def_rotate;
>  	var->bits_per_pixel = fbdev->panel->bpp;
> 
> +	r = register_framebuffer(info);
> +	if (r != 0) {
> +		dev_err(fbdev->dev,
> +			"registering framebuffer failed\n");
> +		return r;
> +	}
> +
>  	set_fb_var(info, var);
>  	set_fb_fix(info);
> 
>  	r = fb_alloc_cmap(&info->cmap, 16, 0);
> -	if (r != 0)
> +	if (r != 0) {
>  		dev_err(fbdev->dev, "unable to allocate color map memory\n");
> +		unregister_framebuffer(info);
> +	}
> 
>  	return r;
>  }
> @@ -1773,15 +1782,8 @@ static int omapfb_do_probe(struct platform_device
> *pdev,
>  	init_state++;
> 
>  	vram = 0;
> -	for (i = 0; i < fbdev->mem_desc.region_cnt; i++) {
> -		r = register_framebuffer(fbdev->fb_info[i]);
> -		if (r != 0) {
> -			dev_err(fbdev->dev,
> -				"registering framebuffer %d failed\n", i);
> -			goto cleanup;
> -		}
> +	for (i = 0; i < fbdev->mem_desc.region_cnt; i++)
>  		vram += fbdev->mem_desc.region[i].size;
> -	}
> 
>  	fbdev->state = OMAPFB_ACTIVE;
> 
> --
> 1.6.3.2
> 

Re: [PATCH] omapfb: Reorder Register_framebuffer call

[Felipe Contreras]

On Mon, Sep 14, 2009 at 5:43 PM, Aguirre Rodriguez, Sergio Alberto
<saaguirre@ti.com> wrote:
> Hi,
>
> This patch has been lying around for some weeks now, with just responses about successful testing (detailed in the description).
>
> Is there anything holding it for merge? Or should I try sending it to other ML?
>
> BTW, It applies cleanly on mainline aswell.

Try running 'get_maintainer' script to find other people that have
worked on this code:
Trilok Soni <soni.trilok@gmail.com>
Andrew Morton <akpm@linux-foundation.org>
Tony Lindgren <tony@atomide.com>
Russell King <rmk+kernel@arm.linux.org.uk>
Krzysztof Helt <krzysztof.h1@wp.pl>

My feedback is: please mention in the message how the bug can be triggered.

And watch your uppercases:
omapfb: reorder register_framebuffer call

-- 
Felipe Contreras

Re: [APPLIED] [PATCH] omapfb: Reorder Register_framebuffer call

[Tony Lindgren]

* Jan Blunck <jblunck@infradead.org> [090922 07:59]:
> On Tue, Sep 22, 2009 at 3:31 AM, Tony Lindgren <tony@atomide.com> wrote:
> > This patch has been applied to the linux-omap
> > by youw fwiendly patch wobot.
> >
> > Branch in linux-omap: omap-fixes
> >
> > Initial commit ID (Likely to change): 9aef1066fb5ca8506068eaab1c552ecca4c85475
> >
> > PatchWorks
> > http://patchwork.kernel.org/patch/47089/
> >
> 

Added back the original Cc's that were dropped from the linux-omap
commit message.

> Is it actually safe to do this? The framebuffer can be used directly
> after it is registered. In this case it would mean it is used before
> it is even fully initialized (set_fb_var(), set_fb_fix(), ... are
> being called).

Good point, dropping the patch.

Also, let's let Tomi Valkeinen deal with queueing up the omap fb code.
I can then merge Tomi's branck into linux-omap master branch as needed.

Regards,

Tony

RE: [APPLIED] [PATCH] omapfb: Reorder Register_framebuffer call

[Aguirre Rodriguez, Sergio Alberto]

From: Tony Lindgren [tony@atomide.com]
Sent: Tuesday, September 22, 2009 11:28 AM
> * Jan Blunck <jblunck@infradead.org> [090922 07:59]:
> > On Tue, Sep 22, 2009 at 3:31 AM, Tony Lindgren <tony@atomide.com> wrote:
> > > This patch has been applied to the linux-omap
> > > by youw fwiendly patch wobot.
> > >
> > > Branch in linux-omap: omap-fixes
> > >
> > > Initial commit ID (Likely to change): 9aef1066fb5ca8506068eaab1c552ecca4c85475
> > >
> > > PatchWorks
> > > http://patchwork.kernel.org/patch/47089/
> > >
> >
> 
> Added back the original Cc's that were dropped from the linux-omap
> commit message.
> 
> > Is it actually safe to do this? The framebuffer can be used directly
> > after it is registered. In this case it would mean it is used before
> > it is even fully initialized (set_fb_var(), set_fb_fix(), ... are
> > being called).
> 
> Good point, dropping the patch.

Hmm, ok. I guess i'll rework this patch considering that..

I ran some framebuffer tests with this patch applied, and they worked fine for me.

The only thing is that i didn't saw Tux on bootup...

Actually, nobody ever gave this kind of feedback, which was the initial idea.

Just people worried about booting properly.

Anyways, thanks for your time! I appreciate it.

Regards,
Sergio
> 
> Also, let's let Tomi Valkeinen deal with queueing up the omap fb code.
> I can then merge Tomi's branck into linux-omap master branch as needed.
> 
> Regards,
> 
> Tony
> 
> 

Re: [APPLIED] [PATCH] omapfb: Reorder Register_framebuffer call

[Jan Blunck]

On Tue, Sep 22, 2009 at 6:57 PM, Aguirre Rodriguez, Sergio Alberto
<saaguirre@ti.com> wrote:
> From: Tony Lindgren [tony@atomide.com]
> Sent: Tuesday, September 22, 2009 11:28 AM
>> * Jan Blunck <jblunck@infradead.org> [090922 07:59]:
>> > On Tue, Sep 22, 2009 at 3:31 AM, Tony Lindgren <tony@atomide.com> wrote:
>> > > This patch has been applied to the linux-omap
>> > > by youw fwiendly patch wobot.
>> > >
>> > > Branch in linux-omap: omap-fixes
>> > >
>> > > Initial commit ID (Likely to change): 9aef1066fb5ca8506068eaab1c552ecca4c85475
>> > >
>> > > PatchWorks
>> > > http://patchwork.kernel.org/patch/47089/
>> > >
>> >
>>
>> Added back the original Cc's that were dropped from the linux-omap
>> commit message.
>>
>> > Is it actually safe to do this? The framebuffer can be used directly
>> > after it is registered. In this case it would mean it is used before
>> > it is even fully initialized (set_fb_var(), set_fb_fix(), ... are
>> > being called).
>>
>> Good point, dropping the patch.
>
> Hmm, ok. I guess i'll rework this patch considering that..
>
> I ran some framebuffer tests with this patch applied, and they worked fine for me.
>
> The only thing is that i didn't saw Tux on bootup...
>
> Actually, nobody ever gave this kind of feedback, which was the initial idea.
>

Sorry, I didn't look into it earlier.

BTW, I actually wonder if it's really necessary to initialize the
mutex in register_framebuffer() or why it couldn't be done during
allocation.

Cheers,
Jan

Re: [Linux-fbdev-devel] [APPLIED] [PATCH] omapfb: Reorder Register_framebuffer call

[Florian Tobias Schandinat]

Jan Blunck schrieb:
> On Tue, Sep 22, 2009 at 6:57 PM, Aguirre Rodriguez, Sergio Alberto
> <saaguirre@ti.com> wrote:
>> From: Tony Lindgren [tony@atomide.com]
>> Sent: Tuesday, September 22, 2009 11:28 AM
>>> * Jan Blunck <jblunck@infradead.org> [090922 07:59]:
>>>> On Tue, Sep 22, 2009 at 3:31 AM, Tony Lindgren <tony@atomide.com> wrote:
>>>>> This patch has been applied to the linux-omap
>>>>> by youw fwiendly patch wobot.
>>>>>
>>>>> Branch in linux-omap: omap-fixes
>>>>>
>>>>> Initial commit ID (Likely to change): 9aef1066fb5ca8506068eaab1c552ecca4c85475
>>>>>
>>>>> PatchWorks
>>>>> http://patchwork.kernel.org/patch/47089/
>>>>>
>>> Added back the original Cc's that were dropped from the linux-omap
>>> commit message.
>>>
>>>> Is it actually safe to do this? The framebuffer can be used directly
>>>> after it is registered. In this case it would mean it is used before
>>>> it is even fully initialized (set_fb_var(), set_fb_fix(), ... are
>>>> being called).
>>> Good point, dropping the patch.
>> Hmm, ok. I guess i'll rework this patch considering that..
>>
>> I ran some framebuffer tests with this patch applied, and they worked fine for me.
>>
>> The only thing is that i didn't saw Tux on bootup...
>>
>> Actually, nobody ever gave this kind of feedback, which was the initial idea.
>>
> 
> Sorry, I didn't look into it earlier.
> 
> BTW, I actually wonder if it's really necessary to initialize the
> mutex in register_framebuffer() or why it couldn't be done during
> allocation.

This small discussion between Linus and Krzysztof might explain it:
http://marc.info/?l=linux-kernel&m=124703449332064&w=2
or to summarize:
It is done the way it is done to keep drivers working that use 
statically declared fb_info.

Although I agree, that it would be cleaner the other way around.


Regards,

Florian Tobias Schandinat