| Message ID | 20240922064026.496422-3-W_Armin@gmx.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: dell-laptop: Battery hook fixes | expand |
On Sun, Sep 22, 2024 at 8:40 AM Armin Wolf <W_Armin@gmx.de> wrote: > > When a battery hook returns an error when adding a new battery, then > the battery hook is automatically unregistered. > However the battery hook provider cannot know that, so it will later > call battery_hook_unregister() on the already unregistered battery > hook, resulting in a crash. > > Fix this by using a boolean flag to mark already unregistered battery > hooks as "dead" so that they can be ignored by > battery_hook_unregister(). > > Fixes: fa93854f7a7e ("battery: Add the battery hooking API") > Signed-off-by: Armin Wolf <W_Armin@gmx.de> > --- > drivers/acpi/battery.c | 11 ++++++++++- > include/acpi/battery.h | 1 + > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > index 10e9136897a7..b31a6183a082 100644 > --- a/drivers/acpi/battery.c > +++ b/drivers/acpi/battery.c > @@ -719,6 +719,7 @@ static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook) > power_supply_changed(battery->bat); > } > list_del(&hook->list); > + hook->dead = true; It looks like you could do list_del_init((&hook->list); here and then do a list_emtpy() check below. > > pr_info("extension unregistered: %s\n", hook->name); > } > @@ -726,7 +727,14 @@ static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook) > void battery_hook_unregister(struct acpi_battery_hook *hook) > { > mutex_lock(&hook_mutex); > - battery_hook_unregister_unlocked(hook); > + /* > + * Ignore already unregistered battery hooks. This might happen > + * if a battery hook was previously unloaded due to an error when > + * adding a new battery. > + */ > + if (!hook->dead) if (!list_empty(&hook->list)) > + battery_hook_unregister_unlocked(hook); > + > mutex_unlock(&hook_mutex); and the new struct field would not be necessary if I'm not mistaken. > } > EXPORT_SYMBOL_GPL(battery_hook_unregister); > @@ -737,6 +745,7 @@ void battery_hook_register(struct acpi_battery_hook *hook) > > mutex_lock(&hook_mutex); > INIT_LIST_HEAD(&hook->list); Also the above statement is redundant, so maybe drop it while you're at it? > + hook->dead = false; > list_add(&hook->list, &battery_hook_list); > /* > * Now that the driver is registered, we need > diff --git a/include/acpi/battery.h b/include/acpi/battery.h > index c93f16dfb944..5cfe132bb7f5 100644 > --- a/include/acpi/battery.h > +++ b/include/acpi/battery.h > @@ -16,6 +16,7 @@ struct acpi_battery_hook { > int (*add_battery)(struct power_supply *battery, struct acpi_battery_hook *hook); > int (*remove_battery)(struct power_supply *battery, struct acpi_battery_hook *hook); > struct list_head list; > + bool dead; > }; > > void battery_hook_register(struct acpi_battery_hook *hook); > --
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index 10e9136897a7..b31a6183a082 100644 --- a/drivers/acpi/battery.c +++ b/drivers/acpi/battery.c @@ -719,6 +719,7 @@ static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook) power_supply_changed(battery->bat); } list_del(&hook->list); + hook->dead = true; pr_info("extension unregistered: %s\n", hook->name); } @@ -726,7 +727,14 @@ static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook) void battery_hook_unregister(struct acpi_battery_hook *hook) { mutex_lock(&hook_mutex); - battery_hook_unregister_unlocked(hook); + /* + * Ignore already unregistered battery hooks. This might happen + * if a battery hook was previously unloaded due to an error when + * adding a new battery. + */ + if (!hook->dead) + battery_hook_unregister_unlocked(hook); + mutex_unlock(&hook_mutex); } EXPORT_SYMBOL_GPL(battery_hook_unregister); @@ -737,6 +745,7 @@ void battery_hook_register(struct acpi_battery_hook *hook) mutex_lock(&hook_mutex); INIT_LIST_HEAD(&hook->list); + hook->dead = false; list_add(&hook->list, &battery_hook_list); /* * Now that the driver is registered, we need diff --git a/include/acpi/battery.h b/include/acpi/battery.h index c93f16dfb944..5cfe132bb7f5 100644 --- a/include/acpi/battery.h +++ b/include/acpi/battery.h @@ -16,6 +16,7 @@ struct acpi_battery_hook { int (*add_battery)(struct power_supply *battery, struct acpi_battery_hook *hook); int (*remove_battery)(struct power_supply *battery, struct acpi_battery_hook *hook); struct list_head list; + bool dead; }; void battery_hook_register(struct acpi_battery_hook *hook);
When a battery hook returns an error when adding a new battery, then the battery hook is automatically unregistered. However the battery hook provider cannot know that, so it will later call battery_hook_unregister() on the already unregistered battery hook, resulting in a crash. Fix this by using a boolean flag to mark already unregistered battery hooks as "dead" so that they can be ignored by battery_hook_unregister(). Fixes: fa93854f7a7e ("battery: Add the battery hooking API") Signed-off-by: Armin Wolf <W_Armin@gmx.de> --- drivers/acpi/battery.c | 11 ++++++++++- include/acpi/battery.h | 1 + 2 files changed, 11 insertions(+), 1 deletion(-) -- 2.39.5