Re: [PATCH] ACPI / battery: safe unregistering of hooks

Rafael J. Wysocki
2018-07-04T09:43:37Z

On Wed, Jul 4, 2018 at 11:29 AM, Jouke Witteveen <j.witteveen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sat, Jun 23, 2018 at 11:08:51AM +0200, Jouke Witteveen wrote:
>> As unregistering a hook takes it off the hook list, we should use a safe
>> for_each loop when we potentially unregister a hook.
>>
>> Signed-off-by: Jouke Witteveen <j.witteveen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>
> I would like to draw attention to this patch. It fixes a very noticable
> regression for owners of Thinkpad 13 laptops. The thinkpad_acpi driver
> currently does not support that line of laptops [1]. As a result, users
> cannot boot the 4.17 kernel on those machines.
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=200333#c1

Well, care to put that information into the patch changelog and
resend it, then?

Ideally, can you identify the kernel commit which introduced the regression?

>
>> drivers/acpi/battery.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index b0113a58..d79ad844 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -717,10 +717,11 @@ void battery_hook_register(struct acpi_battery_hook *hook)
>> */
>> pr_err("extension failed to load: %s", hook->name);
>> __battery_hook_unregister(hook, 0);
>> - return;
>> + goto end;
>> }
>> }
>> pr_info("new extension: %s\n", hook->name);
>> +end:
>> mutex_unlock(&hook_mutex);
>> }
>> EXPORT_SYMBOL_GPL(battery_hook_register);
>> @@ -732,7 +733,7 @@ EXPORT_SYMBOL_GPL(battery_hook_register);
>> */
>> static void battery_hook_add_battery(struct acpi_battery *battery)
>> {
>> - struct acpi_battery_hook *hook_node;
>> + struct acpi_battery_hook *hook_node, *tmp;
>>
>> mutex_lock(&hook_mutex);
>> INIT_LIST_HEAD(&battery->list);
>> @@ -744,15 +745,15 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
>> * when a battery gets hotplugged or initialized
>> * during the battery module initialization.
>> */
>> - list_for_each_entry(hook_node, &battery_hook_list, list) {
>> + list_for_each_entry_safe(hook_node, tmp, &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);
>> + __battery_hook_unregister(hook_node, 0);
>> }
>> }
>> mutex_unlock(&hook_mutex);
>> --
>> 2.18.0
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html