[PATCH RESEND v3 0/3] platform/x86: dell-laptop: Battery hook fixes

[PATCH RESEND v3 0/3] platform/x86: dell-laptop: Battery hook fixes

[Armin Wolf]

This patch series fixes some issues around the battery hook handling
inside the ACPI battery driver and the dell-laptop driver.

The first patch simplifies the locking during battery hook removal as
a preparation for the second patch which fixes a possible crash when
unregistering a battery hook.

The third patch allows the dell-laptop driver to handle systems with
multiple batteries.

All patches where tested on a Dell Inspiron 3505 and appear to work.

Chnages since v2:
- drop boolean flag and use list head instead in patch 2
- drop mjg59@srcf.ucam.org from the CC list since the underlying
adress seems to trigger random errors in proccessing (help?)

Changes since v1:
- fix the underlying issue inside the ACPI battery driver
- reword patch for dell-laptop

Armin Wolf (3):
  ACPI: battery: Simplify battery hook locking
  ACPI: battery: Fix possible crash when unregistering a battery hook
  platform/x86: dell-laptop: Do not fail when encountering unsupported
    batteries

 drivers/acpi/battery.c                  | 28 +++++++++++++++----------
 drivers/platform/x86/dell/dell-laptop.c | 15 ++++++++++---
 2 files changed, 29 insertions(+), 14 deletions(-)

--
2.39.5

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

[Armin Wolf]

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>
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

[PATCH RESEND v3 2/3] ACPI: battery: Fix possible crash when unregistering a battery hook

[Armin Wolf]

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(-)

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

[PATCH RESEND v3 3/3] platform/x86: dell-laptop: Do not fail when encountering unsupported batteries

[Armin Wolf]

If the battery hook encounters a unsupported battery, it will
return an error. This in turn will cause the battery driver to
automatically unregister the battery hook.

On machines with multiple batteries however, this will prevent
the battery hook from handling the primary battery, since it will
always get unregistered upon encountering one of the unsupported
batteries.

Fix this by simply ignoring unsupported batteries.

Reviewed-by: Pali Rohár <pali@kernel.org>
Fixes: ab58016c68cc ("platform/x86:dell-laptop: Add knobs to change battery charge settings")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-laptop.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index a3cd0505f282..5671bd0deee7 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -2391,12 +2391,18 @@ static struct attribute *dell_battery_attrs[] = {
 };
 ATTRIBUTE_GROUPS(dell_battery);

+static bool dell_battery_supported(struct power_supply *battery)
+{
+	/* We currently only support the primary battery */
+	return strcmp(battery->desc->name, "BAT0") == 0;
+}
+
 static int dell_battery_add(struct power_supply *battery,
 		struct acpi_battery_hook *hook)
 {
-	/* this currently only supports the primary battery */
-	if (strcmp(battery->desc->name, "BAT0") != 0)
-		return -ENODEV;
+	/* Return 0 instead of an error to avoid being unloaded */
+	if (!dell_battery_supported(battery))
+		return 0;

 	return device_add_groups(&battery->dev, dell_battery_groups);
 }
@@ -2404,6 +2410,9 @@ static int dell_battery_add(struct power_supply *battery,
 static int dell_battery_remove(struct power_supply *battery,
 		struct acpi_battery_hook *hook)
 {
+	if (!dell_battery_supported(battery))
+		return 0;
+
 	device_remove_groups(&battery->dev, dell_battery_groups);
 	return 0;
 }
--
2.39.5

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

[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.

> 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
>

Re: [PATCH RESEND v3 2/3] ACPI: battery: Fix possible crash when unregistering a battery hook

[Rafael J. Wysocki]

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
>

Re: [PATCH RESEND v3 2/3] ACPI: battery: Fix possible crash when unregistering a battery hook

[Hans de Goede]

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
>>
> 

Re: [PATCH RESEND v3 2/3] ACPI: battery: Fix possible crash when unregistering a battery hook

[Rafael J. Wysocki]

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!

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@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
>>

Re: [PATCH RESEND v3 2/3] ACPI: battery: Fix possible crash when unregistering a battery hook

[Rafael J. Wysocki]

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!

Re: [PATCH RESEND v3 3/3] platform/x86: dell-laptop: Do not fail when encountering unsupported batteries

[Hans de Goede]

Hi,

On 1-Oct-24 11:28 PM, Armin Wolf wrote:
> If the battery hook encounters a unsupported battery, it will
> return an error. This in turn will cause the battery driver to
> automatically unregister the battery hook.
> 
> On machines with multiple batteries however, this will prevent
> the battery hook from handling the primary battery, since it will
> always get unregistered upon encountering one of the unsupported
> batteries.
> 
> Fix this by simply ignoring unsupported batteries.
> 
> Reviewed-by: Pali Rohár <pali@kernel.org>
> Fixes: ab58016c68cc ("platform/x86:dell-laptop: Add knobs to change battery charge settings")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thank you for your patch/series, I've applied this patch
(series) to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in the pdx86 review-hans branch once I've
pushed my local branch there, which might take a while.

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans



> ---
>  drivers/platform/x86/dell/dell-laptop.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index a3cd0505f282..5671bd0deee7 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -2391,12 +2391,18 @@ static struct attribute *dell_battery_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(dell_battery);
> 
> +static bool dell_battery_supported(struct power_supply *battery)
> +{
> +	/* We currently only support the primary battery */
> +	return strcmp(battery->desc->name, "BAT0") == 0;
> +}
> +
>  static int dell_battery_add(struct power_supply *battery,
>  		struct acpi_battery_hook *hook)
>  {
> -	/* this currently only supports the primary battery */
> -	if (strcmp(battery->desc->name, "BAT0") != 0)
> -		return -ENODEV;
> +	/* Return 0 instead of an error to avoid being unloaded */
> +	if (!dell_battery_supported(battery))
> +		return 0;
> 
>  	return device_add_groups(&battery->dev, dell_battery_groups);
>  }
> @@ -2404,6 +2410,9 @@ static int dell_battery_add(struct power_supply *battery,
>  static int dell_battery_remove(struct power_supply *battery,
>  		struct acpi_battery_hook *hook)
>  {
> +	if (!dell_battery_supported(battery))
> +		return 0;
> +
>  	device_remove_groups(&battery->dev, dell_battery_groups);
>  	return 0;
>  }
> --
> 2.39.5
> 
>