Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v13 1/4] battery: Add the battery hooking API
@ 2018-02-07 14:58 Ognjen Galic
  2018-02-08 15:18 ` Rafael J. Wysocki
  2020-06-23 23:42 ` Kristian Klausen
  0 siblings, 2 replies; 4+ messages in thread
From: Ognjen Galic @ 2018-02-07 14:58 UTC (permalink / raw)
  To: Andy Shevchenko, Rafael J. Wysocki, Ognjen Galić,
	Rafael J. Wysocki, Len Brown, Robert Moore, Lv Zheng,
	ACPI Devel Maling List, devel, Darren Hart, Andy Shevchenko,
	Henrique de Moraes Holschuh, Sebastian Reichel, Platform Driver,
	ibm-acpi-devel, Linux PM, Christoph Böhmwalder, Kevin Locke

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.

Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
---

Notes:
     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
    
    v8:
    * Use list_for_each_safe instead of list_for_each for the module
      exit function where deletion of nodes occurs
    
    v9:
    * No changes in this patch in v9
    
    v10:
    * Fix compiler warnings in Intel's 0-day CI
    
    v11:
    * Fix changelog formatting
    * Make lists and mutex static
    
    v12:
    * Change all applicable list_for_each to list_for_each_entry
    * Define pr_fmt to include module name for pr_
    
    v13:
    * Fixed the last list_for_each_entry
    * Move some comments around
    * Prevent battery registering if power_supply_register_no_ws() fails

 drivers/acpi/ac.c      |   2 +-
 drivers/acpi/battery.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/acpi/battery.h |  11 ----
 drivers/acpi/sbs.c     |   2 +-
 include/acpi/battery.h |  21 +++++++
 5 files changed, 167 insertions(+), 16 deletions(-)
 delete mode 100644 drivers/acpi/battery.h
 create mode 100644 include/acpi/battery.h

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index 47a7ed557..2d8de2f8c 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 19bc44082..3fab2cb9e 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -21,8 +21,12 @@
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/jiffies.h>
@@ -42,7 +46,7 @@
 #include <linux/acpi.h>
 #include <linux/power_supply.h>
 
-#include "battery.h"
+#include <acpi/battery.h>
 
 #define PREFIX "ACPI: "
 
@@ -125,6 +129,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;
@@ -630,6 +635,139 @@ 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.
+ *
+ */
+
+static LIST_HEAD(acpi_battery_list);
+static LIST_HEAD(battery_hook_list);
+static DEFINE_MUTEX(hook_mutex);
+
+void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
+{
+	struct acpi_battery *battery;
+	/*
+	 * In order to remove a hook, we first need to
+	 * de-register all the batteries that are registered.
+	 */
+	if (lock)
+		mutex_lock(&hook_mutex);
+	list_for_each_entry(battery, &acpi_battery_list, list) {
+		hook->remove_battery(battery->bat);
+	}
+	list_del(&hook->list);
+	if (lock)
+		mutex_unlock(&hook_mutex);
+	pr_info("extension unregistered: %s\n", hook->name);
+}
+
+void battery_hook_unregister(struct acpi_battery_hook *hook)
+{
+	__battery_hook_unregister(hook, 1);
+}
+EXPORT_SYMBOL_GPL(battery_hook_unregister);
+
+void battery_hook_register(struct acpi_battery_hook *hook)
+{
+	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_entry(battery, &acpi_battery_list, 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("extension failed to load: %s", hook->name);
+			__battery_hook_unregister(hook, 0);
+			return;
+		}
+	}
+	pr_info("new extension: %s\n", hook->name);
+	mutex_unlock(&hook_mutex);
+}
+EXPORT_SYMBOL_GPL(battery_hook_register);
+
+/*
+ * 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.
+*/
+static void battery_hook_add_battery(struct acpi_battery *battery)
+{
+	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_entry(hook_node, &battery_hook_list, 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, 0);
+			pr_err("error in extension, unloading: %s",
+					hook_node->name);
+		}
+	}
+	mutex_unlock(&hook_mutex);
+}
+
+static void battery_hook_remove_battery(struct acpi_battery *battery)
+{
+	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_entry(hook, &battery_hook_list, 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 acpi_battery_hook *hook;
+	struct acpi_battery_hook *ptr;
+	/*
+	 * At this point, the acpi_bus_unregister_driver()
+	 * has called remove for all batteries. We just
+	 * need to remove the hooks.
+	 */
+	list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) {
+		__battery_hook_unregister(hook, 1);
+	}
+	mutex_destroy(&hook_mutex);
+}
+
 static int sysfs_add_battery(struct acpi_battery *battery)
 {
 	struct power_supply_config psy_cfg = { .drv_data = battery, };
@@ -657,6 +795,7 @@ static int sysfs_add_battery(struct acpi_battery *battery)
 		battery->bat = NULL;
 		return result;
 	}
+	battery_hook_add_battery(battery);
 	return device_create_file(&battery->bat->dev, &alarm_attr);
 }
 
@@ -667,7 +806,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;
@@ -1383,8 +1522,10 @@ static int __init acpi_battery_init(void)
 static void __exit acpi_battery_exit(void)
 {
 	async_synchronize_cookie(async_cookie + 1);
-	if (battery_driver_registered)
+	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 225f493d4..000000000
--- 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 a2428e946..295b59271 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 000000000..5d8f5d910
--- /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
-- 
2.14.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v13 1/4] battery: Add the battery hooking API
  2018-02-07 14:58 [PATCH v13 1/4] battery: Add the battery hooking API Ognjen Galic
@ 2018-02-08 15:18 ` Rafael J. Wysocki
  2020-06-23 23:42 ` Kristian Klausen
  1 sibling, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2018-02-08 15:18 UTC (permalink / raw)
  To: Ognjen Galic
  Cc: Andy Shevchenko, Rafael J. Wysocki, Rafael J. Wysocki, Len Brown,
	Robert Moore, Lv Zheng, ACPI Devel Maling List, devel,
	Darren Hart, Andy Shevchenko, Henrique de Moraes Holschuh,
	Sebastian Reichel, Platform Driver, ibm-acpi-devel, Linux PM,
	Christoph Böhmwalder, Kevin Locke

On Wed, Feb 7, 2018 at 3:58 PM, Ognjen Galic <smclt30p@gmail.com> wrote:
> 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.
>
> Signed-off-by: Ognjen Galic <smclt30p@gmail.com>

This is a major update and for timing reasons I'd rather not try to
rush it into 4.16-rc.

If that's not a big issue, I'll queue it up for 4.17.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v13 1/4] battery: Add the battery hooking API
  2018-02-07 14:58 [PATCH v13 1/4] battery: Add the battery hooking API Ognjen Galic
  2018-02-08 15:18 ` Rafael J. Wysocki
@ 2020-06-23 23:42 ` Kristian Klausen
  2020-07-25 19:00   ` Sebastian Reichel
  1 sibling, 1 reply; 4+ messages in thread
From: Kristian Klausen @ 2020-06-23 23:42 UTC (permalink / raw)
  To: Ognjen Galic, Andy Shevchenko, Rafael J. Wysocki,
	Rafael J. Wysocki, Len Brown, Robert Moore, Lv Zheng,
	ACPI Devel Maling List, devel, Darren Hart, Andy Shevchenko,
	Henrique de Moraes Holschuh, Sebastian Reichel, Platform Driver,
	ibm-acpi-devel, Linux PM, Christoph Böhmwalder, Kevin Locke

On 07.02.2018 15.58, Ognjen Galic wrote:
> 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.

Hi

I did that, when I implemented charge threshold support for ASUS 
laptops[1][2].

It works very well but I can't control the threshold with udev (also 
reported by another user here[3]). So I did a bit of digging and the 
doc[4] states: "If attributes are added after the device is registered, 
then userspace won’t get notified and userspace will not know about the 
new attributes.", which seems to be the way the current code works:
power_supply_register_no_ws is called[5] and if it success all the hooks 
are run.

Looking at the code I'm not sure there is a easy way to fix it, do you 
have any good ideas?

Best
Kristian Klausen

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d507a54f5865d8dcbdd16c66a1a2da15640878ca
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7973353e92ee1e7ca3b2eb361a4b7cb66c92abee
[3] 
https://www.reddit.com/r/linuxhardware/comments/g8kpee/psa_kernel_54_added_the_ability_to_set_a_battery/fp8bwgu/
[4] 
https://www.kernel.org/doc/html/v5.8-rc2/driver-api/driver-model/device.html
[5] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/acpi/battery.c?id=8c3f6993c221cc1a2588046e3ff32d64580396b7#n854

>
> Signed-off-by: Ognjen Galic <smclt30p@gmail.com>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v13 1/4] battery: Add the battery hooking API
  2020-06-23 23:42 ` Kristian Klausen
@ 2020-07-25 19:00   ` Sebastian Reichel
  0 siblings, 0 replies; 4+ messages in thread
From: Sebastian Reichel @ 2020-07-25 19:00 UTC (permalink / raw)
  To: Kristian Klausen
  Cc: Ognjen Galic, Andy Shevchenko, Rafael J. Wysocki,
	Rafael J. Wysocki, Len Brown, Robert Moore, Lv Zheng,
	ACPI Devel Maling List, devel, Darren Hart, Andy Shevchenko,
	Henrique de Moraes Holschuh, Platform Driver, ibm-acpi-devel,
	Linux PM, Christoph Böhmwalder, Kevin Locke


[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]

Hi,

On Wed, Jun 24, 2020 at 01:42:26AM +0200, Kristian Klausen wrote:
> On 07.02.2018 15.58, Ognjen Galic wrote:
> > 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.
> 
> Hi
> 
> I did that, when I implemented charge threshold support for ASUS
> laptops[1][2].
> 
> It works very well but I can't control the threshold with udev (also
> reported by another user here[3]). So I did a bit of digging and the doc[4]
> states: "If attributes are added after the device is registered, then
> userspace won’t get notified and userspace will not know about the new
> attributes.", which seems to be the way the current code works:
> power_supply_register_no_ws is called[5] and if it success all the hooks are
> run.
>
> Looking at the code I'm not sure there is a easy way to fix it, do you have
> any good ideas?

That problem is described by Greg in his blog post from 2013:

http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/

The power-supply subsystem offers registering extra attributes
at registration time by filling in the .attr_grp field in the
struct power_supply_config supplied as last parameter to
power_supply_register_*() since 4.21.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-07 14:58 [PATCH v13 1/4] battery: Add the battery hooking API Ognjen Galic
2018-02-08 15:18 ` Rafael J. Wysocki
2020-06-23 23:42 ` Kristian Klausen
2020-07-25 19:00   ` Sebastian Reichel

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git