| Message ID | 20241001212835.341788-2-W_Armin@gmx.de (mailing list archive) |
|---|---|
| State | In Next |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | platform/x86: dell-laptop: Battery hook fixes | expand |
On Tue, Oct 1, 2024 at 11:28 PM Armin Wolf <W_Armin@gmx.de> wrote: > > Move the conditional locking from __battery_hook_unregister() > into battery_hook_unregister() and rename the low-level function > to simplify the locking during battery hook removal. > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Reviewed-by: Rafael J. Wysocki <rafael@kernel.org> Well, "looks good to me" doesn't usually mean "Reviewed-by", but you can retain the tag in this particular case. But in the future please don't add Reviewed-by or Acked-by tags from people to patches if you have not actually received those tags. > Reviewed-by: Pali Rohár <pali@kernel.org> > Signed-off-by: Armin Wolf <W_Armin@gmx.de> > --- > drivers/acpi/battery.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > index f4599261cfc3..dda59ee5a11e 100644 > --- a/drivers/acpi/battery.c > +++ b/drivers/acpi/battery.c > @@ -703,28 +703,28 @@ static LIST_HEAD(acpi_battery_list); > static LIST_HEAD(battery_hook_list); > static DEFINE_MUTEX(hook_mutex); > > -static void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock) > +static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook) > { > struct acpi_battery *battery; > + > /* > * In order to remove a hook, we first need to > * de-register all the batteries that are registered. > */ > - if (lock) > - mutex_lock(&hook_mutex); > list_for_each_entry(battery, &acpi_battery_list, list) { > if (!hook->remove_battery(battery->bat, hook)) > power_supply_changed(battery->bat); > } > list_del(&hook->list); > - if (lock) > - mutex_unlock(&hook_mutex); > + > pr_info("extension unregistered: %s\n", hook->name); > } > > void battery_hook_unregister(struct acpi_battery_hook *hook) > { > - __battery_hook_unregister(hook, 1); > + mutex_lock(&hook_mutex); > + battery_hook_unregister_unlocked(hook); > + mutex_unlock(&hook_mutex); > } > EXPORT_SYMBOL_GPL(battery_hook_unregister); > > @@ -750,7 +750,7 @@ void battery_hook_register(struct acpi_battery_hook *hook) > * hooks. > */ > pr_err("extension failed to load: %s", hook->name); > - __battery_hook_unregister(hook, 0); > + battery_hook_unregister_unlocked(hook); > goto end; > } > > @@ -804,7 +804,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery) > */ > pr_err("error in extension, unloading: %s", > hook_node->name); > - __battery_hook_unregister(hook_node, 0); > + battery_hook_unregister_unlocked(hook_node); > } > } > mutex_unlock(&hook_mutex); > @@ -837,7 +837,7 @@ static void __exit battery_hook_exit(void) > * need to remove the hooks. > */ > list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) { > - __battery_hook_unregister(hook, 1); > + battery_hook_unregister(hook); > } > mutex_destroy(&hook_mutex); > } > -- > 2.39.5 >
Am 02.10.24 um 14:06 schrieb Rafael J. Wysocki: > On Tue, Oct 1, 2024 at 11:28 PM Armin Wolf <W_Armin@gmx.de> wrote: >> Move the conditional locking from __battery_hook_unregister() >> into battery_hook_unregister() and rename the low-level function >> to simplify the locking during battery hook removal. >> >> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org> > Well, "looks good to me" doesn't usually mean "Reviewed-by", but you > can retain the tag in this particular case. > > But in the future please don't add Reviewed-by or Acked-by tags from > people to patches if you have not actually received those tags. Sorry, it seems that i mixed up the Reviewed-by tags. I will try to be a bit more careful next time. Armin Wolf >> Reviewed-by: Pali Rohár <pali@kernel.org> >> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >> --- >> drivers/acpi/battery.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c >> index f4599261cfc3..dda59ee5a11e 100644 >> --- a/drivers/acpi/battery.c >> +++ b/drivers/acpi/battery.c >> @@ -703,28 +703,28 @@ static LIST_HEAD(acpi_battery_list); >> static LIST_HEAD(battery_hook_list); >> static DEFINE_MUTEX(hook_mutex); >> >> -static void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock) >> +static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook) >> { >> struct acpi_battery *battery; >> + >> /* >> * In order to remove a hook, we first need to >> * de-register all the batteries that are registered. >> */ >> - if (lock) >> - mutex_lock(&hook_mutex); >> list_for_each_entry(battery, &acpi_battery_list, list) { >> if (!hook->remove_battery(battery->bat, hook)) >> power_supply_changed(battery->bat); >> } >> list_del(&hook->list); >> - if (lock) >> - mutex_unlock(&hook_mutex); >> + >> pr_info("extension unregistered: %s\n", hook->name); >> } >> >> void battery_hook_unregister(struct acpi_battery_hook *hook) >> { >> - __battery_hook_unregister(hook, 1); >> + mutex_lock(&hook_mutex); >> + battery_hook_unregister_unlocked(hook); >> + mutex_unlock(&hook_mutex); >> } >> EXPORT_SYMBOL_GPL(battery_hook_unregister); >> >> @@ -750,7 +750,7 @@ void battery_hook_register(struct acpi_battery_hook *hook) >> * hooks. >> */ >> pr_err("extension failed to load: %s", hook->name); >> - __battery_hook_unregister(hook, 0); >> + battery_hook_unregister_unlocked(hook); >> goto end; >> } >> >> @@ -804,7 +804,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery) >> */ >> pr_err("error in extension, unloading: %s", >> hook_node->name); >> - __battery_hook_unregister(hook_node, 0); >> + battery_hook_unregister_unlocked(hook_node); >> } >> } >> mutex_unlock(&hook_mutex); >> @@ -837,7 +837,7 @@ static void __exit battery_hook_exit(void) >> * need to remove the hooks. >> */ >> list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) { >> - __battery_hook_unregister(hook, 1); >> + battery_hook_unregister(hook); >> } >> mutex_destroy(&hook_mutex); >> } >> -- >> 2.39.5 >>
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index f4599261cfc3..dda59ee5a11e 100644 --- a/drivers/acpi/battery.c +++ b/drivers/acpi/battery.c @@ -703,28 +703,28 @@ static LIST_HEAD(acpi_battery_list); static LIST_HEAD(battery_hook_list); static DEFINE_MUTEX(hook_mutex); -static void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock) +static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook) { struct acpi_battery *battery; + /* * In order to remove a hook, we first need to * de-register all the batteries that are registered. */ - if (lock) - mutex_lock(&hook_mutex); list_for_each_entry(battery, &acpi_battery_list, list) { if (!hook->remove_battery(battery->bat, hook)) power_supply_changed(battery->bat); } list_del(&hook->list); - if (lock) - mutex_unlock(&hook_mutex); + pr_info("extension unregistered: %s\n", hook->name); } void battery_hook_unregister(struct acpi_battery_hook *hook) { - __battery_hook_unregister(hook, 1); + mutex_lock(&hook_mutex); + battery_hook_unregister_unlocked(hook); + mutex_unlock(&hook_mutex); } EXPORT_SYMBOL_GPL(battery_hook_unregister); @@ -750,7 +750,7 @@ void battery_hook_register(struct acpi_battery_hook *hook) * hooks. */ pr_err("extension failed to load: %s", hook->name); - __battery_hook_unregister(hook, 0); + battery_hook_unregister_unlocked(hook); goto end; } @@ -804,7 +804,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery) */ pr_err("error in extension, unloading: %s", hook_node->name); - __battery_hook_unregister(hook_node, 0); + battery_hook_unregister_unlocked(hook_node); } } mutex_unlock(&hook_mutex); @@ -837,7 +837,7 @@ static void __exit battery_hook_exit(void) * need to remove the hooks. */ list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) { - __battery_hook_unregister(hook, 1); + battery_hook_unregister(hook); } mutex_destroy(&hook_mutex); }