2021-02-01 18:25:42

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH v1 0/5] ACPI: More cleanups related to printing messages

Hi All,

This series is a continuation of the effort to drop ACPICA-specific debug
code from non-ACPICA pieces of the ACPI subsystem and to make the message
printing in there more consistent.

The patches in this series are based on linux-next from today.

Details in the patch changelogs.

Thanks!




2021-02-01 18:25:55

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH v1 3/5] ACPI: button: Clean up printing messages

From: Rafael J. Wysocki <[email protected]>

Replace the ACPI_DEBUG_PRINT() instance in button.c with an
acpi_handle_debug() call, drop the _COMPONENT and ACPI_MODULE_NAME()
definitions that are not used any more, drop the no longer needed
ACPI_BUTTON_COMPONENT definition from the headers and update the
documentation accordingly.

While at it, replace the direct printk() invocations with pr_info()
(that changes the excessive log level for some of them too) and drop
the unneeded PREFIX sybmbol definition from battery.c.

Signed-off-by: Rafael J. Wysocki <[email protected]>
---
Documentation/firmware-guide/acpi/debug.rst | 1 -
drivers/acpi/button.c | 15 +++++----------
drivers/acpi/sysfs.c | 1 -
include/acpi/acpi_drivers.h | 1 -
4 files changed, 5 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/acpi/button.c
===================================================================
--- linux-pm.orig/drivers/acpi/button.c
+++ linux-pm/drivers/acpi/button.c
@@ -21,8 +21,6 @@
#include <linux/dmi.h>
#include <acpi/button.h>

-#define PREFIX "ACPI: "
-
#define ACPI_BUTTON_CLASS "button"
#define ACPI_BUTTON_FILE_STATE "state"
#define ACPI_BUTTON_TYPE_UNKNOWN 0x00
@@ -54,9 +52,6 @@ static const char * const lid_init_state
[ACPI_BUTTON_LID_INIT_DISABLED] = "disabled",
};

-#define _COMPONENT ACPI_BUTTON_COMPONENT
-ACPI_MODULE_NAME("button");
-
MODULE_AUTHOR("Paul Diefenbaugh");
MODULE_DESCRIPTION("ACPI Button Driver");
MODULE_LICENSE("GPL");
@@ -285,7 +280,7 @@ static int acpi_button_add_fs(struct acp
return 0;

if (acpi_button_dir || acpi_lid_dir) {
- printk(KERN_ERR PREFIX "More than one Lid device found!\n");
+ pr_info("More than one Lid device found!\n");
return -EEXIST;
}

@@ -434,8 +429,8 @@ static void acpi_button_notify(struct ac
}
break;
default:
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "Unsupported event [0x%x]\n", event));
+ acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
+ event);
break;
}
}
@@ -523,7 +518,7 @@ static int acpi_button_add(struct acpi_d
ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_LID);
input->open = acpi_lid_input_open;
} else {
- printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
+ pr_info("Unsupported hid [%s]\n", hid);
error = -ENODEV;
goto err_free_input;
}
@@ -567,7 +562,7 @@ static int acpi_button_add(struct acpi_d
}

device_init_wakeup(&device->dev, true);
- printk(KERN_INFO PREFIX "%s [%s]\n", name, acpi_device_bid(device));
+ pr_info("%s [%s]\n", name, acpi_device_bid(device));
return 0;

err_remove_fs:
Index: linux-pm/Documentation/firmware-guide/acpi/debug.rst
===================================================================
--- linux-pm.orig/Documentation/firmware-guide/acpi/debug.rst
+++ linux-pm/Documentation/firmware-guide/acpi/debug.rst
@@ -52,7 +52,6 @@ shows the supported mask values, current
ACPI_CA_DISASSEMBLER 0x00000800
ACPI_COMPILER 0x00001000
ACPI_TOOLS 0x00002000
- ACPI_BUTTON_COMPONENT 0x00080000
ACPI_SBS_COMPONENT 0x00100000
ACPI_FAN_COMPONENT 0x00200000
ACPI_PCI_COMPONENT 0x00400000
Index: linux-pm/drivers/acpi/sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/sysfs.c
+++ linux-pm/drivers/acpi/sysfs.c
@@ -52,7 +52,6 @@ static const struct acpi_dlayer acpi_deb
ACPI_DEBUG_INIT(ACPI_COMPILER),
ACPI_DEBUG_INIT(ACPI_TOOLS),

- ACPI_DEBUG_INIT(ACPI_BUTTON_COMPONENT),
ACPI_DEBUG_INIT(ACPI_SBS_COMPONENT),
ACPI_DEBUG_INIT(ACPI_FAN_COMPONENT),
ACPI_DEBUG_INIT(ACPI_PCI_COMPONENT),
Index: linux-pm/include/acpi/acpi_drivers.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_drivers.h
+++ linux-pm/include/acpi/acpi_drivers.h
@@ -15,7 +15,6 @@
* Please update drivers/acpi/debug.c and Documentation/firmware-guide/acpi/debug.rst
* if you add to this list.
*/
-#define ACPI_BUTTON_COMPONENT 0x00080000
#define ACPI_SBS_COMPONENT 0x00100000
#define ACPI_FAN_COMPONENT 0x00200000
#define ACPI_PCI_COMPONENT 0x00400000



2021-02-01 18:26:47

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH v1 2/5] ACPI: battery: Clean up printing messages

From: Rafael J. Wysocki <[email protected]>

Replace the ACPI_DEBUG_PRINT() and ACPI_EXCEPTION() instances
in battery.c with acpi_handle_debug() and acpi_handle_info() calls,
respectively, drop the _COMPONENT and ACPI_MODULE_NAME() definitions
that are not used any more, drop the no longer needed
ACPI_BATTERY_COMPONENT definition from the headers and update the
documentation accordingly.

While at it, update the pr_fmt() definition and drop the unneeded
PREFIX sybmbol definition from battery.c.

Signed-off-by: Rafael J. Wysocki <[email protected]>
---
Documentation/firmware-guide/acpi/debug.rst | 1
drivers/acpi/battery.c | 29 ++++++++++++++--------------
drivers/acpi/sysfs.c | 1
include/acpi/acpi_drivers.h | 1
4 files changed, 15 insertions(+), 17 deletions(-)

Index: linux-pm/drivers/acpi/battery.c
===================================================================
--- linux-pm.orig/drivers/acpi/battery.c
+++ linux-pm/drivers/acpi/battery.c
@@ -8,7 +8,7 @@
* Copyright (C) 2001, 2002 Paul Diefenbaugh <[email protected]>
*/

-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "ACPI: battery: " fmt

#include <linux/async.h>
#include <linux/delay.h>
@@ -29,8 +29,6 @@

#include <acpi/battery.h>

-#define PREFIX "ACPI: "
-
#define ACPI_BATTERY_VALUE_UNKNOWN 0xFFFFFFFF
#define ACPI_BATTERY_CAPACITY_VALID(capacity) \
((capacity) != 0 && (capacity) != ACPI_BATTERY_VALUE_UNKNOWN)
@@ -44,10 +42,6 @@
#define ACPI_BATTERY_STATE_CHARGING 0x2
#define ACPI_BATTERY_STATE_CRITICAL 0x4

-#define _COMPONENT ACPI_BATTERY_COMPONENT
-
-ACPI_MODULE_NAME("battery");
-
MODULE_AUTHOR("Paul Diefenbaugh");
MODULE_AUTHOR("Alexey Starikovskiy <[email protected]>");
MODULE_DESCRIPTION("ACPI Battery Driver");
@@ -466,7 +460,8 @@ static int extract_package(struct acpi_b
static int acpi_battery_get_status(struct acpi_battery *battery)
{
if (acpi_bus_get_status(battery->device)) {
- ACPI_EXCEPTION((AE_INFO, AE_ERROR, "Evaluating _STA"));
+ acpi_handle_info(battery->device->handle,
+ "_STA evaluation failed\n");
return -ENODEV;
}
return 0;
@@ -535,8 +530,10 @@ static int acpi_battery_get_info(struct
mutex_unlock(&battery->lock);

if (ACPI_FAILURE(status)) {
- ACPI_EXCEPTION((AE_INFO, status, "Evaluating %s",
- use_bix ? "_BIX":"_BIF"));
+ acpi_handle_info(battery->device->handle,
+ "%s evaluation failed: %s\n",
+ use_bix ?"_BIX":"_BIF",
+ acpi_format_exception(status));
} else {
result = extract_battery_info(use_bix,
battery,
@@ -573,7 +570,9 @@ static int acpi_battery_get_state(struct
mutex_unlock(&battery->lock);

if (ACPI_FAILURE(status)) {
- ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BST"));
+ acpi_handle_info(battery->device->handle,
+ "_BST evaluation failed: %s",
+ acpi_format_exception(status));
return -ENODEV;
}

@@ -625,7 +624,9 @@ static int acpi_battery_set_alarm(struct
if (ACPI_FAILURE(status))
return -ENODEV;

- ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Alarm set to %d\n", battery->alarm));
+ acpi_handle_debug(battery->device->handle, "Alarm set to %d\n",
+ battery->alarm);
+
return 0;
}

@@ -1201,7 +1202,7 @@ static int acpi_battery_add(struct acpi_
if (result)
goto fail;

- pr_info(PREFIX "%s Slot [%s] (battery %s)\n",
+ pr_info("%s Slot [%s] (battery %s)\n",
ACPI_BATTERY_DEVICE_NAME, acpi_device_bid(device),
device->status.battery_present ? "present" : "absent");

@@ -1282,7 +1283,7 @@ static void __init acpi_battery_init_asy
if (battery_check_pmic) {
for (i = 0; i < ARRAY_SIZE(acpi_battery_blacklist); i++)
if (acpi_dev_present(acpi_battery_blacklist[i], "1", -1)) {
- pr_info(PREFIX ACPI_BATTERY_DEVICE_NAME
+ pr_info(ACPI_BATTERY_DEVICE_NAME
": found native %s PMIC, not loading\n",
acpi_battery_blacklist[i]);
return;
Index: linux-pm/Documentation/firmware-guide/acpi/debug.rst
===================================================================
--- linux-pm.orig/Documentation/firmware-guide/acpi/debug.rst
+++ linux-pm/Documentation/firmware-guide/acpi/debug.rst
@@ -52,7 +52,6 @@ shows the supported mask values, current
ACPI_CA_DISASSEMBLER 0x00000800
ACPI_COMPILER 0x00001000
ACPI_TOOLS 0x00002000
- ACPI_BATTERY_COMPONENT 0x00040000
ACPI_BUTTON_COMPONENT 0x00080000
ACPI_SBS_COMPONENT 0x00100000
ACPI_FAN_COMPONENT 0x00200000
Index: linux-pm/drivers/acpi/sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/sysfs.c
+++ linux-pm/drivers/acpi/sysfs.c
@@ -52,7 +52,6 @@ static const struct acpi_dlayer acpi_deb
ACPI_DEBUG_INIT(ACPI_COMPILER),
ACPI_DEBUG_INIT(ACPI_TOOLS),

- ACPI_DEBUG_INIT(ACPI_BATTERY_COMPONENT),
ACPI_DEBUG_INIT(ACPI_BUTTON_COMPONENT),
ACPI_DEBUG_INIT(ACPI_SBS_COMPONENT),
ACPI_DEBUG_INIT(ACPI_FAN_COMPONENT),
Index: linux-pm/include/acpi/acpi_drivers.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_drivers.h
+++ linux-pm/include/acpi/acpi_drivers.h
@@ -15,7 +15,6 @@
* Please update drivers/acpi/debug.c and Documentation/firmware-guide/acpi/debug.rst
* if you add to this list.
*/
-#define ACPI_BATTERY_COMPONENT 0x00040000
#define ACPI_BUTTON_COMPONENT 0x00080000
#define ACPI_SBS_COMPONENT 0x00100000
#define ACPI_FAN_COMPONENT 0x00200000



2021-02-01 18:27:40

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH v1 1/5] ACPI: AC: Clean up printing messages

From: Rafael J. Wysocki <[email protected]>

Replace the ACPI_DEBUG_PRINT() and ACPI_EXCEPTION() instances
in ac.c with acpi_handle_debug() and acpi_handle_info() calls,
respectively, drop the _COMPONENT and ACPI_MODULE_NAME() definitions
that are not used any more, drop the no longer needed ACPI_AC_COMPONENT
definition from the headers and update the documentation accordingly.

Signed-off-by: Rafael J. Wysocki <[email protected]>
---
Documentation/firmware-guide/acpi/debug.rst | 1 -
drivers/acpi/ac.c | 12 +++++-------
drivers/acpi/sysfs.c | 1 -
include/acpi/acpi_drivers.h | 1 -
4 files changed, 5 insertions(+), 10 deletions(-)

Index: linux-pm/Documentation/firmware-guide/acpi/debug.rst
===================================================================
--- linux-pm.orig/Documentation/firmware-guide/acpi/debug.rst
+++ linux-pm/Documentation/firmware-guide/acpi/debug.rst
@@ -52,7 +52,6 @@ shows the supported mask values, current
ACPI_CA_DISASSEMBLER 0x00000800
ACPI_COMPILER 0x00001000
ACPI_TOOLS 0x00002000
- ACPI_AC_COMPONENT 0x00020000
ACPI_BATTERY_COMPONENT 0x00040000
ACPI_BUTTON_COMPONENT 0x00080000
ACPI_SBS_COMPONENT 0x00100000
Index: linux-pm/drivers/acpi/ac.c
===================================================================
--- linux-pm.orig/drivers/acpi/ac.c
+++ linux-pm/drivers/acpi/ac.c
@@ -28,9 +28,6 @@
#define ACPI_AC_STATUS_ONLINE 0x01
#define ACPI_AC_STATUS_UNKNOWN 0xFF

-#define _COMPONENT ACPI_AC_COMPONENT
-ACPI_MODULE_NAME("ac");
-
MODULE_AUTHOR("Paul Diefenbaugh");
MODULE_DESCRIPTION("ACPI AC Adapter Driver");
MODULE_LICENSE("GPL");
@@ -102,8 +99,9 @@ static int acpi_ac_get_state(struct acpi
status = acpi_evaluate_integer(ac->device->handle, "_PSR", NULL,
&ac->state);
if (ACPI_FAILURE(status)) {
- ACPI_EXCEPTION((AE_INFO, status,
- "Error reading AC Adapter state"));
+ acpi_handle_info(ac->device->handle,
+ "Error reading AC Adapter state: %s\n",
+ acpi_format_exception(status));
ac->state = ACPI_AC_STATUS_UNKNOWN;
return -ENODEV;
}
@@ -153,8 +151,8 @@ static void acpi_ac_notify(struct acpi_d

switch (event) {
default:
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "Unsupported event [0x%x]\n", event));
+ acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
+ event);
fallthrough;
case ACPI_AC_NOTIFY_STATUS:
case ACPI_NOTIFY_BUS_CHECK:
Index: linux-pm/drivers/acpi/sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/sysfs.c
+++ linux-pm/drivers/acpi/sysfs.c
@@ -52,7 +52,6 @@ static const struct acpi_dlayer acpi_deb
ACPI_DEBUG_INIT(ACPI_COMPILER),
ACPI_DEBUG_INIT(ACPI_TOOLS),

- ACPI_DEBUG_INIT(ACPI_AC_COMPONENT),
ACPI_DEBUG_INIT(ACPI_BATTERY_COMPONENT),
ACPI_DEBUG_INIT(ACPI_BUTTON_COMPONENT),
ACPI_DEBUG_INIT(ACPI_SBS_COMPONENT),
Index: linux-pm/include/acpi/acpi_drivers.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_drivers.h
+++ linux-pm/include/acpi/acpi_drivers.h
@@ -15,7 +15,6 @@
* Please update drivers/acpi/debug.c and Documentation/firmware-guide/acpi/debug.rst
* if you add to this list.
*/
-#define ACPI_AC_COMPONENT 0x00020000
#define ACPI_BATTERY_COMPONENT 0x00040000
#define ACPI_BUTTON_COMPONENT 0x00080000
#define ACPI_SBS_COMPONENT 0x00100000



2021-02-01 18:29:25

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH v1 4/5] ACPI: video: Clean up printing messages

From: Rafael J. Wysocki <[email protected]>

Replace the ACPI_DEBUG_PRINT() instances in acpi_video.c with
acpi_handle_debug() calls and the ACPI_EXCEPTION()/ACPI_ERROR()/
ACPI_WARNING() instances in there with acpi_handle_info() calls.

Drop the _COMPONENT and ACPI_MODULE_NAME() definitions that are not
used any more from acpi_video.c, drop the no longer needed
ACPI_VIDEO_COMPONENT definition from the headers and update the
documentation accordingly.

While at it, add a pr_fmt() definition to acpi_video.c, replace the
direct printk() invocations in there with acpi_handle_info() or
pr_info() (and reduce the excessive log level where applicable) and
drop the PREFIX sybmbol definition which is not necessary any more
from acpi_video.c.

Also make one unrelated janitorial change to fix up white space and
use ACPI_FAILURE() instead of negating ACPI_SUCCESS().

Signed-off-by: Rafael J. Wysocki <[email protected]>
---
Documentation/firmware-guide/acpi/debug.rst | 1
drivers/acpi/acpi_video.c | 95 ++++++++++++++--------------
drivers/acpi/sysfs.c | 1
include/acpi/acpi_drivers.h | 1
4 files changed, 49 insertions(+), 49 deletions(-)

Index: linux-pm/drivers/acpi/acpi_video.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_video.c
+++ linux-pm/drivers/acpi/acpi_video.c
@@ -7,6 +7,8 @@
* Copyright (C) 2006 Thomas Tuttle <[email protected]>
*/

+#define pr_fmt(fmt) "ACPI: video: " fmt
+
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
@@ -26,16 +28,11 @@
#include <acpi/video.h>
#include <linux/uaccess.h>

-#define PREFIX "ACPI: "
-
#define ACPI_VIDEO_BUS_NAME "Video Bus"
#define ACPI_VIDEO_DEVICE_NAME "Video Device"

#define MAX_NAME_LEN 20

-#define _COMPONENT ACPI_VIDEO_COMPONENT
-ACPI_MODULE_NAME("video");
-
MODULE_AUTHOR("Bruno Ducrot");
MODULE_DESCRIPTION("ACPI Video Driver");
MODULE_LICENSE("GPL");
@@ -330,7 +327,7 @@ acpi_video_device_lcd_query_levels(acpi_
return status;
obj = (union acpi_object *)buffer.pointer;
if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
- printk(KERN_ERR PREFIX "Invalid _BCL data\n");
+ acpi_handle_info(handle, "Invalid _BCL data\n");
status = -EFAULT;
goto err;
}
@@ -354,7 +351,7 @@ acpi_video_device_lcd_set_level(struct a
status = acpi_execute_simple_method(device->dev->handle,
"_BCM", level);
if (ACPI_FAILURE(status)) {
- ACPI_ERROR((AE_INFO, "Evaluating _BCM failed"));
+ acpi_handle_info(device->dev->handle, "_BCM evaluation failed\n");
return -EIO;
}

@@ -368,7 +365,7 @@ acpi_video_device_lcd_set_level(struct a
return 0;
}

- ACPI_ERROR((AE_INFO, "Current brightness invalid"));
+ acpi_handle_info(device->dev->handle, "Current brightness invalid\n");
return -EINVAL;
}

@@ -622,9 +619,8 @@ acpi_video_device_lcd_get_level_current(
* BQC returned an invalid level.
* Stop using it.
*/
- ACPI_WARNING((AE_INFO,
- "%s returned an invalid level",
- buf));
+ acpi_handle_info(device->dev->handle,
+ "%s returned an invalid level", buf);
device->cap._BQC = device->cap._BCQ = 0;
} else {
/*
@@ -635,7 +631,8 @@ acpi_video_device_lcd_get_level_current(
* ACPI video backlight still works w/ buggy _BQC.
* http://bugzilla.kernel.org/show_bug.cgi?id=12233
*/
- ACPI_WARNING((AE_INFO, "Evaluating %s failed", buf));
+ acpi_handle_info(device->dev->handle,
+ "%s evaluation failed", buf);
device->cap._BQC = device->cap._BCQ = 0;
}
}
@@ -675,7 +672,7 @@ acpi_video_device_EDID(struct acpi_video
if (obj && obj->type == ACPI_TYPE_BUFFER)
*edid = obj;
else {
- printk(KERN_ERR PREFIX "Invalid _DDC data\n");
+ acpi_handle_info(device->dev->handle, "Invalid _DDC data\n");
status = -EFAULT;
kfree(obj);
}
@@ -827,10 +824,9 @@ int acpi_video_get_levels(struct acpi_de
int result = 0;
u32 value;

- if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device->handle,
- &obj))) {
- ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available "
- "LCD brightness level\n"));
+ if (ACPI_FAILURE(acpi_video_device_lcd_query_levels(device->handle, &obj))) {
+ acpi_handle_debug(device->handle,
+ "Could not query available LCD brightness level\n");
result = -ENODEV;
goto out;
}
@@ -842,7 +838,6 @@ int acpi_video_get_levels(struct acpi_de

br = kzalloc(sizeof(*br), GFP_KERNEL);
if (!br) {
- printk(KERN_ERR "can't allocate memory\n");
result = -ENOMEM;
goto out;
}
@@ -863,7 +858,7 @@ int acpi_video_get_levels(struct acpi_de
for (i = 0; i < obj->package.count; i++) {
o = (union acpi_object *)&obj->package.elements[i];
if (o->type != ACPI_TYPE_INTEGER) {
- printk(KERN_ERR PREFIX "Invalid data\n");
+ acpi_handle_info(device->handle, "Invalid data\n");
continue;
}
value = (u32) o->integer.value;
@@ -900,7 +895,8 @@ int acpi_video_get_levels(struct acpi_de
br->levels[i] = br->levels[i - level_ac_battery];
count += level_ac_battery;
} else if (level_ac_battery > ACPI_VIDEO_FIRST_LEVEL)
- ACPI_ERROR((AE_INFO, "Too many duplicates in _BCL package"));
+ acpi_handle_info(device->handle,
+ "Too many duplicates in _BCL package");

/* Check if the _BCL package is in a reversed order */
if (max_level == br->levels[ACPI_VIDEO_FIRST_LEVEL]) {
@@ -910,8 +906,8 @@ int acpi_video_get_levels(struct acpi_de
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"));
+ acpi_handle_info(device->handle,
+ "Found unordered _BCL package");

br->count = count;
*dev_br = br;
@@ -989,9 +985,9 @@ set_level:
if (result)
goto out_free_levels;

- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "found %d brightness levels\n",
- br->count - ACPI_VIDEO_FIRST_LEVEL));
+ acpi_handle_debug(device->dev->handle, "found %d brightness levels\n",
+ br->count - ACPI_VIDEO_FIRST_LEVEL);
+
return 0;

out_free_levels:
@@ -1023,7 +1019,8 @@ static void acpi_video_device_find_cap(s
if (acpi_has_method(device->dev->handle, "_BQC")) {
device->cap._BQC = 1;
} else if (acpi_has_method(device->dev->handle, "_BCQ")) {
- printk(KERN_WARNING FW_BUG "_BCQ is used instead of _BQC\n");
+ acpi_handle_info(device->dev->handle,
+ "_BCQ is used instead of _BQC\n");
device->cap._BCQ = 1;
}

@@ -1083,8 +1080,7 @@ static int acpi_video_bus_check(struct a
/* Does this device support video switching? */
if (video->cap._DOS || video->cap._DOD) {
if (!video->cap._DOS) {
- printk(KERN_WARNING FW_BUG
- "ACPI(%s) defines _DOD but not _DOS\n",
+ pr_info(FW_BUG "ACPI(%s) defines _DOD but not _DOS\n",
acpi_device_bid(video->device));
}
video->flags.multihead = 1;
@@ -1272,7 +1268,8 @@ acpi_video_device_bind(struct acpi_video
ids = &video->attached_array[i];
if (device->device_id == (ids->value.int_val & 0xffff)) {
ids->bind_info = device;
- ACPI_DEBUG_PRINT((ACPI_DB_INFO, "device_bind %d\n", i));
+ acpi_handle_debug(video->device->handle, "%s: %d\n",
+ __func__, i);
}
}
}
@@ -1325,19 +1322,21 @@ static int acpi_video_device_enumerate(s

status = acpi_evaluate_object(video->device->handle, "_DOD", NULL, &buffer);
if (!ACPI_SUCCESS(status)) {
- ACPI_EXCEPTION((AE_INFO, status, "Evaluating _DOD"));
+ acpi_handle_info(video->device->handle,
+ "_DOD evaluation failed: %s\n",
+ acpi_format_exception(status));
return status;
}

dod = buffer.pointer;
if (!dod || (dod->type != ACPI_TYPE_PACKAGE)) {
- ACPI_EXCEPTION((AE_INFO, status, "Invalid _DOD data"));
+ acpi_handle_info(video->device->handle, "Invalid _DOD data\n");
status = -EFAULT;
goto out;
}

- ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %d video heads in _DOD\n",
- dod->package.count));
+ acpi_handle_debug(video->device->handle, "Found %d video heads in _DOD\n",
+ dod->package.count);

active_list = kcalloc(1 + dod->package.count,
sizeof(struct acpi_video_enumerated_device),
@@ -1352,15 +1351,18 @@ static int acpi_video_device_enumerate(s
obj = &dod->package.elements[i];

if (obj->type != ACPI_TYPE_INTEGER) {
- printk(KERN_ERR PREFIX
- "Invalid _DOD data in element %d\n", i);
+ acpi_handle_info(video->device->handle,
+ "Invalid _DOD data in element %d\n", i);
continue;
}

active_list[count].value.int_val = obj->integer.value;
active_list[count].bind_info = NULL;
- ACPI_DEBUG_PRINT((ACPI_DB_INFO, "dod element[%d] = %d\n", i,
- (int)obj->integer.value));
+
+ acpi_handle_debug(video->device->handle,
+ "_DOD element[%d] = %d\n", i,
+ (int)obj->integer.value);
+
count++;
}

@@ -1451,7 +1453,8 @@ acpi_video_switch_brightness(struct work

out:
if (result)
- printk(KERN_ERR PREFIX "Failed to switch the brightness\n");
+ acpi_handle_info(device->dev->handle,
+ "Failed to switch brightness\n");
}

int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
@@ -1601,8 +1604,8 @@ static void acpi_video_bus_notify(struct
break;

default:
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "Unsupported event [0x%x]\n", event));
+ acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
+ event);
break;
}

@@ -1675,8 +1678,7 @@ static void acpi_video_device_notify(acp
keycode = KEY_DISPLAY_OFF;
break;
default:
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "Unsupported event [0x%x]\n", event));
+ acpi_handle_debug(handle, "Unsupported event [0x%x]\n", event);
break;
}

@@ -1812,11 +1814,12 @@ static void acpi_video_dev_register_back
&device->cooling_dev->device.kobj,
"thermal_cooling");
if (result)
- printk(KERN_ERR PREFIX "Create sysfs link\n");
+ pr_info("sysfs link creation failed\n");
+
result = sysfs_create_link(&device->cooling_dev->device.kobj,
&device->dev->dev.kobj, "device");
if (result)
- printk(KERN_ERR PREFIX "Create sysfs link\n");
+ pr_info("Reverse sysfs link creation failed\n");
}

static void acpi_video_run_bcl_for_osi(struct acpi_video_bus *video)
@@ -2030,7 +2033,7 @@ static int acpi_video_bus_add(struct acp
acpi_video_bus_match, NULL,
device, NULL);
if (status == AE_ALREADY_EXISTS) {
- printk(KERN_WARNING FW_BUG
+ pr_info(FW_BUG
"Duplicate ACPI video bus devices for the"
" same VGA controller, please try module "
"parameter \"video.allow_duplicates=1\""
@@ -2073,7 +2076,7 @@ static int acpi_video_bus_add(struct acp
if (error)
goto err_put_video;

- printk(KERN_INFO PREFIX "%s [%s] (multi-head: %s rom: %s post: %s)\n",
+ pr_info("%s [%s] (multi-head: %s rom: %s post: %s)\n",
ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device),
video->flags.multihead ? "yes" : "no",
video->flags.rom ? "yes" : "no",
Index: linux-pm/Documentation/firmware-guide/acpi/debug.rst
===================================================================
--- linux-pm.orig/Documentation/firmware-guide/acpi/debug.rst
+++ linux-pm/Documentation/firmware-guide/acpi/debug.rst
@@ -59,7 +59,6 @@ shows the supported mask values, current
ACPI_SYSTEM_COMPONENT 0x02000000
ACPI_THERMAL_COMPONENT 0x04000000
ACPI_MEMORY_DEVICE_COMPONENT 0x08000000
- ACPI_VIDEO_COMPONENT 0x10000000
ACPI_PROCESSOR_COMPONENT 0x20000000

debug_level
Index: linux-pm/drivers/acpi/sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/sysfs.c
+++ linux-pm/drivers/acpi/sysfs.c
@@ -59,7 +59,6 @@ static const struct acpi_dlayer acpi_deb
ACPI_DEBUG_INIT(ACPI_SYSTEM_COMPONENT),
ACPI_DEBUG_INIT(ACPI_THERMAL_COMPONENT),
ACPI_DEBUG_INIT(ACPI_MEMORY_DEVICE_COMPONENT),
- ACPI_DEBUG_INIT(ACPI_VIDEO_COMPONENT),
ACPI_DEBUG_INIT(ACPI_PROCESSOR_COMPONENT),
};

Index: linux-pm/include/acpi/acpi_drivers.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_drivers.h
+++ linux-pm/include/acpi/acpi_drivers.h
@@ -22,7 +22,6 @@
#define ACPI_SYSTEM_COMPONENT 0x02000000
#define ACPI_THERMAL_COMPONENT 0x04000000
#define ACPI_MEMORY_DEVICE_COMPONENT 0x08000000
-#define ACPI_VIDEO_COMPONENT 0x10000000
#define ACPI_PROCESSOR_COMPONENT 0x20000000

/*



2021-02-01 18:30:05

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH v1 5/5] ACPI: thermal: Clean up printing messages

From: Rafael J. Wysocki <[email protected]>

Replace the ACPI_DEBUG_PRINT() instances in thermal.c with
acpi_handle_debug() calls and modify the ACPI_THERMAL_TRIPS_EXCEPTION()
macro in there to use acpi_handle_info() internally.

Drop the _COMPONENT and ACPI_MODULE_NAME() definitions that are not
used any more from thermal.c, drop the no longer needed
ACPI_THERMAL_COMPONENT definition from the headers and update the
documentation accordingly.

While at it, add a pr_fmt() definition to thermal.c, drop the PREFIX
definition from there and replace some pr_warn() calls with pr_info()
or acpi_handle_info().

Signed-off-by: Rafael J. Wysocki <[email protected]>
---
Documentation/firmware-guide/acpi/debug.rst | 1
drivers/acpi/sysfs.c | 1
drivers/acpi/thermal.c | 87 +++++++++++++---------------
include/acpi/acpi_drivers.h | 1
4 files changed, 43 insertions(+), 47 deletions(-)

Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -13,6 +13,8 @@
* concepts of 'multiple limiters', upper/lower limits, etc.
*/

+#define pr_fmt(fmt) "ACPI: thermal: " fmt
+
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/dmi.h>
@@ -29,8 +31,6 @@
#include <linux/uaccess.h>
#include <linux/units.h>

-#define PREFIX "ACPI: "
-
#define ACPI_THERMAL_CLASS "thermal_zone"
#define ACPI_THERMAL_DEVICE_NAME "Thermal Zone"
#define ACPI_THERMAL_NOTIFY_TEMPERATURE 0x80
@@ -43,9 +43,6 @@
#define ACPI_THERMAL_MAX_ACTIVE 10
#define ACPI_THERMAL_MAX_LIMIT_STR_LEN 65

-#define _COMPONENT ACPI_THERMAL_COMPONENT
-ACPI_MODULE_NAME("thermal");
-
MODULE_AUTHOR("Paul Diefenbaugh");
MODULE_DESCRIPTION("ACPI Thermal Zone Driver");
MODULE_LICENSE("GPL");
@@ -197,8 +194,9 @@ static int acpi_thermal_get_temperature(
return -ENODEV;

tz->temperature = tmp;
- ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Temperature is %lu dK\n",
- tz->temperature));
+
+ acpi_handle_debug(tz->device->handle, "Temperature is %lu dK\n",
+ tz->temperature);

return 0;
}
@@ -216,8 +214,8 @@ static int acpi_thermal_get_polling_freq
return -ENODEV;

tz->polling_frequency = tmp;
- ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Polling frequency is %lu dS\n",
- tz->polling_frequency));
+ acpi_handle_debug(tz->device->handle, "Polling frequency is %lu dS\n",
+ tz->polling_frequency);

return 0;
}
@@ -254,12 +252,12 @@ static int acpi_thermal_set_cooling_mode
* 2.TODO: Devices listed in _PSL, _ALx, _TZD may change.
* We need to re-bind the cooling devices of a thermal zone when this occurs.
*/
-#define ACPI_THERMAL_TRIPS_EXCEPTION(flags, str) \
+#define ACPI_THERMAL_TRIPS_EXCEPTION(flags, tz, str) \
do { \
if (flags != ACPI_TRIPS_INIT) \
- ACPI_EXCEPTION((AE_INFO, AE_ERROR, \
+ acpi_handle_info(tz->device->handle, \
"ACPI thermal trip point %s changed\n" \
- "Please send acpidump to [email protected]", str)); \
+ "Please report to [email protected]\n", str); \
} while (0)

static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag)
@@ -283,17 +281,17 @@ static int acpi_thermal_trips_update(str
*/
if (ACPI_FAILURE(status)) {
tz->trips.critical.flags.valid = 0;
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "No critical threshold\n"));
+ acpi_handle_debug(tz->device->handle,
+ "No critical threshold\n");
} else if (tmp <= 2732) {
- pr_warn(FW_BUG "Invalid critical threshold (%llu)\n",
+ pr_info(FW_BUG "Invalid critical threshold (%llu)\n",
tmp);
tz->trips.critical.flags.valid = 0;
} else {
tz->trips.critical.flags.valid = 1;
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+ acpi_handle_debug(tz->device->handle,
"Found critical threshold [%lu]\n",
- tz->trips.critical.temperature));
+ tz->trips.critical.temperature);
}
if (tz->trips.critical.flags.valid == 1) {
if (crt == -1) {
@@ -305,8 +303,8 @@ static int acpi_thermal_trips_update(str
* Allow override critical threshold
*/
if (crt_k > tz->trips.critical.temperature)
- pr_warn(PREFIX "Critical threshold %d C\n",
- crt);
+ pr_info("Critical threshold %d C\n", crt);
+
tz->trips.critical.temperature = crt_k;
}
}
@@ -318,14 +316,14 @@ static int acpi_thermal_trips_update(str
"_HOT", NULL, &tmp);
if (ACPI_FAILURE(status)) {
tz->trips.hot.flags.valid = 0;
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "No hot threshold\n"));
+ acpi_handle_debug(tz->device->handle,
+ "No hot threshold\n");
} else {
tz->trips.hot.temperature = tmp;
tz->trips.hot.flags.valid = 1;
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "Found hot threshold [%lu]\n",
- tz->trips.hot.temperature));
+ acpi_handle_debug(tz->device->handle,
+ "Found hot threshold [%lu]\n",
+ tz->trips.hot.temperature);
}
}

@@ -378,7 +376,8 @@ static int acpi_thermal_trips_update(str
status = acpi_evaluate_reference(tz->device->handle, "_PSL",
NULL, &devices);
if (ACPI_FAILURE(status)) {
- pr_warn(PREFIX "Invalid passive threshold\n");
+ acpi_handle_info(tz->device->handle,
+ "Invalid passive threshold\n");
tz->trips.passive.flags.valid = 0;
}
else
@@ -388,12 +387,12 @@ static int acpi_thermal_trips_update(str
sizeof(struct acpi_handle_list))) {
memcpy(&tz->trips.passive.devices, &devices,
sizeof(struct acpi_handle_list));
- ACPI_THERMAL_TRIPS_EXCEPTION(flag, "device");
+ ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device");
}
}
if ((flag & ACPI_TRIPS_PASSIVE) || (flag & ACPI_TRIPS_DEVICES)) {
if (valid != tz->trips.passive.flags.valid)
- ACPI_THERMAL_TRIPS_EXCEPTION(flag, "state");
+ ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "state");
}

/* Active (optional) */
@@ -440,8 +439,8 @@ static int acpi_thermal_trips_update(str
status = acpi_evaluate_reference(tz->device->handle,
name, NULL, &devices);
if (ACPI_FAILURE(status)) {
- pr_warn(PREFIX "Invalid active%d threshold\n",
- i);
+ acpi_handle_info(tz->device->handle,
+ "Invalid active%d threshold\n", i);
tz->trips.active[i].flags.valid = 0;
}
else
@@ -451,12 +450,12 @@ static int acpi_thermal_trips_update(str
sizeof(struct acpi_handle_list))) {
memcpy(&tz->trips.active[i].devices, &devices,
sizeof(struct acpi_handle_list));
- ACPI_THERMAL_TRIPS_EXCEPTION(flag, "device");
+ ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device");
}
}
if ((flag & ACPI_TRIPS_ACTIVE) || (flag & ACPI_TRIPS_DEVICES))
if (valid != tz->trips.active[i].flags.valid)
- ACPI_THERMAL_TRIPS_EXCEPTION(flag, "state");
+ ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "state");

if (!tz->trips.active[i].flags.valid)
break;
@@ -469,7 +468,7 @@ static int acpi_thermal_trips_update(str
if (ACPI_SUCCESS(status)
&& memcmp(&tz->devices, &devices, sizeof(devices))) {
tz->devices = devices;
- ACPI_THERMAL_TRIPS_EXCEPTION(flag, "device");
+ ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device");
}
}

@@ -925,8 +924,8 @@ static void acpi_thermal_notify(struct a
dev_name(&device->dev), event, 0);
break;
default:
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "Unsupported event [0x%x]\n", event));
+ acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
+ event);
break;
}
}
@@ -1074,7 +1073,7 @@ static int acpi_thermal_add(struct acpi_
mutex_init(&tz->thermal_check_lock);
INIT_WORK(&tz->thermal_check_work, acpi_thermal_check_fn);

- pr_info(PREFIX "%s [%s] (%ld C)\n", acpi_device_name(device),
+ pr_info("%s [%s] (%ld C)\n", acpi_device_name(device),
acpi_device_bid(device), deci_kelvin_to_celsius(tz->temperature));
goto end;

@@ -1146,24 +1145,24 @@ static int acpi_thermal_resume(struct de
static int thermal_act(const struct dmi_system_id *d) {

if (act == 0) {
- pr_notice(PREFIX "%s detected: "
- "disabling all active thermal trip points\n", d->ident);
+ pr_notice("%s detected: disabling all active thermal trip points\n",
+ d->ident);
act = -1;
}
return 0;
}
static int thermal_nocrt(const struct dmi_system_id *d) {

- pr_notice(PREFIX "%s detected: "
- "disabling all critical thermal trip point actions.\n", d->ident);
+ pr_notice("%s detected: disabling all critical thermal trip point actions.\n",
+ d->ident);
nocrt = 1;
return 0;
}
static int thermal_tzp(const struct dmi_system_id *d) {

if (tzp == 0) {
- pr_notice(PREFIX "%s detected: "
- "enabling thermal zone polling\n", d->ident);
+ pr_notice("%s detected: enabling thermal zone polling\n",
+ d->ident);
tzp = 300; /* 300 dS = 30 Seconds */
}
return 0;
@@ -1171,8 +1170,8 @@ static int thermal_tzp(const struct dmi_
static int thermal_psv(const struct dmi_system_id *d) {

if (psv == 0) {
- pr_notice(PREFIX "%s detected: "
- "disabling all passive thermal trip points\n", d->ident);
+ pr_notice("%s detected: disabling all passive thermal trip points\n",
+ d->ident);
psv = -1;
}
return 0;
@@ -1225,7 +1224,7 @@ static int __init acpi_thermal_init(void
dmi_check_system(thermal_dmi_table);

if (off) {
- pr_notice(PREFIX "thermal control disabled\n");
+ pr_notice("thermal control disabled\n");
return -ENODEV;
}

Index: linux-pm/Documentation/firmware-guide/acpi/debug.rst
===================================================================
--- linux-pm.orig/Documentation/firmware-guide/acpi/debug.rst
+++ linux-pm/Documentation/firmware-guide/acpi/debug.rst
@@ -57,7 +57,6 @@ shows the supported mask values, current
ACPI_PCI_COMPONENT 0x00400000
ACPI_CONTAINER_COMPONENT 0x01000000
ACPI_SYSTEM_COMPONENT 0x02000000
- ACPI_THERMAL_COMPONENT 0x04000000
ACPI_MEMORY_DEVICE_COMPONENT 0x08000000
ACPI_PROCESSOR_COMPONENT 0x20000000

Index: linux-pm/drivers/acpi/sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/sysfs.c
+++ linux-pm/drivers/acpi/sysfs.c
@@ -57,7 +57,6 @@ static const struct acpi_dlayer acpi_deb
ACPI_DEBUG_INIT(ACPI_PCI_COMPONENT),
ACPI_DEBUG_INIT(ACPI_CONTAINER_COMPONENT),
ACPI_DEBUG_INIT(ACPI_SYSTEM_COMPONENT),
- ACPI_DEBUG_INIT(ACPI_THERMAL_COMPONENT),
ACPI_DEBUG_INIT(ACPI_MEMORY_DEVICE_COMPONENT),
ACPI_DEBUG_INIT(ACPI_PROCESSOR_COMPONENT),
};
Index: linux-pm/include/acpi/acpi_drivers.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_drivers.h
+++ linux-pm/include/acpi/acpi_drivers.h
@@ -20,7 +20,6 @@
#define ACPI_PCI_COMPONENT 0x00400000
#define ACPI_CONTAINER_COMPONENT 0x01000000
#define ACPI_SYSTEM_COMPONENT 0x02000000
-#define ACPI_THERMAL_COMPONENT 0x04000000
#define ACPI_MEMORY_DEVICE_COMPONENT 0x08000000
#define ACPI_PROCESSOR_COMPONENT 0x20000000




2021-02-01 18:39:19

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH v1 2/5] ACPI: battery: Clean up printing messages

On Mon, 2021-02-01 at 19:16 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Replace the ACPI_DEBUG_PRINT() and ACPI_EXCEPTION() instances
> in battery.c with acpi_handle_debug() and acpi_handle_info() calls,
> respectively, drop the _COMPONENT and ACPI_MODULE_NAME() definitions
> that are not used any more, drop the no longer needed
> ACPI_BATTERY_COMPONENT definition from the headers and update the
> documentation accordingly.
>
> While at it, update the pr_fmt() definition and drop the unneeded
> PREFIX sybmbol definition from battery.c.
[]
> --- linux-pm.orig/drivers/acpi/battery.c
[]
> @@ -466,7 +460,8 @@ static int extract_package(struct acpi_b
> ?static int acpi_battery_get_status(struct acpi_battery *battery)
> ?{
> ? if (acpi_bus_get_status(battery->device)) {
> - ACPI_EXCEPTION((AE_INFO, AE_ERROR, "Evaluating _STA"));
> + acpi_handle_info(battery->device->handle,
> + "_STA evaluation failed\n");

I believe this changes the logging level from KERN_ERR to KERN_INFO.

Perhaps this and others should instead use acpi_handle_err()


2021-02-01 18:47:01

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH v1 2/5] ACPI: battery: Clean up printing messages

On Mon, Feb 1, 2021 at 7:37 PM Joe Perches <[email protected]> wrote:
>
> On Mon, 2021-02-01 at 19:16 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Replace the ACPI_DEBUG_PRINT() and ACPI_EXCEPTION() instances
> > in battery.c with acpi_handle_debug() and acpi_handle_info() calls,
> > respectively, drop the _COMPONENT and ACPI_MODULE_NAME() definitions
> > that are not used any more, drop the no longer needed
> > ACPI_BATTERY_COMPONENT definition from the headers and update the
> > documentation accordingly.
> >
> > While at it, update the pr_fmt() definition and drop the unneeded
> > PREFIX sybmbol definition from battery.c.
> []
> > --- linux-pm.orig/drivers/acpi/battery.c
> []
> > @@ -466,7 +460,8 @@ static int extract_package(struct acpi_b
> > static int acpi_battery_get_status(struct acpi_battery *battery)
> > {
> > if (acpi_bus_get_status(battery->device)) {
> > - ACPI_EXCEPTION((AE_INFO, AE_ERROR, "Evaluating _STA"));
> > + acpi_handle_info(battery->device->handle,
> > + "_STA evaluation failed\n");
>
> I believe this changes the logging level from KERN_ERR to KERN_INFO.
>
> Perhaps this and others should instead use acpi_handle_err()

Actually, these log level changes, because the messages in question
are not very urgent.

Something doesn't work and it's kind of good to know that, but there's
not much that can be done about it.

2021-02-02 21:42:58

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH v1 2/5] ACPI: battery: Clean up printing messages

On Mon, 2021-02-01 at 19:44 +0100, Rafael J. Wysocki wrote:
> On Mon, Feb 1, 2021 at 7:37 PM Joe Perches <[email protected]> wrote:
> >
> > On Mon, 2021-02-01 at 19:16 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > Replace the ACPI_DEBUG_PRINT() and ACPI_EXCEPTION() instances
> > > in battery.c with acpi_handle_debug() and acpi_handle_info() calls,
> > > respectively, drop the _COMPONENT and ACPI_MODULE_NAME() definitions
> > > that are not used any more, drop the no longer needed
> > > ACPI_BATTERY_COMPONENT definition from the headers and update the
> > > documentation accordingly.
> > >
> > > While at it, update the pr_fmt() definition and drop the unneeded
> > > PREFIX sybmbol definition from battery.c.
> > []
> > > --- linux-pm.orig/drivers/acpi/battery.c
> > []
> > > @@ -466,7 +460,8 @@ static int extract_package(struct acpi_b
> > > ?static int acpi_battery_get_status(struct acpi_battery *battery)
> > > ?{
> > > ??????if (acpi_bus_get_status(battery->device)) {
> > > - ACPI_EXCEPTION((AE_INFO, AE_ERROR, "Evaluating _STA"));
> > > + acpi_handle_info(battery->device->handle,
> > > + "_STA evaluation failed\n");
> >
> > I believe this changes the logging level from KERN_ERR to KERN_INFO.
> >
> > Perhaps this and others should instead use acpi_handle_err()
>
> Actually, these log level changes, because the messages in question
> are not very urgent.
>
> Something doesn't work and it's kind of good to know that, but there's
> not much that can be done about it.

That more argues for removal of KERN_<LEVEL> filtering.

I fail to see how difficult it is to change these to the existing
KERN_<LEVEL> using a simple acpi_handle_info() -> acpi_handle_err()
substitution where appropriate.

At a minimum, the commit message should note the KERN_<LEVEL> changes.

2021-02-02 22:23:36

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH v1 2/5] ACPI: battery: Clean up printing messages

On Tue, Feb 2, 2021 at 2:38 PM Joe Perches <[email protected]> wrote:
>
> On Mon, 2021-02-01 at 19:44 +0100, Rafael J. Wysocki wrote:
> > On Mon, Feb 1, 2021 at 7:37 PM Joe Perches <[email protected]> wrote:
> > >
> > > On Mon, 2021-02-01 at 19:16 +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <[email protected]>
> > > >
> > > > Replace the ACPI_DEBUG_PRINT() and ACPI_EXCEPTION() instances
> > > > in battery.c with acpi_handle_debug() and acpi_handle_info() calls,
> > > > respectively, drop the _COMPONENT and ACPI_MODULE_NAME() definitions
> > > > that are not used any more, drop the no longer needed
> > > > ACPI_BATTERY_COMPONENT definition from the headers and update the
> > > > documentation accordingly.
> > > >
> > > > While at it, update the pr_fmt() definition and drop the unneeded
> > > > PREFIX sybmbol definition from battery.c.
> > > []
> > > > --- linux-pm.orig/drivers/acpi/battery.c
> > > []
> > > > @@ -466,7 +460,8 @@ static int extract_package(struct acpi_b
> > > > static int acpi_battery_get_status(struct acpi_battery *battery)
> > > > {
> > > > if (acpi_bus_get_status(battery->device)) {
> > > > - ACPI_EXCEPTION((AE_INFO, AE_ERROR, "Evaluating _STA"));
> > > > + acpi_handle_info(battery->device->handle,
> > > > + "_STA evaluation failed\n");
> > >
> > > I believe this changes the logging level from KERN_ERR to KERN_INFO.
> > >
> > > Perhaps this and others should instead use acpi_handle_err()
> >
> > Actually, these log level changes, because the messages in question
> > are not very urgent.
> >
> > Something doesn't work and it's kind of good to know that, but there's
> > not much that can be done about it.
>
> That more argues for removal of KERN_<LEVEL> filtering.
>
> I fail to see how difficult it is to change these to the existing
> KERN_<LEVEL> using a simple acpi_handle_info() -> acpi_handle_err()
> substitution where appropriate.

I'm not really sure what you mean.

It is not a technical problem, but in my view the KERN_ERR log level
is excessive for these messages.

> At a minimum, the commit message should note the KERN_<LEVEL> changes.

OK, let me update the changelogs, then.

2021-02-03 00:38:45

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH v2 0/5] ACPI: More cleanups related to printing messages

Hi All,

On Monday, February 1, 2021 7:14:38 PM CET Rafael J. Wysocki wrote:
>
> This series is a continuation of the effort to drop ACPICA-specific debug
> code from non-ACPICA pieces of the ACPI subsystem and to make the message
> printing in there more consistent.
>
> The patches in this series are based on linux-next from today.
>
> Details in the patch changelogs.

Sending a v2 with updated changelogs to address review comments from Joe.

The code changes are the same as before in all of the patches.

Thanks!



2021-02-03 18:57:28

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH v3 0/5] ACPI: More cleanups related to printing messages

Hi All,

On Tuesday, February 2, 2021 7:11:47 PM CET Rafael J. Wysocki wrote:
>
> On Monday, February 1, 2021 7:14:38 PM CET Rafael J. Wysocki wrote:
> >
> > This series is a continuation of the effort to drop ACPICA-specific debug
> > code from non-ACPICA pieces of the ACPI subsystem and to make the message
> > printing in there more consistent.
> >
> > The patches in this series are based on linux-next from today.
> >
> > Details in the patch changelogs.
>
> Sending a v2 with updated changelogs to address review comments from Joe.
>
> The code changes are the same as before in all of the patches.

Sending a v3 to address review comments from Hanjun.

Thanks!



2021-02-03 18:57:28

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH v3 1/5] ACPI: AC: Clean up printing messages

From: Rafael J. Wysocki <[email protected]>

Replace the ACPI_DEBUG_PRINT() and ACPI_EXCEPTION() instances
in ac.c with acpi_handle_debug() and acpi_handle_info() calls,
respectively, which among other things causes the excessive log
level of the messages previously printed via ACPI_EXCEPTION() to
be increased.

Drop the _COMPONENT and ACPI_MODULE_NAME() definitions that are not
used any more, drop the no longer needed ACPI_AC_COMPONENT definition
from the headers and update the documentation accordingly.

While at it, replace the direct printk() invocation with pr_info(),
add a pr_fmt() definition to ac.c and drop the unneeded PREFIX
symbol definition from there.

Signed-off-by: Rafael J. Wysocki <[email protected]>
---

v2 -> v3: Also add a pr_fmt() definition to ac.c and replace direct
printk() with pr_info (no log level change).

v1 -> v2: Changelog update.

---
Documentation/firmware-guide/acpi/debug.rst | 1 -
drivers/acpi/ac.c | 23 ++++++++++-------------
drivers/acpi/sysfs.c | 1 -
include/acpi/acpi_drivers.h | 1 -
4 files changed, 10 insertions(+), 16 deletions(-)

Index: linux-pm/Documentation/firmware-guide/acpi/debug.rst
===================================================================
--- linux-pm.orig/Documentation/firmware-guide/acpi/debug.rst
+++ linux-pm/Documentation/firmware-guide/acpi/debug.rst
@@ -52,7 +52,6 @@ shows the supported mask values, current
ACPI_CA_DISASSEMBLER 0x00000800
ACPI_COMPILER 0x00001000
ACPI_TOOLS 0x00002000
- ACPI_AC_COMPONENT 0x00020000
ACPI_BATTERY_COMPONENT 0x00040000
ACPI_BUTTON_COMPONENT 0x00080000
ACPI_SBS_COMPONENT 0x00100000
Index: linux-pm/drivers/acpi/ac.c
===================================================================
--- linux-pm.orig/drivers/acpi/ac.c
+++ linux-pm/drivers/acpi/ac.c
@@ -6,6 +6,8 @@
* Copyright (C) 2001, 2002 Paul Diefenbaugh <[email protected]>
*/

+#define pr_fmt(fmt) "ACPI: AC: " fmt
+
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/slab.h>
@@ -18,8 +20,6 @@
#include <linux/acpi.h>
#include <acpi/battery.h>

-#define PREFIX "ACPI: "
-
#define ACPI_AC_CLASS "ac_adapter"
#define ACPI_AC_DEVICE_NAME "AC Adapter"
#define ACPI_AC_FILE_STATE "state"
@@ -28,9 +28,6 @@
#define ACPI_AC_STATUS_ONLINE 0x01
#define ACPI_AC_STATUS_UNKNOWN 0xFF

-#define _COMPONENT ACPI_AC_COMPONENT
-ACPI_MODULE_NAME("ac");
-
MODULE_AUTHOR("Paul Diefenbaugh");
MODULE_DESCRIPTION("ACPI AC Adapter Driver");
MODULE_LICENSE("GPL");
@@ -102,8 +99,9 @@ static int acpi_ac_get_state(struct acpi
status = acpi_evaluate_integer(ac->device->handle, "_PSR", NULL,
&ac->state);
if (ACPI_FAILURE(status)) {
- ACPI_EXCEPTION((AE_INFO, status,
- "Error reading AC Adapter state"));
+ acpi_handle_info(ac->device->handle,
+ "Error reading AC Adapter state: %s\n",
+ acpi_format_exception(status));
ac->state = ACPI_AC_STATUS_UNKNOWN;
return -ENODEV;
}
@@ -153,8 +151,8 @@ static void acpi_ac_notify(struct acpi_d

switch (event) {
default:
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "Unsupported event [0x%x]\n", event));
+ acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
+ event);
fallthrough;
case ACPI_AC_NOTIFY_STATUS:
case ACPI_NOTIFY_BUS_CHECK:
@@ -278,9 +276,8 @@ static int acpi_ac_add(struct acpi_devic
goto end;
}

- printk(KERN_INFO PREFIX "%s [%s] (%s)\n",
- acpi_device_name(device), acpi_device_bid(device),
- ac->state ? "on-line" : "off-line");
+ pr_info("%s [%s] (%s)\n", acpi_device_name(device),
+ acpi_device_bid(device), ac->state ? "on-line" : "off-line");

ac->battery_nb.notifier_call = acpi_ac_battery_notify;
register_acpi_notifier(&ac->battery_nb);
@@ -348,7 +345,7 @@ static int __init acpi_ac_init(void)
for (i = 0; i < ARRAY_SIZE(acpi_ac_blacklist); i++)
if (acpi_dev_present(acpi_ac_blacklist[i].hid, "1",
acpi_ac_blacklist[i].hrv)) {
- pr_info(PREFIX "AC: found native %s PMIC, not loading\n",
+ pr_info("found native %s PMIC, not loading\n",
acpi_ac_blacklist[i].hid);
return -ENODEV;
}
Index: linux-pm/drivers/acpi/sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/sysfs.c
+++ linux-pm/drivers/acpi/sysfs.c
@@ -52,7 +52,6 @@ static const struct acpi_dlayer acpi_deb
ACPI_DEBUG_INIT(ACPI_COMPILER),
ACPI_DEBUG_INIT(ACPI_TOOLS),

- ACPI_DEBUG_INIT(ACPI_AC_COMPONENT),
ACPI_DEBUG_INIT(ACPI_BATTERY_COMPONENT),
ACPI_DEBUG_INIT(ACPI_BUTTON_COMPONENT),
ACPI_DEBUG_INIT(ACPI_SBS_COMPONENT),
Index: linux-pm/include/acpi/acpi_drivers.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_drivers.h
+++ linux-pm/include/acpi/acpi_drivers.h
@@ -15,7 +15,6 @@
* Please update drivers/acpi/debug.c and Documentation/firmware-guide/acpi/debug.rst
* if you add to this list.
*/
-#define ACPI_AC_COMPONENT 0x00020000
#define ACPI_BATTERY_COMPONENT 0x00040000
#define ACPI_BUTTON_COMPONENT 0x00080000
#define ACPI_SBS_COMPONENT 0x00100000



2021-02-04 01:15:41

by Hanjun Guo

[permalink] [raw]
Subject: Re: [PATCH v3 1/5] ACPI: AC: Clean up printing messages

On 2021/2/4 2:43, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Replace the ACPI_DEBUG_PRINT() and ACPI_EXCEPTION() instances
> in ac.c with acpi_handle_debug() and acpi_handle_info() calls,
> respectively, which among other things causes the excessive log
> level of the messages previously printed via ACPI_EXCEPTION() to
> be increased.
>
> Drop the _COMPONENT and ACPI_MODULE_NAME() definitions that are not
> used any more, drop the no longer needed ACPI_AC_COMPONENT definition
> from the headers and update the documentation accordingly.
>
> While at it, replace the direct printk() invocation with pr_info(),
> add a pr_fmt() definition to ac.c and drop the unneeded PREFIX
> symbol definition from there.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> v2 -> v3: Also add a pr_fmt() definition to ac.c and replace direct
> printk() with pr_info (no log level change).

Reviewed-by: Hanjun Guo <[email protected]>

2021-02-04 18:31:34

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH v3 1/5] ACPI: AC: Clean up printing messages

Hi,

On 2/3/21 7:43 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Replace the ACPI_DEBUG_PRINT() and ACPI_EXCEPTION() instances
> in ac.c with acpi_handle_debug() and acpi_handle_info() calls,
> respectively, which among other things causes the excessive log
> level of the messages previously printed via ACPI_EXCEPTION() to
> be increased.
>
> Drop the _COMPONENT and ACPI_MODULE_NAME() definitions that are not
> used any more, drop the no longer needed ACPI_AC_COMPONENT definition
> from the headers and update the documentation accordingly.
>
> While at it, replace the direct printk() invocation with pr_info(),
> add a pr_fmt() definition to ac.c and drop the unneeded PREFIX
> symbol definition from there.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <[email protected]>

Regards,

Hans


> ---
>
> v2 -> v3: Also add a pr_fmt() definition to ac.c and replace direct
> printk() with pr_info (no log level change).
>
> v1 -> v2: Changelog update.
>
> ---
> Documentation/firmware-guide/acpi/debug.rst | 1 -
> drivers/acpi/ac.c | 23 ++++++++++-------------
> drivers/acpi/sysfs.c | 1 -
> include/acpi/acpi_drivers.h | 1 -
> 4 files changed, 10 insertions(+), 16 deletions(-)
>
> Index: linux-pm/Documentation/firmware-guide/acpi/debug.rst
> ===================================================================
> --- linux-pm.orig/Documentation/firmware-guide/acpi/debug.rst
> +++ linux-pm/Documentation/firmware-guide/acpi/debug.rst
> @@ -52,7 +52,6 @@ shows the supported mask values, current
> ACPI_CA_DISASSEMBLER 0x00000800
> ACPI_COMPILER 0x00001000
> ACPI_TOOLS 0x00002000
> - ACPI_AC_COMPONENT 0x00020000
> ACPI_BATTERY_COMPONENT 0x00040000
> ACPI_BUTTON_COMPONENT 0x00080000
> ACPI_SBS_COMPONENT 0x00100000
> Index: linux-pm/drivers/acpi/ac.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/ac.c
> +++ linux-pm/drivers/acpi/ac.c
> @@ -6,6 +6,8 @@
> * Copyright (C) 2001, 2002 Paul Diefenbaugh <[email protected]>
> */
>
> +#define pr_fmt(fmt) "ACPI: AC: " fmt
> +
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> @@ -18,8 +20,6 @@
> #include <linux/acpi.h>
> #include <acpi/battery.h>
>
> -#define PREFIX "ACPI: "
> -
> #define ACPI_AC_CLASS "ac_adapter"
> #define ACPI_AC_DEVICE_NAME "AC Adapter"
> #define ACPI_AC_FILE_STATE "state"
> @@ -28,9 +28,6 @@
> #define ACPI_AC_STATUS_ONLINE 0x01
> #define ACPI_AC_STATUS_UNKNOWN 0xFF
>
> -#define _COMPONENT ACPI_AC_COMPONENT
> -ACPI_MODULE_NAME("ac");
> -
> MODULE_AUTHOR("Paul Diefenbaugh");
> MODULE_DESCRIPTION("ACPI AC Adapter Driver");
> MODULE_LICENSE("GPL");
> @@ -102,8 +99,9 @@ static int acpi_ac_get_state(struct acpi
> status = acpi_evaluate_integer(ac->device->handle, "_PSR", NULL,
> &ac->state);
> if (ACPI_FAILURE(status)) {
> - ACPI_EXCEPTION((AE_INFO, status,
> - "Error reading AC Adapter state"));
> + acpi_handle_info(ac->device->handle,
> + "Error reading AC Adapter state: %s\n",
> + acpi_format_exception(status));
> ac->state = ACPI_AC_STATUS_UNKNOWN;
> return -ENODEV;
> }
> @@ -153,8 +151,8 @@ static void acpi_ac_notify(struct acpi_d
>
> switch (event) {
> default:
> - ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> - "Unsupported event [0x%x]\n", event));
> + acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
> + event);
> fallthrough;
> case ACPI_AC_NOTIFY_STATUS:
> case ACPI_NOTIFY_BUS_CHECK:
> @@ -278,9 +276,8 @@ static int acpi_ac_add(struct acpi_devic
> goto end;
> }
>
> - printk(KERN_INFO PREFIX "%s [%s] (%s)\n",
> - acpi_device_name(device), acpi_device_bid(device),
> - ac->state ? "on-line" : "off-line");
> + pr_info("%s [%s] (%s)\n", acpi_device_name(device),
> + acpi_device_bid(device), ac->state ? "on-line" : "off-line");
>
> ac->battery_nb.notifier_call = acpi_ac_battery_notify;
> register_acpi_notifier(&ac->battery_nb);
> @@ -348,7 +345,7 @@ static int __init acpi_ac_init(void)
> for (i = 0; i < ARRAY_SIZE(acpi_ac_blacklist); i++)
> if (acpi_dev_present(acpi_ac_blacklist[i].hid, "1",
> acpi_ac_blacklist[i].hrv)) {
> - pr_info(PREFIX "AC: found native %s PMIC, not loading\n",
> + pr_info("found native %s PMIC, not loading\n",
> acpi_ac_blacklist[i].hid);
> return -ENODEV;
> }
> Index: linux-pm/drivers/acpi/sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sysfs.c
> +++ linux-pm/drivers/acpi/sysfs.c
> @@ -52,7 +52,6 @@ static const struct acpi_dlayer acpi_deb
> ACPI_DEBUG_INIT(ACPI_COMPILER),
> ACPI_DEBUG_INIT(ACPI_TOOLS),
>
> - ACPI_DEBUG_INIT(ACPI_AC_COMPONENT),
> ACPI_DEBUG_INIT(ACPI_BATTERY_COMPONENT),
> ACPI_DEBUG_INIT(ACPI_BUTTON_COMPONENT),
> ACPI_DEBUG_INIT(ACPI_SBS_COMPONENT),
> Index: linux-pm/include/acpi/acpi_drivers.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_drivers.h
> +++ linux-pm/include/acpi/acpi_drivers.h
> @@ -15,7 +15,6 @@
> * Please update drivers/acpi/debug.c and Documentation/firmware-guide/acpi/debug.rst
> * if you add to this list.
> */
> -#define ACPI_AC_COMPONENT 0x00020000
> #define ACPI_BATTERY_COMPONENT 0x00040000
> #define ACPI_BUTTON_COMPONENT 0x00080000
> #define ACPI_SBS_COMPONENT 0x00100000
>
>
>