| Message ID | 20190419055743.666-1-alex.hung@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | acpi/method: check full and battery power if acpi_video0 exists | expand |
On 19/04/2019 06:57, Alex Hung wrote: > 8b9e5e4c07f4 added checks for full and battery power in _BCL list, > but ACPI spec does not necessary ask them to be included (please > see examples in ACPI spec B.5.2). However, Linux kernel relies on this > behaviours. > > Let's only check full and battery power only if acpi_video0 interface is > used. > > Signed-off-by: Alex Hung <alex.hung@canonical.com> > --- > src/acpi/method/method.c | 64 ++++++++++++++++++++++------------------ > 1 file changed, 35 insertions(+), 29 deletions(-) > > diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c > index 635c4813..5a4de7b3 100644 > --- a/src/acpi/method/method.c > +++ b/src/acpi/method/method.c > @@ -5741,6 +5741,7 @@ static void method_test_BCL_return( > bool ascending_levels = false; > bool matching_levels; > char *str = NULL; > + DIR *acpi_video_dir; > > FWTS_UNUSED(private); > > @@ -5791,40 +5792,45 @@ static void method_test_BCL_return( > failed = true; > } > > - matching_levels = false; > - for (i = 2; i < obj->Package.Count; i++) { > - if (obj->Package.Elements[0].Integer.Value == > - obj->Package.Elements[i].Integer.Value) { > - matching_levels = true; > - break; > + acpi_video_dir = opendir("/sys/class/backlight/acpi_video0"); Does this sysfs interface exist for the all kernel releases that we support? Also, rather than an expensive opendir()/closedir(), it may be preferable to just use access() or stat() to check if the sysfs directory exists. > + if (acpi_video_dir) { > + matching_levels = false; > + for (i = 2; i < obj->Package.Count; i++) { > + if (obj->Package.Elements[0].Integer.Value == > + obj->Package.Elements[i].Integer.Value) { > + matching_levels = true; > + break; > + } > } > - } > > - if (!matching_levels) { > - failed = true; > - fwts_failed(fw, LOG_LEVEL_CRITICAL, > - "Method_BCLFullNotInList", > - "brightness level on full power (%" PRIu64 > - ") is not in brightness levels.", > - obj->Package.Elements[0].Integer.Value); > - } > + if (!matching_levels) { > + failed = true; > + fwts_failed(fw, LOG_LEVEL_CRITICAL, > + "Method_BCLFullNotInList", > + "brightness level on full power (%" PRIu64 > + ") is not in brightness levels.", > + obj->Package.Elements[0].Integer.Value); > + } > > - matching_levels = false; > - for (i = 2; i < obj->Package.Count; i++) { > - if (obj->Package.Elements[1].Integer.Value == > - obj->Package.Elements[i].Integer.Value) { > - matching_levels = true; > - break; > + matching_levels = false; > + for (i = 2; i < obj->Package.Count; i++) { > + if (obj->Package.Elements[1].Integer.Value == > + obj->Package.Elements[i].Integer.Value) { > + matching_levels = true; > + break; > + } > } > - } > > - if (!matching_levels) { > - failed = true; > - fwts_failed(fw, LOG_LEVEL_CRITICAL, > - "Method_BCLBatteryNotInList", > - "brightness level on battery (%" PRIu64 > - ") is not in brightness levels.", > - obj->Package.Elements[1].Integer.Value); > + if (!matching_levels) { > + failed = true; > + fwts_failed(fw, LOG_LEVEL_CRITICAL, > + "Method_BCLBatteryNotInList", > + "brightness level on battery (%" PRIu64 > + ") is not in brightness levels.", > + obj->Package.Elements[1].Integer.Value); > + } > + > + closedir(acpi_video_dir); > } > > fwts_log_info(fw, "Brightness levels for %s:" ,name); >
On Fri, Apr 19, 2019 at 1:51 AM Colin Ian King <colin.king@canonical.com> wrote: > > On 19/04/2019 06:57, Alex Hung wrote: > > 8b9e5e4c07f4 added checks for full and battery power in _BCL list, > > but ACPI spec does not necessary ask them to be included (please > > see examples in ACPI spec B.5.2). However, Linux kernel relies on this > > behaviours. > > > > Let's only check full and battery power only if acpi_video0 interface is > > used. > > > > Signed-off-by: Alex Hung <alex.hung@canonical.com> > > --- > > src/acpi/method/method.c | 64 ++++++++++++++++++++++------------------ > > 1 file changed, 35 insertions(+), 29 deletions(-) > > > > diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c > > index 635c4813..5a4de7b3 100644 > > --- a/src/acpi/method/method.c > > +++ b/src/acpi/method/method.c > > @@ -5741,6 +5741,7 @@ static void method_test_BCL_return( > > bool ascending_levels = false; > > bool matching_levels; > > char *str = NULL; > > + DIR *acpi_video_dir; > > > > FWTS_UNUSED(private); > > > > @@ -5791,40 +5792,45 @@ static void method_test_BCL_return( > > failed = true; > > } > > > > - matching_levels = false; > > - for (i = 2; i < obj->Package.Count; i++) { > > - if (obj->Package.Elements[0].Integer.Value == > > - obj->Package.Elements[i].Integer.Value) { > > - matching_levels = true; > > - break; > > + acpi_video_dir = opendir("/sys/class/backlight/acpi_video0"); > > Does this sysfs interface exist for the all kernel releases that we > support? If ACPI brightness interfaces are used, they exists. Many systems with Ubuntu 16.04 and later use native driver interfaces from Intel and AMD. We will only see weird brightness behaviours only if "/sys/class/backlight/acpi_video0" exists. > > Also, rather than an expensive opendir()/closedir(), it may be > preferable to just use access() or stat() to check if the sysfs > directory exists. Thanks. Will fix it in next revision. > > > + if (acpi_video_dir) { > > + matching_levels = false; > > + for (i = 2; i < obj->Package.Count; i++) { > > + if (obj->Package.Elements[0].Integer.Value == > > + obj->Package.Elements[i].Integer.Value) { > > + matching_levels = true; > > + break; > > + } > > } > > - } > > > > - if (!matching_levels) { > > - failed = true; > > - fwts_failed(fw, LOG_LEVEL_CRITICAL, > > - "Method_BCLFullNotInList", > > - "brightness level on full power (%" PRIu64 > > - ") is not in brightness levels.", > > - obj->Package.Elements[0].Integer.Value); > > - } > > + if (!matching_levels) { > > + failed = true; > > + fwts_failed(fw, LOG_LEVEL_CRITICAL, > > + "Method_BCLFullNotInList", > > + "brightness level on full power (%" PRIu64 > > + ") is not in brightness levels.", > > + obj->Package.Elements[0].Integer.Value); > > + } > > > > - matching_levels = false; > > - for (i = 2; i < obj->Package.Count; i++) { > > - if (obj->Package.Elements[1].Integer.Value == > > - obj->Package.Elements[i].Integer.Value) { > > - matching_levels = true; > > - break; > > + matching_levels = false; > > + for (i = 2; i < obj->Package.Count; i++) { > > + if (obj->Package.Elements[1].Integer.Value == > > + obj->Package.Elements[i].Integer.Value) { > > + matching_levels = true; > > + break; > > + } > > } > > - } > > > > - if (!matching_levels) { > > - failed = true; > > - fwts_failed(fw, LOG_LEVEL_CRITICAL, > > - "Method_BCLBatteryNotInList", > > - "brightness level on battery (%" PRIu64 > > - ") is not in brightness levels.", > > - obj->Package.Elements[1].Integer.Value); > > + if (!matching_levels) { > > + failed = true; > > + fwts_failed(fw, LOG_LEVEL_CRITICAL, > > + "Method_BCLBatteryNotInList", > > + "brightness level on battery (%" PRIu64 > > + ") is not in brightness levels.", > > + obj->Package.Elements[1].Integer.Value); > > + } > > + > > + closedir(acpi_video_dir); > > } > > > > fwts_log_info(fw, "Brightness levels for %s:" ,name); > > > > > -- > fwts-devel mailing list > fwts-devel@lists.ubuntu.com > Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c index 635c4813..5a4de7b3 100644 --- a/src/acpi/method/method.c +++ b/src/acpi/method/method.c @@ -5741,6 +5741,7 @@ static void method_test_BCL_return( bool ascending_levels = false; bool matching_levels; char *str = NULL; + DIR *acpi_video_dir; FWTS_UNUSED(private); @@ -5791,40 +5792,45 @@ static void method_test_BCL_return( failed = true; } - matching_levels = false; - for (i = 2; i < obj->Package.Count; i++) { - if (obj->Package.Elements[0].Integer.Value == - obj->Package.Elements[i].Integer.Value) { - matching_levels = true; - break; + acpi_video_dir = opendir("/sys/class/backlight/acpi_video0"); + if (acpi_video_dir) { + matching_levels = false; + for (i = 2; i < obj->Package.Count; i++) { + if (obj->Package.Elements[0].Integer.Value == + obj->Package.Elements[i].Integer.Value) { + matching_levels = true; + break; + } } - } - if (!matching_levels) { - failed = true; - fwts_failed(fw, LOG_LEVEL_CRITICAL, - "Method_BCLFullNotInList", - "brightness level on full power (%" PRIu64 - ") is not in brightness levels.", - obj->Package.Elements[0].Integer.Value); - } + if (!matching_levels) { + failed = true; + fwts_failed(fw, LOG_LEVEL_CRITICAL, + "Method_BCLFullNotInList", + "brightness level on full power (%" PRIu64 + ") is not in brightness levels.", + obj->Package.Elements[0].Integer.Value); + } - matching_levels = false; - for (i = 2; i < obj->Package.Count; i++) { - if (obj->Package.Elements[1].Integer.Value == - obj->Package.Elements[i].Integer.Value) { - matching_levels = true; - break; + matching_levels = false; + for (i = 2; i < obj->Package.Count; i++) { + if (obj->Package.Elements[1].Integer.Value == + obj->Package.Elements[i].Integer.Value) { + matching_levels = true; + break; + } } - } - if (!matching_levels) { - failed = true; - fwts_failed(fw, LOG_LEVEL_CRITICAL, - "Method_BCLBatteryNotInList", - "brightness level on battery (%" PRIu64 - ") is not in brightness levels.", - obj->Package.Elements[1].Integer.Value); + if (!matching_levels) { + failed = true; + fwts_failed(fw, LOG_LEVEL_CRITICAL, + "Method_BCLBatteryNotInList", + "brightness level on battery (%" PRIu64 + ") is not in brightness levels.", + obj->Package.Elements[1].Integer.Value); + } + + closedir(acpi_video_dir); } fwts_log_info(fw, "Brightness levels for %s:" ,name);
8b9e5e4c07f4 added checks for full and battery power in _BCL list, but ACPI spec does not necessary ask them to be included (please see examples in ACPI spec B.5.2). However, Linux kernel relies on this behaviours. Let's only check full and battery power only if acpi_video0 interface is used. Signed-off-by: Alex Hung <alex.hung@canonical.com> --- src/acpi/method/method.c | 64 ++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 29 deletions(-)