[PATCH v12 1/4] battery: Add the battery hooking API

[PATCH v12 1/4] battery: Add the battery hooking API

[Ognjen Galic]

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

 drivers/acpi/ac.c      |   2 +-
 drivers/acpi/battery.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/acpi/battery.h |  11 ----
 drivers/acpi/sbs.c     |   2 +-
 include/acpi/battery.h |  21 +++++++
 5 files changed, 171 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 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..7089e31 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: "
 
@@ -124,6 +128,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 +631,142 @@ 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 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 (lock)
+		mutex_lock(&hook_mutex);
+	list_for_each(position, &acpi_battery_list) {
+		battery = list_entry(position, struct acpi_battery, 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);
+
+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 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, };
@@ -647,6 +788,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 +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;
@@ -1359,8 +1502,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 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
-- 
2.7.4

Re: [PATCH v12 1/4] battery: Add the battery hooking API

[Andy Shevchenko]

On Wed, Jan 3, 2018 at 1: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.

Thanks for an update. I have couple of minors. Otherwise look pretty much good!

>  drivers/acpi/battery.h |  11 ----
>  include/acpi/battery.h |  21 +++++++

There are -M and -C command line parameters to git format-patch.
They can take an optional argument (percentage) of threshold.

Playing with those numbers you can achieve

rename ...

line and see actual diff.

No need to resend because of this. Just an explanation for the future Git work.

> +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
> +{
> +       struct list_head *position;
> +       struct acpi_battery *battery;

Missed empty line?

> +       /*
> +        * 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(position, &acpi_battery_list) {
> +               battery = list_entry(position, struct acpi_battery, list);

list_for_each_enrty() ?

Or I'm missing something why it can't be used?

> +               hook->remove_battery(battery->bat);
> +       }

> +void battery_hook_register(struct acpi_battery_hook *hook)
> +{
> +       struct acpi_battery *battery;

> +       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);

Can it fit 80 characters?
I mean if it would be one line...

> +                       __battery_hook_unregister(hook, 0);
> +                       return;
> +               }
> +       }
> +       pr_info("new extension: %s\n", hook->name);
> +       mutex_unlock(&hook_mutex);
> +}

> +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.
> +        */

Perhaps it should go before function definition.

> +       struct acpi_battery_hook *hook_node;

> +}


> +static void __exit battery_hook_exit(void)
> +{
> +       struct acpi_battery_hook *hook;
> +       struct acpi_battery_hook *ptr;

Missed empty line?

> +       /*
> +        * At this point, the acpi_bus_unregister_driver
> +        * has called remove for all batteries. We just
> +        * need to remove the hooks.
> +        */

A common pattern to use func() [note parens] when refer to function.

> +       list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) {
> +               __battery_hook_unregister(hook, 1);
> +       }
> +       mutex_destroy(&hook_mutex);
> +}

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

I'm not sure why you need to add hook when power_supply_register_no_ws() failed.

>         }
> -
> +       battery_hook_remove_battery(battery);

No need to remove an empty line.

-- 
With Best Regards,
Andy Shevchenko

Re: [Devel] [PATCH v12 1/4] battery: Add the battery hooking API

[Andy Shevchenko]

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

On Wed, Jan 3, 2018 at 1:58 PM, Ognjen Galic <smclt30p(a)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.

Thanks for an update. I have couple of minors. Otherwise look pretty much good!

>  drivers/acpi/battery.h |  11 ----
>  include/acpi/battery.h |  21 +++++++

There are -M and -C command line parameters to git format-patch.
They can take an optional argument (percentage) of threshold.

Playing with those numbers you can achieve

rename ...

line and see actual diff.

No need to resend because of this. Just an explanation for the future Git work.

> +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
> +{
> +       struct list_head *position;
> +       struct acpi_battery *battery;

Missed empty line?

> +       /*
> +        * 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(position, &acpi_battery_list) {
> +               battery = list_entry(position, struct acpi_battery, list);

list_for_each_enrty() ?

Or I'm missing something why it can't be used?

> +               hook->remove_battery(battery->bat);
> +       }

> +void battery_hook_register(struct acpi_battery_hook *hook)
> +{
> +       struct acpi_battery *battery;

> +       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);

Can it fit 80 characters?
I mean if it would be one line...

> +                       __battery_hook_unregister(hook, 0);
> +                       return;
> +               }
> +       }
> +       pr_info("new extension: %s\n", hook->name);
> +       mutex_unlock(&hook_mutex);
> +}

> +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.
> +        */

Perhaps it should go before function definition.

> +       struct acpi_battery_hook *hook_node;

> +}


> +static void __exit battery_hook_exit(void)
> +{
> +       struct acpi_battery_hook *hook;
> +       struct acpi_battery_hook *ptr;

Missed empty line?

> +       /*
> +        * At this point, the acpi_bus_unregister_driver
> +        * has called remove for all batteries. We just
> +        * need to remove the hooks.
> +        */

A common pattern to use func() [note parens] when refer to function.

> +       list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) {
> +               __battery_hook_unregister(hook, 1);
> +       }
> +       mutex_destroy(&hook_mutex);
> +}

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

I'm not sure why you need to add hook when power_supply_register_no_ws() failed.

>         }
> -
> +       battery_hook_remove_battery(battery);

No need to remove an empty line.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v12 1/4] battery: Add the battery hooking API

[Ognjen Galić]

On Wed, Jan 03, 2018 at 04:25:42PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 3, 2018 at 1: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.
> 
> Thanks for an update. I have couple of minors. Otherwise look pretty much good!
> 
> >  drivers/acpi/battery.h |  11 ----
> >  include/acpi/battery.h |  21 +++++++
> 
> There are -M and -C command line parameters to git format-patch.
> They can take an optional argument (percentage) of threshold.
> 
> Playing with those numbers you can achieve
> 
> rename ...
> 
> line and see actual diff.
> 
> No need to resend because of this. Just an explanation for the future Git work.

I did use thos options. I used the following command:

git format-patch -M -C --notes -v12 -o ~/patches/. @^^^^

I really don't know what you are targeting. :)

> 
> > +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
> > +{
> > +       struct list_head *position;
> > +       struct acpi_battery *battery;
> 
> Missed empty line?

checkpatch.pl complains if there are NOT empty lines between
declarations and statements.

> 
> > +       /*
> > +        * 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(position, &acpi_battery_list) {
> > +               battery = list_entry(position, struct acpi_battery, list);
> 
> list_for_each_enrty() ?
> 
> Or I'm missing something why it can't be used?

I missed that one. Bummer.

I mean, it's not game-breaking, its just minor style stuff. I won't be
sending more revisions because of these small issues, as I think its 
uneccessary to flood both Rafael and the mailing lists with patch
revisions that remove or add a few spaces. No offence, it just got old.

:)

> 
> > +               hook->remove_battery(battery->bat);
> > +       }
> 
> > +void battery_hook_register(struct acpi_battery_hook *hook)
> > +{
> > +       struct acpi_battery *battery;
> 
> > +       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);
> 
> Can it fit 80 characters?
> I mean if it would be one line...

It could, but it does not. :)

> 
> > +                       __battery_hook_unregister(hook, 0);
> > +                       return;
> > +               }
> > +       }
> > +       pr_info("new extension: %s\n", hook->name);
> > +       mutex_unlock(&hook_mutex);
> > +}
> 
> > +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.
> > +        */
> 
> Perhaps it should go before function definition.
> 
> > +       struct acpi_battery_hook *hook_node;
> 
> > +}
> 
> 
> > +static void __exit battery_hook_exit(void)
> > +{
> > +       struct acpi_battery_hook *hook;
> > +       struct acpi_battery_hook *ptr;
> 
> Missed empty line?

See above about the checkpatch.pl stuff.

> 
> > +       /*
> > +        * At this point, the acpi_bus_unregister_driver
> > +        * has called remove for all batteries. We just
> > +        * need to remove the hooks.
> > +        */
> 
> A common pattern to use func() [note parens] when refer to function.

Got it.

> 
> > +       list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) {
> > +               __battery_hook_unregister(hook, 1);
> > +       }
> > +       mutex_destroy(&hook_mutex);
> > +}
> 
> >         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);
> 
> I'm not sure why you need to add hook when power_supply_register_no_ws() failed.

One could add a hook for other various reasons, not just to add or
remove sysfs attributes. I'm talking stuff like desktop notifications or
notifications to other modules to change power modes or other stuff.

> 
> >         }
> > -
> > +       battery_hook_remove_battery(battery);
> 
> No need to remove an empty line.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v12 1/4] battery: Add the battery hooking API

[Andy Shevchenko]

On Wed, Jan 3, 2018 at 4:53 PM, Ognjen Galić <smclt30p@gmail.com> wrote:
> On Wed, Jan 03, 2018 at 04:25:42PM +0200, Andy Shevchenko wrote:
>> On Wed, Jan 3, 2018 at 1:58 PM, Ognjen Galic <smclt30p@gmail.com> wrote:

>> Thanks for an update. I have couple of minors. Otherwise look pretty much good!
>>
>> >  drivers/acpi/battery.h |  11 ----
>> >  include/acpi/battery.h |  21 +++++++
>>
>> There are -M and -C command line parameters to git format-patch.

>> They can take an optional argument (percentage) of threshold.
>>
>> Playing with those numbers you can achieve

^^^^ Pay attention to the above

>>
>> rename ...
>>
>> line and see actual diff.
>>
>> No need to resend because of this. Just an explanation for the future Git work.
>
> I did use thos options. I used the following command:
>
> git format-patch -M -C --notes -v12 -o ~/patches/. @^^^^
>
> I really don't know what you are targeting. :)

Please, read what I wrote above and the manual of git-format-patch.

>> > +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
>> > +{
>> > +       struct list_head *position;
>> > +       struct acpi_battery *battery;
>>
>> Missed empty line?
>
> checkpatch.pl complains if there are NOT empty lines between
> declarations and statements.

checkpatch some times on one hand complains about something which it
should not, on the other didn't take into consideration cases like
this one.

Your statement started with comment, btw.

>> > +       /*
>> > +        * In order to remove a hook, we first need to
>> > +        * de-register all the batteries that are registered.
>> > +        */
>> > +       if (lock)
>> > +               mutex_lock(&hook_mutex);

> I mean, it's not game-breaking, its just minor style stuff. I won't be
> sending more revisions because of these small issues, as I think its
> uneccessary to flood both Rafael and the mailing lists with patch
> revisions that remove or add a few spaces. No offence, it just got old.

Yes, his call anyway to apply or ask you for amendments. I'm just
helping with review.

>> > +static void __exit battery_hook_exit(void)
>> > +{
>> > +       struct acpi_battery_hook *hook;
>> > +       struct acpi_battery_hook *ptr;
>>
>> Missed empty line?
>
> See above about the checkpatch.pl stuff.

See above the answer.


-- 
With Best Regards,
Andy Shevchenko

Re: [Devel] [PATCH v12 1/4] battery: Add the battery hooking API

[Andy Shevchenko]

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

On Wed, Jan 3, 2018 at 4:53 PM, Ognjen Galić <smclt30p(a)gmail.com> wrote:
> On Wed, Jan 03, 2018 at 04:25:42PM +0200, Andy Shevchenko wrote:
>> On Wed, Jan 3, 2018 at 1:58 PM, Ognjen Galic <smclt30p(a)gmail.com> wrote:

>> Thanks for an update. I have couple of minors. Otherwise look pretty much good!
>>
>> >  drivers/acpi/battery.h |  11 ----
>> >  include/acpi/battery.h |  21 +++++++
>>
>> There are -M and -C command line parameters to git format-patch.

>> They can take an optional argument (percentage) of threshold.
>>
>> Playing with those numbers you can achieve

^^^^ Pay attention to the above

>>
>> rename ...
>>
>> line and see actual diff.
>>
>> No need to resend because of this. Just an explanation for the future Git work.
>
> I did use thos options. I used the following command:
>
> git format-patch -M -C --notes -v12 -o ~/patches/. @^^^^
>
> I really don't know what you are targeting. :)

Please, read what I wrote above and the manual of git-format-patch.

>> > +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
>> > +{
>> > +       struct list_head *position;
>> > +       struct acpi_battery *battery;
>>
>> Missed empty line?
>
> checkpatch.pl complains if there are NOT empty lines between
> declarations and statements.

checkpatch some times on one hand complains about something which it
should not, on the other didn't take into consideration cases like
this one.

Your statement started with comment, btw.

>> > +       /*
>> > +        * In order to remove a hook, we first need to
>> > +        * de-register all the batteries that are registered.
>> > +        */
>> > +       if (lock)
>> > +               mutex_lock(&hook_mutex);

> I mean, it's not game-breaking, its just minor style stuff. I won't be
> sending more revisions because of these small issues, as I think its
> uneccessary to flood both Rafael and the mailing lists with patch
> revisions that remove or add a few spaces. No offence, it just got old.

Yes, his call anyway to apply or ask you for amendments. I'm just
helping with review.

>> > +static void __exit battery_hook_exit(void)
>> > +{
>> > +       struct acpi_battery_hook *hook;
>> > +       struct acpi_battery_hook *ptr;
>>
>> Missed empty line?
>
> See above about the checkpatch.pl stuff.

See above the answer.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v12 1/4] battery: Add the battery hooking API

[Ognjen Galić]

On Wed, Jan 03, 2018 at 06:40:12PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 3, 2018 at 4:53 PM, Ognjen Galić <smclt30p@gmail.com> wrote:
> > On Wed, Jan 03, 2018 at 04:25:42PM +0200, Andy Shevchenko wrote:
> >> On Wed, Jan 3, 2018 at 1:58 PM, Ognjen Galic <smclt30p@gmail.com> wrote:
> 
> >> Thanks for an update. I have couple of minors. Otherwise look pretty much good!
> >>
> >> >  drivers/acpi/battery.h |  11 ----
> >> >  include/acpi/battery.h |  21 +++++++
> >>
> >> There are -M and -C command line parameters to git format-patch.
> 
> >> They can take an optional argument (percentage) of threshold.
> >>
> >> Playing with those numbers you can achieve
> 
> ^^^^ Pay attention to the above
> 
> >>
> >> rename ...
> >>
> >> line and see actual diff.
> >>
> >> No need to resend because of this. Just an explanation for the future Git work.
> >
> > I did use thos options. I used the following command:
> >
> > git format-patch -M -C --notes -v12 -o ~/patches/. @^^^^
> >
> > I really don't know what you are targeting. :)
> 
> Please, read what I wrote above and the manual of git-format-patch.
> 
> >> > +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
> >> > +{
> >> > +       struct list_head *position;
> >> > +       struct acpi_battery *battery;
> >>
> >> Missed empty line?
> >
> > checkpatch.pl complains if there are NOT empty lines between
> > declarations and statements.
> 
> checkpatch some times on one hand complains about something which it
> should not, on the other didn't take into consideration cases like
> this one.
> 
> Your statement started with comment, btw.
> 
> >> > +       /*
> >> > +        * In order to remove a hook, we first need to
> >> > +        * de-register all the batteries that are registered.
> >> > +        */
> >> > +       if (lock)
> >> > +               mutex_lock(&hook_mutex);
> 
> > I mean, it's not game-breaking, its just minor style stuff. I won't be
> > sending more revisions because of these small issues, as I think its
> > uneccessary to flood both Rafael and the mailing lists with patch
> > revisions that remove or add a few spaces. No offence, it just got old.
> 
> Yes, his call anyway to apply or ask you for amendments. I'm just
> helping with review.

Rafael, what do you think? Do you want these style/syntax issues fixed
or is it good to go?

Thanks for the time to review Andy! You are a champ! :)

> 
> >> > +static void __exit battery_hook_exit(void)
> >> > +{
> >> > +       struct acpi_battery_hook *hook;
> >> > +       struct acpi_battery_hook *ptr;
> >>
> >> Missed empty line?
> >
> > See above about the checkpatch.pl stuff.
> 
> See above the answer.
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v12 1/4] battery: Add the battery hooking API

[Rafael J. Wysocki]

On Thu, Jan 4, 2018 at 12:13 PM, Ognjen Galić <smclt30p@gmail.com> wrote:
> On Wed, Jan 03, 2018 at 06:40:12PM +0200, Andy Shevchenko wrote:
>> On Wed, Jan 3, 2018 at 4:53 PM, Ognjen Galić <smclt30p@gmail.com> wrote:
>> > On Wed, Jan 03, 2018 at 04:25:42PM +0200, Andy Shevchenko wrote:
>> >> On Wed, Jan 3, 2018 at 1:58 PM, Ognjen Galic <smclt30p@gmail.com> wrote:
>>
>> >> Thanks for an update. I have couple of minors. Otherwise look pretty much good!
>> >>
>> >> >  drivers/acpi/battery.h |  11 ----
>> >> >  include/acpi/battery.h |  21 +++++++
>> >>
>> >> There are -M and -C command line parameters to git format-patch.
>>
>> >> They can take an optional argument (percentage) of threshold.
>> >>
>> >> Playing with those numbers you can achieve
>>
>> ^^^^ Pay attention to the above
>>
>> >>
>> >> rename ...
>> >>
>> >> line and see actual diff.
>> >>
>> >> No need to resend because of this. Just an explanation for the future Git work.
>> >
>> > I did use thos options. I used the following command:
>> >
>> > git format-patch -M -C --notes -v12 -o ~/patches/. @^^^^
>> >
>> > I really don't know what you are targeting. :)
>>
>> Please, read what I wrote above and the manual of git-format-patch.
>>
>> >> > +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
>> >> > +{
>> >> > +       struct list_head *position;
>> >> > +       struct acpi_battery *battery;
>> >>
>> >> Missed empty line?
>> >
>> > checkpatch.pl complains if there are NOT empty lines between
>> > declarations and statements.
>>
>> checkpatch some times on one hand complains about something which it
>> should not, on the other didn't take into consideration cases like
>> this one.
>>
>> Your statement started with comment, btw.
>>
>> >> > +       /*
>> >> > +        * In order to remove a hook, we first need to
>> >> > +        * de-register all the batteries that are registered.
>> >> > +        */
>> >> > +       if (lock)
>> >> > +               mutex_lock(&hook_mutex);
>>
>> > I mean, it's not game-breaking, its just minor style stuff. I won't be
>> > sending more revisions because of these small issues, as I think its
>> > uneccessary to flood both Rafael and the mailing lists with patch
>> > revisions that remove or add a few spaces. No offence, it just got old.
>>
>> Yes, his call anyway to apply or ask you for amendments. I'm just
>> helping with review.
>
> Rafael, what do you think? Do you want these style/syntax issues fixed
> or is it good to go?

As long as the code is all correct technically, they are secondary.

That said I still need to look into your patches in detail and I need
more time for that.

Thanks,
Rafael

Re: [Devel] [PATCH v12 1/4] battery: Add the battery hooking API

[Rafael J. Wysocki]

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

On Thu, Jan 4, 2018 at 12:13 PM, Ognjen Galić <smclt30p(a)gmail.com> wrote:
> On Wed, Jan 03, 2018 at 06:40:12PM +0200, Andy Shevchenko wrote:
>> On Wed, Jan 3, 2018 at 4:53 PM, Ognjen Galić <smclt30p(a)gmail.com> wrote:
>> > On Wed, Jan 03, 2018 at 04:25:42PM +0200, Andy Shevchenko wrote:
>> >> On Wed, Jan 3, 2018 at 1:58 PM, Ognjen Galic <smclt30p(a)gmail.com> wrote:
>>
>> >> Thanks for an update. I have couple of minors. Otherwise look pretty much good!
>> >>
>> >> >  drivers/acpi/battery.h |  11 ----
>> >> >  include/acpi/battery.h |  21 +++++++
>> >>
>> >> There are -M and -C command line parameters to git format-patch.
>>
>> >> They can take an optional argument (percentage) of threshold.
>> >>
>> >> Playing with those numbers you can achieve
>>
>> ^^^^ Pay attention to the above
>>
>> >>
>> >> rename ...
>> >>
>> >> line and see actual diff.
>> >>
>> >> No need to resend because of this. Just an explanation for the future Git work.
>> >
>> > I did use thos options. I used the following command:
>> >
>> > git format-patch -M -C --notes -v12 -o ~/patches/. @^^^^
>> >
>> > I really don't know what you are targeting. :)
>>
>> Please, read what I wrote above and the manual of git-format-patch.
>>
>> >> > +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
>> >> > +{
>> >> > +       struct list_head *position;
>> >> > +       struct acpi_battery *battery;
>> >>
>> >> Missed empty line?
>> >
>> > checkpatch.pl complains if there are NOT empty lines between
>> > declarations and statements.
>>
>> checkpatch some times on one hand complains about something which it
>> should not, on the other didn't take into consideration cases like
>> this one.
>>
>> Your statement started with comment, btw.
>>
>> >> > +       /*
>> >> > +        * In order to remove a hook, we first need to
>> >> > +        * de-register all the batteries that are registered.
>> >> > +        */
>> >> > +       if (lock)
>> >> > +               mutex_lock(&hook_mutex);
>>
>> > I mean, it's not game-breaking, its just minor style stuff. I won't be
>> > sending more revisions because of these small issues, as I think its
>> > uneccessary to flood both Rafael and the mailing lists with patch
>> > revisions that remove or add a few spaces. No offence, it just got old.
>>
>> Yes, his call anyway to apply or ask you for amendments. I'm just
>> helping with review.
>
> Rafael, what do you think? Do you want these style/syntax issues fixed
> or is it good to go?

As long as the code is all correct technically, they are secondary.

That said I still need to look into your patches in detail and I need
more time for that.

Thanks,
Rafael

Re: [PATCH v12 1/4] battery: Add the battery hooking API

[Rafael J. Wysocki]

On Wednesday, January 3, 2018 12:58:47 PM CET 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.
> 
> 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_*
> 
>  drivers/acpi/ac.c      |   2 +-
>  drivers/acpi/battery.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/acpi/battery.h |  11 ----
>  drivers/acpi/sbs.c     |   2 +-
>  include/acpi/battery.h |  21 +++++++
>  5 files changed, 171 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 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..7089e31 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: "
>  
> @@ -124,6 +128,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 +631,142 @@ 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 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 (lock)
> +		mutex_lock(&hook_mutex);
> +	list_for_each(position, &acpi_battery_list) {
> +		battery = list_entry(position, struct acpi_battery, 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);
> +
> +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 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, };
> @@ -647,6 +788,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);

I seem to see a problem here, as this may be called before
battery_hook_register() in some cases if I'm not mistaken and there seems to
be nothing to ensure that the "hook" code will actually see the battery then.

Thanks,
Rafael

Re: [Devel] [PATCH v12 1/4] battery: Add the battery hooking API

[Rafael J. Wysocki]

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

On Wednesday, January 3, 2018 12:58:47 PM CET 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.
> 
> Signed-off-by: Ognjen Galic <smclt30p(a)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_*
> 
>  drivers/acpi/ac.c      |   2 +-
>  drivers/acpi/battery.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/acpi/battery.h |  11 ----
>  drivers/acpi/sbs.c     |   2 +-
>  include/acpi/battery.h |  21 +++++++
>  5 files changed, 171 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 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..7089e31 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: "
>  
> @@ -124,6 +128,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 +631,142 @@ 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 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 (lock)
> +		mutex_lock(&hook_mutex);
> +	list_for_each(position, &acpi_battery_list) {
> +		battery = list_entry(position, struct acpi_battery, 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);
> +
> +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 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, };
> @@ -647,6 +788,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);

I seem to see a problem here, as this may be called before
battery_hook_register() in some cases if I'm not mistaken and there seems to
be nothing to ensure that the "hook" code will actually see the battery then.

Thanks,
Rafael

Re: [PATCH v12 1/4] battery: Add the battery hooking API

[Ognjen Galić]

On Sun, Feb 04, 2018 at 09:52:33AM +0100, Rafael J. Wysocki wrote:
> On Wednesday, January 3, 2018 12:58:47 PM CET 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.
> > 
> > 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_*
> > 
> >  drivers/acpi/ac.c      |   2 +-
> >  drivers/acpi/battery.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  drivers/acpi/battery.h |  11 ----
> >  drivers/acpi/sbs.c     |   2 +-
> >  include/acpi/battery.h |  21 +++++++
> >  5 files changed, 171 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 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..7089e31 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: "
> >  
> > @@ -124,6 +128,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 +631,142 @@ 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 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 (lock)
> > +		mutex_lock(&hook_mutex);
> > +	list_for_each(position, &acpi_battery_list) {
> > +		battery = list_entry(position, struct acpi_battery, 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);
> > +
> > +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 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, };
> > @@ -647,6 +788,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);
> 
> I seem to see a problem here, as this may be called before
> battery_hook_register() in some cases if I'm not mistaken and there seems to
> be nothing to ensure that the "hook" code will actually see the battery then.

There is a mechanism for late-loading of modules.
battery_hook_add_battery adds each battery to a list of batteries:

> static LIST_HEAD(acpi_battery_list);

When the ACPI battery drivers adds a battery to sysfs, the call to
battery_hook_add_battery adds the battery to the list that is being
added.

> ...
>	struct acpi_battery_hook *hook_node;
>
>	mutex_lock(&hook_mutex);
>	INIT_LIST_HEAD(&battery->list);
>	list_add(&battery->list, &acpi_battery_list);
> ...

Then, after adding the battery to the list it notifies the already
existing hooks of the new battery:

>	list_for_each_entry(hook_node, &battery_hook_list, list) {
>		if (hook_node->add_battery(battery->bat)) {
>			__battery_hook_unregister(hook_node, 0);
>			pr_err("error in extension, unloading: %s",
>					hook_node->name);
>		}
>	}

If and and after some module calls battery_hook_register AFTER
the battery has been added, the module is THEN notified of 
ALL the batteries added to sysfs, regardless of loading 
time or hooking of that module.

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

The order is like this for late hook loading:

* battery module loads
* battery constructs a list of batteries
* battery is ready

And some other module that NEEDS the battery hook API *MUST* load
*AFTER* battery, and it will *NOT* load if battery is not loaded. It
fails with a pre-load "symbol missing" error, *NOT* a runtime error.

* thinkpad_acpi loads
* thinkpad_acpi hooks into battery
* battery goes through the list of batteries and registers then in
  thinkpad_acpi
* thinkpad_acpi is ready

I tested this back and forth, removing batteries, adding them, loading
then adding batteries, back and forth and it registers the battery each
time. 

Thanks for the time.
Ognjen

> 
> Thanks,
> Rafael
> 

Re: [PATCH v12 1/4] battery: Add the battery hooking API

[Rafael J. Wysocki]

On Sun, Feb 4, 2018 at 10:11 AM, Ognjen Galić <smclt30p@gmail.com> wrote:
> On Sun, Feb 04, 2018 at 09:52:33AM +0100, Rafael J. Wysocki wrote:
>> On Wednesday, January 3, 2018 12:58:47 PM CET 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.
>> >
>> > 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_*
>> >
>> >  drivers/acpi/ac.c      |   2 +-
>> >  drivers/acpi/battery.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++-
>> >  drivers/acpi/battery.h |  11 ----
>> >  drivers/acpi/sbs.c     |   2 +-
>> >  include/acpi/battery.h |  21 +++++++
>> >  5 files changed, 171 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 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..7089e31 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: "
>> >
>> > @@ -124,6 +128,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 +631,142 @@ 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 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 (lock)
>> > +           mutex_lock(&hook_mutex);
>> > +   list_for_each(position, &acpi_battery_list) {
>> > +           battery = list_entry(position, struct acpi_battery, 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);
>> > +
>> > +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 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, };
>> > @@ -647,6 +788,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);
>>
>> I seem to see a problem here, as this may be called before
>> battery_hook_register() in some cases if I'm not mistaken and there seems to
>> be nothing to ensure that the "hook" code will actually see the battery then.
>
> There is a mechanism for late-loading of modules.
> battery_hook_add_battery adds each battery to a list of batteries:
>
>> static LIST_HEAD(acpi_battery_list);

OK, that should be fine then.

Please respin the series with the comments from Andy addressed and I
will queue it up.

Thanks,
Rafael

Re: [Devel] [PATCH v12 1/4] battery: Add the battery hooking API

[Rafael J. Wysocki]

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

On Sun, Feb 4, 2018 at 10:11 AM, Ognjen Galić <smclt30p(a)gmail.com> wrote:
> On Sun, Feb 04, 2018 at 09:52:33AM +0100, Rafael J. Wysocki wrote:
>> On Wednesday, January 3, 2018 12:58:47 PM CET 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.
>> >
>> > Signed-off-by: Ognjen Galic <smclt30p(a)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_*
>> >
>> >  drivers/acpi/ac.c      |   2 +-
>> >  drivers/acpi/battery.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++-
>> >  drivers/acpi/battery.h |  11 ----
>> >  drivers/acpi/sbs.c     |   2 +-
>> >  include/acpi/battery.h |  21 +++++++
>> >  5 files changed, 171 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 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..7089e31 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: "
>> >
>> > @@ -124,6 +128,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 +631,142 @@ 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 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 (lock)
>> > +           mutex_lock(&hook_mutex);
>> > +   list_for_each(position, &acpi_battery_list) {
>> > +           battery = list_entry(position, struct acpi_battery, 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);
>> > +
>> > +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 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, };
>> > @@ -647,6 +788,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);
>>
>> I seem to see a problem here, as this may be called before
>> battery_hook_register() in some cases if I'm not mistaken and there seems to
>> be nothing to ensure that the "hook" code will actually see the battery then.
>
> There is a mechanism for late-loading of modules.
> battery_hook_add_battery adds each battery to a list of batteries:
>
>> static LIST_HEAD(acpi_battery_list);

OK, that should be fine then.

Please respin the series with the comments from Andy addressed and I
will queue it up.

Thanks,
Rafael