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@xxxxxxxxx>
>
> check the return value of acpi_video_device_lcd_get_level_current
>
> Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
