* Re: [PATCH V2] omap2/omapfb: make DBG() more resistant in if-else
From: Tomi Valkeinen @ 2011-05-10 12:16 UTC (permalink / raw)
To: Niels de Vos
Cc: linux-omap, linux-fbdev, linux-kernel, Geert Uytterhoeven,
Sanjeev Premi
In-Reply-To: <1305025653-17138-1-git-send-email-ndevos@redhat.com>
On Tue, 2011-05-10 at 12:07 +0100, Niels de Vos wrote:
> When DBG() is used in a simple if-else, the resulting code path
> currently depends on the definition of DBG(). Inserting the statement in
> a "do { ... } while (0)" prevents this possible misuse.
>
> Signed-off-by: Niels de Vos <ndevos@redhat.com>
Thanks, I'll add this to the dss2 tree.
Tomi
^ permalink raw reply
* Re: [PATCH] omap2/omapfb: make DBG() more resistant in if-else constructions
From: Geert Uytterhoeven @ 2011-05-10 12:14 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Niels de Vos, linux-omap, linux-fbdev, linux-kernel
In-Reply-To: <1305029285.2045.38.camel@deskari>
On Tue, May 10, 2011 at 14:08, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2011-05-10 at 11:42 +0200, Geert Uytterhoeven wrote:
>> What about using the standard pr_debug()/dev_dbg() instead?
>> With dynamic debug, it can be enabled at run time.
>> As a bonus, you get printf()-style format checking if debugging is disabled.
>
> Yes, dev_dbg & co. would be better.
>
> However, one thing I dislike about them is the extra stuff they print.
> For example, for omapfb and omapdss dev_dbg will print:
>
> omapfb omapfb: foo
> omapdss_dss omapdss_dss: foo
>
> I originally added the debug macros to omapdss to be able to
> automatically print the DSS module name, as at that point there was only
> one big omapdss device. And I guess I just followed with similar macro
> in omapfb also. But I believe both omapdss and omapfb should be changed
> to dev_* prints sometime soon.
If you don't want the extra baggage, do
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
and use pr_debug().
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] omap2/omapfb: make DBG() more resistant in if-else
From: Tomi Valkeinen @ 2011-05-10 12:08 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Niels de Vos, linux-omap, linux-fbdev, linux-kernel
In-Reply-To: <BANLkTinEJZ=fvJmWRkQ7kKyxbFRaJnum7g@mail.gmail.com>
On Tue, 2011-05-10 at 11:42 +0200, Geert Uytterhoeven wrote:
> What about using the standard pr_debug()/dev_dbg() instead?
> With dynamic debug, it can be enabled at run time.
> As a bonus, you get printf()-style format checking if debugging is disabled.
Yes, dev_dbg & co. would be better.
However, one thing I dislike about them is the extra stuff they print.
For example, for omapfb and omapdss dev_dbg will print:
omapfb omapfb: foo
omapdss_dss omapdss_dss: foo
I originally added the debug macros to omapdss to be able to
automatically print the DSS module name, as at that point there was only
one big omapdss device. And I guess I just followed with similar macro
in omapfb also. But I believe both omapdss and omapfb should be changed
to dev_* prints sometime soon.
Tomi
^ permalink raw reply
* [PATCH V2] omap2/omapfb: make DBG() more resistant in if-else constructions
From: Niels de Vos @ 2011-05-10 11:07 UTC (permalink / raw)
To: linux-omap
Cc: linux-fbdev, linux-kernel, Geert Uytterhoeven, Sanjeev Premi,
Niels de Vos
In-Reply-To: <BANLkTinEJZ=fvJmWRkQ7kKyxbFRaJnum7g@mail.gmail.com>
When DBG() is used in a simple if-else, the resulting code path
currently depends on the definition of DBG(). Inserting the statement in
a "do { ... } while (0)" prevents this possible misuse.
Signed-off-by: Niels de Vos <ndevos@redhat.com>
---
V2: add the missing closing }
Note, I have not found any offenders, but a mistake can easily be made.
The following example shows what can go wrong if little intention is
paid to the definition of the DBG() macro.
Example:
if something_went_wrong()
DBG("oh no, something went wrong!\n");
else
printk("all went fine\n");
Old result where the else is placed inside the first if-statment:
if something_went_wrong() {
if (omapfb_debug) {
printk(KERN_DEBUG "oh no, something went wrong!\n");
} else {
printk("all went fine\n");
}
}
New result where the else is an alternative to the first if-statement:
if something_went_wrong() {
do {
if (omapfb_debug)
printk(KERN_DEBUG "oh no, something went wrong!\n");
} while (0);
} else {
printk("all went fine\n");
}
---
drivers/video/omap2/omapfb/omapfb.h | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/video/omap2/omapfb/omapfb.h b/drivers/video/omap2/omapfb/omapfb.h
index 1305fc9..456c586 100644
--- a/drivers/video/omap2/omapfb/omapfb.h
+++ b/drivers/video/omap2/omapfb/omapfb.h
@@ -34,8 +34,10 @@
#ifdef DEBUG
extern unsigned int omapfb_debug;
#define DBG(format, ...) \
- if (omapfb_debug) \
- printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
+ do { \
+ if (omapfb_debug) \
+ printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
+ } while (0)
#else
#define DBG(format, ...)
#endif
--
1.7.4.4
^ permalink raw reply related
* Re: [PATCH] omap2/omapfb: make DBG() more resistant in if-else constructions
From: Niels de Vos @ 2011-05-10 10:57 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linux-omap, linux-fbdev, linux-kernel, Premi, Sanjeev
In-Reply-To: <BANLkTinEJZ=fvJmWRkQ7kKyxbFRaJnum7g@mail.gmail.com>
On Tue, May 10, 2011 at 10:42 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, May 10, 2011 at 11:20, Niels de Vos <ndevos@redhat.com> wrote:
>> When DBG() is used in a simple if-else, the resulting code path
>> currently depends on the definition of DBG(). Inserting the statement in
>> a "do { ... } while (0)" prevents this possible misuse.
>>
>> Signed-off-by: Niels de Vos <ndevos@redhat.com>
>
>> --- a/drivers/video/omap2/omapfb/omapfb.h
>> +++ b/drivers/video/omap2/omapfb/omapfb.h
>> @@ -34,8 +34,10 @@
>> #ifdef DEBUG
>> extern unsigned int omapfb_debug;
>> #define DBG(format, ...) \
>> - if (omapfb_debug) \
>> - printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
>> + do { \
>> + if (omapfb_debug) \
>> + printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
>> + while (0)
>
> Where's the closing '}'?
Good catch! That's in a "fixup!" that I forgot to squash :-/
I'll post an update version in a bit.
>> #else
>> #define DBG(format, ...)
>
> BTW, no printf()-style format checking here.
>
>> #endif
>
> What about using the standard pr_debug()/dev_dbg() instead?
> With dynamic debug, it can be enabled at run time.
> As a bonus, you get printf()-style format checking if debugging is disabled.
I think removing DBG() and the omapfb_debug module-parameter is surely
a good thing. Unfortunately DBG() is used quite a bit in the code and
replacing them 'll take some more time. I don't know yet when I find
some time to do and test that.
Thanks for the pointers,
Niels
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>
^ permalink raw reply
* RE: [PATCH] omap2/omapfb: make DBG() more resistant in if-else
From: Premi, Sanjeev @ 2011-05-10 9:49 UTC (permalink / raw)
To: Niels de Vos, linux-omap@vger.kernel.org
Cc: linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1305019249-9898-1-git-send-email-ndevos@redhat.com>
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Niels de Vos
> Sent: Tuesday, May 10, 2011 2:51 PM
> To: linux-omap@vger.kernel.org
> Cc: linux-fbdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; Niels de Vos
> Subject: [PATCH] omap2/omapfb: make DBG() more resistant in
> if-else constructions
>
> When DBG() is used in a simple if-else, the resulting code path
> currently depends on the definition of DBG(). Inserting the
> statement in
> a "do { ... } while (0)" prevents this possible misuse.
>
> Signed-off-by: Niels de Vos <ndevos@redhat.com>
>
> ---
> Note, I have not found any offenders, but a mistake can
> easily be made.
> The following example shows what can go wrong if little intention is
> paid to the definition of the DBG() macro.
>
> Example:
> if something_went_wrong()
> DBG("oh no, something went wrong!\n");
> else
> printk("all went fine\n");
>
> Old result where the else is placed inside the first if-statment:
> if something_went_wrong() {
> if (omapfb_debug) {
> printk(KERN_DEBUG "oh no, something
> went wrong!\n");
> } else {
> printk("all went fine\n");
> }
> }
>
> New result where the else is an alternative to the first if-statement:
> if something_went_wrong() {
> do {
> if (omapfb_debug)
> printk(KERN_DEBUG "oh no,
> something went wrong!\n");
> } while (0);
> } else {
> printk("all went fine\n");
> }
> ---
> drivers/video/omap2/omapfb/omapfb.h | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/omap2/omapfb/omapfb.h
> b/drivers/video/omap2/omapfb/omapfb.h
> index 1305fc9..a01b0bb 100644
> --- a/drivers/video/omap2/omapfb/omapfb.h
> +++ b/drivers/video/omap2/omapfb/omapfb.h
> @@ -34,8 +34,10 @@
> #ifdef DEBUG
> extern unsigned int omapfb_debug;
> #define DBG(format, ...) \
> - if (omapfb_debug) \
> - printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
> + do { \
> + if (omapfb_debug) \
> + printk(KERN_DEBUG "OMAPFB: " format, ##
> __VA_ARGS__); \
> + while (0)
A real good find. Wondering if it really didn't create any problems so far...
~sanjeev
> #else
> #define DBG(format, ...)
> #endif
> --
> 1.7.4.4
>
> --
> 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
>
^ permalink raw reply
* Re: [PATCH] omap2/omapfb: make DBG() more resistant in if-else constructions
From: Geert Uytterhoeven @ 2011-05-10 9:42 UTC (permalink / raw)
To: Niels de Vos; +Cc: linux-omap, linux-fbdev, linux-kernel
In-Reply-To: <1305019249-9898-1-git-send-email-ndevos@redhat.com>
On Tue, May 10, 2011 at 11:20, Niels de Vos <ndevos@redhat.com> wrote:
> When DBG() is used in a simple if-else, the resulting code path
> currently depends on the definition of DBG(). Inserting the statement in
> a "do { ... } while (0)" prevents this possible misuse.
>
> Signed-off-by: Niels de Vos <ndevos@redhat.com>
> --- a/drivers/video/omap2/omapfb/omapfb.h
> +++ b/drivers/video/omap2/omapfb/omapfb.h
> @@ -34,8 +34,10 @@
> #ifdef DEBUG
> extern unsigned int omapfb_debug;
> #define DBG(format, ...) \
> - if (omapfb_debug) \
> - printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
> + do { \
> + if (omapfb_debug) \
> + printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
> + while (0)
Where's the closing '}'?
> #else
> #define DBG(format, ...)
BTW, no printf()-style format checking here.
> #endif
What about using the standard pr_debug()/dev_dbg() instead?
With dynamic debug, it can be enabled at run time.
As a bonus, you get printf()-style format checking if debugging is disabled.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH] omap2/omapfb: make DBG() more resistant in if-else constructions
From: Niels de Vos @ 2011-05-10 9:20 UTC (permalink / raw)
To: linux-omap; +Cc: linux-fbdev, linux-kernel, Niels de Vos
When DBG() is used in a simple if-else, the resulting code path
currently depends on the definition of DBG(). Inserting the statement in
a "do { ... } while (0)" prevents this possible misuse.
Signed-off-by: Niels de Vos <ndevos@redhat.com>
---
Note, I have not found any offenders, but a mistake can easily be made.
The following example shows what can go wrong if little intention is
paid to the definition of the DBG() macro.
Example:
if something_went_wrong()
DBG("oh no, something went wrong!\n");
else
printk("all went fine\n");
Old result where the else is placed inside the first if-statment:
if something_went_wrong() {
if (omapfb_debug) {
printk(KERN_DEBUG "oh no, something went wrong!\n");
} else {
printk("all went fine\n");
}
}
New result where the else is an alternative to the first if-statement:
if something_went_wrong() {
do {
if (omapfb_debug)
printk(KERN_DEBUG "oh no, something went wrong!\n");
} while (0);
} else {
printk("all went fine\n");
}
---
drivers/video/omap2/omapfb/omapfb.h | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/video/omap2/omapfb/omapfb.h b/drivers/video/omap2/omapfb/omapfb.h
index 1305fc9..a01b0bb 100644
--- a/drivers/video/omap2/omapfb/omapfb.h
+++ b/drivers/video/omap2/omapfb/omapfb.h
@@ -34,8 +34,10 @@
#ifdef DEBUG
extern unsigned int omapfb_debug;
#define DBG(format, ...) \
- if (omapfb_debug) \
- printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
+ do { \
+ if (omapfb_debug) \
+ printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
+ while (0)
#else
#define DBG(format, ...)
#endif
--
1.7.4.4
^ permalink raw reply related
* Re: [PATCH] viafb: Automatic OLPC XO-1.5 configuration
From: Florian Tobias Schandinat @ 2011-05-09 21:58 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20110509211439.AF70A9D401C@zog.reactivated.net>
Hi Daniel,
On 05/09/2011 09:14 PM, Daniel Drake wrote:
> Detect presence of the OLPC laptop and configure default settings
> accordingly. This means the kernel can now boot on XO-1.5 without
> needing long, hardcoded boot options.
The purpose of this patch is to allow people use their own kernel configuration
and just require them to enable viafb but not to copy the built-in command line,
right?
Well I usually dislike platform specific code but given that we probably won't
be able to detect everything OLPC specific even with a working autodetection and
having already a lot of OLPC code in here I think adding a little more would be
acceptable.
>
> Signed-off-by: Daniel Drake<dsd@laptop.org>
> ---
> drivers/video/via/global.c | 2 +-
> drivers/video/via/viafbdev.c | 55 ++++++++++++++++++++++++++++++++----------
> 2 files changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/video/via/global.c b/drivers/video/via/global.c
> index e10d824..b994e3b 100644
> --- a/drivers/video/via/global.c
> +++ b/drivers/video/via/global.c
> @@ -29,7 +29,7 @@ int viafb_refresh = 60;
> int viafb_refresh1 = 60;
> int viafb_lcd_dsp_method = LCD_EXPANDSION;
> int viafb_lcd_mode = LCD_OPENLDI;
> -int viafb_CRT_ON = 1;
> +int viafb_CRT_ON = STATE_ON;
Unnecessary but okay
> int viafb_DVI_ON;
> int viafb_LCD_ON ;
> int viafb_LCD2_ON;
> diff --git a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c
> index 7b4390e..75b5fc7 100644
> --- a/drivers/video/via/viafbdev.c
> +++ b/drivers/video/via/viafbdev.c
> @@ -24,6 +24,7 @@
> #include<linux/slab.h>
> #include<linux/stat.h>
> #include<linux/via-core.h>
> +#include<asm/olpc.h>
>
> #define _MASTER_FILE
> #include "global.h"
> @@ -36,6 +37,8 @@ static char *viafb_mode;
> static char *viafb_mode1;
> static int viafb_bpp = 32;
> static int viafb_bpp1 = 32;
> +static int viafb_default_mode_xres = 640;
> +static int viafb_default_mode_yres = 480;
Nack. As OLPC will probably be ever the only platform where the default is
different and as we only use it once, we shouldn't add an extra var for it.
>
> static unsigned int viafb_second_xres = 640;
> static unsigned int viafb_second_yres = 480;
> @@ -1001,19 +1004,18 @@ static void retrieve_device_setting(struct viafb_ioctl_setting
>
> static int __init parse_active_dev(void)
> {
> - viafb_CRT_ON = STATE_OFF;
> viafb_DVI_ON = STATE_OFF;
> - viafb_LCD_ON = STATE_OFF;
> viafb_LCD2_ON = STATE_OFF;
> +
> + if (!viafb_active_dev)
> + return 0;
> +
> /* 1. Modify the active status of devices. */
> /* 2. Keep the order of devices, so we can set corresponding
> IGA path to devices in SAMM case. */
> /* Note: The previous of active_dev is primary device,
> and the following is secondary device. */
> - if (!viafb_active_dev) {
Just add here an
if (machine_is_olpc())
and handle it correct. Changing so much generic code for one platform is not a
good thing. And it certainly is a better design to start with everything off and
enable things needed and not mix it up.
> - viafb_CRT_ON = STATE_ON;
> - viafb_SAMM_ON = STATE_OFF;
> - } else if (!strcmp(viafb_active_dev, "CRT+DVI")) {
> + if (!strcmp(viafb_active_dev, "CRT+DVI")) {
> /* CRT+DVI */
> viafb_CRT_ON = STATE_ON;
> viafb_DVI_ON = STATE_ON;
> @@ -1035,19 +1037,23 @@ static int __init parse_active_dev(void)
> viafb_primary_dev = LCD_Device;
> } else if (!strcmp(viafb_active_dev, "DVI+LCD")) {
> /* DVI+LCD */
> + viafb_CRT_ON = STATE_OFF;
> viafb_DVI_ON = STATE_ON;
> viafb_LCD_ON = STATE_ON;
> viafb_primary_dev = DVI_Device;
> } else if (!strcmp(viafb_active_dev, "LCD+DVI")) {
> /* LCD+DVI */
> + viafb_CRT_ON = STATE_OFF;
> viafb_DVI_ON = STATE_ON;
> viafb_LCD_ON = STATE_ON;
> viafb_primary_dev = LCD_Device;
> } else if (!strcmp(viafb_active_dev, "LCD+LCD2")) {
> + viafb_CRT_ON = STATE_OFF;
> viafb_LCD_ON = STATE_ON;
> viafb_LCD2_ON = STATE_ON;
> viafb_primary_dev = LCD_Device;
> } else if (!strcmp(viafb_active_dev, "LCD2+LCD")) {
> + viafb_CRT_ON = STATE_OFF;
> viafb_LCD_ON = STATE_ON;
> viafb_LCD2_ON = STATE_ON;
> viafb_primary_dev = LCD2_Device;
> @@ -1057,10 +1063,12 @@ static int __init parse_active_dev(void)
> viafb_SAMM_ON = STATE_OFF;
> } else if (!strcmp(viafb_active_dev, "DVI")) {
> /* DVI only */
> + viafb_CRT_ON = STATE_OFF;
> viafb_DVI_ON = STATE_ON;
> viafb_SAMM_ON = STATE_OFF;
> } else if (!strcmp(viafb_active_dev, "LCD")) {
> /* LCD only */
> + viafb_CRT_ON = STATE_OFF;
> viafb_LCD_ON = STATE_ON;
> viafb_SAMM_ON = STATE_OFF;
> } else
> @@ -1665,8 +1673,8 @@ static int parse_mode(const char *str, u32 *xres, u32 *yres)
> char *ptr;
>
> if (!str) {
Add the OLPC mode here as in
if (machine_is_olpc()) {
*xres = 1200;
*yres = 900;
} else {
*xres = 640;
*yres = 480;
... perhaps this might change to autodetect someday as far as possible
}
> - *xres = 640;
> - *yres = 480;
> + *xres = viafb_default_mode_xres;
> + *yres = viafb_default_mode_yres;
> return 0;
> }
>
> @@ -1922,11 +1930,16 @@ void __devexit via_fb_pci_remove(struct pci_dev *pdev)
> }
>
> #ifndef MODULE
> -static int __init viafb_setup(char *options)
> +static int __init viafb_setup(void)
> {
> char *this_opt;
> + char *options;
> +
> DEBUG_MSG(KERN_INFO "viafb_setup!\n");
>
> + if (fb_get_options("viafb",&options))
> + return -ENODEV;
You move the return value here....
> +
> if (!options || !*options)
> return 0;
>
> @@ -1994,17 +2007,33 @@ static int __init viafb_setup(char *options)
> }
> #endif
>
> +static void __init viafb_platform_setup(void)
Probably after my comments it does no longer make any sense to do it in an extra
function as the only thing left is
viafb_lcd_panel_id = 23;
> +{
> + if (machine_is_olpc()) {
> + /* Apply XO-1.5-specific configuration. */
> + viafb_lcd_panel_id = 23;
> + viafb_bpp = 24;
> + viafb_default_mode_xres = 1200;
> + viafb_default_mode_yres = 900;
> +
> + /* LCD only */
> + viafb_CRT_ON = STATE_OFF;
> + viafb_LCD_ON = STATE_ON;
> + viafb_SAMM_ON = STATE_OFF;
> + }
> +}
> +
> /*
> * These are called out of via-core for now.
> */
> int __init viafb_init(void)
> {
> u32 dummy_x, dummy_y;
> +
> + viafb_platform_setup();
> +
> #ifndef MODULE
> - char *option = NULL;
> - if (fb_get_options("viafb",&option))
> - return -ENODEV;
> - viafb_setup(option);
> + viafb_setup();
...and you don't handle the return value here.
> #endif
> if (parse_mode(viafb_mode,&dummy_x,&dummy_y)
> || !viafb_get_mode(dummy_x, dummy_y)
Regards,
Florian Tobias Schandinat
^ permalink raw reply
* [PATCH] viafb: Automatic OLPC XO-1.5 configuration
From: Daniel Drake @ 2011-05-09 21:14 UTC (permalink / raw)
To: linux-fbdev
Detect presence of the OLPC laptop and configure default settings
accordingly. This means the kernel can now boot on XO-1.5 without
needing long, hardcoded boot options.
Signed-off-by: Daniel Drake <dsd@laptop.org>
---
drivers/video/via/global.c | 2 +-
drivers/video/via/viafbdev.c | 55 ++++++++++++++++++++++++++++++++----------
2 files changed, 43 insertions(+), 14 deletions(-)
diff --git a/drivers/video/via/global.c b/drivers/video/via/global.c
index e10d824..b994e3b 100644
--- a/drivers/video/via/global.c
+++ b/drivers/video/via/global.c
@@ -29,7 +29,7 @@ int viafb_refresh = 60;
int viafb_refresh1 = 60;
int viafb_lcd_dsp_method = LCD_EXPANDSION;
int viafb_lcd_mode = LCD_OPENLDI;
-int viafb_CRT_ON = 1;
+int viafb_CRT_ON = STATE_ON;
int viafb_DVI_ON;
int viafb_LCD_ON ;
int viafb_LCD2_ON;
diff --git a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c
index 7b4390e..75b5fc7 100644
--- a/drivers/video/via/viafbdev.c
+++ b/drivers/video/via/viafbdev.c
@@ -24,6 +24,7 @@
#include <linux/slab.h>
#include <linux/stat.h>
#include <linux/via-core.h>
+#include <asm/olpc.h>
#define _MASTER_FILE
#include "global.h"
@@ -36,6 +37,8 @@ static char *viafb_mode;
static char *viafb_mode1;
static int viafb_bpp = 32;
static int viafb_bpp1 = 32;
+static int viafb_default_mode_xres = 640;
+static int viafb_default_mode_yres = 480;
static unsigned int viafb_second_xres = 640;
static unsigned int viafb_second_yres = 480;
@@ -1001,19 +1004,18 @@ static void retrieve_device_setting(struct viafb_ioctl_setting
static int __init parse_active_dev(void)
{
- viafb_CRT_ON = STATE_OFF;
viafb_DVI_ON = STATE_OFF;
- viafb_LCD_ON = STATE_OFF;
viafb_LCD2_ON = STATE_OFF;
+
+ if (!viafb_active_dev)
+ return 0;
+
/* 1. Modify the active status of devices. */
/* 2. Keep the order of devices, so we can set corresponding
IGA path to devices in SAMM case. */
/* Note: The previous of active_dev is primary device,
and the following is secondary device. */
- if (!viafb_active_dev) {
- viafb_CRT_ON = STATE_ON;
- viafb_SAMM_ON = STATE_OFF;
- } else if (!strcmp(viafb_active_dev, "CRT+DVI")) {
+ if (!strcmp(viafb_active_dev, "CRT+DVI")) {
/* CRT+DVI */
viafb_CRT_ON = STATE_ON;
viafb_DVI_ON = STATE_ON;
@@ -1035,19 +1037,23 @@ static int __init parse_active_dev(void)
viafb_primary_dev = LCD_Device;
} else if (!strcmp(viafb_active_dev, "DVI+LCD")) {
/* DVI+LCD */
+ viafb_CRT_ON = STATE_OFF;
viafb_DVI_ON = STATE_ON;
viafb_LCD_ON = STATE_ON;
viafb_primary_dev = DVI_Device;
} else if (!strcmp(viafb_active_dev, "LCD+DVI")) {
/* LCD+DVI */
+ viafb_CRT_ON = STATE_OFF;
viafb_DVI_ON = STATE_ON;
viafb_LCD_ON = STATE_ON;
viafb_primary_dev = LCD_Device;
} else if (!strcmp(viafb_active_dev, "LCD+LCD2")) {
+ viafb_CRT_ON = STATE_OFF;
viafb_LCD_ON = STATE_ON;
viafb_LCD2_ON = STATE_ON;
viafb_primary_dev = LCD_Device;
} else if (!strcmp(viafb_active_dev, "LCD2+LCD")) {
+ viafb_CRT_ON = STATE_OFF;
viafb_LCD_ON = STATE_ON;
viafb_LCD2_ON = STATE_ON;
viafb_primary_dev = LCD2_Device;
@@ -1057,10 +1063,12 @@ static int __init parse_active_dev(void)
viafb_SAMM_ON = STATE_OFF;
} else if (!strcmp(viafb_active_dev, "DVI")) {
/* DVI only */
+ viafb_CRT_ON = STATE_OFF;
viafb_DVI_ON = STATE_ON;
viafb_SAMM_ON = STATE_OFF;
} else if (!strcmp(viafb_active_dev, "LCD")) {
/* LCD only */
+ viafb_CRT_ON = STATE_OFF;
viafb_LCD_ON = STATE_ON;
viafb_SAMM_ON = STATE_OFF;
} else
@@ -1665,8 +1673,8 @@ static int parse_mode(const char *str, u32 *xres, u32 *yres)
char *ptr;
if (!str) {
- *xres = 640;
- *yres = 480;
+ *xres = viafb_default_mode_xres;
+ *yres = viafb_default_mode_yres;
return 0;
}
@@ -1922,11 +1930,16 @@ void __devexit via_fb_pci_remove(struct pci_dev *pdev)
}
#ifndef MODULE
-static int __init viafb_setup(char *options)
+static int __init viafb_setup(void)
{
char *this_opt;
+ char *options;
+
DEBUG_MSG(KERN_INFO "viafb_setup!\n");
+ if (fb_get_options("viafb", &options))
+ return -ENODEV;
+
if (!options || !*options)
return 0;
@@ -1994,17 +2007,33 @@ static int __init viafb_setup(char *options)
}
#endif
+static void __init viafb_platform_setup(void)
+{
+ if (machine_is_olpc()) {
+ /* Apply XO-1.5-specific configuration. */
+ viafb_lcd_panel_id = 23;
+ viafb_bpp = 24;
+ viafb_default_mode_xres = 1200;
+ viafb_default_mode_yres = 900;
+
+ /* LCD only */
+ viafb_CRT_ON = STATE_OFF;
+ viafb_LCD_ON = STATE_ON;
+ viafb_SAMM_ON = STATE_OFF;
+ }
+}
+
/*
* These are called out of via-core for now.
*/
int __init viafb_init(void)
{
u32 dummy_x, dummy_y;
+
+ viafb_platform_setup();
+
#ifndef MODULE
- char *option = NULL;
- if (fb_get_options("viafb", &option))
- return -ENODEV;
- viafb_setup(option);
+ viafb_setup();
#endif
if (parse_mode(viafb_mode, &dummy_x, &dummy_y)
|| !viafb_get_mode(dummy_x, dummy_y)
--
1.7.4.4
^ permalink raw reply related
* [bug] drmfb does not set physical screen dimensions
From: Roger Leigh @ 2011-05-09 18:08 UTC (permalink / raw)
To: dri-devel, linux-fbdev, David Airlie
[-- Attachment #1: Type: text/plain, Size: 3331 bytes --]
Hi,
drivers/gpu/drm/drm_fb_helper.c is not setting width and height
in struct fb_var_screeninfo. drm_fb_helper_fill_var sets them to -1
rather than using the real values:
info->var.height = -1;
info->var.width = -1;
Since the physical dimensions are most likely known from the monitor
EDID, it would be ideal if this could be set here. If not, it might
be nice to assume a default of 96dpi and compute the size based upon
the current resolution (so applications don't need to implement
fallbacks when unset). On my hardware the radeon driver certainly
does have this information.
This information is needed in order to do accurate font rendering and
drawing. It's available in X, and it would be great if it was also
available using the framebuffer.
I've also attached a small patch to fbset to allow reporting of the
current state in fb_var_screeninfo such as resolution, size, depth etc.
Could be extended to report more info if desired.
I'm not entirely sure who is best to submit this to, so my apologies
if this is not you.
I've included the dri and fbdev lists because I'm not sure if it's
specific to the drmfb code or the generic framebuffer code. Likewise
if you set a default 96dpi size, I'm not sure if it's a generic issue
or specific to drmfb.
Thanks,
Roger
diff -urN /tmp/fbset-2.1/fbset.c ./fbset.c
--- /tmp/fbset-2.1/fbset.c 2011-05-09 18:47:47.000000000 +0100
+++ ./fbset.c 2011-05-09 18:46:32.142945642 +0100
@@ -281,7 +281,8 @@
static struct VideoMode *FindVideoMode(const char *name);
static void ModifyVideoMode(struct VideoMode *vmode);
static void DisplayVModeInfo(struct VideoMode *vmode);
-static void DisplayFBInfo(struct fb_fix_screeninfo *fix);
+static void DisplayFBInfo(struct fb_fix_screeninfo *fix,
+ struct fb_var_screeninfo *var);
static int FillScanRates(struct VideoMode *vmode);
static void Usage(void) __attribute__ ((noreturn));
int main(int argc, char *argv[]);
@@ -758,7 +759,8 @@
* Display the Frame Buffer Device Information
*/
-static void DisplayFBInfo(struct fb_fix_screeninfo *fix)
+static void DisplayFBInfo(struct fb_fix_screeninfo *fix,
+ struct fb_var_screeninfo *var)
{
int i;
@@ -845,6 +847,16 @@
puts(Accelerators[i].name);
else
printf("Unknown (%d)\n", fix->accel);
+
+ printf(" Dimensions : %dx%d pixels", var->xres, var->yres);
+ if (var->width != -1 && var->height != -1)
+ printf(" (%dx%d mm)", var->width, var->height);
+ putc('\n', stdout);
+ printf(" Virtual : %dx%d pixels\n", var->xres_virtual, var->yres_virtual);
+ printf(" Offset : %dx%d pixels\n", var->xoffset, var->yoffset);
+ printf(" Bits/Pixel : %d\n", var->bits_per_pixel);
+ if (var->grayscale)
+ printf(" Graylevels : %d\n", var->grayscale);
}
@@ -1101,7 +1113,7 @@
if (Opt_verbose)
puts("Getting further frame buffer information");
GetFixScreenInfo(fh, &fix);
- DisplayFBInfo(&fix);
+ DisplayFBInfo(&fix, &var);
}
/*
--
.''`. Roger Leigh
: :' : Debian GNU/Linux http://people.debian.org/~rleigh/
`. `' Printing on GNU/Linux? http://gutenprint.sourceforge.net/
`- GPG Public Key: 0x25BFB848 Please GPG sign your mail.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH 0/3] MERAM support for LCDC
From: Magnus Damm @ 2011-05-09 16:22 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1301369758-18394-1-git-send-email-dhobsong@igel.co.jp>
Hi Damian,
On Mon, May 9, 2011 at 4:03 PM, Damian Hobson-Garcia
<dhobsong@igel.co.jp> wrote:
> Hi Magnus,
>
>> Please add Runtime PM support to the MERAM driver. The MSTP113 bit of
>> SMSTPCR1 should be dynamically controlled using pm_runtime_get_sync()
>> and pm_runtime_put_sync().
>
> So one thing that I noticed while adding in the runtime PM support is
> that the MERAM bit in RMSTPCR1 seems to be enabled as a power on default
> on the chip. (I determined this by dumping the register from u-boot and
> poking around in the u-boot code to verify that u-boot doesn't seem to
> be enabling this).
>
> Even if pm_runtime_get_sync() and pm_runtime_put_sync() correctly enable
> and disable the SMSTPCR1, with the RMSTPCR1 enabled, the clock doesn't
> actually turn off.
>
> While I still think that its a good idea to add the
> pm_runtime_get_sync() and pm_runtime_put_sync() calls to do their thing
> on the SMSTPCR1 side, with the current configuration we won't actually
> be saving any power.
Good catch, yes, the MSTP bits need to be disabled on both sides for
the clocks to be stopped. I recall the default state of each MSTP bit
to vary somehow. I think we simply should add code to initialize all
SH-side MSTP bits as disabled, that's the easiest.
Thanks,
/ magnus
^ permalink raw reply
* Re: [PATCH] OMAP2: avoid descending into disabled framebuffer dirs
From: Tomi Valkeinen @ 2011-05-09 16:06 UTC (permalink / raw)
To: Mike Frysinger; +Cc: linux-fbdev, Paul Mundt, linux-omap
In-Reply-To: <1304952024-28296-1-git-send-email-vapier@gentoo.org>
On Mon, 2011-05-09 at 10:40 -0400, Mike Frysinger wrote:
> Rather than always add the omap2 dirs to the build list (and thus
> force everyone to generate a useless built-in.o), bind the dirs to
> their relevant kconfig symbol.
>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> drivers/video/omap2/Makefile | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/omap2/Makefile b/drivers/video/omap2/Makefile
> index d853d05..5ddef12 100644
> --- a/drivers/video/omap2/Makefile
> +++ b/drivers/video/omap2/Makefile
> @@ -1,6 +1,6 @@
> obj-$(CONFIG_OMAP2_VRAM) += vram.o
> obj-$(CONFIG_OMAP2_VRFB) += vrfb.o
>
> -obj-y += dss/
> -obj-y += omapfb/
> +obj-$(CONFIG_OMAP2_DSS) += dss/
> +obj-$(CONFIG_FB_OMAP2) += omapfb/
> obj-y += displays/
Looks fine to me, applying to DSS tree. Thanks!
Tomi
^ permalink raw reply
* [PATCH] OMAP2: avoid descending into disabled framebuffer dirs
From: Mike Frysinger @ 2011-05-09 14:40 UTC (permalink / raw)
To: linux-fbdev, Paul Mundt, linux-omap, Tomi Valkeinen
Rather than always add the omap2 dirs to the build list (and thus
force everyone to generate a useless built-in.o), bind the dirs to
their relevant kconfig symbol.
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
drivers/video/omap2/Makefile | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/omap2/Makefile b/drivers/video/omap2/Makefile
index d853d05..5ddef12 100644
--- a/drivers/video/omap2/Makefile
+++ b/drivers/video/omap2/Makefile
@@ -1,6 +1,6 @@
obj-$(CONFIG_OMAP2_VRAM) += vram.o
obj-$(CONFIG_OMAP2_VRFB) += vrfb.o
-obj-y += dss/
-obj-y += omapfb/
+obj-$(CONFIG_OMAP2_DSS) += dss/
+obj-$(CONFIG_FB_OMAP2) += omapfb/
obj-y += displays/
--
1.7.5.rc3
^ permalink raw reply related
* Re: [RFC][PATCH 0/3] MERAM support for LCDC
From: Damian Hobson-Garcia @ 2011-05-09 7:03 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1301369758-18394-1-git-send-email-dhobsong@igel.co.jp>
Hi Magnus,
> Please add Runtime PM support to the MERAM driver. The MSTP113 bit of
> SMSTPCR1 should be dynamically controlled using pm_runtime_get_sync()
> and pm_runtime_put_sync().
So one thing that I noticed while adding in the runtime PM support is
that the MERAM bit in RMSTPCR1 seems to be enabled as a power on default
on the chip. (I determined this by dumping the register from u-boot and
poking around in the u-boot code to verify that u-boot doesn't seem to
be enabling this).
Even if pm_runtime_get_sync() and pm_runtime_put_sync() correctly enable
and disable the SMSTPCR1, with the RMSTPCR1 enabled, the clock doesn't
actually turn off.
While I still think that its a good idea to add the
pm_runtime_get_sync() and pm_runtime_put_sync() calls to do their thing
on the SMSTPCR1 side, with the current configuration we won't actually
be saving any power.
Damian
^ permalink raw reply
* Re: [2.6.39-rc2, framebuffer] use after free oops
From: Daniel J Blueman @ 2011-05-08 11:25 UTC (permalink / raw)
To: linux-fbdev, Linux Kernel, Dave Airlie, Paul Mundt
Cc: Alan Cox, Bruno Prémont, Anca Emanuel
In-Reply-To: <BANLkTi=x76zkTpZxaHUnSUeP+_O0SsK0Zw@mail.gmail.com>
On 7 May 2011 23:24, Anca Emanuel <anca.emanuel@gmail.com> wrote:
> Hi, Daniel J Blueman.
>
> Did you test https://lkml.org/lkml/2011/5/5/208 ? And it works for you ?
> Then please reply with your error and an Tested-by.
>
> And CC: "Dave Airlie" <airlied@redhat.com>
Tested against 2.6.39-rc6. Instrumentation and debug catches the
(silent without debug) use-after-free case, which now doesn't show up
with this patch, so looks good. Probably good sense to get into
-stable too.
Tested-by: Daniel J Blueman <daniel.blueman@gmail.com>
Thanks,
Daniel
--
Daniel J Blueman
^ permalink raw reply
* Re: [2.6.39-rc2, framebuffer] use after free oops
From: Anca Emanuel @ 2011-05-07 15:24 UTC (permalink / raw)
To: Daniel J Blueman
Cc: Alan Cox, Bruno Prémont, Paul Mundt, linux-fbdev,
Linux Kernel, Dave Airlie
In-Reply-To: <BANLkTikF5YL8tJ_f5510yy1Ywm69rOUK2A@mail.gmail.com>
Hi, Daniel J Blueman.
Did you test https://lkml.org/lkml/2011/5/5/208 ? And it works for you ?
Then please reply with your error and an Tested-by.
And CC: "Dave Airlie" <airlied@redhat.com>
^ permalink raw reply
* Re: [2.6.39-rc2, framebuffer] use after free oops
From: Daniel J Blueman @ 2011-05-06 2:38 UTC (permalink / raw)
To: Alan Cox, Bruno Prémont, Paul Mundt, linux-fbdev,
Linux Kernel
In-Reply-To: <20110420105631.70695dfa@lxorguk.ukuu.org.uk>
On 20 April 2011 17:56, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Wed, 20 Apr 2011 08:05:35 +0200
> Bruno Prémont <bonbons@linux-vserver.org> wrote:
>
>> On Wed, 20 Apr 2011 13:50:10 Daniel J Blueman <daniel.blueman@gmail.com> wrote:
>> > Any ideas on how best to address this issue [0], since it causes
>> > silent corruption, or at best crashes?
>>
>> There is probably no easy short-term fix to this...
>
> The short term fix would be to deliberately leak the buffer. That should
> go into 2.6.39-rc right now with a comment explaining the situation.
> Otherwise who knows what corruption may occur to user data if unlucky.
>
> The other 'cheat' might be to tweak the API so the removal API isn't a
> 'destroy' interface but a 'shut down' and has a matching 'restart' one
> for when the intelfb unloads at which point vga16fb can carry on with the
> original fb_info 8)
It looks like Andy Whitcroft addressed this issue some time ago, but
forgot to send the fix upstream:
http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-natty.git;a=patch;hÅa742b5f78e161d6a13853a7e3e6e1dfa429e69;hp&a1443f67eea17d4b78ef75df701782cc8bf35b
Let's hope it can hit -rc7 since it's been in Ubuntu's kernel tree for
considerable time, and fixes a silent corrupter:
http://groups.google.com/group/linux.kernel/browse_thread/thread/fc9083f6f380ed5b/f801112b840785cb?show_docidø01112b840785cb
Thanks,
Daniel
--
Daniel J Blueman
^ permalink raw reply
* Re: [PATCH] fbcon -- fix race between open and removal of
From: Greg KH @ 2011-05-06 1:44 UTC (permalink / raw)
To: Anca Emanuel
Cc: Jack Stone, tim.gardner, linux-fbdev, lethal, linux-kernel,
Andy Whitcroft, Leann Ogasawara, Greg KH
In-Reply-To: <BANLkTikG=6xyFg--u=M6HwH8yQExAauA8w@mail.gmail.com>
On Fri, May 06, 2011 at 04:09:44AM +0300, Anca Emanuel wrote:
> @Greg: This is stable material for 2.6.38
> link to the patch: http://is.gd/otIfGc
<form_letter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.
thanks,
greg k-h
</form_letter>
^ permalink raw reply
* Re: [PATCH] fbcon -- fix race between open and removal of framebuffers
From: Anca Emanuel @ 2011-05-06 1:09 UTC (permalink / raw)
To: Jack Stone
Cc: tim.gardner, linux-fbdev, lethal, linux-kernel, Andy Whitcroft,
Leann Ogasawara, Greg KH, Greg KH
In-Reply-To: <4DC30FFA.9030708@fastmail.fm>
@Greg: This is stable material for 2.6.38
link to the patch: http://is.gd/otIfGc
^ permalink raw reply
* Re: [PATCH] fbcon -- fix race between open and removal of framebuffers
From: Anca Emanuel @ 2011-05-06 0:21 UTC (permalink / raw)
To: tim.gardner
Cc: linux-fbdev, lethal, linux-kernel, Andy Whitcroft,
Leann Ogasawara
In-Reply-To: <1304617307-7389-2-git-send-email-tim.gardner@canonical.com>
On Thu, May 5, 2011 at 8:41 PM, <tim.gardner@canonical.com> wrote:
> From: Andy Whitcroft <apw@canonical.com>
>
> Currently there is no locking for updates to the registered_fb list.
> This allows an open through /dev/fbN to pick up a registered framebuffer
> pointer in parallel with it being released, as happens when a conflicting
> framebuffer is ejected or on module unload. There is also no reference
> counting on the framebuffer descriptor which is referenced from all open
> files, leading to references to released or reused memory to persist on
> these open files.
>
> This patch adds a reference count to the framebuffer descriptor to prevent
> it from being released until after all pending opens are closed. This
> allows the pending opens to detect the closed status and unmap themselves.
> It also adds locking to the framebuffer lookup path, locking it against
> the removal path such that it is possible to atomically lookup and take a
> reference to the descriptor. It also adds locking to the read and write
> paths which currently could access the framebuffer descriptor after it
> has been freed. Finally it moves the device to FBINFO_STATE_REMOVED to
> indicate that all access should be errored for this device.
>
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> Acked-by: Stefan Bader <stefan.bader@canonical.com>
> Signed-off-by: Leann Ogasawara <leann.ogasawara@canonical.com>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
> drivers/video/fbmem.c | 132 ++++++++++++++++++++++++++++++++++++++-----------
> include/linux/fb.h | 2 +
> 2 files changed, 105 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index e0c2284..c8562c1 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -42,6 +42,8 @@
>
> #define FBPIXMAPSIZE (1024 * 8)
>
> +/* Protects the registered framebuffer list and count. */
> +static DEFINE_SPINLOCK(registered_lock);
> struct fb_info *registered_fb[FB_MAX] __read_mostly;
> int num_registered_fb __read_mostly;
>
> @@ -694,9 +696,7 @@ static ssize_t
> fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> {
> unsigned long p = *ppos;
> - struct inode *inode = file->f_path.dentry->d_inode;
> - int fbidx = iminor(inode);
> - struct fb_info *info = registered_fb[fbidx];
> + struct fb_info *info = file->private_data;
> u8 *buffer, *dst;
> u8 __iomem *src;
> int c, cnt = 0, err = 0;
> @@ -705,19 +705,28 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> if (!info || ! info->screen_base)
> return -ENODEV;
>
> - if (info->state != FBINFO_STATE_RUNNING)
> - return -EPERM;
> + if (!lock_fb_info(info))
> + return -ENODEV;
> +
> + if (info->state != FBINFO_STATE_RUNNING) {
> + err = -EPERM;
> + goto out_fb_info;
> + }
>
> - if (info->fbops->fb_read)
> - return info->fbops->fb_read(info, buf, count, ppos);
> + if (info->fbops->fb_read) {
> + err = info->fbops->fb_read(info, buf, count, ppos);
> + goto out_fb_info;
> + }
>
> total_size = info->screen_size;
>
> if (total_size = 0)
> total_size = info->fix.smem_len;
>
> - if (p >= total_size)
> - return 0;
> + if (p >= total_size) {
> + err = 0;
> + goto out_fb_info;
> + }
>
> if (count >= total_size)
> count = total_size;
> @@ -727,8 +736,10 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>
> buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
> GFP_KERNEL);
> - if (!buffer)
> - return -ENOMEM;
> + if (!buffer) {
> + err = -ENOMEM;
> + goto out_fb_info;
> + }
>
> src = (u8 __iomem *) (info->screen_base + p);
>
> @@ -751,19 +762,21 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> cnt += c;
> count -= c;
> }
> + if (!err)
> + err = cnt;
>
> kfree(buffer);
> +out_fb_info:
> + unlock_fb_info(info);
>
> - return (err) ? err : cnt;
> + return err;
> }
>
> static ssize_t
> fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> {
> unsigned long p = *ppos;
> - struct inode *inode = file->f_path.dentry->d_inode;
> - int fbidx = iminor(inode);
> - struct fb_info *info = registered_fb[fbidx];
> + struct fb_info *info = file->private_data;
> u8 *buffer, *src;
> u8 __iomem *dst;
> int c, cnt = 0, err = 0;
> @@ -772,8 +785,13 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> if (!info || !info->screen_base)
> return -ENODEV;
>
> - if (info->state != FBINFO_STATE_RUNNING)
> - return -EPERM;
> + if (!lock_fb_info(info))
> + return -ENODEV;
> +
> + if (info->state != FBINFO_STATE_RUNNING) {
> + err = -EPERM;
> + goto out_fb_info;
> + }
>
> if (info->fbops->fb_write)
> return info->fbops->fb_write(info, buf, count, ppos);
> @@ -783,8 +801,10 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> if (total_size = 0)
> total_size = info->fix.smem_len;
>
> - if (p > total_size)
> - return -EFBIG;
> + if (p > total_size) {
> + err = -EFBIG;
> + goto out_fb_info;
> + }
>
> if (count > total_size) {
> err = -EFBIG;
> @@ -800,8 +820,10 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>
> buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
> GFP_KERNEL);
> - if (!buffer)
> - return -ENOMEM;
> + if (!buffer) {
> + err = -ENOMEM;
> + goto out_fb_info;
> + }
>
> dst = (u8 __iomem *) (info->screen_base + p);
>
> @@ -825,10 +847,14 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> cnt += c;
> count -= c;
> }
> + if (cnt)
> + err = cnt;
>
> kfree(buffer);
> +out_fb_info:
> + unlock_fb_info(info);
>
> - return (cnt) ? cnt : err;
> + return err;
> }
>
> int
> @@ -1303,8 +1329,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
> static int
> fb_mmap(struct file *file, struct vm_area_struct * vma)
> {
> - int fbidx = iminor(file->f_path.dentry->d_inode);
> - struct fb_info *info = registered_fb[fbidx];
> + struct fb_info * const info = file->private_data;
> struct fb_ops *fb = info->fbops;
> unsigned long off;
> unsigned long start;
> @@ -1316,6 +1341,11 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> if (!fb)
> return -ENODEV;
> mutex_lock(&info->mm_lock);
> + if (info->state = FBINFO_STATE_REMOVED) {
> + mutex_unlock(&info->mm_lock);
> + return -ENODEV;
> + }
> +
> if (fb->fb_mmap) {
> int res;
> res = fb->fb_mmap(info, vma);
> @@ -1352,6 +1382,34 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> return 0;
> }
>
> +static struct fb_info *get_framebuffer_info(int idx)
> +__acquires(®istered_lock)
> +__releases(®istered_lock)
> +{
> + struct fb_info *fb_info;
> +
> + spin_lock(®istered_lock);
> + fb_info = registered_fb[idx];
> + fb_info->ref_count++;
> + spin_unlock(®istered_lock);
> +
> + return fb_info;
> +}
> +
> +static void put_framebuffer_info(struct fb_info *fb_info)
> +__acquires(®istered_lock)
> +__releases(®istered_lock)
> +{
> + int keep;
> +
> + spin_lock(®istered_lock);
> + keep = --fb_info->ref_count;
> + spin_unlock(®istered_lock);
> +
> + if (!keep && fb_info->fbops->fb_destroy)
> + fb_info->fbops->fb_destroy(fb_info);
> +}
> +
> static int
> fb_open(struct inode *inode, struct file *file)
> __acquires(&info->lock)
> @@ -1363,13 +1421,17 @@ __releases(&info->lock)
>
> if (fbidx >= FB_MAX)
> return -ENODEV;
> - info = registered_fb[fbidx];
> + info = get_framebuffer_info(fbidx);
> if (!info)
> request_module("fb%d", fbidx);
> - info = registered_fb[fbidx];
> + info = get_framebuffer_info(fbidx);
> if (!info)
> return -ENODEV;
> mutex_lock(&info->lock);
> + if (info->state = FBINFO_STATE_REMOVED) {
> + res = -ENODEV;
> + goto out;
> + }
> if (!try_module_get(info->fbops->owner)) {
> res = -ENODEV;
> goto out;
> @@ -1386,6 +1448,8 @@ __releases(&info->lock)
> #endif
> out:
> mutex_unlock(&info->lock);
> + if (res)
> + put_framebuffer_info(info);
> return res;
> }
>
> @@ -1401,6 +1465,7 @@ __releases(&info->lock)
> info->fbops->fb_release(info,1);
> module_put(info->fbops->owner);
> mutex_unlock(&info->lock);
> + put_framebuffer_info(info);
> return 0;
> }
>
> @@ -1549,6 +1614,7 @@ register_framebuffer(struct fb_info *fb_info)
> fb_info->node = i;
> mutex_init(&fb_info->lock);
> mutex_init(&fb_info->mm_lock);
> + fb_info->ref_count = 1;
>
> fb_info->dev = device_create(fb_class, fb_info->device,
> MKDEV(FB_MAJOR, i), NULL, "fb%d", i);
> @@ -1592,7 +1658,6 @@ register_framebuffer(struct fb_info *fb_info)
> return 0;
> }
>
> -
> /**
> * unregister_framebuffer - releases a frame buffer device
> * @fb_info: frame buffer info structure
> @@ -1627,6 +1692,16 @@ unregister_framebuffer(struct fb_info *fb_info)
> return -ENODEV;
> event.info = fb_info;
> ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
> + if (!ret) {
> + mutex_lock(&fb_info->mm_lock);
> + /*
> + * We must prevent any operations for this transition, we
> + * already have info->lock so grab the info->mm_lock to hold
> + * the remainder.
> + */
> + fb_info->state = FBINFO_STATE_REMOVED;
> + mutex_unlock(&fb_info->mm_lock);
> + }
> unlock_fb_info(fb_info);
>
> if (ret) {
> @@ -1646,8 +1721,7 @@ unregister_framebuffer(struct fb_info *fb_info)
> fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
>
> /* this may free fb info */
> - if (fb_info->fbops->fb_destroy)
> - fb_info->fbops->fb_destroy(fb_info);
> + put_framebuffer_info(fb_info);
> done:
> return ret;
> }
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index df728c1..60de3fa 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -834,6 +834,7 @@ struct fb_tile_ops {
> struct fb_info {
> int node;
> int flags;
> + int ref_count;
> struct mutex lock; /* Lock for open/release/ioctl funcs */
> struct mutex mm_lock; /* Lock for fb_mmap and smem_* fields */
> struct fb_var_screeninfo var; /* Current var */
> @@ -873,6 +874,7 @@ struct fb_info {
> void *pseudo_palette; /* Fake palette of 16 colors */
> #define FBINFO_STATE_RUNNING 0
> #define FBINFO_STATE_SUSPENDED 1
> +#define FBINFO_STATE_REMOVED 2
> u32 state; /* Hardware state i.e suspend */
> void *fbcon_par; /* fbcon use-only private area */
> /* From here on everything is device dependent */
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Tested-by: Anca Emanuel <anca.emanuel.gmail.com>
I can not use S3 resume without this.
[ 21.964367] BUG: unable to handle kernel paging request at 0000010a00000010
[ 21.964396] IP: [<ffffffff8130abe0>] fb_release+0x30/0x70
[ 21.964410] PGD 0
[ 21.964416] Oops: 0000 [#1] SMP
[ 21.964424] last sysfs file: /sys/devices/virtual/vtconsole/vtcon1/uevent
[ 21.964434] CPU 1
[ 21.964438] Modules linked in: parport_pc ppdev
snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm
adt7475 hwmon_vid snd_seq_midi snd_rawmidi snd_seq_midi_event nouveau
snd_seq snd_timer snd_seq_device ttm drm_kms_helper snd intel_agp
psmouse soundcore serio_raw intel_gtt snd_page_alloc drm i2c_algo_bit
video lp parport pata_marvell ahci libahci r8169 mii
[ 21.964528]
[ 21.964533] Pid: 221, comm: plymouthd Not tainted 2.6.39-rc6 #7
MICRO-STAR INTERNATIONAL CO.,LTD MS-7360/MS-7360
[ 21.964548] RIP: 0010:[<ffffffff8130abe0>] [<ffffffff8130abe0>]
fb_release+0x30/0x70
[ 21.964560] RSP: 0018:ffff880037211eb8 EFLAGS: 00010286
[ 21.964566] RAX: ffff880037210000 RBX: ffff88007f817000 RCX: 0000000000000001
[ 21.964573] RDX: 0000010a00000000 RSI: ffff8800370f5540 RDI: ffff88007f817008
[ 21.964580] RBP: ffff880037211ec8 R08: 0000000000000000 R09: 0000000000000000
[ 21.964588] R10: ffff8800370f5550 R11: 0000000000000246 R12: ffff88007f817008
[ 21.964595] R13: ffff88007d3db540 R14: ffff88007be34d90 R15: ffff88007be34d90
[ 21.964604] FS: 00007fb335025720(0000) GS:ffff88007fc80000(0000)
knlGS:0000000000000000
[ 21.964739] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 21.964746] CR2: 0000010a00000010 CR3: 000000007b41a000 CR4: 00000000000006e0
[ 21.964754] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 21.964762] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 21.964770] Process plymouthd (pid: 221, threadinfo
ffff880037210000, task ffff880036cd16c0)
[ 21.964778] Stack:
[ 21.964782] ffff8800370f5540 0000000000000008 ffff880037211f18
ffffffff8115cfaa
[ 21.964797] ffff8800370f5550 ffff8800793c7b00 ffff88006744fcd0
ffff8800370f5540
[ 21.964811] ffff88007c3b9080 0000000000000000 000000000000000b
0000000000000000
[ 21.964825] Call Trace:
[ 21.964834] [<ffffffff8115cfaa>] fput+0xea/0x220
[ 21.964842] [<ffffffff811591f6>] filp_close+0x66/0x90
[ 21.964849] [<ffffffff811597c7>] sys_close+0xb7/0x120
[ 21.964858] [<ffffffff815b3002>] system_call_fastpath+0x16/0x1b
[ 21.964865] Code: 83 ec 10 48 89 1c 24 4c 89 64 24 08 0f 1f 44 00
00 48 8b 9e a0 00 00 00 4c 8d 63 08 4c 89 e7 e8 d7 ea 29 00 48 8b 93
b8 03 00 00
[ 21.964944] 8b 42 10 48 85 c0 74 11 be 01 00 00 00 48 89 df ff d0 48 8b
[ 21.964983] RIP [<ffffffff8130abe0>] fb_release+0x30/0x70
[ 21.964992] RSP <ffff880037211eb8>
[ 21.964997] CR2: 0000010a00000010
^ permalink raw reply
* Re: [PATCH] fbcon -- fix race between open and removal of framebuffers
From: Jack Stone @ 2011-05-05 21:00 UTC (permalink / raw)
To: tim.gardner
Cc: linux-fbdev, lethal, linux-kernel, Andy Whitcroft,
Leann Ogasawara
In-Reply-To: <1304617307-7389-2-git-send-email-tim.gardner@canonical.com>
On 05/05/2011 18:41, tim.gardner@canonical.com wrote:
> +static struct fb_info *get_framebuffer_info(int idx)
> +__acquires(®istered_lock)
> +__releases(®istered_lock)
> +{
> + struct fb_info *fb_info;
> +
> + spin_lock(®istered_lock);
> + fb_info = registered_fb[idx];
> + fb_info->ref_count++;
> + spin_unlock(®istered_lock);
> +
> + return fb_info;
> +}
> +
> static int
> fb_open(struct inode *inode, struct file *file)
> __acquires(&info->lock)
> @@ -1363,13 +1421,17 @@ __releases(&info->lock)
>
> if (fbidx >= FB_MAX)
> return -ENODEV;
> - info = registered_fb[fbidx];
> + info = get_framebuffer_info(fbidx);
> if (!info)
> request_module("fb%d", fbidx);
> - info = registered_fb[fbidx];
> + info = get_framebuffer_info(fbidx);
> if (!info)
> return -ENODEV;
If the first get_framebuffer_info succeeds don't you up the ref count
twice? Shouldn't this be:
info = get_framebuffer_info(fbidx);
if (!info) {
request_module("fb%d", fbidx);
info = get_framebuffer_info(fbidx);
}
if (!info)
return -ENODEV;
Thanks,
Jack
^ permalink raw reply
* Re: [PATCH] fbcon -- fix race between open and removal of
From: Bruno Prémont @ 2011-05-05 18:30 UTC (permalink / raw)
To: tim.gardner
Cc: linux-fbdev, lethal, linux-kernel, Andy Whitcroft,
Leann Ogasawara
In-Reply-To: <1304617307-7389-2-git-send-email-tim.gardner@canonical.com>
On Thu, 05 May 2011 tim.gardner@canonical.com wrote:
> From: Andy Whitcroft <apw@canonical.com>
>
> Currently there is no locking for updates to the registered_fb list.
> This allows an open through /dev/fbN to pick up a registered framebuffer
> pointer in parallel with it being released, as happens when a conflicting
> framebuffer is ejected or on module unload. There is also no reference
> counting on the framebuffer descriptor which is referenced from all open
> files, leading to references to released or reused memory to persist on
> these open files.
>
> This patch adds a reference count to the framebuffer descriptor to prevent
> it from being released until after all pending opens are closed. This
> allows the pending opens to detect the closed status and unmap themselves.
> It also adds locking to the framebuffer lookup path, locking it against
> the removal path such that it is possible to atomically lookup and take a
> reference to the descriptor. It also adds locking to the read and write
> paths which currently could access the framebuffer descriptor after it
> has been freed. Finally it moves the device to FBINFO_STATE_REMOVED to
> indicate that all access should be errored for this device.
Is there a good reason to not use kref for the refcounting? Except for
(un)registering framebuffers this would avoid the need for taking
registered_lock.
Unfortunately fbcon also accesses registered_fb (quite a lot!) but it
probably is save enough through use of the notifiers.
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> Acked-by: Stefan Bader <stefan.bader@canonical.com>
> Signed-off-by: Leann Ogasawara <leann.ogasawara@canonical.com>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
> drivers/video/fbmem.c | 132 ++++++++++++++++++++++++++++++++++++++-----------
> include/linux/fb.h | 2 +
> 2 files changed, 105 insertions(+), 29 deletions(-)
>
...
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index df728c1..60de3fa 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -834,6 +834,7 @@ struct fb_tile_ops {
> struct fb_info {
> int node;
> int flags;
> + int ref_count;
> struct mutex lock; /* Lock for open/release/ioctl funcs */
> struct mutex mm_lock; /* Lock for fb_mmap and smem_* fields */
> struct fb_var_screeninfo var; /* Current var */
> @@ -873,6 +874,7 @@ struct fb_info {
> void *pseudo_palette; /* Fake palette of 16 colors */
> #define FBINFO_STATE_RUNNING 0
> #define FBINFO_STATE_SUSPENDED 1
> +#define FBINFO_STATE_REMOVED 2
> u32 state; /* Hardware state i.e suspend */
> void *fbcon_par; /* fbcon use-only private area */
> /* From here on everything is device dependent */
^ permalink raw reply
* [PATCH] fbcon -- fix race between open and removal of framebuffers
From: tim.gardner @ 2011-05-05 17:41 UTC (permalink / raw)
To: linux-fbdev
Cc: lethal, linux-kernel, Andy Whitcroft, Leann Ogasawara,
Tim Gardner
In-Reply-To: <1304617307-7389-1-git-send-email-tim.gardner@canonical.com>
From: Andy Whitcroft <apw@canonical.com>
Currently there is no locking for updates to the registered_fb list.
This allows an open through /dev/fbN to pick up a registered framebuffer
pointer in parallel with it being released, as happens when a conflicting
framebuffer is ejected or on module unload. There is also no reference
counting on the framebuffer descriptor which is referenced from all open
files, leading to references to released or reused memory to persist on
these open files.
This patch adds a reference count to the framebuffer descriptor to prevent
it from being released until after all pending opens are closed. This
allows the pending opens to detect the closed status and unmap themselves.
It also adds locking to the framebuffer lookup path, locking it against
the removal path such that it is possible to atomically lookup and take a
reference to the descriptor. It also adds locking to the read and write
paths which currently could access the framebuffer descriptor after it
has been freed. Finally it moves the device to FBINFO_STATE_REMOVED to
indicate that all access should be errored for this device.
Signed-off-by: Andy Whitcroft <apw@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Leann Ogasawara <leann.ogasawara@canonical.com>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
drivers/video/fbmem.c | 132 ++++++++++++++++++++++++++++++++++++++-----------
include/linux/fb.h | 2 +
2 files changed, 105 insertions(+), 29 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index e0c2284..c8562c1 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -42,6 +42,8 @@
#define FBPIXMAPSIZE (1024 * 8)
+/* Protects the registered framebuffer list and count. */
+static DEFINE_SPINLOCK(registered_lock);
struct fb_info *registered_fb[FB_MAX] __read_mostly;
int num_registered_fb __read_mostly;
@@ -694,9 +696,7 @@ static ssize_t
fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct inode *inode = file->f_path.dentry->d_inode;
- int fbidx = iminor(inode);
- struct fb_info *info = registered_fb[fbidx];
+ struct fb_info *info = file->private_data;
u8 *buffer, *dst;
u8 __iomem *src;
int c, cnt = 0, err = 0;
@@ -705,19 +705,28 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
if (!info || ! info->screen_base)
return -ENODEV;
- if (info->state != FBINFO_STATE_RUNNING)
- return -EPERM;
+ if (!lock_fb_info(info))
+ return -ENODEV;
+
+ if (info->state != FBINFO_STATE_RUNNING) {
+ err = -EPERM;
+ goto out_fb_info;
+ }
- if (info->fbops->fb_read)
- return info->fbops->fb_read(info, buf, count, ppos);
+ if (info->fbops->fb_read) {
+ err = info->fbops->fb_read(info, buf, count, ppos);
+ goto out_fb_info;
+ }
total_size = info->screen_size;
if (total_size = 0)
total_size = info->fix.smem_len;
- if (p >= total_size)
- return 0;
+ if (p >= total_size) {
+ err = 0;
+ goto out_fb_info;
+ }
if (count >= total_size)
count = total_size;
@@ -727,8 +736,10 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
GFP_KERNEL);
- if (!buffer)
- return -ENOMEM;
+ if (!buffer) {
+ err = -ENOMEM;
+ goto out_fb_info;
+ }
src = (u8 __iomem *) (info->screen_base + p);
@@ -751,19 +762,21 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
cnt += c;
count -= c;
}
+ if (!err)
+ err = cnt;
kfree(buffer);
+out_fb_info:
+ unlock_fb_info(info);
- return (err) ? err : cnt;
+ return err;
}
static ssize_t
fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
{
unsigned long p = *ppos;
- struct inode *inode = file->f_path.dentry->d_inode;
- int fbidx = iminor(inode);
- struct fb_info *info = registered_fb[fbidx];
+ struct fb_info *info = file->private_data;
u8 *buffer, *src;
u8 __iomem *dst;
int c, cnt = 0, err = 0;
@@ -772,8 +785,13 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
if (!info || !info->screen_base)
return -ENODEV;
- if (info->state != FBINFO_STATE_RUNNING)
- return -EPERM;
+ if (!lock_fb_info(info))
+ return -ENODEV;
+
+ if (info->state != FBINFO_STATE_RUNNING) {
+ err = -EPERM;
+ goto out_fb_info;
+ }
if (info->fbops->fb_write)
return info->fbops->fb_write(info, buf, count, ppos);
@@ -783,8 +801,10 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
if (total_size = 0)
total_size = info->fix.smem_len;
- if (p > total_size)
- return -EFBIG;
+ if (p > total_size) {
+ err = -EFBIG;
+ goto out_fb_info;
+ }
if (count > total_size) {
err = -EFBIG;
@@ -800,8 +820,10 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
GFP_KERNEL);
- if (!buffer)
- return -ENOMEM;
+ if (!buffer) {
+ err = -ENOMEM;
+ goto out_fb_info;
+ }
dst = (u8 __iomem *) (info->screen_base + p);
@@ -825,10 +847,14 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
cnt += c;
count -= c;
}
+ if (cnt)
+ err = cnt;
kfree(buffer);
+out_fb_info:
+ unlock_fb_info(info);
- return (cnt) ? cnt : err;
+ return err;
}
int
@@ -1303,8 +1329,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd,
static int
fb_mmap(struct file *file, struct vm_area_struct * vma)
{
- int fbidx = iminor(file->f_path.dentry->d_inode);
- struct fb_info *info = registered_fb[fbidx];
+ struct fb_info * const info = file->private_data;
struct fb_ops *fb = info->fbops;
unsigned long off;
unsigned long start;
@@ -1316,6 +1341,11 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
if (!fb)
return -ENODEV;
mutex_lock(&info->mm_lock);
+ if (info->state = FBINFO_STATE_REMOVED) {
+ mutex_unlock(&info->mm_lock);
+ return -ENODEV;
+ }
+
if (fb->fb_mmap) {
int res;
res = fb->fb_mmap(info, vma);
@@ -1352,6 +1382,34 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
return 0;
}
+static struct fb_info *get_framebuffer_info(int idx)
+__acquires(®istered_lock)
+__releases(®istered_lock)
+{
+ struct fb_info *fb_info;
+
+ spin_lock(®istered_lock);
+ fb_info = registered_fb[idx];
+ fb_info->ref_count++;
+ spin_unlock(®istered_lock);
+
+ return fb_info;
+}
+
+static void put_framebuffer_info(struct fb_info *fb_info)
+__acquires(®istered_lock)
+__releases(®istered_lock)
+{
+ int keep;
+
+ spin_lock(®istered_lock);
+ keep = --fb_info->ref_count;
+ spin_unlock(®istered_lock);
+
+ if (!keep && fb_info->fbops->fb_destroy)
+ fb_info->fbops->fb_destroy(fb_info);
+}
+
static int
fb_open(struct inode *inode, struct file *file)
__acquires(&info->lock)
@@ -1363,13 +1421,17 @@ __releases(&info->lock)
if (fbidx >= FB_MAX)
return -ENODEV;
- info = registered_fb[fbidx];
+ info = get_framebuffer_info(fbidx);
if (!info)
request_module("fb%d", fbidx);
- info = registered_fb[fbidx];
+ info = get_framebuffer_info(fbidx);
if (!info)
return -ENODEV;
mutex_lock(&info->lock);
+ if (info->state = FBINFO_STATE_REMOVED) {
+ res = -ENODEV;
+ goto out;
+ }
if (!try_module_get(info->fbops->owner)) {
res = -ENODEV;
goto out;
@@ -1386,6 +1448,8 @@ __releases(&info->lock)
#endif
out:
mutex_unlock(&info->lock);
+ if (res)
+ put_framebuffer_info(info);
return res;
}
@@ -1401,6 +1465,7 @@ __releases(&info->lock)
info->fbops->fb_release(info,1);
module_put(info->fbops->owner);
mutex_unlock(&info->lock);
+ put_framebuffer_info(info);
return 0;
}
@@ -1549,6 +1614,7 @@ register_framebuffer(struct fb_info *fb_info)
fb_info->node = i;
mutex_init(&fb_info->lock);
mutex_init(&fb_info->mm_lock);
+ fb_info->ref_count = 1;
fb_info->dev = device_create(fb_class, fb_info->device,
MKDEV(FB_MAJOR, i), NULL, "fb%d", i);
@@ -1592,7 +1658,6 @@ register_framebuffer(struct fb_info *fb_info)
return 0;
}
-
/**
* unregister_framebuffer - releases a frame buffer device
* @fb_info: frame buffer info structure
@@ -1627,6 +1692,16 @@ unregister_framebuffer(struct fb_info *fb_info)
return -ENODEV;
event.info = fb_info;
ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
+ if (!ret) {
+ mutex_lock(&fb_info->mm_lock);
+ /*
+ * We must prevent any operations for this transition, we
+ * already have info->lock so grab the info->mm_lock to hold
+ * the remainder.
+ */
+ fb_info->state = FBINFO_STATE_REMOVED;
+ mutex_unlock(&fb_info->mm_lock);
+ }
unlock_fb_info(fb_info);
if (ret) {
@@ -1646,8 +1721,7 @@ unregister_framebuffer(struct fb_info *fb_info)
fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
/* this may free fb info */
- if (fb_info->fbops->fb_destroy)
- fb_info->fbops->fb_destroy(fb_info);
+ put_framebuffer_info(fb_info);
done:
return ret;
}
diff --git a/include/linux/fb.h b/include/linux/fb.h
index df728c1..60de3fa 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -834,6 +834,7 @@ struct fb_tile_ops {
struct fb_info {
int node;
int flags;
+ int ref_count;
struct mutex lock; /* Lock for open/release/ioctl funcs */
struct mutex mm_lock; /* Lock for fb_mmap and smem_* fields */
struct fb_var_screeninfo var; /* Current var */
@@ -873,6 +874,7 @@ struct fb_info {
void *pseudo_palette; /* Fake palette of 16 colors */
#define FBINFO_STATE_RUNNING 0
#define FBINFO_STATE_SUSPENDED 1
+#define FBINFO_STATE_REMOVED 2
u32 state; /* Hardware state i.e suspend */
void *fbcon_par; /* fbcon use-only private area */
/* From here on everything is device dependent */
--
1.7.0.4
^ permalink raw reply related
* [PATCH 0/1] fbcon -- fix race between open and removal of framebuffers
From: tim.gardner @ 2011-05-05 17:41 UTC (permalink / raw)
To: linux-fbdev; +Cc: lethal, linux-kernel
I'm sending this out for Andy since he is a bit busy this week and has been
gone for the past several weeks. Its been on our todo list to get this
upstreamed, but release pressures have intervened. Mea culpa. I've also had
a couple of requests recently which have served to remind me to get off my butt.
https://lists.ubuntu.com/archives/kernel-team/2011-May/015544.html
rtg
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox