Re: [PATCH] thinkpad_acpi: don't overwrite supported battery operations

Re: [PATCH] thinkpad_acpi: don't overwrite supported battery operations

[Henrique de Moraes Holschuh]

On Mon, 30 Jul 2018, Thomas Weißschuh wrote:
> On Sat, 2018-06-16T11:39+0200, Thomas Weißschuh wrote:
> > Previously the struct containing the supported operations for all
> > batteries was zeroed every time a battery was probed.
> > This prevented all batteries except the lastly probed one from being
> > configured.
> 
> Is something wrong with this patch?
> If so I would like to remedy it.

Nah, it is fine.  Please resend it to
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA@public.gmane.org and I will ack it there.

However, it will have a trivial conflict with an already accepted patch
that adds quirk handling to tpacpi_battery_init().  The fix is obvious
(place the memset before the quirk handler is called).

-- 
  Henrique Holschuh

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

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

[Thomas Weißschuh]

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


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

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

[Thomas Weißschuh]

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

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


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

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

[Andy Shevchenko]

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
>



-- 
With Best Regards,
Andy Shevchenko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

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

[Yves-Alexis Perez]

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


_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

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

[Thomas Weißschuh]

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


_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

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

[Yves-Alexis Perez]

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


_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

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

[Henrique de Moraes Holschuh]

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.

-- 
  Henrique Holschuh


_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel