From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Ognjen_Gali=c4=87?= Subject: Re: [PATCH 1/3 v6] battery: Add the battery hooking API Date: Wed, 20 Dec 2017 20:42:51 +0100 Message-ID: <4c3cd47d-e569-bb1b-eb55-ed0860b6ec63@gmail.com> References: <20171215165654.GA6209@thinkpad> <20171220193243.GA7459@thinkpad> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: Received: from mail-wr0-f194.google.com ([209.85.128.194]:34292 "EHLO mail-wr0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755534AbdLTTm4 (ORCPT ); Wed, 20 Dec 2017 14:42:56 -0500 In-Reply-To: <20171220193243.GA7459@thinkpad> Content-Language: en-US Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: "Rafael J. Wysocki" Cc: "Rafael J. Wysocki" , Len Brown , Robert Moore , Lv Zheng , ACPI Devel Maling List , devel@acpica.org, Darren Hart , Andy Shevchenko , Henrique de Moraes Holschuh , Sebastian Reichel , Platform Driver , ibm-acpi-devel@lists.sourceforge.net, Linux PM On 20.12.2017 20:32, Ognjen Galic wrote: > On Mon, Dec 18, 2017 at 07:04:07PM +0100, Rafael J. Wysocki wrote: >> On Fri, Dec 15, 2017 at 5:56 PM, 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. >>> >>> Tested-by: Kevin Locke >>> Tested-by: Christoph Böhmwalder >>> Signed-off-by: Ognjen Galic >>> --- >>> drivers/acpi/ac.c | 2 +- >>> drivers/acpi/battery.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++- >>> drivers/acpi/battery.h | 11 ---- >>> drivers/acpi/sbs.c | 2 +- >>> include/acpi/battery.h | 21 ++++++++ >>> 5 files changed, 160 insertions(+), 15 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 >>> #include >>> #include >>> -#include "battery.h" >>> +#include >>> >>> #define PREFIX "ACPI: " >>> >>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c >>> index 13e7b56..2951d07 100644 >>> --- a/drivers/acpi/battery.c >>> +++ b/drivers/acpi/battery.c >>> @@ -30,6 +30,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #include >>> >>> @@ -42,7 +43,7 @@ >>> #include >>> #include >>> >>> -#include "battery.h" >>> +#include >>> >>> #define PREFIX "ACPI: " >>> >>> @@ -124,6 +125,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 +628,137 @@ static const struct device_attribute alarm_attr = { >>> .store = acpi_battery_alarm_store, >>> }; >>> >>> +/* >>> + * The Battery Hooking API >>> + * >>> + * This API is used inside other drivers that need to expose >>> + * platform-specific behaviour within the generic driver in a >>> + * generic way. >>> + * >>> + */ >>> + >>> +LIST_HEAD(acpi_battery_list); >>> +LIST_HEAD(battery_hook_list); >>> + >>> +void battery_hook_unregister(struct acpi_battery_hook *hook) >>> +{ >>> + >>> + 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. >>> + */ >>> + >>> + list_for_each(position, &acpi_battery_list) { >>> + battery = list_entry(position, struct acpi_battery, list); >>> + hook->remove_battery(battery->bat); >>> + } >>> + >>> + /* Then, just remove the hook */ >>> + >>> + list_del(&hook->list); >>> + pr_info("battery: extension unregistered: %s\n", hook->name); >>> + >>> +} >>> +EXPORT_SYMBOL_GPL(battery_hook_unregister); >>> + >>> +void battery_hook_register(struct acpi_battery_hook *hook) >>> +{ >>> + struct list_head *position; >>> + struct acpi_battery *battery; >>> + >>> + INIT_LIST_HEAD(&hook->list); >>> + list_add(&hook->list, &battery_hook_list); >>> + >>> + /* >>> + * Now that the driver is registered, we need >>> + * to notify the hook that a battery is available >>> + * for each battery, so that the driver may add >>> + * its attributes. >>> + */ >>> + list_for_each(position, &acpi_battery_list) { >>> + battery = list_entry(position, struct acpi_battery, list); >>> + if (hook->add_battery(battery->bat)) { >>> + >>> + /* >>> + * If a add-battery returns non-zero, >>> + * the registration of the extension has failed, >>> + * we will unload it. >>> + */ >>> + pr_err("battery: extension failed to load: %s", >>> + hook->name); >>> + battery_hook_unregister(hook); >>> + return; >>> + >>> + } >>> + } >>> + >>> + pr_info("battery: new extension: %s\n", hook->name); >>> + >>> +} >>> +EXPORT_SYMBOL_GPL(battery_hook_register); >>> + >>> +static void battery_hook_add_battery(struct acpi_battery *battery) >>> +{ >>> + >>> + /* >>> + * This function gets called right after the battery sysfs >>> + * attributes have been added, so that the drivers that >>> + * define custom sysfs attributes can add their own. >>> + */ >>> + >>> + struct list_head *position; >>> + struct acpi_battery_hook *hook_node; >>> + >>> + INIT_LIST_HEAD(&battery->list); >>> + list_add(&battery->list, &acpi_battery_list); >>> + >>> + /* >>> + * Since we added a new battery to the list, we need to >>> + * iterate over the hooks and call add_battery for each >>> + * hook that was registered. This usually happens >>> + * when a battery gets hotplugged or initialized >>> + * during the battery module initialization. >>> + */ >>> + >>> + list_for_each(position, &battery_hook_list) { >>> + hook_node = list_entry(position, struct acpi_battery_hook, list); >>> + if (hook_node->add_battery(battery->bat)) { >>> + >>> + /* >>> + * The notification of the extensions has failed, to >>> + * prevent further errors we will unload the extension. >>> + */ >>> + battery_hook_unregister(hook_node); >>> + pr_err("battery: error in extension, unloading: %s", >>> + hook_node->name); >>> + } >>> + } >>> + >>> +} >>> + >>> +static void battery_hook_remove_battery(struct acpi_battery *battery) >>> +{ >>> + struct list_head *position; >>> + struct acpi_battery_hook *hook; >>> + >>> + /* >>> + * Before removing the hook, we need to remove all >>> + * custom attributes from the battery. >>> + */ >>> + list_for_each(position, &battery_hook_list) { >>> + hook = list_entry(position, struct acpi_battery_hook, list); >>> + hook->remove_battery(battery->bat); >>> + } >>> + >>> + /* Then, just remove the battery from the list */ >>> + >>> + list_del(&battery->list); >>> + >>> +} >>> + >>> static int sysfs_add_battery(struct acpi_battery *battery) >>> { >>> struct power_supply_config psy_cfg = { .drv_data = battery, }; >>> @@ -647,6 +780,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); >> What if a module registering the hook is loaded after >> sysfs_add_battery() has run for the battery object in question? > The battery module keeps a list of acpi_battery objects, and regardless > of the time the hook was registered the batteries will always be > registered. > >> What if it attempts to register a hook while this code is running? > I could not test that as the only module that uses this API is > thinkpad_acpi. > >> What if two entities try to add or remove hooks simultaneously? > Currently there is only one module using this API, so race conditions > can't occur, currently. However, this could and will change in the > future. See below. > >> Don't you think any locking or other synchronization is necessary? >> > In the last version of the patch, version 7, I have implemented locking > via mutual exclusions to future-proof the code. I have tested various > scenarios of loading and unloading the battery and thinkpad_acpi modules > and there are no obvious bugs or memory corruption. > >> Also, what if someone attempts to add a hook when the ACPI battery >> module is not loaded? Will that still work? > The loading of the module that uses the API will fail with the message > that symbols that the module needs are missing, so a module can't use > the hooks if battery is not loaded. > >> Thanks, >> Rafael I'm sorry for all the double emails and double replies, my e-mail client (mutt) is either buggy, my connection is bugging or I don't know how to use it. Probably the last. Sorry again. Ognjen