battery: Add the battery hooking API

PATCH Superseded Ognjen Galic 2 participants 5 messages Dec 20, 2017 – Dec 23, 2017

Subsystems (3)

ACPI x86 platform drivers ACPI component architecture (acpica)

lore

Share & tools

lore

Ognjen Galic

author

2017-12-20 15:15 UTC #0 lore raw

PATCH v7 1/3 battery: Add the battery hooking API

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 <[email protected]>

---
 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

drivers/acpi/ac.c +1 -1

index 47a7ed5..2d8de2f 100644
@@ -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: "

drivers/acpi/battery.c +173 -2

223 lines not on this page · raw message

drivers/acpi/battery.h deleted +0 ~~-11~~

deleted file mode 100644
index 225f493..0000000
@@ -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

drivers/acpi/sbs.c +1 -1

index a2428e9..295b592 100644
@@ -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: "

include/acpi/battery.h new file +21 -0

new file mode 100644
index 0000000..5d8f5d9
@@ -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

Signature · 1 line

2.7.4

kbuild test robot

build bot

2017-12-23 07:36 UTC #1 lore raw

Thank you for the patch! Perhaps something to improve:

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(a)redhat.com> :::::: CC: Rafael J. Wysocki <rafael.j.wysocki(a)intel.com>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

kbuild test robot

build bot

2017-12-23 09:08 UTC #2 lore raw

In reply to Ognjen Galic

Thank you for the patch! Perhaps something to improve:

Hi Ognjen,

Thank you for the patch! Perhaps something to improve:

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(a)redhat.com> :::::: CC: Rafael J. Wysocki <rafael.j.wysocki(a)intel.com>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

kbuild test robot

build bot

2017-12-23 11:04 UTC #3 lore raw

[Devel] [RFC PATCH] battery: hook_mutex can be static

In reply to Ognjen Galic

Fixes: 19380b8e214c ("battery: Add the battery hooking API")

Fixes 19380b8e214c ("battery: Add the battery hooking API")

Signed-off-by Fengguang Wu <fengguang.wu(a)intel.com>

---
 battery.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/acpi/battery.c +2 -2

index 1794408..9324591 100644
@@ -640,9 +640,9 @@ static const struct device_attribute alarm_attr = {
 
 LIST_HEAD(acpi_battery_list);
 LIST_HEAD(battery_hook_list);
-DEFINE_MUTEX(hook_mutex);
+static DEFINE_MUTEX(hook_mutex);
 
-void __battery_hook_unregister(struct acpi_battery_hook *hook, int force)
+static void __battery_hook_unregister(struct acpi_battery_hook *hook, int force)
 {
 	struct list_head *position;
 	struct acpi_battery *battery;

kbuild test robot

build bot

2017-12-23 11:04 UTC #4 lore raw

In reply to Ognjen Galic

Thank you for the patch! Perhaps something to improve:

Hi Ognjen,

Thank you for the patch! Perhaps something to improve:

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