| Message ID | 20241001212835.341788-3-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: > > 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 the list head to mark already unregistered battery > hooks as already being unregistered 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> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Hans, are you going to take this series or should I apply it? > --- > drivers/acpi/battery.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > index dda59ee5a11e..1c45ff6dbb83 100644 > --- a/drivers/acpi/battery.c > +++ b/drivers/acpi/battery.c > @@ -715,7 +715,7 @@ static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook) > if (!hook->remove_battery(battery->bat, hook)) > power_supply_changed(battery->bat); > } > - list_del(&hook->list); > + list_del_init(&hook->list); > > pr_info("extension unregistered: %s\n", hook->name); > } > @@ -723,7 +723,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 (!list_empty(&hook->list)) > + battery_hook_unregister_unlocked(hook); > + > mutex_unlock(&hook_mutex); > } > EXPORT_SYMBOL_GPL(battery_hook_unregister); > @@ -733,7 +740,6 @@ void battery_hook_register(struct acpi_battery_hook *hook) > struct acpi_battery *battery; > > mutex_lock(&hook_mutex); > - INIT_LIST_HEAD(&hook->list); > list_add(&hook->list, &battery_hook_list); > /* > * Now that the driver is registered, we need > -- > 2.39.5 >
Hi, On 2-Oct-24 2:08 PM, Rafael J. Wysocki wrote: > On Tue, Oct 1, 2024 at 11:28 PM 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 the list head to mark already unregistered battery >> hooks as already being unregistered 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> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Hans, are you going to take this series or should I apply it? AFAICT the patches don't really depend on each other, so my plan was that you take patches 1-2 and I take patch 3 as a fix for 6.12-rc# . Does that work for you ? Regards, Hans > >> --- >> drivers/acpi/battery.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c >> index dda59ee5a11e..1c45ff6dbb83 100644 >> --- a/drivers/acpi/battery.c >> +++ b/drivers/acpi/battery.c >> @@ -715,7 +715,7 @@ static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook) >> if (!hook->remove_battery(battery->bat, hook)) >> power_supply_changed(battery->bat); >> } >> - list_del(&hook->list); >> + list_del_init(&hook->list); >> >> pr_info("extension unregistered: %s\n", hook->name); >> } >> @@ -723,7 +723,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 (!list_empty(&hook->list)) >> + battery_hook_unregister_unlocked(hook); >> + >> mutex_unlock(&hook_mutex); >> } >> EXPORT_SYMBOL_GPL(battery_hook_unregister); >> @@ -733,7 +740,6 @@ void battery_hook_register(struct acpi_battery_hook *hook) >> struct acpi_battery *battery; >> >> mutex_lock(&hook_mutex); >> - INIT_LIST_HEAD(&hook->list); >> list_add(&hook->list, &battery_hook_list); >> /* >> * Now that the driver is registered, we need >> -- >> 2.39.5 >> >
On Wed, Oct 2, 2024 at 2:35 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 2-Oct-24 2:08 PM, Rafael J. Wysocki wrote: > > On Tue, Oct 1, 2024 at 11:28 PM 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 the list head to mark already unregistered battery > >> hooks as already being unregistered 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> > > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Hans, are you going to take this series or should I apply it? > > AFAICT the patches don't really depend on each other, OK > so my plan was that you take patches 1-2 and I take patch 3 as a fix for > 6.12-rc# . > > Does that work for you ? Yes, it does, thanks!
On Wed, Oct 2, 2024 at 2:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Oct 2, 2024 at 2:35 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > Hi, > > > > On 2-Oct-24 2:08 PM, Rafael J. Wysocki wrote: > > > On Tue, Oct 1, 2024 at 11:28 PM 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 the list head to mark already unregistered battery > > >> hooks as already being unregistered 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> > > > > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Hans, are you going to take this series or should I apply it? > > > > AFAICT the patches don't really depend on each other, > > OK > > > so my plan was that you take patches 1-2 and I take patch 3 as a fix for > > 6.12-rc# . > > > > Does that work for you ? > > Yes, it does, thanks! Now queued up for 6.12-rc along with the [1/3], thanks!
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index dda59ee5a11e..1c45ff6dbb83 100644 --- a/drivers/acpi/battery.c +++ b/drivers/acpi/battery.c @@ -715,7 +715,7 @@ static void battery_hook_unregister_unlocked(struct acpi_battery_hook *hook) if (!hook->remove_battery(battery->bat, hook)) power_supply_changed(battery->bat); } - list_del(&hook->list); + list_del_init(&hook->list); pr_info("extension unregistered: %s\n", hook->name); } @@ -723,7 +723,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 (!list_empty(&hook->list)) + battery_hook_unregister_unlocked(hook); + mutex_unlock(&hook_mutex); } EXPORT_SYMBOL_GPL(battery_hook_unregister); @@ -733,7 +740,6 @@ void battery_hook_register(struct acpi_battery_hook *hook) struct acpi_battery *battery; mutex_lock(&hook_mutex); - INIT_LIST_HEAD(&hook->list); list_add(&hook->list, &battery_hook_list); /* * Now that the driver is registered, we need
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 the list head to mark already unregistered battery hooks as already being unregistered 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 | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) -- 2.39.5