Am 22.09.24 um 09:45 schrieb Andres Salomon:
On Sun, 22 Sep 2024 08:40:23 +0200
Armin Wolf <W_Armin@xxxxxx> wrote:
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.
Can you tell me more about the system? What type of battery is the second
battery, and how is it attached? What do the kernel logs look like when the
two batteries are registered? I'm still confused as to how the same
battery->dev ends up being reused for multiple physical batteries.
Hi,
i think there is a misunderstanding here, each battery->dev is associated with a separate
battery. The issue is that the ACPI battery driver responsible for managing battery hooks
does not correctly handle battery hook errors during runtime. For example:
1. Driver registers a batter hook.
2. New ACPI battery is discovered, add_battery callback of the battery hook is called.
3. The callback returns an error for some reason.
4. ACPI battery driver automatically unregisters the battery hook.
5. Driver unregisters battery hook during removal.
6. Crash since battery hook was already unregistered by the ACPI battery driver.
This patch series fixes this by adding a boolean flag to signal that a battery hook was
already unregistered, so the ACPI battery driver can ignore those battery hooks in
battery_hook_unregister().
The patches look good to me, btw; feel free to add my Reviewed-by
if that's helpful.
Also, with the caveat that I'm not quite understanding the aforementioned
battery->dev conflict - worth noting that dell-laptop isn't the only driver
that could have this problem with multiple batteries. A quick glance
through some other drivers:
- asus-wmi.c does basically the same thing in checking for just the first
battery, and the comment implies that there may be multiple batteries.
- system76.c claims that the EC only supports one battery, so maybe that
one is okay? But to be on the safe side, it should probably do the same
thing.
- thinkpad_acpi.c actually supports multiple batteries, so maybe it
doesn't have the problem. But if tpacpi_battery_probe() fails for one
of the batteries and the battery->dev is shared between the two
batteries, then same issue?
Good point regarding asus-wmi and system76, maybe we should provide some documentation
for writing battery hook providers.
Thanks,
Armin Wolf
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 | 27 ++++++++++++++++---------
drivers/platform/x86/dell/dell-laptop.c | 15 +++++++++++---
include/acpi/battery.h | 1 +
3 files changed, 31 insertions(+), 12 deletions(-)
--
2.39.5
