|
From: Ognjen G. <smc...@gm...> - 2017-12-20 19:43:01
|
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 <smc...@gm...> 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 <ke...@ke...>
>>> Tested-by: Christoph Böhmwalder <chr...@bo...>
>>> Signed-off-by: Ognjen Galic <smc...@gm...>
>>> ---
>>> 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 <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..2951d07 100644
>>> --- a/drivers/acpi/battery.c
>>> +++ b/drivers/acpi/battery.c
>>> @@ -30,6 +30,7 @@
>>> #include <linux/dmi.h>
>>> #include <linux/delay.h>
>>> #include <linux/slab.h>
>>> +#include <linux/list.h>
>>> #include <linux/suspend.h>
>>> #include <asm/unaligned.h>
>>>
>>> @@ -42,7 +43,7 @@
>>> #include <linux/acpi.h>
>>> #include <linux/power_supply.h>
>>>
>>> -#include "battery.h"
>>> +#include <acpi/battery.h>
>>>
>>> #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
|
