| Message ID | 1996712.1aSbzQgNOC@kreacher |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Mon, Feb 1, 2021 at 7:37 PM Joe Perches <joe@perches.com> wrote: > > On Mon, 2021-02-01 at 19:16 +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > 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.
On Mon, 2021-02-01 at 19:44 +0100, Rafael J. Wysocki wrote: > On Mon, Feb 1, 2021 at 7:37 PM Joe Perches <joe@perches.com> wrote: > > > > On Mon, 2021-02-01 at 19:16 +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > 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.
On Tue, Feb 2, 2021 at 2:38 PM Joe Perches <joe@perches.com> 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 <joe@perches.com> wrote: > > > > > > On Mon, 2021-02-01 at 19:16 +0100, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > 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.
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 <paul.s.diefenbaugh@intel.com> */ -#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 <astarikovskiy@suse.de>"); 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