[PATCH] ACPI / battery: safe unregistering of hooks

[PATCH] ACPI / battery: safe unregistering of hooks

[Jouke Witteveen]

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


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

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

[Jouke Witteveen]

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

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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

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

[Rafael J. Wysocki]

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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot