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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Feb 4, 2018 at 10:11 AM, Ognjen Galić <smclt30p@xxxxxxxxx> 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@xxxxxxxxx>
>> > ---
>> >
>> > 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




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux