[PATCH] acpi: video: get rid of magic numbers and use enum instead

[PATCH] acpi: video: get rid of magic numbers and use enum instead

[Dmitry Frank]

The first two items in the _BCL method response are special:

  - Level when machine has full power
  - Level when machine is on batteries
  - .... actual supported levels go there ....

So this commits adds an enum and uses its descriptive elements
throughout the code, instead of magic numbers.

Some subtle cases are commented additionally (the comment for
acpi_video_bqc_quirk is by Felipe Contreras, CCed)

Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
---
 drivers/acpi/acpi_video.c | 134 ++++++++++++++++++++++++++++++----------------
 1 file changed, 89 insertions(+), 45 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index d00bc0ef87a6..856694e5f325 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -88,6 +88,18 @@ static int acpi_video_bus_remove(struct acpi_device *device);
 static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
 void acpi_video_detect_exit(void);
 
+/*
+ * Indices in the _BCL method response: the first two items are special,
+ * the rest are all supported levels.
+ *
+ * See page 575 of the ACPI spec 3.0
+ */
+enum lvl_idx {
+	LVL_IDX_AC,		/* level when machine has full power */
+	LVL_IDX_BATTERY,	/* level when machine is on batteries */
+	LVL_IDX_FIRST,		/* actual supported levels begin here */
+};
+
 static const struct acpi_device_id video_device_ids[] = {
 	{ACPI_VIDEO_HID, 0},
 	{"", 0},
@@ -132,7 +144,7 @@ struct acpi_video_device_attrib {
 				   the VGA device. */
 	u32 pipe_id:3;		/* For VGA multiple-head devices. */
 	u32 reserved:10;	/* Must be 0 */
-	u32 device_id_scheme:1;	/* Device ID Scheme */
+	u32 device_id_scheme:1;	/* Whether the device ID follows this scheme above */
 };
 
 struct acpi_video_enumerated_device {
@@ -217,20 +229,16 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
 
 	if (acpi_video_device_lcd_get_level_current(vd, &cur_level, false))
 		return -EINVAL;
-	for (i = 2; i < vd->brightness->count; i++) {
+	for (i = LVL_IDX_FIRST; i < vd->brightness->count; i++) {
 		if (vd->brightness->levels[i] == cur_level)
-			/*
-			 * The first two entries are special - see page 575
-			 * of the ACPI spec 3.0
-			 */
-			return i - 2;
+			return i - LVL_IDX_FIRST;
 	}
 	return 0;
 }
 
 static int acpi_video_set_brightness(struct backlight_device *bd)
 {
-	int request_level = bd->props.brightness + 2;
+	int request_level = bd->props.brightness + LVL_IDX_FIRST;
 	struct acpi_video_device *vd = bl_get_data(bd);
 
 	cancel_delayed_work(&vd->switch_brightness_work);
@@ -244,18 +252,18 @@ static const struct backlight_ops acpi_backlight_ops = {
 };
 
 /* thermal cooling device callbacks */
-static int video_get_max_state(struct thermal_cooling_device *cooling_dev, unsigned
-			       long *state)
+static int video_get_max_state(struct thermal_cooling_device *cooling_dev,
+			       unsigned long *state)
 {
 	struct acpi_device *device = cooling_dev->devdata;
 	struct acpi_video_device *video = acpi_driver_data(device);
 
-	*state = video->brightness->count - 3;
+	*state = video->brightness->count - LVL_IDX_FIRST - 1;
 	return 0;
 }
 
-static int video_get_cur_state(struct thermal_cooling_device *cooling_dev, unsigned
-			       long *state)
+static int video_get_cur_state(struct thermal_cooling_device *cooling_dev,
+			       unsigned long *state)
 {
 	struct acpi_device *device = cooling_dev->devdata;
 	struct acpi_video_device *video = acpi_driver_data(device);
@@ -264,7 +272,7 @@ static int video_get_cur_state(struct thermal_cooling_device *cooling_dev, unsig
 
 	if (acpi_video_device_lcd_get_level_current(video, &level, false))
 		return -EINVAL;
-	for (offset = 2; offset < video->brightness->count; offset++)
+	for (offset = LVL_IDX_FIRST; offset < video->brightness->count; offset++)
 		if (level == video->brightness->levels[offset]) {
 			*state = video->brightness->count - offset - 1;
 			return 0;
@@ -280,7 +288,7 @@ video_set_cur_state(struct thermal_cooling_device *cooling_dev, unsigned long st
 	struct acpi_video_device *video = acpi_driver_data(device);
 	int level;
 
-	if (state >= video->brightness->count - 2)
+	if (state >= video->brightness->count - LVL_IDX_FIRST)
 		return -EINVAL;
 
 	state = video->brightness->count - state;
@@ -345,10 +353,11 @@ acpi_video_device_lcd_set_level(struct acpi_video_device *device, int level)
 	}
 
 	device->brightness->curr = level;
-	for (state = 2; state < device->brightness->count; state++)
+	for (state = LVL_IDX_FIRST; state < device->brightness->count; state++)
 		if (level == device->brightness->levels[state]) {
 			if (device->backlight)
-				device->backlight->props.brightness = state - 2;
+				device->backlight->props.brightness =
+					state - LVL_IDX_FIRST;
 			return 0;
 		}
 
@@ -530,14 +539,15 @@ acpi_video_bqc_value_to_level(struct acpi_video_device *device,
 
 	if (device->brightness->flags._BQC_use_index) {
 		/*
-		 * _BQC returns an index that doesn't account for
-		 * the first 2 items with special meaning, so we need
+		 * _BQC returns an index that doesn't account for the first 2
+		 * items with special meaning (see enum lvl_idx), so we need
 		 * to compensate for that by offsetting ourselves
 		 */
 		if (device->brightness->flags._BCL_reversed)
-			bqc_value = device->brightness->count - 3 - bqc_value;
+			bqc_value = device->brightness->count -
+				LVL_IDX_FIRST - 1 - bqc_value;
 
-		level = device->brightness->levels[bqc_value + 2];
+		level = device->brightness->levels[bqc_value + LVL_IDX_FIRST];
 	} else {
 		level = bqc_value;
 	}
@@ -571,7 +581,7 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 
 			*level = acpi_video_bqc_value_to_level(device, *level);
 
-			for (i = 2; i < device->brightness->count; i++)
+			for (i = LVL_IDX_FIRST; i < device->brightness->count; i++)
 				if (device->brightness->levels[i] == *level) {
 					device->brightness->curr = *level;
 					return 0;
@@ -714,9 +724,37 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device,
 
 	/*
 	 * Some systems always report current brightness level as maximum
-	 * through _BQC, we need to test another value for them.
+	 * through _BQC, we need to test another value for them. However,
+	 * there is a subtlety:
+	 *
+	 * If the _BCL package ordering is descending, the first level
+	 * (br->levels[2]) is likely to be 0, and if the number of levels
+	 * matches the number of steps, we might confuse a returned level to
+	 * mean the index.
+	 *
+	 * For example:
+	 *
+	 *     current_level = max_level = 100
+	 *     test_level = 0
+	 *     returned level = 100
+	 *
+	 * In this case 100 means the level, not the index, and _BCM failed.
+	 * Still, if the _BCL package ordering is descending, the index of
+	 * level 0 is also 100, so we assume _BQC is indexed, when it's not.
+	 *
+	 * This causes all _BQC calls to return bogus values causing weird
+	 * behavior from the user's perspective.  For example:
+	 *
+	 * xbacklight -set 10; xbacklight -set 20;
+	 *
+	 * would flash to 90% and then slowly down to the desired level (20).
+	 *
+	 * The solution is simple; test anything other than the first level
+	 * (e.g. 1).
 	 */
-	test_level = current_level == max_level ? br->levels[3] : max_level;
+	test_level = current_level == max_level
+		? br->levels[LVL_IDX_FIRST + 1]
+		: max_level;
 
 	result = acpi_video_device_lcd_set_level(device, test_level);
 	if (result)
@@ -730,8 +768,8 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device,
 		/* buggy _BQC found, need to find out if it uses index */
 		if (level < br->count) {
 			if (br->flags._BCL_reversed)
-				level = br->count - 3 - level;
-			if (br->levels[level + 2] == test_level)
+				level = br->count - LVL_IDX_FIRST - 1 - level;
+			if (br->levels[level + LVL_IDX_FIRST] == test_level)
 				br->flags._BQC_use_index = 1;
 		}
 
@@ -761,7 +799,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 		goto out;
 	}
 
-	if (obj->package.count < 2) {
+	if (obj->package.count < LVL_IDX_FIRST) {
 		result = -EINVAL;
 		goto out;
 	}
@@ -773,8 +811,13 @@ int acpi_video_get_levels(struct acpi_device *device,
 		goto out;
 	}
 
-	br->levels = kmalloc((obj->package.count + 2) * sizeof *(br->levels),
-				GFP_KERNEL);
+	/*
+	 * Note that we have to reserve 2 extra items (LVL_IDX_FIRST), in order
+	 * to account for buggy BIOS which don't export the first two special
+	 * levels (see below)
+	 */
+	br->levels = kmalloc((obj->package.count + LVL_IDX_FIRST) * sizeof(*br->levels),
+			     GFP_KERNEL);
 	if (!br->levels) {
 		result = -ENOMEM;
 		goto out_free;
@@ -788,7 +831,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 		}
 		value = (u32) o->integer.value;
 		/* Skip duplicate entries */
-		if (count > 2 && br->levels[count - 1] == value)
+		if (count > LVL_IDX_FIRST && br->levels[count - 1] == value)
 			continue;
 
 		br->levels[count] = value;
@@ -804,27 +847,28 @@ int acpi_video_get_levels(struct acpi_device *device,
 	 * In this case, the first two elements in _BCL packages
 	 * are also supported brightness levels that OS should take care of.
 	 */
-	for (i = 2; i < count; i++) {
-		if (br->levels[i] == br->levels[0])
+	for (i = LVL_IDX_FIRST; i < count; i++) {
+		if (br->levels[i] == br->levels[LVL_IDX_AC])
 			level_ac_battery++;
-		if (br->levels[i] == br->levels[1])
+		if (br->levels[i] == br->levels[LVL_IDX_BATTERY])
 			level_ac_battery++;
 	}
 
-	if (level_ac_battery < 2) {
-		level_ac_battery = 2 - level_ac_battery;
+	if (level_ac_battery < LVL_IDX_FIRST) {
+		level_ac_battery = LVL_IDX_FIRST - level_ac_battery;
 		br->flags._BCL_no_ac_battery_levels = 1;
-		for (i = (count - 1 + level_ac_battery); i >= 2; i--)
+		for (i = (count - 1 + level_ac_battery); i >= LVL_IDX_FIRST; i--)
 			br->levels[i] = br->levels[i - level_ac_battery];
 		count += level_ac_battery;
-	} else if (level_ac_battery > 2)
+	} else if (level_ac_battery > LVL_IDX_FIRST)
 		ACPI_ERROR((AE_INFO, "Too many duplicates in _BCL package"));
 
 	/* Check if the _BCL package is in a reversed order */
-	if (max_level == br->levels[2]) {
+	if (max_level == br->levels[LVL_IDX_FIRST]) {
 		br->flags._BCL_reversed = 1;
-		sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
-			acpi_video_cmp_level, NULL);
+		sort(&br->levels[LVL_IDX_FIRST],
+		     count - LVL_IDX_FIRST, sizeof(br->levels[LVL_IDX_FIRST]),
+		     acpi_video_cmp_level, NULL);
 	} else if (max_level != br->levels[count - 1])
 		ACPI_ERROR((AE_INFO,
 			    "Found unordered _BCL package"));
@@ -894,7 +938,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 	 * level_old is invalid (no matter whether it's a level
 	 * or an index). Set the backlight to max_level in this case.
 	 */
-	for (i = 2; i < br->count; i++)
+	for (i = LVL_IDX_FIRST; i < br->count; i++)
 		if (level == br->levels[i])
 			break;
 	if (i == br->count || !level)
@@ -906,7 +950,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 		goto out_free_levels;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-			  "found %d brightness levels\n", br->count - 2));
+			  "found %d brightness levels\n", br->count - LVL_IDX_FIRST));
 	return 0;
 
 out_free_levels:
@@ -1297,7 +1341,7 @@ acpi_video_get_next_level(struct acpi_video_device *device,
 	max = max_below = 0;
 	min = min_above = 255;
 	/* Find closest level to level_current */
-	for (i = 2; i < device->brightness->count; i++) {
+	for (i = LVL_IDX_FIRST; i < device->brightness->count; i++) {
 		l = device->brightness->levels[i];
 		if (abs(l - level_current) < abs(delta)) {
 			delta = l - level_current;
@@ -1307,7 +1351,7 @@ acpi_video_get_next_level(struct acpi_video_device *device,
 	}
 	/* Ajust level_current to closest available level */
 	level_current += delta;
-	for (i = 2; i < device->brightness->count; i++) {
+	for (i = LVL_IDX_FIRST; i < device->brightness->count; i++) {
 		l = device->brightness->levels[i];
 		if (l < min)
 			min = l;
@@ -1680,7 +1724,7 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_FIRMWARE;
-	props.max_brightness = device->brightness->count - 3;
+	props.max_brightness = device->brightness->count - LVL_IDX_FIRST - 1;
 	device->backlight = backlight_device_register(name,
 						      parent,
 						      device,
-- 
2.11.0

Re: [PATCH] acpi: video: get rid of magic numbers and use enum instead

[Rafael J. Wysocki]

Hi,

On Tue, Apr 18, 2017 at 2:35 PM, Dmitry Frank <mail@dmitryfrank.com> wrote:
> The first two items in the _BCL method response are special:
>
>   - Level when machine has full power
>   - Level when machine is on batteries
>   - .... actual supported levels go there ....
>
> So this commits adds an enum and uses its descriptive elements
> throughout the code, instead of magic numbers.
>
> Some subtle cases are commented additionally (the comment for
> acpi_video_bqc_quirk is by Felipe Contreras, CCed)

So that should be a separate patch.  Please split.

>
> Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
> ---
>  drivers/acpi/acpi_video.c | 134 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 89 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index d00bc0ef87a6..856694e5f325 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -88,6 +88,18 @@ static int acpi_video_bus_remove(struct acpi_device *device);
>  static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
>  void acpi_video_detect_exit(void);
>
> +/*
> + * Indices in the _BCL method response: the first two items are special,
> + * the rest are all supported levels.
> + *
> + * See page 575 of the ACPI spec 3.0
> + */
> +enum lvl_idx {
> +       LVL_IDX_AC,             /* level when machine has full power */
> +       LVL_IDX_BATTERY,        /* level when machine is on batteries */
> +       LVL_IDX_FIRST,          /* actual supported levels begin here */

What about using different symbol names, like ACPI_VIDEO_AC_LEVEL,
ACPI_VIDEO_BATTERY_LEVEL and ACPI_VIDEO_FIRST_LEVEL, respectively?

> +};
> +
>  static const struct acpi_device_id video_device_ids[] = {
>         {ACPI_VIDEO_HID, 0},
>         {"", 0},
> @@ -132,7 +144,7 @@ struct acpi_video_device_attrib {
>                                    the VGA device. */
>         u32 pipe_id:3;          /* For VGA multiple-head devices. */
>         u32 reserved:10;        /* Must be 0 */
> -       u32 device_id_scheme:1; /* Device ID Scheme */
> +       u32 device_id_scheme:1; /* Whether the device ID follows this scheme above */

That still is not particularly clear (although admittedly it is better
than what we have).

Thanks,
Rafael

[PATCH v2 0/3] ACPI: video: get rid of magic numbers, do other refactoring

[Dmitry Frank]

Hi,

This patchset makes the code more self-documented and clean.

Initially I was chasing a bug with the backlight on my laptop. Even though it's
clear now what causes the issue, I'm not sure what's the proper way to fix it,
so for now I just wrote about my research with the hope that someone could
help: http://www.spinics.net/lists/linux-acpi/msg73638.html , comments are
appreciated!

So this patchset is a cleanup which I've done on my way.

Dmitry Frank (3):
  ACPI: video: get rid of magic numbers and use enum instead
  ACPI: video: rename the global flag device_id_scheme
  ACPI: video: add comments about subtle cases

 drivers/acpi/acpi_video.c | 169 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 117 insertions(+), 52 deletions(-)

-- 
2.11.0

[PATCH v2 1/3] ACPI: video: get rid of magic numbers and use enum instead

[Dmitry Frank]

The first two items in the _BCL method response are special:

  - Level when machine has full power
  - Level when machine is on batteries
  - .... actual supported levels go there ....

So this commits adds an enum and uses its descriptive elements
throughout the code, instead of magic numbers.

Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
---
 drivers/acpi/acpi_video.c | 110 +++++++++++++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 44 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index d00bc0ef87a6..9a607af971e7 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -88,6 +88,18 @@ static int acpi_video_bus_remove(struct acpi_device *device);
 static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
 void acpi_video_detect_exit(void);
 
+/*
+ * Indices in the _BCL method response: the first two items are special,
+ * the rest are all supported levels.
+ *
+ * See page 575 of the ACPI spec 3.0
+ */
+enum acpi_video_level_idx {
+	ACPI_VIDEO_AC_LEVEL,		/* level when machine has full power */
+	ACPI_VIDEO_BATTERY_LEVEL,	/* level when machine is on batteries */
+	ACPI_VIDEO_FIRST_LEVEL,		/* actual supported levels begin here */
+};
+
 static const struct acpi_device_id video_device_ids[] = {
 	{ACPI_VIDEO_HID, 0},
 	{"", 0},
@@ -217,20 +229,16 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
 
 	if (acpi_video_device_lcd_get_level_current(vd, &cur_level, false))
 		return -EINVAL;
-	for (i = 2; i < vd->brightness->count; i++) {
+	for (i = ACPI_VIDEO_FIRST_LEVEL; i < vd->brightness->count; i++) {
 		if (vd->brightness->levels[i] == cur_level)
-			/*
-			 * The first two entries are special - see page 575
-			 * of the ACPI spec 3.0
-			 */
-			return i - 2;
+			return i - ACPI_VIDEO_FIRST_LEVEL;
 	}
 	return 0;
 }
 
 static int acpi_video_set_brightness(struct backlight_device *bd)
 {
-	int request_level = bd->props.brightness + 2;
+	int request_level = bd->props.brightness + ACPI_VIDEO_FIRST_LEVEL;
 	struct acpi_video_device *vd = bl_get_data(bd);
 
 	cancel_delayed_work(&vd->switch_brightness_work);
@@ -244,18 +252,18 @@ static const struct backlight_ops acpi_backlight_ops = {
 };
 
 /* thermal cooling device callbacks */
-static int video_get_max_state(struct thermal_cooling_device *cooling_dev, unsigned
-			       long *state)
+static int video_get_max_state(struct thermal_cooling_device *cooling_dev,
+			       unsigned long *state)
 {
 	struct acpi_device *device = cooling_dev->devdata;
 	struct acpi_video_device *video = acpi_driver_data(device);
 
-	*state = video->brightness->count - 3;
+	*state = video->brightness->count - ACPI_VIDEO_FIRST_LEVEL - 1;
 	return 0;
 }
 
-static int video_get_cur_state(struct thermal_cooling_device *cooling_dev, unsigned
-			       long *state)
+static int video_get_cur_state(struct thermal_cooling_device *cooling_dev,
+			       unsigned long *state)
 {
 	struct acpi_device *device = cooling_dev->devdata;
 	struct acpi_video_device *video = acpi_driver_data(device);
@@ -264,7 +272,8 @@ static int video_get_cur_state(struct thermal_cooling_device *cooling_dev, unsig
 
 	if (acpi_video_device_lcd_get_level_current(video, &level, false))
 		return -EINVAL;
-	for (offset = 2; offset < video->brightness->count; offset++)
+	for (offset = ACPI_VIDEO_FIRST_LEVEL; offset < video->brightness->count;
+	     offset++)
 		if (level == video->brightness->levels[offset]) {
 			*state = video->brightness->count - offset - 1;
 			return 0;
@@ -280,7 +289,7 @@ video_set_cur_state(struct thermal_cooling_device *cooling_dev, unsigned long st
 	struct acpi_video_device *video = acpi_driver_data(device);
 	int level;
 
-	if (state >= video->brightness->count - 2)
+	if (state >= video->brightness->count - ACPI_VIDEO_FIRST_LEVEL)
 		return -EINVAL;
 
 	state = video->brightness->count - state;
@@ -345,10 +354,12 @@ acpi_video_device_lcd_set_level(struct acpi_video_device *device, int level)
 	}
 
 	device->brightness->curr = level;
-	for (state = 2; state < device->brightness->count; state++)
+	for (state = ACPI_VIDEO_FIRST_LEVEL; state < device->brightness->count;
+	     state++)
 		if (level == device->brightness->levels[state]) {
 			if (device->backlight)
-				device->backlight->props.brightness = state - 2;
+				device->backlight->props.brightness =
+					state - ACPI_VIDEO_FIRST_LEVEL;
 			return 0;
 		}
 
@@ -530,14 +541,16 @@ acpi_video_bqc_value_to_level(struct acpi_video_device *device,
 
 	if (device->brightness->flags._BQC_use_index) {
 		/*
-		 * _BQC returns an index that doesn't account for
-		 * the first 2 items with special meaning, so we need
-		 * to compensate for that by offsetting ourselves
+		 * _BQC returns an index that doesn't account for the first 2
+		 * items with special meaning (see enum acpi_video_level_idx),
+		 * so we need to compensate for that by offsetting ourselves
 		 */
 		if (device->brightness->flags._BCL_reversed)
-			bqc_value = device->brightness->count - 3 - bqc_value;
+			bqc_value = device->brightness->count -
+				ACPI_VIDEO_FIRST_LEVEL - 1 - bqc_value;
 
-		level = device->brightness->levels[bqc_value + 2];
+		level = device->brightness->levels[bqc_value +
+		                                   ACPI_VIDEO_FIRST_LEVEL];
 	} else {
 		level = bqc_value;
 	}
@@ -571,7 +584,8 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 
 			*level = acpi_video_bqc_value_to_level(device, *level);
 
-			for (i = 2; i < device->brightness->count; i++)
+			for (i = ACPI_VIDEO_FIRST_LEVEL;
+			     i < device->brightness->count; i++)
 				if (device->brightness->levels[i] == *level) {
 					device->brightness->curr = *level;
 					return 0;
@@ -716,7 +730,9 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device,
 	 * Some systems always report current brightness level as maximum
 	 * through _BQC, we need to test another value for them.
 	 */
-	test_level = current_level == max_level ? br->levels[3] : max_level;
+	test_level = current_level == max_level
+		? br->levels[ACPI_VIDEO_FIRST_LEVEL + 1]
+		: max_level;
 
 	result = acpi_video_device_lcd_set_level(device, test_level);
 	if (result)
@@ -730,8 +746,8 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device,
 		/* buggy _BQC found, need to find out if it uses index */
 		if (level < br->count) {
 			if (br->flags._BCL_reversed)
-				level = br->count - 3 - level;
-			if (br->levels[level + 2] == test_level)
+				level = br->count - ACPI_VIDEO_FIRST_LEVEL - 1 - level;
+			if (br->levels[level + ACPI_VIDEO_FIRST_LEVEL] == test_level)
 				br->flags._BQC_use_index = 1;
 		}
 
@@ -761,7 +777,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 		goto out;
 	}
 
-	if (obj->package.count < 2) {
+	if (obj->package.count < ACPI_VIDEO_FIRST_LEVEL) {
 		result = -EINVAL;
 		goto out;
 	}
@@ -773,8 +789,8 @@ int acpi_video_get_levels(struct acpi_device *device,
 		goto out;
 	}
 
-	br->levels = kmalloc((obj->package.count + 2) * sizeof *(br->levels),
-				GFP_KERNEL);
+	br->levels = kmalloc((obj->package.count + ACPI_VIDEO_FIRST_LEVEL) *
+	                     sizeof(*br->levels), GFP_KERNEL);
 	if (!br->levels) {
 		result = -ENOMEM;
 		goto out_free;
@@ -788,7 +804,8 @@ int acpi_video_get_levels(struct acpi_device *device,
 		}
 		value = (u32) o->integer.value;
 		/* Skip duplicate entries */
-		if (count > 2 && br->levels[count - 1] == value)
+		if (count > ACPI_VIDEO_FIRST_LEVEL
+		    && br->levels[count - 1] == value)
 			continue;
 
 		br->levels[count] = value;
@@ -804,27 +821,30 @@ int acpi_video_get_levels(struct acpi_device *device,
 	 * In this case, the first two elements in _BCL packages
 	 * are also supported brightness levels that OS should take care of.
 	 */
-	for (i = 2; i < count; i++) {
-		if (br->levels[i] == br->levels[0])
+	for (i = ACPI_VIDEO_FIRST_LEVEL; i < count; i++) {
+		if (br->levels[i] == br->levels[ACPI_VIDEO_AC_LEVEL])
 			level_ac_battery++;
-		if (br->levels[i] == br->levels[1])
+		if (br->levels[i] == br->levels[ACPI_VIDEO_BATTERY_LEVEL])
 			level_ac_battery++;
 	}
 
-	if (level_ac_battery < 2) {
-		level_ac_battery = 2 - level_ac_battery;
+	if (level_ac_battery < ACPI_VIDEO_FIRST_LEVEL) {
+		level_ac_battery = ACPI_VIDEO_FIRST_LEVEL - level_ac_battery;
 		br->flags._BCL_no_ac_battery_levels = 1;
-		for (i = (count - 1 + level_ac_battery); i >= 2; i--)
+		for (i = (count - 1 + level_ac_battery);
+		     i >= ACPI_VIDEO_FIRST_LEVEL; i--)
 			br->levels[i] = br->levels[i - level_ac_battery];
 		count += level_ac_battery;
-	} else if (level_ac_battery > 2)
+	} else if (level_ac_battery > ACPI_VIDEO_FIRST_LEVEL)
 		ACPI_ERROR((AE_INFO, "Too many duplicates in _BCL package"));
 
 	/* Check if the _BCL package is in a reversed order */
-	if (max_level == br->levels[2]) {
+	if (max_level == br->levels[ACPI_VIDEO_FIRST_LEVEL]) {
 		br->flags._BCL_reversed = 1;
-		sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
-			acpi_video_cmp_level, NULL);
+		sort(&br->levels[ACPI_VIDEO_FIRST_LEVEL],
+		     count - ACPI_VIDEO_FIRST_LEVEL,
+		     sizeof(br->levels[ACPI_VIDEO_FIRST_LEVEL]),
+		     acpi_video_cmp_level, NULL);
 	} else if (max_level != br->levels[count - 1])
 		ACPI_ERROR((AE_INFO,
 			    "Found unordered _BCL package"));
@@ -894,7 +914,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 	 * level_old is invalid (no matter whether it's a level
 	 * or an index). Set the backlight to max_level in this case.
 	 */
-	for (i = 2; i < br->count; i++)
+	for (i = ACPI_VIDEO_FIRST_LEVEL; i < br->count; i++)
 		if (level == br->levels[i])
 			break;
 	if (i == br->count || !level)
@@ -906,7 +926,8 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 		goto out_free_levels;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-			  "found %d brightness levels\n", br->count - 2));
+	                  "found %d brightness levels\n",
+	                  br->count - ACPI_VIDEO_FIRST_LEVEL));
 	return 0;
 
 out_free_levels:
@@ -1297,7 +1318,7 @@ acpi_video_get_next_level(struct acpi_video_device *device,
 	max = max_below = 0;
 	min = min_above = 255;
 	/* Find closest level to level_current */
-	for (i = 2; i < device->brightness->count; i++) {
+	for (i = ACPI_VIDEO_FIRST_LEVEL; i < device->brightness->count; i++) {
 		l = device->brightness->levels[i];
 		if (abs(l - level_current) < abs(delta)) {
 			delta = l - level_current;
@@ -1307,7 +1328,7 @@ acpi_video_get_next_level(struct acpi_video_device *device,
 	}
 	/* Ajust level_current to closest available level */
 	level_current += delta;
-	for (i = 2; i < device->brightness->count; i++) {
+	for (i = ACPI_VIDEO_FIRST_LEVEL; i < device->brightness->count; i++) {
 		l = device->brightness->levels[i];
 		if (l < min)
 			min = l;
@@ -1680,7 +1701,8 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_FIRMWARE;
-	props.max_brightness = device->brightness->count - 3;
+	props.max_brightness =
+		device->brightness->count - ACPI_VIDEO_FIRST_LEVEL - 1;
 	device->backlight = backlight_device_register(name,
 						      parent,
 						      device,
-- 
2.11.0

[PATCH v2 2/3] ACPI: video: rename the global flag device_id_scheme

[Dmitry Frank]

This flag forces device_id_scheme bit in the struct
acpi_video_device_attrib to be assumed even if not actually set, so a
better name for the global flag would be "force_device_id_scheme".

Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
---
 drivers/acpi/acpi_video.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 9a607af971e7..cfd3bed57bed 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -73,8 +73,12 @@ module_param(report_key_events, int, 0644);
 MODULE_PARM_DESC(report_key_events,
 	"0: none, 1: output changes, 2: brightness changes, 3: all");
 
-static bool device_id_scheme = false;
-module_param(device_id_scheme, bool, 0444);
+/*
+ * Whether the struct acpi_video_device_attrib::device_id_scheme bit should be
+ * assumed even if not actually set.
+ */
+static bool force_device_id_scheme = false;
+module_param(force_device_id_scheme, bool, 0444);
 
 static bool only_lcd = false;
 module_param(only_lcd, bool, 0444);
@@ -387,9 +391,9 @@ static int video_disable_backlight_sysfs_if(
 	return 0;
 }
 
-static int video_set_device_id_scheme(const struct dmi_system_id *d)
+static int video_set_force_device_id_scheme(const struct dmi_system_id *d)
 {
-	device_id_scheme = true;
+	force_device_id_scheme = true;
 	return 0;
 }
 
@@ -491,7 +495,7 @@ static struct dmi_system_id video_dmi_table[] = {
 	 */
 	{
 	 /* https://bugzilla.kernel.org/show_bug.cgi?id=104121 */
-	 .callback = video_set_device_id_scheme,
+	 .callback = video_set_force_device_id_scheme,
 	 .ident = "ESPRIMO Mobile M9410",
 	 .matches = {
 		DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
@@ -1111,7 +1115,7 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 
 	attribute = acpi_video_get_device_attr(video, device_id);
 
-	if (attribute && (attribute->device_id_scheme || device_id_scheme)) {
+	if (attribute && (attribute->device_id_scheme || force_device_id_scheme)) {
 		switch (attribute->display_type) {
 		case ACPI_VIDEO_DISPLAY_CRT:
 			data->flags.crt = 1;
-- 
2.11.0

[PATCH v2 3/3] ACPI: video: add comments about subtle cases

[Dmitry Frank]

The comment for acpi_video_bqc_quirk is by Felipe Contreras, taken from
the git history.

Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
---
 drivers/acpi/acpi_video.c | 43 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index cfd3bed57bed..7fd16efa84c8 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -148,7 +148,15 @@ struct acpi_video_device_attrib {
 				   the VGA device. */
 	u32 pipe_id:3;		/* For VGA multiple-head devices. */
 	u32 reserved:10;	/* Must be 0 */
-	u32 device_id_scheme:1;	/* Device ID Scheme */
+
+	/*
+	 * The device ID might not actually follow the scheme described by this
+	 * struct acpi_video_device_attrib. If it does, then this bit
+	 * device_id_scheme is set; otherwise, other fields should be ignored.
+	 *
+	 * (but also see the global flag force_device_id_scheme)
+	 */
+	u32 device_id_scheme:1;
 };
 
 struct acpi_video_enumerated_device {
@@ -732,7 +740,33 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device,
 
 	/*
 	 * Some systems always report current brightness level as maximum
-	 * through _BQC, we need to test another value for them.
+	 * through _BQC, we need to test another value for them. However,
+	 * there is a subtlety:
+	 *
+	 * If the _BCL package ordering is descending, the first level
+	 * (br->levels[2]) is likely to be 0, and if the number of levels
+	 * matches the number of steps, we might confuse a returned level to
+	 * mean the index.
+	 *
+	 * For example:
+	 *
+	 *     current_level = max_level = 100
+	 *     test_level = 0
+	 *     returned level = 100
+	 *
+	 * In this case 100 means the level, not the index, and _BCM failed.
+	 * Still, if the _BCL package ordering is descending, the index of
+	 * level 0 is also 100, so we assume _BQC is indexed, when it's not.
+	 *
+	 * This causes all _BQC calls to return bogus values causing weird
+	 * behavior from the user's perspective.  For example:
+	 *
+	 * xbacklight -set 10; xbacklight -set 20;
+	 *
+	 * would flash to 90% and then slowly down to the desired level (20).
+	 *
+	 * The solution is simple; test anything other than the first level
+	 * (e.g. 1).
 	 */
 	test_level = current_level == max_level
 		? br->levels[ACPI_VIDEO_FIRST_LEVEL + 1]
@@ -793,6 +827,11 @@ int acpi_video_get_levels(struct acpi_device *device,
 		goto out;
 	}
 
+	/*
+	 * Note that we have to reserve 2 extra items (ACPI_VIDEO_FIRST_LEVEL),
+	 * in order to account for buggy BIOS which don't export the first two
+	 * special levels (see below)
+	 */
 	br->levels = kmalloc((obj->package.count + ACPI_VIDEO_FIRST_LEVEL) *
 	                     sizeof(*br->levels), GFP_KERNEL);
 	if (!br->levels) {
-- 
2.11.0

Re: [PATCH] acpi: video: get rid of magic numbers and use enum instead

[Dmitry Frank]

On 04/19, Rafael J. Wysocki wrote:
> Hi,
> 
> On Tue, Apr 18, 2017 at 2:35 PM, Dmitry Frank <mail@dmitryfrank.com> wrote:
> > The first two items in the _BCL method response are special:
> >
> >   - Level when machine has full power
> >   - Level when machine is on batteries
> >   - .... actual supported levels go there ....
> >
> > So this commits adds an enum and uses its descriptive elements
> > throughout the code, instead of magic numbers.
> >
> > Some subtle cases are commented additionally (the comment for
> > acpi_video_bqc_quirk is by Felipe Contreras, CCed)
> 
> So that should be a separate patch.  Please split.
> 

Sure, done; please see the "v2" patchset. (It contains one more change:
renaming of the global flag device_id_scheme, since the old name is
misleading)

> >
> > Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
> > ---
> >  drivers/acpi/acpi_video.c | 134 ++++++++++++++++++++++++++++++----------------
> >  1 file changed, 89 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> > index d00bc0ef87a6..856694e5f325 100644
> > --- a/drivers/acpi/acpi_video.c
> > +++ b/drivers/acpi/acpi_video.c
> > @@ -88,6 +88,18 @@ static int acpi_video_bus_remove(struct acpi_device *device);
> >  static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
> >  void acpi_video_detect_exit(void);
> >
> > +/*
> > + * Indices in the _BCL method response: the first two items are special,
> > + * the rest are all supported levels.
> > + *
> > + * See page 575 of the ACPI spec 3.0
> > + */
> > +enum lvl_idx {
> > +       LVL_IDX_AC,             /* level when machine has full power */
> > +       LVL_IDX_BATTERY,        /* level when machine is on batteries */
> > +       LVL_IDX_FIRST,          /* actual supported levels begin here */
> 
> What about using different symbol names, like ACPI_VIDEO_AC_LEVEL,
> ACPI_VIDEO_BATTERY_LEVEL and ACPI_VIDEO_FIRST_LEVEL, respectively?
> 

Honestly, I find ACPI_VIDEO_..._LEVEL a bit misleading, because these
values are not levels, they are indices of levels. So I'd probably
prefer ..._LEVEL_INDEX, or ..._LEVEL_IDX, but it probably gets too long,
so...

Anyway, the patchset v2 contains the names you proposed; if you agree
that LEVEL_INDEX or LEVEL_IDX would be better, please let me know, and
I'll be happy to craft v3. Otherwise, I'm fine with these names too.

> > +};
> > +
> >  static const struct acpi_device_id video_device_ids[] = {
> >         {ACPI_VIDEO_HID, 0},
> >         {"", 0},
> > @@ -132,7 +144,7 @@ struct acpi_video_device_attrib {
> >                                    the VGA device. */
> >         u32 pipe_id:3;          /* For VGA multiple-head devices. */
> >         u32 reserved:10;        /* Must be 0 */
> > -       u32 device_id_scheme:1; /* Device ID Scheme */
> > +       u32 device_id_scheme:1; /* Whether the device ID follows this scheme above */
> 
> That still is not particularly clear (although admittedly it is better
> than what we have).
> 

Ok, I added a descriptive comment above the field.

> Thanks,
> Rafael

Re: [PATCH] acpi: video: get rid of magic numbers and use enum instead

[Dmitry Frank]

Hi Rafael,

Thanks for the review.

On 04/19, Rafael J. Wysocki wrote:
> Hi,
> 
> On Tue, Apr 18, 2017 at 2:35 PM, Dmitry Frank <mail@dmitryfrank.com> wrote:
> > The first two items in the _BCL method response are special:
> >
> >   - Level when machine has full power
> >   - Level when machine is on batteries
> >   - .... actual supported levels go there ....
> >
> > So this commits adds an enum and uses its descriptive elements
> > throughout the code, instead of magic numbers.
> >
> > Some subtle cases are commented additionally (the comment for
> > acpi_video_bqc_quirk is by Felipe Contreras, CCed)
> 
> So that should be a separate patch.  Please split.
> 

Sure, done; please see the "v2" patchset. (It contains one more change:
renaming of the global flag device_id_scheme, since the old name is
misleading)

> >
> > Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
> > ---
> >  drivers/acpi/acpi_video.c | 134 ++++++++++++++++++++++++++++++----------------
> >  1 file changed, 89 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> > index d00bc0ef87a6..856694e5f325 100644
> > --- a/drivers/acpi/acpi_video.c
> > +++ b/drivers/acpi/acpi_video.c
> > @@ -88,6 +88,18 @@ static int acpi_video_bus_remove(struct acpi_device *device);
> >  static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
> >  void acpi_video_detect_exit(void);
> >
> > +/*
> > + * Indices in the _BCL method response: the first two items are special,
> > + * the rest are all supported levels.
> > + *
> > + * See page 575 of the ACPI spec 3.0
> > + */
> > +enum lvl_idx {
> > +       LVL_IDX_AC,             /* level when machine has full power */
> > +       LVL_IDX_BATTERY,        /* level when machine is on batteries */
> > +       LVL_IDX_FIRST,          /* actual supported levels begin here */
> 
> What about using different symbol names, like ACPI_VIDEO_AC_LEVEL,
> ACPI_VIDEO_BATTERY_LEVEL and ACPI_VIDEO_FIRST_LEVEL, respectively?
> 

Honestly, I find ACPI_VIDEO_..._LEVEL a bit misleading, because these
values are not levels, they are indices of levels. So I'd probably
prefer ..._LEVEL_INDEX, or ..._LEVEL_IDX, but it probably gets too long,
so...

Anyway, the patchset v2 contains the names you proposed; if you agree
that LEVEL_INDEX or LEVEL_IDX would be better, please let me know, and
I'll be happy to craft v3. Otherwise, I'm fine with these names too.

> > +};
> > +
> >  static const struct acpi_device_id video_device_ids[] = {
> >         {ACPI_VIDEO_HID, 0},
> >         {"", 0},
> > @@ -132,7 +144,7 @@ struct acpi_video_device_attrib {
> >                                    the VGA device. */
> >         u32 pipe_id:3;          /* For VGA multiple-head devices. */
> >         u32 reserved:10;        /* Must be 0 */
> > -       u32 device_id_scheme:1; /* Device ID Scheme */
> > +       u32 device_id_scheme:1; /* Whether the device ID follows this scheme above */
> 
> That still is not particularly clear (although admittedly it is better
> than what we have).
> 

Ok, I added a descriptive comment above the field.

Thanks,
Dmitry

Re: [PATCH v2 2/3] ACPI: video: rename the global flag device_id_scheme

[Rafael J. Wysocki]

On Wed, Apr 19, 2017 at 11:48 AM, Dmitry Frank <mail@dmitryfrank.com> wrote:
> This flag forces device_id_scheme bit in the struct
> acpi_video_device_attrib to be assumed even if not actually set, so a
> better name for the global flag would be "force_device_id_scheme".
>
> Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
> ---
>  drivers/acpi/acpi_video.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 9a607af971e7..cfd3bed57bed 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -73,8 +73,12 @@ module_param(report_key_events, int, 0644);
>  MODULE_PARM_DESC(report_key_events,
>         "0: none, 1: output changes, 2: brightness changes, 3: all");
>
> -static bool device_id_scheme = false;
> -module_param(device_id_scheme, bool, 0444);
> +/*
> + * Whether the struct acpi_video_device_attrib::device_id_scheme bit should be
> + * assumed even if not actually set.
> + */
> +static bool force_device_id_scheme = false;
> +module_param(force_device_id_scheme, bool, 0444);

The concern here is that if somebody uses that module parameter with
current (or previous) kernels, your change will break their setups.

It is better to simply add a comment clarifying the meaning of this flag IMO.

Thanks,
Rafael

[PATCH v3 0/2] ACPI: video: get rid of magic numbers, do other refactoring

[Dmitry Frank]

Hi,

This patchset makes the code more self-documented and clean.

Initially I was chasing a bug with the backlight on my laptop. Even though it's
clear now what causes the issue, I'm not sure what's the proper way to fix it,
so for now I just wrote about my research with the hope that someone could
help: http://www.spinics.net/lists/linux-acpi/msg73638.html , comments are
appreciated!

So this patchset is a cleanup which I've done on my way.

Dmitry Frank (2):
  ACPI: video: get rid of magic numbers and use enum instead
  ACPI: video: add comments about subtle cases

 drivers/acpi/acpi_video.c | 157 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 111 insertions(+), 46 deletions(-)

-- 
2.11.0

[PATCH v3 1/2] ACPI: video: get rid of magic numbers and use enum instead

[Dmitry Frank]

The first two items in the _BCL method response are special:

  - Level when machine has full power
  - Level when machine is on batteries
  - .... actual supported levels go there ....

So this commits adds an enum and uses its descriptive elements
throughout the code, instead of magic numbers.

Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
---
 drivers/acpi/acpi_video.c | 110 +++++++++++++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 44 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index d00bc0ef87a6..9a607af971e7 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -88,6 +88,18 @@ static int acpi_video_bus_remove(struct acpi_device *device);
 static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
 void acpi_video_detect_exit(void);
 
+/*
+ * Indices in the _BCL method response: the first two items are special,
+ * the rest are all supported levels.
+ *
+ * See page 575 of the ACPI spec 3.0
+ */
+enum acpi_video_level_idx {
+	ACPI_VIDEO_AC_LEVEL,		/* level when machine has full power */
+	ACPI_VIDEO_BATTERY_LEVEL,	/* level when machine is on batteries */
+	ACPI_VIDEO_FIRST_LEVEL,		/* actual supported levels begin here */
+};
+
 static const struct acpi_device_id video_device_ids[] = {
 	{ACPI_VIDEO_HID, 0},
 	{"", 0},
@@ -217,20 +229,16 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
 
 	if (acpi_video_device_lcd_get_level_current(vd, &cur_level, false))
 		return -EINVAL;
-	for (i = 2; i < vd->brightness->count; i++) {
+	for (i = ACPI_VIDEO_FIRST_LEVEL; i < vd->brightness->count; i++) {
 		if (vd->brightness->levels[i] == cur_level)
-			/*
-			 * The first two entries are special - see page 575
-			 * of the ACPI spec 3.0
-			 */
-			return i - 2;
+			return i - ACPI_VIDEO_FIRST_LEVEL;
 	}
 	return 0;
 }
 
 static int acpi_video_set_brightness(struct backlight_device *bd)
 {
-	int request_level = bd->props.brightness + 2;
+	int request_level = bd->props.brightness + ACPI_VIDEO_FIRST_LEVEL;
 	struct acpi_video_device *vd = bl_get_data(bd);
 
 	cancel_delayed_work(&vd->switch_brightness_work);
@@ -244,18 +252,18 @@ static const struct backlight_ops acpi_backlight_ops = {
 };
 
 /* thermal cooling device callbacks */
-static int video_get_max_state(struct thermal_cooling_device *cooling_dev, unsigned
-			       long *state)
+static int video_get_max_state(struct thermal_cooling_device *cooling_dev,
+			       unsigned long *state)
 {
 	struct acpi_device *device = cooling_dev->devdata;
 	struct acpi_video_device *video = acpi_driver_data(device);
 
-	*state = video->brightness->count - 3;
+	*state = video->brightness->count - ACPI_VIDEO_FIRST_LEVEL - 1;
 	return 0;
 }
 
-static int video_get_cur_state(struct thermal_cooling_device *cooling_dev, unsigned
-			       long *state)
+static int video_get_cur_state(struct thermal_cooling_device *cooling_dev,
+			       unsigned long *state)
 {
 	struct acpi_device *device = cooling_dev->devdata;
 	struct acpi_video_device *video = acpi_driver_data(device);
@@ -264,7 +272,8 @@ static int video_get_cur_state(struct thermal_cooling_device *cooling_dev, unsig
 
 	if (acpi_video_device_lcd_get_level_current(video, &level, false))
 		return -EINVAL;
-	for (offset = 2; offset < video->brightness->count; offset++)
+	for (offset = ACPI_VIDEO_FIRST_LEVEL; offset < video->brightness->count;
+	     offset++)
 		if (level == video->brightness->levels[offset]) {
 			*state = video->brightness->count - offset - 1;
 			return 0;
@@ -280,7 +289,7 @@ video_set_cur_state(struct thermal_cooling_device *cooling_dev, unsigned long st
 	struct acpi_video_device *video = acpi_driver_data(device);
 	int level;
 
-	if (state >= video->brightness->count - 2)
+	if (state >= video->brightness->count - ACPI_VIDEO_FIRST_LEVEL)
 		return -EINVAL;
 
 	state = video->brightness->count - state;
@@ -345,10 +354,12 @@ acpi_video_device_lcd_set_level(struct acpi_video_device *device, int level)
 	}
 
 	device->brightness->curr = level;
-	for (state = 2; state < device->brightness->count; state++)
+	for (state = ACPI_VIDEO_FIRST_LEVEL; state < device->brightness->count;
+	     state++)
 		if (level == device->brightness->levels[state]) {
 			if (device->backlight)
-				device->backlight->props.brightness = state - 2;
+				device->backlight->props.brightness =
+					state - ACPI_VIDEO_FIRST_LEVEL;
 			return 0;
 		}
 
@@ -530,14 +541,16 @@ acpi_video_bqc_value_to_level(struct acpi_video_device *device,
 
 	if (device->brightness->flags._BQC_use_index) {
 		/*
-		 * _BQC returns an index that doesn't account for
-		 * the first 2 items with special meaning, so we need
-		 * to compensate for that by offsetting ourselves
+		 * _BQC returns an index that doesn't account for the first 2
+		 * items with special meaning (see enum acpi_video_level_idx),
+		 * so we need to compensate for that by offsetting ourselves
 		 */
 		if (device->brightness->flags._BCL_reversed)
-			bqc_value = device->brightness->count - 3 - bqc_value;
+			bqc_value = device->brightness->count -
+				ACPI_VIDEO_FIRST_LEVEL - 1 - bqc_value;
 
-		level = device->brightness->levels[bqc_value + 2];
+		level = device->brightness->levels[bqc_value +
+		                                   ACPI_VIDEO_FIRST_LEVEL];
 	} else {
 		level = bqc_value;
 	}
@@ -571,7 +584,8 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 
 			*level = acpi_video_bqc_value_to_level(device, *level);
 
-			for (i = 2; i < device->brightness->count; i++)
+			for (i = ACPI_VIDEO_FIRST_LEVEL;
+			     i < device->brightness->count; i++)
 				if (device->brightness->levels[i] == *level) {
 					device->brightness->curr = *level;
 					return 0;
@@ -716,7 +730,9 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device,
 	 * Some systems always report current brightness level as maximum
 	 * through _BQC, we need to test another value for them.
 	 */
-	test_level = current_level == max_level ? br->levels[3] : max_level;
+	test_level = current_level == max_level
+		? br->levels[ACPI_VIDEO_FIRST_LEVEL + 1]
+		: max_level;
 
 	result = acpi_video_device_lcd_set_level(device, test_level);
 	if (result)
@@ -730,8 +746,8 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device,
 		/* buggy _BQC found, need to find out if it uses index */
 		if (level < br->count) {
 			if (br->flags._BCL_reversed)
-				level = br->count - 3 - level;
-			if (br->levels[level + 2] == test_level)
+				level = br->count - ACPI_VIDEO_FIRST_LEVEL - 1 - level;
+			if (br->levels[level + ACPI_VIDEO_FIRST_LEVEL] == test_level)
 				br->flags._BQC_use_index = 1;
 		}
 
@@ -761,7 +777,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 		goto out;
 	}
 
-	if (obj->package.count < 2) {
+	if (obj->package.count < ACPI_VIDEO_FIRST_LEVEL) {
 		result = -EINVAL;
 		goto out;
 	}
@@ -773,8 +789,8 @@ int acpi_video_get_levels(struct acpi_device *device,
 		goto out;
 	}
 
-	br->levels = kmalloc((obj->package.count + 2) * sizeof *(br->levels),
-				GFP_KERNEL);
+	br->levels = kmalloc((obj->package.count + ACPI_VIDEO_FIRST_LEVEL) *
+	                     sizeof(*br->levels), GFP_KERNEL);
 	if (!br->levels) {
 		result = -ENOMEM;
 		goto out_free;
@@ -788,7 +804,8 @@ int acpi_video_get_levels(struct acpi_device *device,
 		}
 		value = (u32) o->integer.value;
 		/* Skip duplicate entries */
-		if (count > 2 && br->levels[count - 1] == value)
+		if (count > ACPI_VIDEO_FIRST_LEVEL
+		    && br->levels[count - 1] == value)
 			continue;
 
 		br->levels[count] = value;
@@ -804,27 +821,30 @@ int acpi_video_get_levels(struct acpi_device *device,
 	 * In this case, the first two elements in _BCL packages
 	 * are also supported brightness levels that OS should take care of.
 	 */
-	for (i = 2; i < count; i++) {
-		if (br->levels[i] == br->levels[0])
+	for (i = ACPI_VIDEO_FIRST_LEVEL; i < count; i++) {
+		if (br->levels[i] == br->levels[ACPI_VIDEO_AC_LEVEL])
 			level_ac_battery++;
-		if (br->levels[i] == br->levels[1])
+		if (br->levels[i] == br->levels[ACPI_VIDEO_BATTERY_LEVEL])
 			level_ac_battery++;
 	}
 
-	if (level_ac_battery < 2) {
-		level_ac_battery = 2 - level_ac_battery;
+	if (level_ac_battery < ACPI_VIDEO_FIRST_LEVEL) {
+		level_ac_battery = ACPI_VIDEO_FIRST_LEVEL - level_ac_battery;
 		br->flags._BCL_no_ac_battery_levels = 1;
-		for (i = (count - 1 + level_ac_battery); i >= 2; i--)
+		for (i = (count - 1 + level_ac_battery);
+		     i >= ACPI_VIDEO_FIRST_LEVEL; i--)
 			br->levels[i] = br->levels[i - level_ac_battery];
 		count += level_ac_battery;
-	} else if (level_ac_battery > 2)
+	} else if (level_ac_battery > ACPI_VIDEO_FIRST_LEVEL)
 		ACPI_ERROR((AE_INFO, "Too many duplicates in _BCL package"));
 
 	/* Check if the _BCL package is in a reversed order */
-	if (max_level == br->levels[2]) {
+	if (max_level == br->levels[ACPI_VIDEO_FIRST_LEVEL]) {
 		br->flags._BCL_reversed = 1;
-		sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
-			acpi_video_cmp_level, NULL);
+		sort(&br->levels[ACPI_VIDEO_FIRST_LEVEL],
+		     count - ACPI_VIDEO_FIRST_LEVEL,
+		     sizeof(br->levels[ACPI_VIDEO_FIRST_LEVEL]),
+		     acpi_video_cmp_level, NULL);
 	} else if (max_level != br->levels[count - 1])
 		ACPI_ERROR((AE_INFO,
 			    "Found unordered _BCL package"));
@@ -894,7 +914,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 	 * level_old is invalid (no matter whether it's a level
 	 * or an index). Set the backlight to max_level in this case.
 	 */
-	for (i = 2; i < br->count; i++)
+	for (i = ACPI_VIDEO_FIRST_LEVEL; i < br->count; i++)
 		if (level == br->levels[i])
 			break;
 	if (i == br->count || !level)
@@ -906,7 +926,8 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 		goto out_free_levels;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
-			  "found %d brightness levels\n", br->count - 2));
+	                  "found %d brightness levels\n",
+	                  br->count - ACPI_VIDEO_FIRST_LEVEL));
 	return 0;
 
 out_free_levels:
@@ -1297,7 +1318,7 @@ acpi_video_get_next_level(struct acpi_video_device *device,
 	max = max_below = 0;
 	min = min_above = 255;
 	/* Find closest level to level_current */
-	for (i = 2; i < device->brightness->count; i++) {
+	for (i = ACPI_VIDEO_FIRST_LEVEL; i < device->brightness->count; i++) {
 		l = device->brightness->levels[i];
 		if (abs(l - level_current) < abs(delta)) {
 			delta = l - level_current;
@@ -1307,7 +1328,7 @@ acpi_video_get_next_level(struct acpi_video_device *device,
 	}
 	/* Ajust level_current to closest available level */
 	level_current += delta;
-	for (i = 2; i < device->brightness->count; i++) {
+	for (i = ACPI_VIDEO_FIRST_LEVEL; i < device->brightness->count; i++) {
 		l = device->brightness->levels[i];
 		if (l < min)
 			min = l;
@@ -1680,7 +1701,8 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_FIRMWARE;
-	props.max_brightness = device->brightness->count - 3;
+	props.max_brightness =
+		device->brightness->count - ACPI_VIDEO_FIRST_LEVEL - 1;
 	device->backlight = backlight_device_register(name,
 						      parent,
 						      device,
-- 
2.11.0

[PATCH v3 2/2] ACPI: video: add comments about subtle cases

[Dmitry Frank]

The comment for acpi_video_bqc_quirk is by Felipe Contreras, taken from
the git history.

Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
---
 drivers/acpi/acpi_video.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 9a607af971e7..e88fe3632dd6 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -73,6 +73,10 @@ module_param(report_key_events, int, 0644);
 MODULE_PARM_DESC(report_key_events,
 	"0: none, 1: output changes, 2: brightness changes, 3: all");
 
+/*
+ * Whether the struct acpi_video_device_attrib::device_id_scheme bit should be
+ * assumed even if not actually set.
+ */
 static bool device_id_scheme = false;
 module_param(device_id_scheme, bool, 0444);
 
@@ -144,7 +148,15 @@ struct acpi_video_device_attrib {
 				   the VGA device. */
 	u32 pipe_id:3;		/* For VGA multiple-head devices. */
 	u32 reserved:10;	/* Must be 0 */
-	u32 device_id_scheme:1;	/* Device ID Scheme */
+
+	/*
+	 * The device ID might not actually follow the scheme described by this
+	 * struct acpi_video_device_attrib. If it does, then this bit
+	 * device_id_scheme is set; otherwise, other fields should be ignored.
+	 *
+	 * (but also see the global flag device_id_scheme)
+	 */
+	u32 device_id_scheme:1;
 };
 
 struct acpi_video_enumerated_device {
@@ -728,7 +740,33 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device,
 
 	/*
 	 * Some systems always report current brightness level as maximum
-	 * through _BQC, we need to test another value for them.
+	 * through _BQC, we need to test another value for them. However,
+	 * there is a subtlety:
+	 *
+	 * If the _BCL package ordering is descending, the first level
+	 * (br->levels[2]) is likely to be 0, and if the number of levels
+	 * matches the number of steps, we might confuse a returned level to
+	 * mean the index.
+	 *
+	 * For example:
+	 *
+	 *     current_level = max_level = 100
+	 *     test_level = 0
+	 *     returned level = 100
+	 *
+	 * In this case 100 means the level, not the index, and _BCM failed.
+	 * Still, if the _BCL package ordering is descending, the index of
+	 * level 0 is also 100, so we assume _BQC is indexed, when it's not.
+	 *
+	 * This causes all _BQC calls to return bogus values causing weird
+	 * behavior from the user's perspective.  For example:
+	 *
+	 * xbacklight -set 10; xbacklight -set 20;
+	 *
+	 * would flash to 90% and then slowly down to the desired level (20).
+	 *
+	 * The solution is simple; test anything other than the first level
+	 * (e.g. 1).
 	 */
 	test_level = current_level == max_level
 		? br->levels[ACPI_VIDEO_FIRST_LEVEL + 1]
@@ -789,6 +827,11 @@ int acpi_video_get_levels(struct acpi_device *device,
 		goto out;
 	}
 
+	/*
+	 * Note that we have to reserve 2 extra items (ACPI_VIDEO_FIRST_LEVEL),
+	 * in order to account for buggy BIOS which don't export the first two
+	 * special levels (see below)
+	 */
 	br->levels = kmalloc((obj->package.count + ACPI_VIDEO_FIRST_LEVEL) *
 	                     sizeof(*br->levels), GFP_KERNEL);
 	if (!br->levels) {
-- 
2.11.0

Re: [PATCH v2 2/3] ACPI: video: rename the global flag device_id_scheme

[Dmitry Frank]

Hi Rafael,

On 04/19, Rafael J. Wysocki wrote:
> On Wed, Apr 19, 2017 at 11:48 AM, Dmitry Frank <mail@dmitryfrank.com> wrote:
> > This flag forces device_id_scheme bit in the struct
> > acpi_video_device_attrib to be assumed even if not actually set, so a
> > better name for the global flag would be "force_device_id_scheme".
> >
> > Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
> > ---
> >  drivers/acpi/acpi_video.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> > index 9a607af971e7..cfd3bed57bed 100644
> > --- a/drivers/acpi/acpi_video.c
> > +++ b/drivers/acpi/acpi_video.c
> > @@ -73,8 +73,12 @@ module_param(report_key_events, int, 0644);
> >  MODULE_PARM_DESC(report_key_events,
> >         "0: none, 1: output changes, 2: brightness changes, 3: all");
> >
> > -static bool device_id_scheme = false;
> > -module_param(device_id_scheme, bool, 0444);
> > +/*
> > + * Whether the struct acpi_video_device_attrib::device_id_scheme bit should be
> > + * assumed even if not actually set.
> > + */
> > +static bool force_device_id_scheme = false;
> > +module_param(force_device_id_scheme, bool, 0444);
> 
> The concern here is that if somebody uses that module parameter with
> current (or previous) kernels, your change will break their setups.
> 
> It is better to simply add a comment clarifying the meaning of this flag IMO.
> 

Ah, true, sorry for overlooking that. Fixed in v3.

Thanks,
Dmitry

Re: [PATCH v3 1/2] ACPI: video: get rid of magic numbers and use enum instead

[Rafael J. Wysocki]

On Wed, Apr 19, 2017 at 12:36 PM, Dmitry Frank <mail@dmitryfrank.com> wrote:
> The first two items in the _BCL method response are special:
>
>   - Level when machine has full power
>   - Level when machine is on batteries
>   - .... actual supported levels go there ....
>
> So this commits adds an enum and uses its descriptive elements
> throughout the code, instead of magic numbers.
>
> Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>

Are there any differences between this and the previous version of the
patch you posted?

Thanks,
Rafael

Re: [PATCH v3 1/2] ACPI: video: get rid of magic numbers and use enum instead

[Dmitry Frank]

Hi Rafael,

On 04/19, Rafael J. Wysocki wrote:
> On Wed, Apr 19, 2017 at 12:36 PM, Dmitry Frank <mail@dmitryfrank.com> wrote:
> > The first two items in the _BCL method response are special:
> >
> >   - Level when machine has full power
> >   - Level when machine is on batteries
> >   - .... actual supported levels go there ....
> >
> > So this commits adds an enum and uses its descriptive elements
> > throughout the code, instead of magic numbers.
> >
> > Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
> 
> Are there any differences between this and the previous version of the
> patch you posted?
> 

No; this patch is identical in both v2 and v3 patchsets.

Differences between v3 and v2 are as follows: In v3 I removed the middle
patch (which was renaming the global flag device_id_scheme), and also
amended the last patch: added the comment for that flag. Everything else
is identical.

Thanks,
Dmitry

Re: [PATCH v3 1/2] ACPI: video: get rid of magic numbers and use enum instead

[Rafael J. Wysocki]

On Wed, Apr 19, 2017 at 9:44 PM, Dmitry Frank <mail@dmitryfrank.com> wrote:
> Hi Rafael,
>
> On 04/19, Rafael J. Wysocki wrote:
>> On Wed, Apr 19, 2017 at 12:36 PM, Dmitry Frank <mail@dmitryfrank.com> wrote:
>> > The first two items in the _BCL method response are special:
>> >
>> >   - Level when machine has full power
>> >   - Level when machine is on batteries
>> >   - .... actual supported levels go there ....
>> >
>> > So this commits adds an enum and uses its descriptive elements
>> > throughout the code, instead of magic numbers.
>> >
>> > Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
>>
>> Are there any differences between this and the previous version of the
>> patch you posted?
>>
>
> No; this patch is identical in both v2 and v3 patchsets.
>
> Differences between v3 and v2 are as follows: In v3 I removed the middle
> patch (which was renaming the global flag device_id_scheme), and also
> amended the last patch: added the comment for that flag. Everything else
> is identical.

OK, thanks!

Re: [PATCH v3 1/2] ACPI: video: get rid of magic numbers and use enum instead

[Rafael J. Wysocki]

On Wed, Apr 19, 2017 at 10:15 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Apr 19, 2017 at 9:44 PM, Dmitry Frank <mail@dmitryfrank.com> wrote:
>> Hi Rafael,
>>
>> On 04/19, Rafael J. Wysocki wrote:
>>> On Wed, Apr 19, 2017 at 12:36 PM, Dmitry Frank <mail@dmitryfrank.com> wrote:
>>> > The first two items in the _BCL method response are special:
>>> >
>>> >   - Level when machine has full power
>>> >   - Level when machine is on batteries
>>> >   - .... actual supported levels go there ....
>>> >
>>> > So this commits adds an enum and uses its descriptive elements
>>> > throughout the code, instead of magic numbers.
>>> >
>>> > Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
>>>
>>> Are there any differences between this and the previous version of the
>>> patch you posted?
>>>
>>
>> No; this patch is identical in both v2 and v3 patchsets.
>>
>> Differences between v3 and v2 are as follows: In v3 I removed the middle
>> patch (which was renaming the global flag device_id_scheme), and also
>> amended the last patch: added the comment for that flag. Everything else
>> is identical.
>
> OK, thanks!

The v2 has been applied.

Thanks,
Rafael

Re: [PATCH v3 1/2] ACPI: video: get rid of magic numbers and use enum instead

[Dmitry Frank]

Hi Rafael,

On 04/20, Rafael J. Wysocki wrote:
> On Wed, Apr 19, 2017 at 10:15 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Apr 19, 2017 at 9:44 PM, Dmitry Frank <mail@dmitryfrank.com> wrote:
> >> Hi Rafael,
> >>
> >> On 04/19, Rafael J. Wysocki wrote:
> >>> On Wed, Apr 19, 2017 at 12:36 PM, Dmitry Frank <mail@dmitryfrank.com> wrote:
> >>> > The first two items in the _BCL method response are special:
> >>> >
> >>> >   - Level when machine has full power
> >>> >   - Level when machine is on batteries
> >>> >   - .... actual supported levels go there ....
> >>> >
> >>> > So this commits adds an enum and uses its descriptive elements
> >>> > throughout the code, instead of magic numbers.
> >>> >
> >>> > Signed-off-by: Dmitry Frank <mail@dmitryfrank.com>
> >>>
> >>> Are there any differences between this and the previous version of the
> >>> patch you posted?
> >>>
> >>
> >> No; this patch is identical in both v2 and v3 patchsets.
> >>
> >> Differences between v3 and v2 are as follows: In v3 I removed the middle
> >> patch (which was renaming the global flag device_id_scheme), and also
> >> amended the last patch: added the comment for that flag. Everything else
> >> is identical.
> >
> > OK, thanks!
> 
> The v2 has been applied.
> 

Thank you!

> Thanks,
> Rafael