diff mbox series

[v2] platform/x86: thinkpad_acpi: Fix multi-battery bug

Message ID 20180801222418.21892-1-linux@weissschuh.net (mailing list archive)
State Accepted, archived
Delegated to: Andy Shevchenko
Headers show
Series [v2] platform/x86: thinkpad_acpi: Fix multi-battery bug | expand

Commit Message

Thomas Weißschuh Aug. 1, 2018, 10:24 p.m. UTC
The struct containing the supported operations for all batteries is
being zeroed on each battery probe.  This prevents all other batteries
except the lastly probed one from being configured.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---

Changes since v1:

* Missing sign-off added

 drivers/platform/x86/thinkpad_acpi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Aug. 6, 2018, 1:24 p.m. UTC | #1
On Thu, Aug 2, 2018 at 1:24 AM, Thomas Weißschuh <linux@weissschuh.net> wrote:
> The struct containing the supported operations for all batteries is
> being zeroed on each battery probe.  This prevents all other batteries
> except the lastly probed one from being configured.

You forgot to include subsystem maintainers along with driver maintainer.

I pushed this to my review and testing queue, meanwhile I would wait
for Ack from Henrique.

>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>
> Changes since v1:
>
> * Missing sign-off added
>
>  drivers/platform/x86/thinkpad_acpi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 3dbff2dda9be..60de1c577ce0 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9374,7 +9374,9 @@ static int tpacpi_battery_probe(int battery)
>  {
>         int ret = 0;
>
> -       memset(&battery_info, 0, sizeof(struct tpacpi_battery_driver_data));
> +       memset(&battery_info.batteries[battery], 0,
> +               sizeof(battery_info.batteries[battery]));
> +
>         /*
>          * 1) Get the current start threshold
>          * 2) Check for support
> @@ -9616,6 +9618,8 @@ static const struct tpacpi_quirk battery_quirk_table[] __initconst = {
>
>  static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
>  {
> +       memset(&battery_info, 0, sizeof(battery_info));
> +
>         tp_features.battery_force_primary = tpacpi_check_quirks(
>                                         battery_quirk_table,
>                                         ARRAY_SIZE(battery_quirk_table));
> --
> 2.18.0
>
Yves-Alexis Perez Sept. 7, 2018, 11:41 a.m. UTC | #2
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Mon, 2018-08-06 at 16:24 +0300, Andy Shevchenko wrote:
> On Thu, Aug 2, 2018 at 1:24 AM, Thomas Weißschuh <linux@weissschuh.net> wrote:
> > The struct containing the supported operations for all batteries is
> > being zeroed on each battery probe.  This prevents all other batteries
> > except the lastly probed one from being configured.
> 
> You forgot to include subsystem maintainers along with driver maintainer.
> 
> I pushed this to my review and testing queue, meanwhile I would wait
> for Ack from Henrique.

Hi Andy, Thomas and Henrique,

was this included in any tree and is it on route to Linus somehow?

My ThinkPad X250 has two batteries and I can only configure threshold on BAT0,
and I guess it's because of this, so it'd be nice to have it fixed (and maybe
backported to relevant stable kernels).

Regards,
- -- 
Yves-Alexis
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEE8vi34Qgfo83x35gF3rYcyPpXRFsFAluSY80ACgkQ3rYcyPpX
RFsc9Qf+NJ4zlKNzfxh2Ehtr4n8No6X/eqSHD1Jwq28eNzYm8wUmuIwQdzRyYCgA
pnsexe+7QDeDYNHgr251lTdvj+6Iz3dwLE/WCLPCxnXa8Ja02MGZxRyYt1XiGiYu
xmGP8P62Ae384xNalUti8HxX9juGJIHupTCFUUYcKntgmYs7YSW/Ukm/JX/NuQ2h
QdexjuqugxsSM35dHYzC9S9VumFvs5ed8oHodeIkUMoFgH1BCiqA3Nih87Mkn+Zd
wuFWJm9vkem6lySXGjlusRbOrkhOadCOP6RtNG4cSfJPYNr2GbPkyb5w1uVv+M95
XN9xWc0pv2wTvJs93sgZgUR1Fa+/JQ==
=vN7W
-----END PGP SIGNATURE-----
Thomas Weißschuh Sept. 7, 2018, 5:59 p.m. UTC | #3
Hi Yves-Alexis,

On Fri, 2018-09-07T13:41+0200, Yves-Alexis Perez wrote:
> On Mon, 2018-08-06 at 16:24 +0300, Andy Shevchenko wrote:
> > On Thu, Aug 2, 2018 at 1:24 AM, Thomas Weißschuh <linux@weissschuh.net> wrote:
> > > The struct containing the supported operations for all batteries is
> > > being zeroed on each battery probe.  This prevents all other batteries
> > > except the lastly probed one from being configured.
> > 
> > You forgot to include subsystem maintainers along with driver maintainer.
> > 
> > I pushed this to my review and testing queue, meanwhile I would wait
> > for Ack from Henrique.
> 
> was this included in any tree and is it on route to Linus somehow?

This is on track to be released in 4.19.

> My ThinkPad X250 has two batteries and I can only configure threshold on BAT0,
> and I guess it's because of this, so it'd be nice to have it fixed (and maybe
> backported to relevant stable kernels).

As far as I know only changes that fix behaviour that worked before are
eligible for stable. As this specific functionality never worked before I
figured it would be moot to also send it to stable.

(If it is fine to send stuff like this to stable, we could try, though)

Thomas
Yves-Alexis Perez Sept. 7, 2018, 9:25 p.m. UTC | #4
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Fri, 2018-09-07 at 19:59 +0200, Thomas Weißschuh wrote:
> > was this included in any tree and is it on route to Linus somehow?
> 
> This is on track to be released in 4.19.

Thanks. I've applied the three patches (this one and the two from Jouke) on
top of 4.18.6 and I confirm I can set the thresholds on BAT1.
> 
> > My ThinkPad X250 has two batteries and I can only configure threshold on BAT0,
> > and I guess it's because of this, so it'd be nice to have it fixed (and maybe
> > backported to relevant stable kernels).
> 
> As far as I know only changes that fix behaviour that worked before are
> eligible for stable. As this specific functionality never worked before I
> figured it would be moot to also send it to stable.

Well, it does somehow work when you have one battery, but it's really
frustrating to have it not work for the second one.
> 
> (If it is fine to send stuff like this to stable, we could try, though)

Indeed, not sure if “frustrating” is reason enough for stable :)

Thanks for your work anyway.

Regards,
- -- 
Yves-Alexis
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEE8vi34Qgfo83x35gF3rYcyPpXRFsFAluS7NMACgkQ3rYcyPpX
RFtA1wgAv4JmftXLoE6ZJy3wIVaEM5sC5ys3Q1hBMca7tfqECupR6WMyPmaholdl
birWkLVLF6N6WZowlzB/LuLSHHvBwsWzF5VEufnziOTsyxuqGRRdBm9yiNPpG4dj
NEGxg2n0ju98DLTEn9HzxusqF/ahje11DfHItE0xU1gZHrPR7JVLJDvHlmbrEu0R
jBEBMmpAtpqFVlkSSxgalu6Ms9FvM7cLEOu4tGCv5tjjvK3+1ZKVqs9nOVrBDGmn
eWjGod3HDjmC/npJ3LdiXAMyySr9ohnt0WEBmcUAPFkpaGyNctzgiOYlrQrDsegg
lD5acn7wbt7XvamzAKjRbFhFpnQNrQ==
=56YF
-----END PGP SIGNATURE-----
Henrique de Moraes Holschuh Sept. 7, 2018, 9:37 p.m. UTC | #5
On Fri, 07 Sep 2018, Yves-Alexis Perez wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> On Fri, 2018-09-07 at 19:59 +0200, Thomas Weißschuh wrote:
> > > was this included in any tree and is it on route to Linus somehow?
> > 
> > This is on track to be released in 4.19.
> 
> Thanks. I've applied the three patches (this one and the two from Jouke) on
> top of 4.18.6 and I confirm I can set the thresholds on BAT1.
> > 
> > > My ThinkPad X250 has two batteries and I can only configure threshold on BAT0,
> > > and I guess it's because of this, so it'd be nice to have it fixed (and maybe
> > > backported to relevant stable kernels).
> > 
> > As far as I know only changes that fix behaviour that worked before are
> > eligible for stable. As this specific functionality never worked before I
> > figured it would be moot to also send it to stable.
> 
> Well, it does somehow work when you have one battery, but it's really
> frustrating to have it not work for the second one.
> > 
> > (If it is fine to send stuff like this to stable, we could try, though)
> 
> Indeed, not sure if “frustrating” is reason enough for stable :)

FWIW, once given enough testing, yes, I think this should be proposed
for -stable.
diff mbox series

Patch

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 3dbff2dda9be..60de1c577ce0 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9374,7 +9374,9 @@  static int tpacpi_battery_probe(int battery)
 {
 	int ret = 0;
 
-	memset(&battery_info, 0, sizeof(struct tpacpi_battery_driver_data));
+	memset(&battery_info.batteries[battery], 0,
+		sizeof(battery_info.batteries[battery]));
+
 	/*
 	 * 1) Get the current start threshold
 	 * 2) Check for support
@@ -9616,6 +9618,8 @@  static const struct tpacpi_quirk battery_quirk_table[] __initconst = {
 
 static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
 {
+	memset(&battery_info, 0, sizeof(battery_info));
+
 	tp_features.battery_force_primary = tpacpi_check_quirks(
 					battery_quirk_table,
 					ARRAY_SIZE(battery_quirk_table));