[RFC PATCH 1/5] ACPI video: check the return value of acpi_video_device_lcd_get_level_current

[RFC PATCH 1/5] ACPI video: check the return value of acpi_video_device_lcd_get_level_current

[Zhang Rui]

Subject: check the return value of acpi_video_device_lcd_get_level_current
From: Zhang Rui <rui.zhang@intel.com>

check the return value of acpi_video_device_lcd_get_level_current

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/video.c |   69 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 47 insertions(+), 22 deletions(-)

Index: linux-2.6/drivers/acpi/video.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video.c
+++ linux-2.6/drivers/acpi/video.c
@@ -294,7 +294,7 @@ static int acpi_video_device_lcd_get_lev
 			unsigned long long *level);
 static int acpi_video_get_next_level(struct acpi_video_device *device,
 				     u32 level_current, u32 event);
-static void acpi_video_switch_brightness(struct acpi_video_device *device,
+static int acpi_video_switch_brightness(struct acpi_video_device *device,
 					 int event);
 static int acpi_video_device_get_state(struct acpi_video_device *device,
 			    unsigned long long *state);
@@ -308,7 +308,9 @@ static int acpi_video_get_brightness(str
 	int i;
 	struct acpi_video_device *vd =
 		(struct acpi_video_device *)bl_get_data(bd);
-	acpi_video_device_lcd_get_level_current(vd, &cur_level);
+
+	if (acpi_video_device_lcd_get_level_current(vd, &cur_level))
+		return -EINVAL;
 	for (i = 2; i < vd->brightness->count; i++) {
 		if (vd->brightness->levels[i] == cur_level)
 			/* The first two entries are special - see page 575
@@ -373,7 +375,8 @@ static int video_get_cur_state(struct th
 	unsigned long long level;
 	int state;
 
-	acpi_video_device_lcd_get_level_current(video, &level);
+	if (acpi_video_device_lcd_get_level_current(video, &level))
+		return -EINVAL;
 	for (state = 2; state < video->brightness->count; state++)
 		if (level == video->brightness->levels[state])
 			return sprintf(buf, "%d\n",
@@ -502,11 +505,29 @@ static int
 acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 					unsigned long long *level)
 {
-	if (device->cap._BQC)
-		return acpi_evaluate_integer(device->dev->handle, "_BQC", NULL,
-					     level);
+	acpi_status status = AE_OK;
+
+	if (device->cap._BQC) {
+		status = acpi_evaluate_integer(device->dev->handle, "_BQC",
+						NULL, level);
+		if (ACPI_SUCCESS(status)) {
+			device->brightness->curr = *level;
+			return 0;
+		} else {
+			/* Fixme:
+			 * should we return an error or ignore this failure?
+			 * dev->brightness->curr is a cached value which stores
+			 * the correct current backlight level in most cases.
+			 * ACPI video backlight still works w/ buggy _BQC.
+			 * http://bugzilla.kernel.org/show_bug.cgi?id=12233
+			 */
+			ACPI_WARNING((AE_INFO, "Evaluating _BQC failed"));
+			device->cap._BQC = 0;
+		}
+	}
+
 	*level = device->brightness->curr;
-	return AE_OK;
+	return 0;
 }
 
 static int
@@ -773,18 +794,8 @@ static void acpi_video_device_find_cap(s
 		device->backlight = backlight_device_register(name,
 			NULL, device, &acpi_backlight_ops);
 		device->backlight->props.max_brightness = device->brightness->count-3;
-		/*
-		 * If there exists the _BQC object, the _BQC object will be
-		 * called to get the current backlight brightness. Otherwise
-		 * the brightness will be set to the maximum.
-		 */
-		if (device->cap._BQC)
-			device->backlight->props.brightness =
-				acpi_video_get_brightness(device->backlight);
-		else
-			device->backlight->props.brightness =
-				device->backlight->props.max_brightness;
-		backlight_update_status(device->backlight);
+		device->backlight->props.brightness =
+			acpi_video_get_brightness(device->backlight);
 		kfree(name);
 
 		device->cdev = thermal_cooling_device_register("LCD",
@@ -1749,15 +1760,29 @@ acpi_video_get_next_level(struct acpi_vi
 	}
 }
 
-static void
+static int
 acpi_video_switch_brightness(struct acpi_video_device *device, int event)
 {
 	unsigned long long level_current, level_next;
+	int result = -EINVAL;
+
 	if (!device->brightness)
-		return;
-	acpi_video_device_lcd_get_level_current(device, &level_current);
+		goto out;
+
+	result = acpi_video_device_lcd_get_level_current(device,
+							 &level_current);
+	if (result)
+		goto out;
+
 	level_next = acpi_video_get_next_level(device, level_current, event);
+
 	acpi_video_device_lcd_set_level(device, level_next);
+
+out:
+	if (result)
+		printk(KERN_ERR PREFIX "Failed to switch the brightness\n");
+
+	return result;
 }
 
 static int

Re: [RFC PATCH 1/5] ACPI video: check the return value of acpi_video_device_lcd_get_level_current

[Thomas Renninger]

On Tuesday 10 March 2009 09:03:25 Zhang Rui wrote:
> Subject: check the return value of 
acpi_video_device_lcd_get_level_current
> From: Zhang Rui <rui.zhang@intel.com>
> 
> check the return value of acpi_video_device_lcd_get_level_current
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/video.c |   69 
++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 47 insertions(+), 22 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/video.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/video.c
> +++ linux-2.6/drivers/acpi/video.c
> @@ -294,7 +294,7 @@ static int acpi_video_device_lcd_get_lev
>  			unsigned long long *level);
>  static int acpi_video_get_next_level(struct acpi_video_device *device,
>  				     u32 level_current, u32 event);
> -static void acpi_video_switch_brightness(struct acpi_video_device 
*device,
> +static int acpi_video_switch_brightness(struct acpi_video_device 
*device,
>  					 int event);
>  static int acpi_video_device_get_state(struct acpi_video_device *device,
>  			    unsigned long long *state);
> @@ -308,7 +308,9 @@ static int acpi_video_get_brightness(str
>  	int i;
>  	struct acpi_video_device *vd =
>  		(struct acpi_video_device *)bl_get_data(bd);
> -	acpi_video_device_lcd_get_level_current(vd, &cur_level);
> +
> +	if (acpi_video_device_lcd_get_level_current(vd, &cur_level))
> +		return -EINVAL;
>  	for (i = 2; i < vd->brightness->count; i++) {
>  		if (vd->brightness->levels[i] == cur_level)
>  			/* The first two entries are special - see page 575
> @@ -373,7 +375,8 @@ static int video_get_cur_state(struct th
>  	unsigned long long level;
>  	int state;
>  
> -	acpi_video_device_lcd_get_level_current(video, &level);
> +	if (acpi_video_device_lcd_get_level_current(video, &level))
> +		return -EINVAL;
>  	for (state = 2; state < video->brightness->count; state++)
>  		if (level == video->brightness->levels[state])
>  			return sprintf(buf, "%d\n",
> @@ -502,11 +505,29 @@ static int
>  acpi_video_device_lcd_get_level_current(struct acpi_video_device 
*device,
>  					unsigned long long *level)
>  {
> -	if (device->cap._BQC)
> -		return acpi_evaluate_integer(device->dev->handle, "_BQC", NULL,
> -					     level);
> +	acpi_status status = AE_OK;
> +
> +	if (device->cap._BQC) {
> +		status = acpi_evaluate_integer(device->dev->handle, "_BQC",
> +						NULL, level);
> +		if (ACPI_SUCCESS(status)) {
> +			device->brightness->curr = *level;
> +			return 0;
> +		} else {
> +			/* Fixme:
> +			 * should we return an error or ignore this failure?
> +			 * dev->brightness->curr is a cached value which stores
> +			 * the correct current backlight level in most cases.
> +			 * ACPI video backlight still works w/ buggy _BQC.
> +			 * http://bugzilla.kernel.org/show_bug.cgi?id=12233
> +			 */
I wonder what should go wrong there at all.
We should add a warning (using printk(.. FW_BUG...)) if we do ACPI video 
brightness switching without _BQC. This should be supported, but is a 
firmware bug and likely fails.
(BTW, I recently saw a BIOS with _BCQ function. They said they are going to 
fix it, but it may be more widespread, e.g. this also is often the case 
(missing _BQC) on Samsung). I found it by luck disassembling and 
recompiling the DSDT, a runtime warning would be nice (if it does not 
already exist).
Anyway, if we have _BQC (cap._BQC), evaluation should not fail?
Ok, double checking is always a good idea...

> +			ACPI_WARNING((AE_INFO, "Evaluating _BQC failed"));
> +			device->cap._BQC = 0;
> +		}
> +	}
> +
>  	*level = device->brightness->curr;
> -	return AE_OK;
> +	return 0;
>  }
>  
>  static int
> @@ -773,18 +794,8 @@ static void acpi_video_device_find_cap(s
>  		device->backlight = backlight_device_register(name,
>  			NULL, device, &acpi_backlight_ops);
>  		device->backlight->props.max_brightness = device->brightness->count-3;
> -		/*
> -		 * If there exists the _BQC object, the _BQC object will be
> -		 * called to get the current backlight brightness. Otherwise
> -		 * the brightness will be set to the maximum.
> -		 */
> -		if (device->cap._BQC)
> -			device->backlight->props.brightness =
> -				acpi_video_get_brightness(device->backlight);
> -		else
> -			device->backlight->props.brightness =
> -				device->backlight->props.max_brightness;
> -		backlight_update_status(device->backlight);
> +		device->backlight->props.brightness =
> +			acpi_video_get_brightness(device->backlight);
I could imagine this introduces a regression on machines without _BQC.
IIRC the value which brightness is currently active must be initialized if 
there is no _BQC function and that seems to happen here.

     Thomas

Re: [RFC PATCH 1/5] ACPI video: check the return value of acpi_video_device_lcd_get_level_current

[Matthew Garrett]

On Wed, Mar 11, 2009 at 01:56:00PM +0100, Thomas Renninger wrote:

> (BTW, I recently saw a BIOS with _BCQ function. They said they are going to 
> fix it, but it may be more widespread, e.g. this also is often the case 
> (missing _BQC) on Samsung). I found it by luck disassembling and 
> recompiling the DSDT, a runtime warning would be nice (if it does not 
> already exist).

It's not that uncommon - there's a few machines with _BCQ. I actually 
thought we handled it already, but it seems not. Just adding a cap._BCQ 
and using it if there's no _BQC sounds like a safe idea.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC PATCH 1/5] ACPI video: check the return value of acpi_video_device_lcd_get_level_current

[Thomas Renninger]

On Wednesday 11 March 2009 14:04:32 Matthew Garrett wrote:
> On Wed, Mar 11, 2009 at 01:56:00PM +0100, Thomas Renninger wrote:
> 
> > (BTW, I recently saw a BIOS with _BCQ function. They said they are going to 
> > fix it, but it may be more widespread, e.g. this also is often the case 
> > (missing _BQC) on Samsung). I found it by luck disassembling and 
> > recompiling the DSDT, a runtime warning would be nice (if it does not 
> > already exist).
> 
> It's not that uncommon - there's a few machines with _BCQ. I actually 
> thought we handled it already, but it seems not. Just adding a cap._BCQ 
> and using it if there's no _BQC sounds like a safe idea.
As said, the current brightness must be initialized if there is
no _BQC and Rui seem to have been removed that.

Also a:
printk (KERN_WARN FW_BUG PREFIX "ACPI brightness control misses _BQC function\n");
should be added.
Rui, if you are there already, do you mind to add such test.
I found by pure luck that a vendor mixed up
_BQC and _BCQ by getting one single warning recompiling the DSDT:
"_BCQ not a predefined function" (or similar). They are now adding it,
but a warning is appropriate IMO in _BQC missing case.
Not adding _BQC and doing brightness switching through ACPI brightness
functions is a really bad idea in general.

    Thomas

Re: [RFC PATCH 1/5] ACPI video: check the return value of acpi_video_device_lcd_get_level_current

[Zhang Rui]

On Wed, 2009-03-11 at 20:56 +0800, Thomas Renninger wrote:
> On Tuesday 10 March 2009 09:03:25 Zhang Rui wrote:
> > Subject: check the return value of 
> acpi_video_device_lcd_get_level_current
> > From: Zhang Rui <rui.zhang@intel.com>
> > 
> > check the return value of acpi_video_device_lcd_get_level_current
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/acpi/video.c |   69 
> ++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 47 insertions(+), 22 deletions(-)
> > 
> > Index: linux-2.6/drivers/acpi/video.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/video.c
> > +++ linux-2.6/drivers/acpi/video.c
> > @@ -502,11 +505,29 @@ static int
> >  acpi_video_device_lcd_get_level_current(struct acpi_video_device 
> *device,
> >  					unsigned long long *level)
> >  {
> > -	if (device->cap._BQC)
> > -		return acpi_evaluate_integer(device->dev->handle, "_BQC", NULL,
> > -					     level);
> > +	acpi_status status = AE_OK;
> > +
> > +	if (device->cap._BQC) {
> > +		status = acpi_evaluate_integer(device->dev->handle, "_BQC",
> > +						NULL, level);
> > +		if (ACPI_SUCCESS(status)) {
> > +			device->brightness->curr = *level;
> > +			return 0;
> > +		} else {
> > +			/* Fixme:
> > +			 * should we return an error or ignore this failure?
> > +			 * dev->brightness->curr is a cached value which stores
> > +			 * the correct current backlight level in most cases.
> > +			 * ACPI video backlight still works w/ buggy _BQC.
> > +			 * http://bugzilla.kernel.org/show_bug.cgi?id=12233
> > +			 */
> I wonder what should go wrong there at all.
> We should add a warning (using printk(.. FW_BUG...)) if we do ACPI video 
> brightness switching without _BQC. This should be supported, but is a 
> firmware bug and likely fails.
I think we should do it in video_detect.c
a simple patch like this:

---
 drivers/acpi/video_detect.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6/drivers/acpi/video_detect.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video_detect.c
+++ linux-2.6/drivers/acpi/video_detect.c
@@ -55,6 +55,8 @@ acpi_backlight_cap_match(acpi_handle han
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found generic backlight "
 				  "support\n"));
 		*cap |= ACPI_VIDEO_BACKLIGHT;
+		if (ACPI_FAILURE(acpi_get_handle(handle, "_BQC", &h_dummy)))
+			printk(KERN_WARNING FW_BUG "No _BQC available\n");
 		/* We have backlight support, no need to scan further */
 		return AE_CTRL_TERMINATE;
 	}

But the ACPI warning here is still needed, because the _BQC method is
implemented but there are some errors when evaluating it.

> (BTW, I recently saw a BIOS with _BCQ function. They said they are going to 
> fix it, but it may be more widespread, e.g. this also is often the case 
> (missing _BQC) on Samsung). I found it by luck disassembling and 
> recompiling the DSDT, a runtime warning would be nice (if it does not 
> already exist).
Interesting, if this is tree, we can add the support for _BCQ in a
separate patch.
BTW: could you please attach the acpidump of that box please because I
never noticed this problem before. :)

> Anyway, if we have _BQC (cap._BQC), evaluation should not fail?
> Ok, double checking is always a good idea...
> 
No, evaluating _BQC may return fail, like the one in bug 12233,
        Method (_BQC, 0, NotSerialized)
        {
            And (\_SB.PCI0.LPCB.FJEX.GBLS, 0xFF, Local0)
            Return (DerefOf (Index (BCLP, Add (Local0, 0x02))))
        }
but \_SB.PCI0.LPCB.FJEX.GBLS doesn't exist at all...

> > +			ACPI_WARNING((AE_INFO, "Evaluating _BQC failed"));
> > +			device->cap._BQC = 0;
> > +		}
> > +	}
> > +
> >  	*level = device->brightness->curr;
> > -	return AE_OK;
> > +	return 0;
> >  }
> >  
> >  static int
> > @@ -773,18 +794,8 @@ static void acpi_video_device_find_cap(s
> >  		device->backlight = backlight_device_register(name,
> >  			NULL, device, &acpi_backlight_ops);
> >  		device->backlight->props.max_brightness = device->brightness->count-3;
> > -		/*
> > -		 * If there exists the _BQC object, the _BQC object will be
> > -		 * called to get the current backlight brightness. Otherwise
> > -		 * the brightness will be set to the maximum.
> > -		 */
> > -		if (device->cap._BQC)
> > -			device->backlight->props.brightness =
> > -				acpi_video_get_brightness(device->backlight);
> > -		else
> > -			device->backlight->props.brightness =
> > -				device->backlight->props.max_brightness;
> > -		backlight_update_status(device->backlight);
> > +		device->backlight->props.brightness =
> > +			acpi_video_get_brightness(device->backlight);
> I could imagine this introduces a regression on machines without _BQC.
> IIRC the value which brightness is currently active must be initialized if 
> there is no _BQC function and that seems to happen here.
> 
right, this should probably be done in patch 5.
where we have set the backlight in acpi_video_init_brightness already.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html