Re: [PATCH RESEND v3 1/3] ACPI: battery: Simplify battery hook locking

[Armin Wolf]

Am 02.10.24 um 14:06 schrieb Rafael J. Wysocki:

> On Tue, Oct 1, 2024 at 11:28 PM Armin Wolf <W_Armin@xxxxxx> 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@xxxxxxxxxxxxxxx>
> > Reviewed-by: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> > Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
> > ---
> >   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