Re: [PATCH v2 0/3] platform/x86: dell-laptop: Battery hook fixes

["Pali Rohár" <pali@kernel.org>]

On Sunday 22 September 2024 03:45:13 Andres Salomon wrote:
> On Sun, 22 Sep 2024 08:40:23 +0200
> Armin Wolf <W_Armin@gmx.de> 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.
> 
> The patches look good to me, btw; feel free to add my Reviewed-by
> if that's helpful.

For me these patches also looks good, so you can add also my
Reviewed-by. But please wait for the review from ACPI people.

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

It is possible that there is same issue.

> 
> > 
> > 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
> > 
> 
> 
> 
> -- 
> I'm available for contract & employment work, please contact me if
> interested.