| Message ID | 20171220151536.GA12278@thinkpad (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Ognjen,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on platform-drivers-x86/for-next]
[also build test WARNING on v4.15-rc4 next-20171222]
[cannot apply to pm/linux-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ognjen-Galic/battery-Add-the-battery-hooking-API/20171223-144855
base: git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git for-next
config: i386-randconfig-x007-201751 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:10:0,
from drivers//acpi/battery.c:24:
drivers//acpi/battery.c: In function 'acpi_battery_exit':
include/linux/compiler.h:58:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> drivers//acpi/battery.c:1532:2: note: in expansion of macro 'if'
if (battery_driver_registered)
^~
drivers//acpi/battery.c:1534:3: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
battery_hook_exit();
^~~~~~~~~~~~~~~~~
In file included from include/linux/kernel.h:10:0,
from drivers//acpi/battery.c:24:
drivers//acpi/battery.c: At top level:
include/linux/compiler.h:64:4: warning: '______f' is static but declared in inline function 'strcpy' which is not static
______f = { \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:422:2: note: in expansion of macro 'if'
if (p_size == (size_t)-1 && q_size == (size_t)-1)
^~
include/linux/compiler.h:64:4: warning: '______f' is static but declared in inline function 'kmemdup' which is not static
______f = { \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:412:2: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:64:4: warning: '______f' is static but declared in inline function 'kmemdup' which is not static
______f = { \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:410:2: note: in expansion of macro 'if'
if (__builtin_constant_p(size) && p_size < size)
^~
include/linux/compiler.h:64:4: warning: '______f' is static but declared in inline function 'memchr_inv' which is not static
______f = { \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:401:2: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:64:4: warning: '______f' is static but declared in inline function 'memchr_inv' which is not static
______f = { \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:399:2: note: in expansion of macro 'if'
if (__builtin_constant_p(size) && p_size < size)
^~
include/linux/compiler.h:64:4: warning: '______f' is static but declared in inline function 'memchr' which is not static
______f = { \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:390:2: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:64:4: warning: '______f' is static but declared in inline function 'memchr' which is not static
______f = { \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:388:2: note: in expansion of macro 'if'
if (__builtin_constant_p(size) && p_size < size)
^~
include/linux/compiler.h:64:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:380:2: note: in expansion of macro 'if'
if (p_size < size || q_size < size)
^~
include/linux/compiler.h:64:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:377:3: note: in expansion of macro 'if'
if (q_size < size)
^~
include/linux/compiler.h:64:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:375:3: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:64:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
vim +/if +1532 drivers//acpi/battery.c
^1da177e Linus Torvalds 2005-04-16 1528
4be44fcd Len Brown 2005-08-05 1529 static void __exit acpi_battery_exit(void)
^1da177e Linus Torvalds 2005-04-16 1530 {
5dfa0c73 Chris Wilson 2016-05-19 1531 async_synchronize_cookie(async_cookie + 1);
bc39fbcf Hans de Goede 2017-04-19 @1532 if (battery_driver_registered)
^1da177e Linus Torvalds 2005-04-16 1533 acpi_bus_unregister_driver(&acpi_battery_driver);
19380b8e Ognjen Galic 2017-12-20 1534 battery_hook_exit();
3a670cc7 Lan Tianyu 2014-05-04 1535 #ifdef CONFIG_ACPI_PROCFS_POWER
bc39fbcf Hans de Goede 2017-04-19 1536 if (acpi_battery_dir)
3a670cc7 Lan Tianyu 2014-05-04 1537 acpi_unlock_battery_dir(acpi_battery_dir);
3a670cc7 Lan Tianyu 2014-05-04 1538 #endif
^1da177e Linus Torvalds 2005-04-16 1539 }
^1da177e Linus Torvalds 2005-04-16 1540
:::::: The code at line 1532 was first introduced by commit
:::::: bc39fbcf9c782970263bdc5b428e4a755db16efb ACPI / battery: Fix acpi_battery_exit on acpi_battery_init_async errors
:::::: TO: Hans de Goede <hdegoede@redhat.com>
:::::: CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Ognjen,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on platform-drivers-x86/for-next]
[also build test WARNING on v4.15-rc4 next-20171222]
[cannot apply to pm/linux-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ognjen-Galic/battery-Add-the-battery-hooking-API/20171223-144855
base: git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git for-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
drivers/acpi/battery.c: In function 'acpi_battery_exit':
>> drivers/acpi/battery.c:1532:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (battery_driver_registered)
^~
drivers/acpi/battery.c:1534:3: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
battery_hook_exit();
^~~~~~~~~~~~~~~~~
vim +/if +1532 drivers/acpi/battery.c
^1da177e Linus Torvalds 2005-04-16 1528
4be44fcd Len Brown 2005-08-05 1529 static void __exit acpi_battery_exit(void)
^1da177e Linus Torvalds 2005-04-16 1530 {
5dfa0c73 Chris Wilson 2016-05-19 1531 async_synchronize_cookie(async_cookie + 1);
bc39fbcf Hans de Goede 2017-04-19 @1532 if (battery_driver_registered)
^1da177e Linus Torvalds 2005-04-16 1533 acpi_bus_unregister_driver(&acpi_battery_driver);
19380b8e Ognjen Galic 2017-12-20 1534 battery_hook_exit();
3a670cc7 Lan Tianyu 2014-05-04 1535 #ifdef CONFIG_ACPI_PROCFS_POWER
bc39fbcf Hans de Goede 2017-04-19 1536 if (acpi_battery_dir)
3a670cc7 Lan Tianyu 2014-05-04 1537 acpi_unlock_battery_dir(acpi_battery_dir);
3a670cc7 Lan Tianyu 2014-05-04 1538 #endif
^1da177e Linus Torvalds 2005-04-16 1539 }
^1da177e Linus Torvalds 2005-04-16 1540
:::::: The code at line 1532 was first introduced by commit
:::::: bc39fbcf9c782970263bdc5b428e4a755db16efb ACPI / battery: Fix acpi_battery_exit on acpi_battery_init_async errors
:::::: TO: Hans de Goede <hdegoede@redhat.com>
:::::: CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Ognjen,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on platform-drivers-x86/for-next]
[also build test WARNING on v4.15-rc4 next-20171222]
[cannot apply to pm/linux-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ognjen-Galic/battery-Add-the-battery-hooking-API/20171223-144855
base: git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git for-next
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c index 47a7ed5..2d8de2f 100644 --- a/drivers/acpi/ac.c +++ b/drivers/acpi/ac.c @@ -33,7 +33,7 @@ #include <linux/platform_device.h> #include <linux/power_supply.h> #include <linux/acpi.h> -#include "battery.h" +#include <acpi/battery.h> #define PREFIX "ACPI: " diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index 13e7b56..1794408 100644 --- a/drivers/acpi/battery.c +++ b/drivers/acpi/battery.c @@ -23,6 +23,7 @@ #include <linux/kernel.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/init.h> #include <linux/types.h> #include <linux/jiffies.h> @@ -30,6 +31,7 @@ #include <linux/dmi.h> #include <linux/delay.h> #include <linux/slab.h> +#include <linux/list.h> #include <linux/suspend.h> #include <asm/unaligned.h> @@ -42,7 +44,7 @@ #include <linux/acpi.h> #include <linux/power_supply.h> -#include "battery.h" +#include <acpi/battery.h> #define PREFIX "ACPI: " @@ -124,6 +126,7 @@ struct acpi_battery { struct power_supply_desc bat_desc; struct acpi_device *device; struct notifier_block pm_nb; + struct list_head list; unsigned long update_time; int revision; int rate_now; @@ -626,6 +629,171 @@ static const struct device_attribute alarm_attr = { .store = acpi_battery_alarm_store, }; +/* + * The Battery Hooking API + * + * This API is used inside other drivers that need to expose + * platform-specific behaviour within the generic driver in a + * generic way. + * + */ + +LIST_HEAD(acpi_battery_list); +LIST_HEAD(battery_hook_list); +DEFINE_MUTEX(hook_mutex); + +void __battery_hook_unregister(struct acpi_battery_hook *hook, int force) +{ + struct list_head *position; + struct acpi_battery *battery; + + /* + * In order to remove a hook, we first need to + * de-register all the batteries that are registered. + */ + + if (!force) + mutex_lock(&hook_mutex); + + list_for_each(position, &acpi_battery_list) { + battery = list_entry(position, struct acpi_battery, list); + hook->remove_battery(battery->bat); + } + + /* Then, just remove the hook */ + + list_del(&hook->list); + + if (!force) + mutex_unlock(&hook_mutex); + + pr_info("battery: extension unregistered: %s\n", hook->name); +} + +void battery_hook_unregister(struct acpi_battery_hook *hook) +{ + __battery_hook_unregister(hook, 0); +} +EXPORT_SYMBOL_GPL(battery_hook_unregister); + +void battery_hook_register(struct acpi_battery_hook *hook) +{ + struct list_head *position; + struct acpi_battery *battery; + + mutex_lock(&hook_mutex); + INIT_LIST_HEAD(&hook->list); + list_add(&hook->list, &battery_hook_list); + + /* + * Now that the driver is registered, we need + * to notify the hook that a battery is available + * for each battery, so that the driver may add + * its attributes. + */ + list_for_each(position, &acpi_battery_list) { + battery = list_entry(position, struct acpi_battery, list); + if (hook->add_battery(battery->bat)) { + + /* + * If a add-battery returns non-zero, + * the registration of the extension has failed, + * and we will not add it to the list of loaded + * hooks. + */ + pr_err("battery: extension failed to load: %s", + hook->name); + __battery_hook_unregister(hook, 1); + return; + + } + } + + pr_info("battery: new extension: %s\n", hook->name); + mutex_unlock(&hook_mutex); +} +EXPORT_SYMBOL_GPL(battery_hook_register); + +static void battery_hook_add_battery(struct acpi_battery *battery) +{ + + /* + * This function gets called right after the battery sysfs + * attributes have been added, so that the drivers that + * define custom sysfs attributes can add their own. + */ + + struct list_head *position; + struct acpi_battery_hook *hook_node; + + mutex_lock(&hook_mutex); + INIT_LIST_HEAD(&battery->list); + list_add(&battery->list, &acpi_battery_list); + + /* + * Since we added a new battery to the list, we need to + * iterate over the hooks and call add_battery for each + * hook that was registered. This usually happens + * when a battery gets hotplugged or initialized + * during the battery module initialization. + */ + + list_for_each(position, &battery_hook_list) { + hook_node = list_entry(position, struct acpi_battery_hook, list); + if (hook_node->add_battery(battery->bat)) { + + /* + * The notification of the extensions has failed, to + * prevent further errors we will unload the extension. + */ + __battery_hook_unregister(hook_node, 1); + pr_err("battery: error in extension, unloading: %s", + hook_node->name); + } + } + + mutex_unlock(&hook_mutex); +} + +static void battery_hook_remove_battery(struct acpi_battery *battery) +{ + struct list_head *position; + struct acpi_battery_hook *hook; + + mutex_lock(&hook_mutex); + /* + * Before removing the hook, we need to remove all + * custom attributes from the battery. + */ + list_for_each(position, &battery_hook_list) { + hook = list_entry(position, struct acpi_battery_hook, list); + hook->remove_battery(battery->bat); + } + + /* Then, just remove the battery from the list */ + + list_del(&battery->list); + mutex_unlock(&hook_mutex); +} + +static void __exit battery_hook_exit(void) +{ + struct list_head *position; + struct acpi_battery_hook *hook; + + /* + * At this point, the acpi_bus_unregister_driver + * has called remove for all batteries. We just + * need to remove the hooks. + */ + list_for_each(position, &battery_hook_list) { + hook = list_entry(position, struct acpi_battery_hook, list); + __battery_hook_unregister(hook, 0); + } + + mutex_destroy(&hook_mutex); +} + static int sysfs_add_battery(struct acpi_battery *battery) { struct power_supply_config psy_cfg = { .drv_data = battery, }; @@ -647,6 +815,8 @@ static int sysfs_add_battery(struct acpi_battery *battery) battery->bat = power_supply_register_no_ws(&battery->device->dev, &battery->bat_desc, &psy_cfg); + battery_hook_add_battery(battery); + if (IS_ERR(battery->bat)) { int result = PTR_ERR(battery->bat); @@ -663,7 +833,7 @@ static void sysfs_remove_battery(struct acpi_battery *battery) mutex_unlock(&battery->sysfs_lock); return; } - + battery_hook_remove_battery(battery); device_remove_file(&battery->bat->dev, &alarm_attr); power_supply_unregister(battery->bat); battery->bat = NULL; @@ -1361,6 +1531,7 @@ static void __exit acpi_battery_exit(void) async_synchronize_cookie(async_cookie + 1); if (battery_driver_registered) acpi_bus_unregister_driver(&acpi_battery_driver); + battery_hook_exit(); #ifdef CONFIG_ACPI_PROCFS_POWER if (acpi_battery_dir) acpi_unlock_battery_dir(acpi_battery_dir); diff --git a/drivers/acpi/battery.h b/drivers/acpi/battery.h deleted file mode 100644 index 225f493..0000000 --- a/drivers/acpi/battery.h +++ /dev/null @@ -1,11 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -#ifndef __ACPI_BATTERY_H -#define __ACPI_BATTERY_H - -#define ACPI_BATTERY_CLASS "battery" - -#define ACPI_BATTERY_NOTIFY_STATUS 0x80 -#define ACPI_BATTERY_NOTIFY_INFO 0x81 -#define ACPI_BATTERY_NOTIFY_THRESHOLD 0x82 - -#endif diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c index a2428e9..295b592 100644 --- a/drivers/acpi/sbs.c +++ b/drivers/acpi/sbs.c @@ -32,9 +32,9 @@ #include <linux/delay.h> #include <linux/power_supply.h> #include <linux/platform_data/x86/apple.h> +#include <acpi/battery.h> #include "sbshc.h" -#include "battery.h" #define PREFIX "ACPI: " diff --git a/include/acpi/battery.h b/include/acpi/battery.h new file mode 100644 index 0000000..5d8f5d9 --- /dev/null +++ b/include/acpi/battery.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __ACPI_BATTERY_H +#define __ACPI_BATTERY_H + +#define ACPI_BATTERY_CLASS "battery" + +#define ACPI_BATTERY_NOTIFY_STATUS 0x80 +#define ACPI_BATTERY_NOTIFY_INFO 0x81 +#define ACPI_BATTERY_NOTIFY_THRESHOLD 0x82 + +struct acpi_battery_hook { + const char *name; + int (*add_battery)(struct power_supply *battery); + int (*remove_battery)(struct power_supply *battery); + struct list_head list; +}; + +void battery_hook_register(struct acpi_battery_hook *hook); +void battery_hook_unregister(struct acpi_battery_hook *hook); + +#endif
This is a patch that implements a generic hooking API for the generic ACPI battery driver. With this new generic API, drivers can expose platform specific behaviour via sysfs attributes in /sys/class/power_supply/BATn/ in a generic way. A perfect example of the need for this API are Lenovo ThinkPads. Lenovo ThinkPads have a ACPI extension that allows the setting of start and stop charge thresholds in the EC and battery firmware via ACPI. The thinkpad_acpi module can use this API to expose sysfs attributes that it controls inside the ACPI battery driver sysfs tree, under /sys/class/power_supply/BATN/. The file drivers/acpi/battery.h has been moved to include/acpi/battery.h and the includes inside ac.c, sbs.c, and battery.c have been adjusted to reflect that. When drivers hooks into the API, the API calls add_battery() for each battery in the system that passes it a acpi_battery struct. Then, the drivers can use device_create_file() to create new sysfs attributes with that struct and identify the batteries for per-battery attributes. v7: * Implemented mutual exclusion for hooking methods and battery callbacks * Fixed a BUG where errors in other modules would occur when the modules that depend on battery get unloaded Signed-off-by: Ognjen Galic <smclt30p@gmail.com> --- drivers/acpi/ac.c | 2 +- drivers/acpi/battery.c | 175 ++++++++++++++++++++++++++++++++++++++++++++++++- drivers/acpi/battery.h | 11 ---- drivers/acpi/sbs.c | 2 +- include/acpi/battery.h | 21 ++++++ 5 files changed, 196 insertions(+), 15 deletions(-) delete mode 100644 drivers/acpi/battery.h create mode 100644 include/acpi/battery.h