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