[RFC] fbcon: fb_set_par error handling

[RFC] fbcon: fb_set_par error handling

[Florian Tobias Schandinat]

Hi all,

at the moment about half of the framebuffer drivers can return an error
code in fb_set_par. Until now it would be silently ignored by fbmem.c
and fbcon.c. The following patch fixes fbmem.c to pass it on but I have
no idea what the right behaviour in fbcon.c would look like.
Any comments?

The following files might return a non-zero value:
(false positives are possible as I scanned them only superficial)

drivers/gpu/drm/i915/intel_fb.c
drivers/gpu/drm/radeon/radeon_fb.c
drivers/video/matrox/matroxfb_crtc2.c
drivers/video/matrox/matroxfb_base.c
drivers/video/vt8623fb.c
drivers/video/vermilion/vermilion.c
drivers/video/uvesafb.c
drivers/video/neofb.c
drivers/video/aty/radeon_base.c
drivers/video/aty/atyfb_base.c
drivers/video/aty/aty128fb.c
drivers/video/platinumfb.c
drivers/video/intelfb/intelfbdrv.c
drivers/video/omap/omapfb_main.c
drivers/video/skeletonfb.c
drivers/video/s3fb.c
drivers/video/savage/savagefb_driver.c
drivers/video/gxt4500.c
drivers/video/tmiofb.c
drivers/video/pxafb.c
drivers/video/mbx/mbxfb.c
drivers/video/sis/sis_main.c
drivers/video/mx3fb.c
drivers/video/arkfb.c
drivers/video/fsl-diu-fb.c
drivers/video/valkyriefb.c
drivers/video/pm2fb.c
drivers/video/sstfb.c
drivers/video/imsttfb.c
drivers/video/carminefb.c
drivers/video/riva/fbdev.c
drivers/video/sh7760fb.c
drivers/video/sm501fb.c
drivers/video/ps3fb.c
drivers/video/controlfb.c

[PATCH] fb: do not ignore fb_set_par errors

[Florian Tobias Schandinat]

fb: do not ignore fb_set_par errors

At the moment about half of the framebuffer drivers can return an error
code in fb_set_par. Until now it would be silently ignored by fbmem.c
and fbcon.c. This patch fixes fbmem.c to return the error code and
restore var on error. But it is not clear in which video mode the device
is when fb_set_par fails. It would be good and reasonable if it were in
the old state but there is no guarantee that this is true for all
existing drivers.
Although most errors should be caught by the previous fb_check_var some
errors can't as they are dynamic (memory allocations, ...) and can only
be detected while performing the operations which is forbidden in
fb_check_var.
This patch shouldn't have a negative impact on normal operation as all
drivers return 0 on success. The impact in case of error depends heavily
on the driver and caller but it's expected to be better than before.

Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
---
 drivers/video/fbmem.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index a85c818..85c1595 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -954,6 +954,7 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 			goto done;
 
 		if ((var->activate & FB_ACTIVATE_MASK) == FB_ACTIVATE_NOW) {
+			struct fb_var_screeninfo old_var;
 			struct fb_videomode mode;
 
 			if (info->fbops->fb_get_caps) {
@@ -963,10 +964,17 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 					goto done;
 			}
 
+			old_var = info->var;
 			info->var = *var;
 
-			if (info->fbops->fb_set_par)
-				info->fbops->fb_set_par(info);
+			if (info->fbops->fb_set_par) {
+				ret = info->fbops->fb_set_par(info);
+
+				if (ret) {
+					info->var = old_var;
+					goto done;
+				}
+			}
 
 			fb_pan_display(info, &info->var);
 			fb_set_cmap(&info->cmap, info);
-- 
1.6.3.2

Re: [PATCH] fb: do not ignore fb_set_par errors

[Andrew Morton]

On Thu, 20 Aug 2009 11:59:56 +0000
Florian Tobias Schandinat <FlorianSchandinat@gmx.de> wrote:

> at the moment about half of the framebuffer drivers can return an error
> code in fb_set_par. Until now it would be silently ignored by fbmem.c
> and fbcon.c. The following patch fixes fbmem.c to pass it on but I have
> no idea what the right behaviour in fbcon.c would look like.
> Any comments?

Nobody really maintains this code any more and I don't know to whom
your question should be addressed.  All I can suggest is that you
decide what is the best path to take, and propose that we take it!  

On Thu, 20 Aug 2009 11:59:57 +0000
Florian Tobias Schandinat <FlorianSchandinat@gmx.de> wrote:

> fb: do not ignore fb_set_par errors
> 
> At the moment about half of the framebuffer drivers can return an error
> code in fb_set_par. Until now it would be silently ignored by fbmem.c
> and fbcon.c. This patch fixes fbmem.c to return the error code and
> restore var on error. But it is not clear in which video mode the device
> is when fb_set_par fails. It would be good and reasonable if it were in
> the old state but there is no guarantee that this is true for all
> existing drivers.
> Although most errors should be caught by the previous fb_check_var some
> errors can't as they are dynamic (memory allocations, ...) and can only
> be detected while performing the operations which is forbidden in
> fb_check_var.
> This patch shouldn't have a negative impact on normal operation as all
> drivers return 0 on success. The impact in case of error depends heavily
> on the driver and caller but it's expected to be better than before.
> 
> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> ---
>  drivers/video/fbmem.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index a85c818..85c1595 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -954,6 +954,7 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>  			goto done;
>  
>  		if ((var->activate & FB_ACTIVATE_MASK) == FB_ACTIVATE_NOW) {
> +			struct fb_var_screeninfo old_var;
>  			struct fb_videomode mode;
>  
>  			if (info->fbops->fb_get_caps) {
> @@ -963,10 +964,17 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
>  					goto done;
>  			}
>  
> +			old_var = info->var;
>  			info->var = *var;
>  
> -			if (info->fbops->fb_set_par)
> -				info->fbops->fb_set_par(info);
> +			if (info->fbops->fb_set_par) {
> +				ret = info->fbops->fb_set_par(info);
> +
> +				if (ret) {
> +					info->var = old_var;
> +					goto done;
> +				}
> +			}

Yes, it seems reasonable to me - if we ignore this the machine will
probably crash later anyway.

A well-behaved driver should leave the hardware in an unchanged state
if it's going to return an error (IMO).

Do you know of any bug reports which are due to this lack of checking?

If you're concerned about the effects of this change, perhaps you
should add a nice big printk when we decide to error out, so that it
makes it easier for bug reporters (and responders!) to work out what
happened?

Re: [PATCH] fb: do not ignore fb_set_par errors

[Florian Tobias Schandinat]

Andrew Morton schrieb:
> On Thu, 20 Aug 2009 11:59:56 +0000
> Florian Tobias Schandinat <FlorianSchandinat@gmx.de> wrote:
> 
>> at the moment about half of the framebuffer drivers can return an error
>> code in fb_set_par. Until now it would be silently ignored by fbmem.c
>> and fbcon.c. The following patch fixes fbmem.c to pass it on but I have
>> no idea what the right behaviour in fbcon.c would look like.
>> Any comments?
> 
> Nobody really maintains this code any more and I don't know to whom
> your question should be addressed.  All I can suggest is that you
> decide what is the best path to take, and propose that we take it!  

Okay. I think it's difficult to ensure that we always end up with a 
working console. I assume that this is a very very rare case and as a 
start it should be enough to detect those cases and print an error message.

> Yes, it seems reasonable to me - if we ignore this the machine will
> probably crash later anyway.
> 
> A well-behaved driver should leave the hardware in an unchanged state
> if it's going to return an error (IMO).
> 
> Do you know of any bug reports which are due to this lack of checking?

No, I just hit the problem by thinking about how dynamic errors can be 
handled and looking at some drivers and fbmem.c.

> If you're concerned about the effects of this change, perhaps you
> should add a nice big printk when we decide to error out, so that it
> makes it easier for bug reporters (and responders!) to work out what
> happened?

That's a very good idea. Will add this too and resend the patch.


Thanks,

Florian Tobias Schandinat

[PATCH v2] fb: do not ignore fb_set_par errors

[Florian Tobias Schandinat]

fb: do not ignore fb_set_par errors

At the moment about half of the framebuffer drivers can return an error
code in fb_set_par. Until now it would be silently ignored by fbmem.c
and fbcon.c. This patch fixes fbmem.c to return the error code and
restore var on error. But it is not clear in which video mode the device
is when fb_set_par fails. It would be good and reasonable if it were in
the old state but there is no guarantee that this is true for all
existing drivers. Additionally print a message if a failing fb_set_par
is detected in fbmem.c or fbcon.c.
Although most errors should be caught by the previous fb_check_var some
errors can't as they are dynamic (memory allocations, ...) and can only
be detected while performing the operations which is forbidden in
fb_check_var.
This patch shouldn't have a negative impact on normal operation as all
drivers return 0 on success. The impact in case of error depends heavily
on the driver and caller but it's expected to be better than before.

Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
---
v2: print warning/error messages if fb_set_par fails
---
 drivers/video/console/fbcon.c |   48 +++++++++++++++++++++++++++++++---------
 drivers/video/fbmem.c         |   15 +++++++++++-
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index 3a44695..4bee7c2 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -725,7 +725,7 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
 				  int oldidx, int found)
 {
 	struct fbcon_ops *ops = oldinfo->fbcon_par;
-	int err = 0;
+	int err = 0, ret;
 
 	if (oldinfo->fbops->fb_release &&
 	    oldinfo->fbops->fb_release(oldinfo, 0)) {
@@ -752,8 +752,14 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo,
 		  newinfo in an undefined state. Thus, a call to
 		  fb_set_par() may be needed for the newinfo.
 		*/
-		if (newinfo->fbops->fb_set_par)
-			newinfo->fbops->fb_set_par(newinfo);
+		if (newinfo->fbops->fb_set_par) {
+			ret = newinfo->fbops->fb_set_par(newinfo);
+
+			if (ret)
+				printk(KERN_ERR "con2fb_release_oldinfo: "
+					"detected unhandled fb_set_par error, "
+					"error code %d\n", ret);
+		}
 	}
 
 	return err;
@@ -763,11 +769,18 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
 				int unit, int show_logo)
 {
 	struct fbcon_ops *ops = info->fbcon_par;
+	int ret;
 
 	ops->currcon = fg_console;
 
-	if (info->fbops->fb_set_par && !(ops->flags & FBCON_FLAGS_INIT))
-		info->fbops->fb_set_par(info);
+	if (info->fbops->fb_set_par && !(ops->flags & FBCON_FLAGS_INIT)) {
+		ret = info->fbops->fb_set_par(info);
+
+		if (ret)
+			printk(KERN_ERR "con2fb_init_display: detected "
+				"unhandled fb_set_par error, "
+				"error code %d\n", ret);
+	}
 
 	ops->flags |= FBCON_FLAGS_INIT;
 	ops->graphics = 0;
@@ -1006,7 +1019,7 @@ static void fbcon_init(struct vc_data *vc, int init)
 	struct vc_data *svc = *default_mode;
 	struct display *t, *p = &fb_display[vc->vc_num];
 	int logo = 1, new_rows, new_cols, rows, cols, charcnt = 256;
-	int cap;
+	int cap, ret;
 
 	if (info_idx == -1 || info == NULL)
 	    return;
@@ -1092,8 +1105,15 @@ static void fbcon_init(struct vc_data *vc, int init)
 	 */
 	if (CON_IS_VISIBLE(vc) && vc->vc_mode == KD_TEXT) {
 		if (info->fbops->fb_set_par &&
-		    !(ops->flags & FBCON_FLAGS_INIT))
-			info->fbops->fb_set_par(info);
+		    !(ops->flags & FBCON_FLAGS_INIT)) {
+			ret = info->fbops->fb_set_par(info);
+
+			if (ret)
+				printk(KERN_ERR "fbcon_init: detected "
+					"unhandled fb_set_par error, "
+					"error code %d\n", ret);
+		}
+
 		ops->flags |= FBCON_FLAGS_INIT;
 	}
 
@@ -2119,7 +2139,7 @@ static int fbcon_switch(struct vc_data *vc)
 	struct fbcon_ops *ops;
 	struct display *p = &fb_display[vc->vc_num];
 	struct fb_var_screeninfo var;
-	int i, prev_console, charcnt = 256;
+	int i, ret, prev_console, charcnt = 256;
 
 	info = registered_fb[con2fb_map[vc->vc_num]];
 	ops = info->fbcon_par;
@@ -2174,8 +2194,14 @@ static int fbcon_switch(struct vc_data *vc)
 
 	if (old_info != NULL && (old_info != info ||
 				 info->flags & FBINFO_MISC_ALWAYS_SETPAR)) {
-		if (info->fbops->fb_set_par)
-			info->fbops->fb_set_par(info);
+		if (info->fbops->fb_set_par) {
+			ret = info->fbops->fb_set_par(info);
+
+			if (ret)
+				printk(KERN_ERR "fbcon_switch: detected "
+					"unhandled fb_set_par error, "
+					"error code %d\n", ret);
+		}
 
 		if (old_info != info)
 			fbcon_del_cursor_timer(old_info);
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index a85c818..008f95a 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -954,6 +954,7 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 			goto done;
 
 		if ((var->activate & FB_ACTIVATE_MASK) == FB_ACTIVATE_NOW) {
+			struct fb_var_screeninfo old_var;
 			struct fb_videomode mode;
 
 			if (info->fbops->fb_get_caps) {
@@ -963,10 +964,20 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 					goto done;
 			}
 
+			old_var = info->var;
 			info->var = *var;
 
-			if (info->fbops->fb_set_par)
-				info->fbops->fb_set_par(info);
+			if (info->fbops->fb_set_par) {
+				ret = info->fbops->fb_set_par(info);
+
+				if (ret) {
+					info->var = old_var;
+					printk(KERN_WARNING "detected "
+						"fb_set_par error, "
+						"error code: %d\n", ret);
+					goto done;
+				}
+			}
 
 			fb_pan_display(info, &info->var);
 			fb_set_cmap(&info->cmap, info);
-- 
1.6.3.2