[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <115271db-b7dd-4490-9f24-9e0a99dbf133@gmx.de>
Date: Thu, 19 Sep 2024 09:48:02 +0200
From: Armin Wolf <W_Armin@....de>
To: Andres Salomon <dilinger@...ued.net>
Cc: mjg59@...f.ucam.org, pali@...nel.org, rafael@...nel.org, lenb@...nel.org,
hdegoede@...hat.com, ilpo.jarvinen@...ux.intel.com,
platform-driver-x86@...r.kernel.org, linux-acpi@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: dell-laptop: Fix crash when unregistering
battery hook
Am 19.09.24 um 09:13 schrieb Andres Salomon:
> On Thu, 19 Sep 2024 08:33:32 +0200
> Armin Wolf <W_Armin@....de> 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.
>>
>> However as soon as the driver itself attempts to unregister the
>> already unregistered battery hook, a crash occurs due to a
>> corrupted linked list.
>>
>> Fix this by simply ignoring unsupported batteries.
>>
>> Tested on a Dell Inspiron 3505.
>>
>> Fixes: ab58016c68cc ("platform/x86:dell-laptop: Add knobs to change battery charge settings")
>> Signed-off-by: Armin Wolf <W_Armin@....de>
>> ---
>> I CCed the maintainers of the ACPI battery driver since i believe
>> that this patch highlights a general issue inside the battery hook
>> mechanism.
>>
>> This is because the same crash will be triggered should for example
>> device_add_groups() fail.
>>
>> Any ideas on how to solve this problem?
>> ---
> Hm, I see that when battery_hook_register() has the add_battery hook fail,
> then __battery_hook_unregister() calls the remove_battery hook. Does
> something like the following fix it?
>
> (note: not any kind of final patch, just a test.)
Not really, since the error can happen even while a new battery is being added,
which can happen anytime (even during battery hook removal). This means that using
a global variable would be prone to race conditions.
The issue can only be solved inside the ACPI battery driver itself, that is the reason
why i CCed the ACPI maintainers.
Aside from that, ignoring unsupported batteries allows the driver to work on
machines with multiple batteries.
Thanks,
Armin Wolf
>
> ---
> drivers/platform/x86/dell/dell-laptop.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index a9de19799f01..c7b92b2f7ed2 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -2390,21 +2390,29 @@ static struct attribute *dell_battery_attrs[] = {
> NULL,
> };
> ATTRIBUTE_GROUPS(dell_battery);
> +static bool bgroup_registered = false;
>
> static int dell_battery_add(struct power_supply *battery,
> struct acpi_battery_hook *hook)
> {
> + int err;
> +
> /* this currently only supports the primary battery */
> if (strcmp(battery->desc->name, "BAT0") != 0)
> return -ENODEV;
>
> - return device_add_groups(&battery->dev, dell_battery_groups);
> + err = device_add_groups(&battery->dev, dell_battery_groups);
> + if (!err)
> + bgroup_registered = true;
> +
> + return err;
> }
>
> static int dell_battery_remove(struct power_supply *battery,
> struct acpi_battery_hook *hook)
> {
> - device_remove_groups(&battery->dev, dell_battery_groups);
> + if (bgroup_registered)
> + device_remove_groups(&battery->dev, dell_battery_groups);
> return 0;
> }
>
Powered by blists - more mailing lists