Skip to contentSkip to conversation

ACPI / battery: safe unregistering of hooks

PATCH Superseded Jouke Witteveen 2 participants 3 messages Jun 23, 2018 – Jul 4, 2018 1 message not in the archive ACPI

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Jouke Witteveen

author
#0 lore raw

PATCH 1/1 ACPI / battery: safe unregistering of hooks

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 <[email protected]>
---
 drivers/acpi/battery.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/acpi/battery.c +5 -4
index b0113a58..d79ad844 100644
@@ -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);
Signature · 6 lines
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
author
#1 lore raw
Quoted 5 lines · 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 <[email protected]>

---

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

Quoted 50 lines
 drivers/acpi/battery.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/acpi/battery.c +5 -4
index b0113a58..d79ad844 100644
@@ -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);
Signature · 1 line
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

A message that is not in the archive (sent off-list or not yet received): 20180704092927.GA6544-7Pk1HLXsvWqf6HGZUO+WniZi+YwRKgec@public.gmane.org

#3 lore raw
Quoted 7 lines · Jouke Witteveen
Quoted 5 lines · 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 <[email protected]>

---

Quoted 6 lines

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?

Quoted 47 lines
Quoted 47 lines
 drivers/acpi/battery.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/acpi/battery.c +5 -4
index b0113a58..d79ad844 100644
@@ -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);
              }
      }
Quoted 8 lines
Quoted 4 lines
drivers/acpi/battery.c +0 -1
      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 [email protected] 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