ibm-acpi-devel — ibm-acpi Linux driver development

Re: [ibm-acpi-devel] 2nd Fan quirk for Thinkpad P50 causes spurios touchpad/trackpoint events on ThinkPad L570

[Jouke W. <j.w...@gm...>]

On Tue, Oct 23, 2018 at 6:06 AM Henrique de Moraes Holschuh
<hm...@hm...> wrote:
>
> On Mon, 15 Oct 2018, Dmitry Torokhov wrote:
> > On Sat, Oct 13, 2018 at 6:32 AM Jaak Ristioja <ja...@ri...> wrote:
> > > On 27.08.2018 19:22, Jaak Ristioja wrote:
> > > > Upgrading Linux from 4.16 to 4.17, a ThinkPad L570 started receiving
> > > > spurious input events, mostly right mouse button click events, but also
> > > > cursor jumps.
> > > >
> > > > I have not attempted to understand whether these events come from the
> > > > trackpoint or touchpad or some other driver, but I managed to bisect
> > > > this issue to commit a986c75a7df0 titled "platform/x86: thinkpad_acpi:
> > > > Add 2nd Fan Support for Thinkpad P50" by Alexander Kappner.
> > > >
> > > > Apparently the quirk mitigation is applied when the BIOS version begins
> > > > with N1. The BIOS version on the L570 in question is N1XET57W (1.35 )
> > > > which is probably why this commit causes the described problems on the
> > > > ThinkPad L570. How exactly? - I don't know.
> > > >
> > > > The issue did not reproduce when running some stable 4.17 and 4.18
> > > > kernels with commit a986c75a7df0 reverted.
> > > >
> > > > Please fix this for future kernels. Thanks! :)
> > >
> > > Ping. Do you need any additional information?
> >
> > Sounds like we need tighter check for the quirk, maybe based on
> > DMI/Board name? Can we revert the offending commit for now?
>
> Apparently it has to be fixed properly, because if I understood the
> issue correctly, every two-character quirk will trigger on *any*
> three-character model that starts with those two characters.

This should be (and hopefully is) incorrect.
In the tpacpi_check_quirks function, equality between the quirk bios
and the actual model bios is tested. For two-character models there
are three nonzero bytes, for two character models there are only two.
Therefore, a two-character quirk should not trigger on any
three-character model.

> We could revert the three-character quirk support, but this would cause
> regressions as well.

I doubt this would fix anything. Maybe the bios id for the L570 that
is used by the driver is not the one Jaak Ristioja reported? The
detected string is reported in the dmesg output.

That said, I do wonder why the fan quirks redefine all matching macros
(TPACPI_FAN_Q*) and don't use the generic ones (TPACPI_Q*). This looks
like an easy thing to clean up.

Regards,
- Jouke

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

Re: [ibm-acpi-devel] 2nd Fan quirk for Thinkpad P50 causes spurios touchpad/trackpoint events on ThinkPad L570

[Henrique de M. H. <hm...@hm...>]

On Mon, 15 Oct 2018, Dmitry Torokhov wrote:
> On Sat, Oct 13, 2018 at 6:32 AM Jaak Ristioja <ja...@ri...> wrote:
> > On 27.08.2018 19:22, Jaak Ristioja wrote:
> > > Upgrading Linux from 4.16 to 4.17, a ThinkPad L570 started receiving
> > > spurious input events, mostly right mouse button click events, but also
> > > cursor jumps.
> > >
> > > I have not attempted to understand whether these events come from the
> > > trackpoint or touchpad or some other driver, but I managed to bisect
> > > this issue to commit a986c75a7df0 titled "platform/x86: thinkpad_acpi:
> > > Add 2nd Fan Support for Thinkpad P50" by Alexander Kappner.
> > >
> > > Apparently the quirk mitigation is applied when the BIOS version begins
> > > with N1. The BIOS version on the L570 in question is N1XET57W (1.35 )
> > > which is probably why this commit causes the described problems on the
> > > ThinkPad L570. How exactly? - I don't know.
> > >
> > > The issue did not reproduce when running some stable 4.17 and 4.18
> > > kernels with commit a986c75a7df0 reverted.
> > >
> > > Please fix this for future kernels. Thanks! :)
> >
> > Ping. Do you need any additional information?
> 
> Sounds like we need tighter check for the quirk, maybe based on
> DMI/Board name? Can we revert the offending commit for now?

Apparently it has to be fixed properly, because if I understood the
issue correctly, every two-character quirk will trigger on *any*
three-character model that starts with those two characters.

We could revert the three-character quirk support, but this would cause
regressions as well.

-- 
  Henrique Holschuh

Re: [ibm-acpi-devel] 2nd Fan quirk for Thinkpad P50 causes spurios touchpad/trackpoint events on ThinkPad L570

[Henrique de M. H. <hm...@hm...>]

On Sat, 13 Oct 2018, Jaak Ristioja wrote:
> On 27.08.2018 19:22, Jaak Ristioja wrote:
> > Upgrading Linux from 4.16 to 4.17, a ThinkPad L570 started receiving
> > spurious input events, mostly right mouse button click events, but also
> > cursor jumps.
> > 
> > I have not attempted to understand whether these events come from the
> > trackpoint or touchpad or some other driver, but I managed to bisect
> > this issue to commit a986c75a7df0 titled "platform/x86: thinkpad_acpi:
> > Add 2nd Fan Support for Thinkpad P50" by Alexander Kappner.
> > 
> > Apparently the quirk mitigation is applied when the BIOS version begins
> > with N1. The BIOS version on the L570 in question is N1XET57W (1.35 )
> > which is probably why this commit causes the described problems on the
> > ThinkPad L570. How exactly? - I don't know.
> > 
> > The issue did not reproduce when running some stable 4.17 and 4.18
> > kernels with commit a986c75a7df0 reverted.
> > 
> > Please fix this for future kernels. Thanks! :)
> 
> Ping. Do you need any additional information?

It looks like it is failing to detect N1X and N1 as different models,
the bug is likely on this commit:

	commit 846a416b4630fc960c10500b4194ad504bc22d4b
	platform/x86: thinkpad_acpi: Proper model/release matching

Real life has been **extremely** busy, so I'd appreciate a patch.
Otherwise I will eventually get to it, but it might take a few
weeks(!!).

-- 
  Henrique Holschuh

Re: [ibm-acpi-devel] 2nd Fan quirk for Thinkpad P50 causes spurios touchpad/trackpoint events on ThinkPad L570

[Dmitry T. <dmi...@gm...>]

On Sat, Oct 13, 2018 at 6:32 AM Jaak Ristioja <ja...@ri...> wrote:
>
> On 27.08.2018 19:22, Jaak Ristioja wrote:
> > Upgrading Linux from 4.16 to 4.17, a ThinkPad L570 started receiving
> > spurious input events, mostly right mouse button click events, but also
> > cursor jumps.
> >
> > I have not attempted to understand whether these events come from the
> > trackpoint or touchpad or some other driver, but I managed to bisect
> > this issue to commit a986c75a7df0 titled "platform/x86: thinkpad_acpi:
> > Add 2nd Fan Support for Thinkpad P50" by Alexander Kappner.
> >
> > Apparently the quirk mitigation is applied when the BIOS version begins
> > with N1. The BIOS version on the L570 in question is N1XET57W (1.35 )
> > which is probably why this commit causes the described problems on the
> > ThinkPad L570. How exactly? - I don't know.
> >
> > The issue did not reproduce when running some stable 4.17 and 4.18
> > kernels with commit a986c75a7df0 reverted.
> >
> > Please fix this for future kernels. Thanks! :)
>
> Ping. Do you need any additional information?

Sounds like we need tighter check for the quirk, maybe based on
DMI/Board name? Can we revert the offending commit for now?

Thanks.

-- 
Dmitry

[ibm-acpi-devel] X1 Yoga 2nd: Tablet mode disables Bluetooth Thinkpad Keyboard

[Raghav R. <rag...@gm...>]

Linux raghav-ThinkPad-X1-Yoga-2nd 4.18.10-041810-generic #201809260332 SMP
Wed Sep 26 07:34:01 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

Very grateful for the tablet mode support but in tablet mode my input from
my ThinkPad Bluetooth Kayboard with Trackpoint gets ignored as well,
possibly because they have the same internals?

I am new to C and could not find how the keyboard detection works but my
guess is that it checks for every device that matches the ThinkPad keyboard
vendor ID for example. My guess is that this includes the USB and Bluetooth
ThinkPad keyboards with Trackpoint.

I have both on hand and I am willing to help debug in any way I can, I just
do not know how.

Thanks.

Sincerely,
Raghav

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

[Henrique de M. H. <hm...@hm...>]

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

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

[Yves-Alexis P. <co...@de...>]

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

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

[Thomas W. <li...@we...>]

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 <li...@we...> 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

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

[Yves-Alexis P. <co...@de...>]

-----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 <li...@we...> 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] unhandled HKEY event 0x6032

[MMS <mm...@pr...>]

Aug 31 20:52:18 T580-Test kernel: thinkpad_acpi: unknown possible thermal 
alarm or keyboard event received
Aug 31 20:52:18 T580-Test kernel: thinkpad_acpi: unhandled HKEY event 0x6032
Aug 31 20:52:18 T580-Test kernel: thinkpad_acpi: please report the conditions 
when this event happened to ibm...@li...
-------
This is what I get in my Lenovo T580 with MX150 dGPU when running glxsphere to 
test performance.
Together with that, the TouchPad get unresponsive until not working anymore, 
until I stop glxsphere.

What infos are needed? I'll provide.
System is running Kubuntu 18.04.01
with Nidia 396.

Re: [ibm-acpi-devel] [PATCH v2] platform/x86: thinkpad_acpi: extend battery quirk coverage

[Andy S. <and...@gm...>]

On Thu, Aug 2, 2018 at 11:28 PM, Jouke Witteveen <j.w...@gm...> wrote:
> Based on bug reports and a web search for
>     "Thinkpad_acpi: Error probing battery 2"
> four more models were found that require the battery quirk:
> Lenovo B5400, Thinkpad 11e, Thinkpad 11e gen 3, Thinkpad 13 gen 3.
>

Pushed to my review and testing queue, thanks!

> Signed-off-by: Jouke Witteveen <j.w...@gm...>
> Tested-by: James Cheshire <jer...@gm...>
> Acked-by: Henrique de Moraes Holschuh <hm...@hm...>
> ---
>
> I avoided the ping-pong style and added James Cheshire as a tester.
> Andy suggested to Cc: him, but if I understand the kernel documentation on
> submitting patches correctly, a Tested-by: is in order. James tested on a
> Thinkpad 11e and agreed to be included in the commit message.
>
>  drivers/platform/x86/thinkpad_acpi.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 3dbff2dd..fd49855a 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9611,7 +9611,11 @@ static const struct tpacpi_quirk battery_quirk_table[] __initconst = {
>          * Individual addressing is broken on models that expose the
>          * primary battery as BAT1.
>          */
> +       TPACPI_Q_LNV('J', '7', true),       /* B5400 */
> +       TPACPI_Q_LNV('J', 'I', true),       /* Thinkpad 11e */
> +       TPACPI_Q_LNV3('R', '0', 'B', true), /* Thinkpad 11e gen 3 */
>         TPACPI_Q_LNV3('R', '0', 'C', true), /* Thinkpad 13 */
> +       TPACPI_Q_LNV3('R', '0', 'J', true), /* Thinkpad 13 gen 2 */
>  };
>
>  static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
> --
> 2.18.0
>



-- 
With Best Regards,
Andy Shevchenko

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

[Andy S. <and...@gm...>]

On Thu, Aug 2, 2018 at 1:24 AM, Thomas Weißschuh <li...@we...> 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 <li...@we...>
> ---
>
> 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

[ibm-acpi-devel] [PATCH v2] platform/x86: thinkpad_acpi: extend battery quirk coverage

[Jouke W. <j.w...@gm...>]

Based on bug reports and a web search for
    "Thinkpad_acpi: Error probing battery 2"
four more models were found that require the battery quirk:
Lenovo B5400, Thinkpad 11e, Thinkpad 11e gen 3, Thinkpad 13 gen 3.

Signed-off-by: Jouke Witteveen <j.w...@gm...>
Tested-by: James Cheshire <jer...@gm...>
Acked-by: Henrique de Moraes Holschuh <hm...@hm...>
---

I avoided the ping-pong style and added James Cheshire as a tester.
Andy suggested to Cc: him, but if I understand the kernel documentation on
submitting patches correctly, a Tested-by: is in order. James tested on a
Thinkpad 11e and agreed to be included in the commit message.

 drivers/platform/x86/thinkpad_acpi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 3dbff2dd..fd49855a 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9611,7 +9611,11 @@ static const struct tpacpi_quirk battery_quirk_table[] __initconst = {
 	 * Individual addressing is broken on models that expose the
 	 * primary battery as BAT1.
 	 */
+	TPACPI_Q_LNV('J', '7', true),       /* B5400 */
+	TPACPI_Q_LNV('J', 'I', true),       /* Thinkpad 11e */
+	TPACPI_Q_LNV3('R', '0', 'B', true), /* Thinkpad 11e gen 3 */
 	TPACPI_Q_LNV3('R', '0', 'C', true), /* Thinkpad 13 */
+	TPACPI_Q_LNV3('R', '0', 'J', true), /* Thinkpad 13 gen 2 */
 };
 
 static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
-- 
2.18.0

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

[Thomas W. <li...@we...>]

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 <li...@we...>
---

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

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

[Thomas W. <li...@we...>]

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

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Extend battery quirk coverage

[Henrique de M. H. <hm...@hm...>]

On Wed, 01 Aug 2018, Andy Shevchenko wrote:
> On Wed, Aug 1, 2018 at 5:32 PM, Jouke Witteveen <j.w...@gm...> wrote:
> > Based on bug reports and a web search for
> >     "Thinkpad_acpi: Error probing battery 2"
> > four more models were found that require the battery quirk:
> > Lenovo B5400, Thinkpad 11e (original and gen 3), Thinkpad 13 gen 3.
> 
> I think you may add Henrique's ACK here since he did for previous "v2".
> But I let him speak for himself.

Yeah, the ack is still valid.

-- 
  Henrique Holschuh

Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Extend battery quirk coverage

[Andy S. <and...@gm...>]

On Wed, Aug 1, 2018 at 5:32 PM, Jouke Witteveen <j.w...@gm...> wrote:
> Based on bug reports and a web search for
>     "Thinkpad_acpi: Error probing battery 2"
> four more models were found that require the battery quirk:
> Lenovo B5400, Thinkpad 11e (original and gen 3), Thinkpad 13 gen 3.

I think you may add Henrique's ACK here since he did for previous "v2".
But I let him speak for himself.

> I could only get someone to verify this on the 11e. Other requests have
> gone unanswered. The error message (Error probing battery 2) is a pretty
> clear indication that the quirk is needed, however.

I see. Care to Cc that guy (if they wish, of course, to be Cc:ed)?

> -       TPACPI_Q_LNV3('R', '0', 'C', true), /* Thinkpad 13 */

> +       TPACPI_Q_LNV3('R', '0', 'C', true), /* 13 */

Please, avoid ping-pong style of fixes. Here just leave an original line.

-- 
With Best Regards,
Andy Shevchenko

[ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Extend battery quirk coverage

[Jouke W. <j.w...@gm...>]

Based on bug reports and a web search for
    "Thinkpad_acpi: Error probing battery 2"
four more models were found that require the battery quirk:
Lenovo B5400, Thinkpad 11e (original and gen 3), Thinkpad 13 gen 3.

Signed-off-by: Jouke Witteveen <j.w...@gm...>
---

I could only get someone to verify this on the 11e. Other requests have
gone unanswered. The error message (Error probing battery 2) is a pretty
clear indication that the quirk is needed, however.

@Andy Shevchenko: Thanks for applying my previous patches! Sorry for not
noticing this.

 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 3dbff2dd..8f0bdb1d 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9611,7 +9611,11 @@ static const struct tpacpi_quirk battery_quirk_table[] __initconst = {
 	 * Individual addressing is broken on models that expose the
 	 * primary battery as BAT1.
 	 */
-	TPACPI_Q_LNV3('R', '0', 'C', true), /* Thinkpad 13 */
+	TPACPI_Q_LNV('J', '7', true),       /* B5400 */
+	TPACPI_Q_LNV('J', 'I', true),       /* 11e */
+	TPACPI_Q_LNV3('R', '0', 'B', true), /* 11e gen 3 */
+	TPACPI_Q_LNV3('R', '0', 'C', true), /* 13 */
+	TPACPI_Q_LNV3('R', '0', 'J', true), /* 13 gen 2 */
 };
 
 static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
-- 
2.18.0

Re: [ibm-acpi-devel] [PATCH v2 2/2] platform/x86: thinkpad_acpi: Support battery quirk

[Henrique de M. H. <hm...@hm...>]

On Wed, 01 Aug 2018, Jouke Witteveen wrote:
> Some Thinkpads have a single battery, but expose it as BAT1. Use the quirks
> engine to force these machines into always addressing the primary battery.
> Without this, the battery name would resolve to the non-existent secondary
> battery and ACPI calls would fail.
> 
> Signed-off-by: Jouke Witteveen <j.w...@gm...>

Acked-by: Henrique de Moraes Holschuh <hm...@hm...>

> Compared to the previous version of this patch, four new models were found
> that need the quirk. It was tested on the Thinkpad 13 (three character ID)
> and the Thinkpad 11e (two character ID).
> 
> The other three IDs were scraped from bug reports where the driver reports
> 
>     Thinkpad_acpi: Error probing battery 2
> 
> These bug reports were found by a simple web search.
> 
>  drivers/platform/x86/thinkpad_acpi.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 2cd3ca7e..bc9a11b8 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -335,6 +335,7 @@ static struct {
>  	u32 second_fan:1;
>  	u32 beep_needs_two_args:1;
>  	u32 mixer_no_level_control:1;
> +	u32 battery_force_primary:1;
>  	u32 input_device_registered:1;
>  	u32 platform_drv_registered:1;
>  	u32 platform_drv_attrs_registered:1;
> @@ -343,7 +344,6 @@ static struct {
>  	u32 sensors_pdev_attrs_registered:1;
>  	u32 hotkey_poll_active:1;
>  	u32 has_adaptive_kbd:1;
> -	u32 battery:1;
>  } tp_features;
>  
>  static struct {
> @@ -471,6 +471,12 @@ do {									\
>  	  .ec = TPACPI_MATCH_ANY,		\
>  	  .quirks = (__quirk) }
>  
> +#define TPACPI_Q_LNV3(__id1, __id2, __id3, __quirk) \
> +	{ .vendor = PCI_VENDOR_ID_LENOVO,	\
> +	  .bios = TPID3(__id1, __id2, __id3),	\
> +	  .ec = TPACPI_MATCH_ANY,		\
> +	  .quirks = (__quirk) }
> +
>  #define TPACPI_QEC_LNV(__id1, __id2, __quirk)	\
>  	{ .vendor = PCI_VENDOR_ID_LENOVO,	\
>  	  .bios = TPACPI_MATCH_ANY,		\
> @@ -9423,7 +9429,8 @@ static int tpacpi_battery_probe(int battery)
>  static int tpacpi_battery_get_id(const char *battery_name)
>  {
>  
> -	if (strcmp(battery_name, "BAT0") == 0)
> +	if (strcmp(battery_name, "BAT0") == 0 ||
> +	    tp_features.battery_force_primary)
>  		return BAT_PRIMARY;
>  	if (strcmp(battery_name, "BAT1") == 0)
>  		return BAT_SECONDARY;
> @@ -9599,8 +9606,24 @@ static struct acpi_battery_hook battery_hook = {
>  
>  /* Subdriver init/exit */
>  
> +static const struct tpacpi_quirk battery_quirk_table[] __initconst = {
> +	/*
> +	 * Individual addressing is broken on models that expose the
> +	 * primary battery as BAT1.
> +	 */
> +	TPACPI_Q_LNV('J', '7', true),       /* B5400 */
> +	TPACPI_Q_LNV('J', 'I', true),       /* 11e */
> +	TPACPI_Q_LNV3('R', '0', 'B', true), /* 11e gen 3 */
> +	TPACPI_Q_LNV3('R', '0', 'C', true), /* 13 */
> +	TPACPI_Q_LNV3('R', '0', 'J', true), /* 13 gen 2 */
> +};
> +
>  static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
>  {
> +	tp_features.battery_force_primary = tpacpi_check_quirks(
> +					battery_quirk_table,
> +					ARRAY_SIZE(battery_quirk_table));
> +
>  	battery_hook_register(&battery_hook);
>  	return 0;
>  }

-- 
  Henrique Holschuh

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot

Re: [ibm-acpi-devel] [PATCH v2 1/2] platform/x86: thinkpad_acpi: Proper model/release matching

[Henrique de M. H. <hm...@hm...>]

On Wed, 01 Aug 2018, Jouke Witteveen wrote:
> Modern Thinkpads have three character model designators. Previously, they
> would be accepted, but recorded incompletely. Revision matching extracted
> the wrong bytes from the ID string. This made the use of quirks for modern
> machines impossible.
> 
> Fixes: 1b0eb5bc2413
> Signed-off-by: Jouke Witteveen <j.w...@gm...>

Acked-by: Henrique de Moraes Holschuh <hm...@hm...>

> This has now been tested on both 3-character models (Thinkpad 13) and two
> character models (Thinkpad 11e).
> The part one of this series is identical to the previously submitted patch.
> 
>  drivers/platform/x86/thinkpad_acpi.c | 98 ++++++++++++++--------------
>  1 file changed, 48 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index cae9b059..2cd3ca7e 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -358,9 +358,9 @@ struct thinkpad_id_data {
>  	char *bios_version_str;	/* Something like 1ZET51WW (1.03z) */
>  	char *ec_version_str;	/* Something like 1ZHT51WW-1.04a */
>  
> -	u16 bios_model;		/* 1Y = 0x5931, 0 = unknown */
> -	u16 ec_model;
> -	u16 bios_release;	/* 1ZETK1WW = 0x314b, 0 = unknown */
> +	u32 bios_model;		/* 1Y = 0x3159, 0 = unknown */
> +	u32 ec_model;
> +	u16 bios_release;	/* 1ZETK1WW = 0x4b31, 0 = unknown */
>  	u16 ec_release;
>  
>  	char *model_str;	/* ThinkPad T43 */
> @@ -444,17 +444,20 @@ do {									\
>  /*
>   * Quirk handling helpers
>   *
> - * ThinkPad IDs and versions seen in the field so far
> - * are two-characters from the set [0-9A-Z], i.e. base 36.
> + * ThinkPad IDs and versions seen in the field so far are
> + * two or three characters from the set [0-9A-Z], i.e. base 36.
>   *
>   * We use values well outside that range as specials.
>   */
>  
> -#define TPACPI_MATCH_ANY		0xffffU
> +#define TPACPI_MATCH_ANY		0xffffffffU
> +#define TPACPI_MATCH_ANY_VERSION	0xffffU
>  #define TPACPI_MATCH_UNKNOWN		0U
>  
> -/* TPID('1', 'Y') == 0x5931 */
> -#define TPID(__c1, __c2) (((__c2) << 8) | (__c1))
> +/* TPID('1', 'Y') == 0x3159 */
> +#define TPID(__c1, __c2)	(((__c1) << 8) | (__c2))
> +#define TPID3(__c1, __c2, __c3)	(((__c1) << 16) | ((__c2) << 8) | (__c3))
> +#define TPVER TPID
>  
>  #define TPACPI_Q_IBM(__id1, __id2, __quirk)	\
>  	{ .vendor = PCI_VENDOR_ID_IBM,		\
> @@ -476,8 +479,8 @@ do {									\
>  
>  struct tpacpi_quirk {
>  	unsigned int vendor;
> -	u16 bios;
> -	u16 ec;
> +	u32 bios;
> +	u32 ec;
>  	unsigned long quirks;
>  };
>  
> @@ -1647,16 +1650,16 @@ static void tpacpi_remove_driver_attributes(struct device_driver *drv)
>  	{ .vendor	= (__v),			\
>  	  .bios		= TPID(__id1, __id2),		\
>  	  .ec		= TPACPI_MATCH_ANY,		\
> -	  .quirks	= TPACPI_MATCH_ANY << 16	\
> -			  | (__bv1) << 8 | (__bv2) }
> +	  .quirks	= TPACPI_MATCH_ANY_VERSION << 16 \
> +			  | TPVER(__bv1, __bv2) }
>  
>  #define TPV_Q_X(__v, __bid1, __bid2, __bv1, __bv2,	\
>  		__eid, __ev1, __ev2)			\
>  	{ .vendor	= (__v),			\
>  	  .bios		= TPID(__bid1, __bid2),		\
>  	  .ec		= __eid,			\
> -	  .quirks	= (__ev1) << 24 | (__ev2) << 16 \
> -			  | (__bv1) << 8 | (__bv2) }
> +	  .quirks	= TPVER(__ev1, __ev2) << 16	\
> +			  | TPVER(__bv1, __bv2) }
>  
>  #define TPV_QI0(__id1, __id2, __bv1, __bv2) \
>  	TPV_Q(PCI_VENDOR_ID_IBM, __id1, __id2, __bv1, __bv2)
> @@ -1798,7 +1801,7 @@ static void __init tpacpi_check_outdated_fw(void)
>  	/* note that unknown versions are set to 0x0000 and we use that */
>  	if ((bios_version > thinkpad_id.bios_release) ||
>  	    (ec_version > thinkpad_id.ec_release &&
> -				ec_version != TPACPI_MATCH_ANY)) {
> +				ec_version != TPACPI_MATCH_ANY_VERSION)) {
>  		/*
>  		 * The changelogs would let us track down the exact
>  		 * reason, but it is just too much of a pain to track
> @@ -9808,36 +9811,37 @@ static int __init ibm_init(struct ibm_init_struct *iibm)
>  
>  /* Probing */
>  
> -static bool __pure __init tpacpi_is_fw_digit(const char c)
> +static char __init tpacpi_parse_fw_id(const char * const s,
> +				      u32 * model, u16 * release)
>  {
> -	return (c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z');
> -}
> +	int i;
> +
> +	if (!s || strlen(s) < 8)
> +		goto invalid;
> +
> +	for (i = 0; i < 8; i++)
> +		if (!((s[i] >= '0' && s[i] <= '9') ||
> +		      (s[i] >= 'A' && s[i] <= 'Z')))
> +			goto invalid;
>  
> -static bool __pure __init tpacpi_is_valid_fw_id(const char * const s,
> -						const char t)
> -{
>  	/*
>  	 * Most models: xxyTkkWW (#.##c)
>  	 * Ancient 570/600 and -SL lacks (#.##c)
>  	 */
> -	if (s && strlen(s) >= 8 &&
> -		tpacpi_is_fw_digit(s[0]) &&
> -		tpacpi_is_fw_digit(s[1]) &&
> -		s[2] == t &&
> -		(s[3] == 'T' || s[3] == 'N') &&
> -		tpacpi_is_fw_digit(s[4]) &&
> -		tpacpi_is_fw_digit(s[5]))
> -		return true;
> +	if (s[3] == 'T' || s[3] == 'N') {
> +		*model = TPID(s[0], s[1]);
> +		*release = TPVER(s[4], s[5]);
> +		return s[2];
>  
>  	/* New models: xxxyTkkW (#.##c); T550 and some others */
> -	return s && strlen(s) >= 8 &&
> -		tpacpi_is_fw_digit(s[0]) &&
> -		tpacpi_is_fw_digit(s[1]) &&
> -		tpacpi_is_fw_digit(s[2]) &&
> -		s[3] == t &&
> -		(s[4] == 'T' || s[4] == 'N') &&
> -		tpacpi_is_fw_digit(s[5]) &&
> -		tpacpi_is_fw_digit(s[6]);
> +	} else if (s[4] == 'T' || s[4] == 'N') {
> +		*model = TPID3(s[0], s[1], s[2]);
> +		*release = TPVER(s[5], s[6]);
> +		return s[3];
> +	}
> +
> +invalid:
> +	return '\0';
>  }
>  
>  /* returns 0 - probe ok, or < 0 - probe error.
> @@ -9849,6 +9853,7 @@ static int __must_check __init get_thinkpad_model_data(
>  	const struct dmi_device *dev = NULL;
>  	char ec_fw_string[18];
>  	char const *s;
> +	char t;
>  
>  	if (!tp)
>  		return -EINVAL;
> @@ -9868,15 +9873,11 @@ static int __must_check __init get_thinkpad_model_data(
>  		return -ENOMEM;
>  
>  	/* Really ancient ThinkPad 240X will fail this, which is fine */
> -	if (!(tpacpi_is_valid_fw_id(tp->bios_version_str, 'E') ||
> -	      tpacpi_is_valid_fw_id(tp->bios_version_str, 'C')))
> +	t = tpacpi_parse_fw_id(tp->bios_version_str,
> +			       &tp->bios_model, &tp->bios_release);
> +	if (t != 'E' && t != 'C')
>  		return 0;
>  
> -	tp->bios_model = tp->bios_version_str[0]
> -			 | (tp->bios_version_str[1] << 8);
> -	tp->bios_release = (tp->bios_version_str[4] << 8)
> -			 | tp->bios_version_str[5];
> -
>  	/*
>  	 * ThinkPad T23 or newer, A31 or newer, R50e or newer,
>  	 * X32 or newer, all Z series;  Some models must have an
> @@ -9895,12 +9896,9 @@ static int __must_check __init get_thinkpad_model_data(
>  			if (!tp->ec_version_str)
>  				return -ENOMEM;
>  
> -			if (tpacpi_is_valid_fw_id(ec_fw_string, 'H')) {
> -				tp->ec_model = ec_fw_string[0]
> -						| (ec_fw_string[1] << 8);
> -				tp->ec_release = (ec_fw_string[4] << 8)
> -						| ec_fw_string[5];
> -			} else {
> +			t = tpacpi_parse_fw_id(ec_fw_string,
> +					       &tp->ec_model, &tp->ec_release);
> +			if (t != 'H') {
>  				pr_notice("ThinkPad firmware release %s doesn't match the known patterns\n",
>  					  ec_fw_string);
>  				pr_notice("please report this to %s\n",

-- 
  Henrique Holschuh

Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: don't overwrite supported battery operations

[Henrique de M. H. <hm...@hm...>]

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
pla...@vg... 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

Re: [ibm-acpi-devel] [PATCH v2 1/2] platform/x86: thinkpad_acpi: Proper model/release matching

[Andy S. <and...@gm...>]

On Wed, Aug 1, 2018 at 12:22 PM, Jouke Witteveen <j.w...@gm...> wrote:
> Modern Thinkpads have three character model designators. Previously, they
> would be accepted, but recorded incompletely. Revision matching extracted
> the wrong bytes from the ID string. This made the use of quirks for modern
> machines impossible.
>
> Fixes: 1b0eb5bc2413
> Signed-off-by: Jouke Witteveen <j.w...@gm...>
> ---
>
> This has now been tested on both 3-character models (Thinkpad 13) and two
> character models (Thinkpad 11e).
> The part one of this series is identical to the previously submitted patch.

The patches you submitted before had been applied.
Please, rebase your stuff against our for-next branch and resend.

-- 
With Best Regards,
Andy Shevchenko

[ibm-acpi-devel] [PATCH v2 2/2] platform/x86: thinkpad_acpi: Support battery quirk

[Jouke W. <j.w...@gm...>]

Some Thinkpads have a single battery, but expose it as BAT1. Use the quirks
engine to force these machines into always addressing the primary battery.
Without this, the battery name would resolve to the non-existent secondary
battery and ACPI calls would fail.

Signed-off-by: Jouke Witteveen <j.w...@gm...>
---

Compared to the previous version of this patch, four new models were found
that need the quirk. It was tested on the Thinkpad 13 (three character ID)
and the Thinkpad 11e (two character ID).

The other three IDs were scraped from bug reports where the driver reports

    Thinkpad_acpi: Error probing battery 2

These bug reports were found by a simple web search.

 drivers/platform/x86/thinkpad_acpi.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 2cd3ca7e..bc9a11b8 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -335,6 +335,7 @@ static struct {
 	u32 second_fan:1;
 	u32 beep_needs_two_args:1;
 	u32 mixer_no_level_control:1;
+	u32 battery_force_primary:1;
 	u32 input_device_registered:1;
 	u32 platform_drv_registered:1;
 	u32 platform_drv_attrs_registered:1;
@@ -343,7 +344,6 @@ static struct {
 	u32 sensors_pdev_attrs_registered:1;
 	u32 hotkey_poll_active:1;
 	u32 has_adaptive_kbd:1;
-	u32 battery:1;
 } tp_features;
 
 static struct {
@@ -471,6 +471,12 @@ do {									\
 	  .ec = TPACPI_MATCH_ANY,		\
 	  .quirks = (__quirk) }
 
+#define TPACPI_Q_LNV3(__id1, __id2, __id3, __quirk) \
+	{ .vendor = PCI_VENDOR_ID_LENOVO,	\
+	  .bios = TPID3(__id1, __id2, __id3),	\
+	  .ec = TPACPI_MATCH_ANY,		\
+	  .quirks = (__quirk) }
+
 #define TPACPI_QEC_LNV(__id1, __id2, __quirk)	\
 	{ .vendor = PCI_VENDOR_ID_LENOVO,	\
 	  .bios = TPACPI_MATCH_ANY,		\
@@ -9423,7 +9429,8 @@ static int tpacpi_battery_probe(int battery)
 static int tpacpi_battery_get_id(const char *battery_name)
 {
 
-	if (strcmp(battery_name, "BAT0") == 0)
+	if (strcmp(battery_name, "BAT0") == 0 ||
+	    tp_features.battery_force_primary)
 		return BAT_PRIMARY;
 	if (strcmp(battery_name, "BAT1") == 0)
 		return BAT_SECONDARY;
@@ -9599,8 +9606,24 @@ static struct acpi_battery_hook battery_hook = {
 
 /* Subdriver init/exit */
 
+static const struct tpacpi_quirk battery_quirk_table[] __initconst = {
+	/*
+	 * Individual addressing is broken on models that expose the
+	 * primary battery as BAT1.
+	 */
+	TPACPI_Q_LNV('J', '7', true),       /* B5400 */
+	TPACPI_Q_LNV('J', 'I', true),       /* 11e */
+	TPACPI_Q_LNV3('R', '0', 'B', true), /* 11e gen 3 */
+	TPACPI_Q_LNV3('R', '0', 'C', true), /* 13 */
+	TPACPI_Q_LNV3('R', '0', 'J', true), /* 13 gen 2 */
+};
+
 static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
 {
+	tp_features.battery_force_primary = tpacpi_check_quirks(
+					battery_quirk_table,
+					ARRAY_SIZE(battery_quirk_table));
+
 	battery_hook_register(&battery_hook);
 	return 0;
 }
-- 
2.18.0

[ibm-acpi-devel] [PATCH v2 1/2] platform/x86: thinkpad_acpi: Proper model/release matching

[Jouke W. <j.w...@gm...>]

Modern Thinkpads have three character model designators. Previously, they
would be accepted, but recorded incompletely. Revision matching extracted
the wrong bytes from the ID string. This made the use of quirks for modern
machines impossible.

Fixes: 1b0eb5bc2413
Signed-off-by: Jouke Witteveen <j.w...@gm...>
---

This has now been tested on both 3-character models (Thinkpad 13) and two
character models (Thinkpad 11e).
The part one of this series is identical to the previously submitted patch.

 drivers/platform/x86/thinkpad_acpi.c | 98 ++++++++++++++--------------
 1 file changed, 48 insertions(+), 50 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index cae9b059..2cd3ca7e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -358,9 +358,9 @@ struct thinkpad_id_data {
 	char *bios_version_str;	/* Something like 1ZET51WW (1.03z) */
 	char *ec_version_str;	/* Something like 1ZHT51WW-1.04a */
 
-	u16 bios_model;		/* 1Y = 0x5931, 0 = unknown */
-	u16 ec_model;
-	u16 bios_release;	/* 1ZETK1WW = 0x314b, 0 = unknown */
+	u32 bios_model;		/* 1Y = 0x3159, 0 = unknown */
+	u32 ec_model;
+	u16 bios_release;	/* 1ZETK1WW = 0x4b31, 0 = unknown */
 	u16 ec_release;
 
 	char *model_str;	/* ThinkPad T43 */
@@ -444,17 +444,20 @@ do {									\
 /*
  * Quirk handling helpers
  *
- * ThinkPad IDs and versions seen in the field so far
- * are two-characters from the set [0-9A-Z], i.e. base 36.
+ * ThinkPad IDs and versions seen in the field so far are
+ * two or three characters from the set [0-9A-Z], i.e. base 36.
  *
  * We use values well outside that range as specials.
  */
 
-#define TPACPI_MATCH_ANY		0xffffU
+#define TPACPI_MATCH_ANY		0xffffffffU
+#define TPACPI_MATCH_ANY_VERSION	0xffffU
 #define TPACPI_MATCH_UNKNOWN		0U
 
-/* TPID('1', 'Y') == 0x5931 */
-#define TPID(__c1, __c2) (((__c2) << 8) | (__c1))
+/* TPID('1', 'Y') == 0x3159 */
+#define TPID(__c1, __c2)	(((__c1) << 8) | (__c2))
+#define TPID3(__c1, __c2, __c3)	(((__c1) << 16) | ((__c2) << 8) | (__c3))
+#define TPVER TPID
 
 #define TPACPI_Q_IBM(__id1, __id2, __quirk)	\
 	{ .vendor = PCI_VENDOR_ID_IBM,		\
@@ -476,8 +479,8 @@ do {									\
 
 struct tpacpi_quirk {
 	unsigned int vendor;
-	u16 bios;
-	u16 ec;
+	u32 bios;
+	u32 ec;
 	unsigned long quirks;
 };
 
@@ -1647,16 +1650,16 @@ static void tpacpi_remove_driver_attributes(struct device_driver *drv)
 	{ .vendor	= (__v),			\
 	  .bios		= TPID(__id1, __id2),		\
 	  .ec		= TPACPI_MATCH_ANY,		\
-	  .quirks	= TPACPI_MATCH_ANY << 16	\
-			  | (__bv1) << 8 | (__bv2) }
+	  .quirks	= TPACPI_MATCH_ANY_VERSION << 16 \
+			  | TPVER(__bv1, __bv2) }
 
 #define TPV_Q_X(__v, __bid1, __bid2, __bv1, __bv2,	\
 		__eid, __ev1, __ev2)			\
 	{ .vendor	= (__v),			\
 	  .bios		= TPID(__bid1, __bid2),		\
 	  .ec		= __eid,			\
-	  .quirks	= (__ev1) << 24 | (__ev2) << 16 \
-			  | (__bv1) << 8 | (__bv2) }
+	  .quirks	= TPVER(__ev1, __ev2) << 16	\
+			  | TPVER(__bv1, __bv2) }
 
 #define TPV_QI0(__id1, __id2, __bv1, __bv2) \
 	TPV_Q(PCI_VENDOR_ID_IBM, __id1, __id2, __bv1, __bv2)
@@ -1798,7 +1801,7 @@ static void __init tpacpi_check_outdated_fw(void)
 	/* note that unknown versions are set to 0x0000 and we use that */
 	if ((bios_version > thinkpad_id.bios_release) ||
 	    (ec_version > thinkpad_id.ec_release &&
-				ec_version != TPACPI_MATCH_ANY)) {
+				ec_version != TPACPI_MATCH_ANY_VERSION)) {
 		/*
 		 * The changelogs would let us track down the exact
 		 * reason, but it is just too much of a pain to track
@@ -9808,36 +9811,37 @@ static int __init ibm_init(struct ibm_init_struct *iibm)
 
 /* Probing */
 
-static bool __pure __init tpacpi_is_fw_digit(const char c)
+static char __init tpacpi_parse_fw_id(const char * const s,
+				      u32 * model, u16 * release)
 {
-	return (c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z');
-}
+	int i;
+
+	if (!s || strlen(s) < 8)
+		goto invalid;
+
+	for (i = 0; i < 8; i++)
+		if (!((s[i] >= '0' && s[i] <= '9') ||
+		      (s[i] >= 'A' && s[i] <= 'Z')))
+			goto invalid;
 
-static bool __pure __init tpacpi_is_valid_fw_id(const char * const s,
-						const char t)
-{
 	/*
 	 * Most models: xxyTkkWW (#.##c)
 	 * Ancient 570/600 and -SL lacks (#.##c)
 	 */
-	if (s && strlen(s) >= 8 &&
-		tpacpi_is_fw_digit(s[0]) &&
-		tpacpi_is_fw_digit(s[1]) &&
-		s[2] == t &&
-		(s[3] == 'T' || s[3] == 'N') &&
-		tpacpi_is_fw_digit(s[4]) &&
-		tpacpi_is_fw_digit(s[5]))
-		return true;
+	if (s[3] == 'T' || s[3] == 'N') {
+		*model = TPID(s[0], s[1]);
+		*release = TPVER(s[4], s[5]);
+		return s[2];
 
 	/* New models: xxxyTkkW (#.##c); T550 and some others */
-	return s && strlen(s) >= 8 &&
-		tpacpi_is_fw_digit(s[0]) &&
-		tpacpi_is_fw_digit(s[1]) &&
-		tpacpi_is_fw_digit(s[2]) &&
-		s[3] == t &&
-		(s[4] == 'T' || s[4] == 'N') &&
-		tpacpi_is_fw_digit(s[5]) &&
-		tpacpi_is_fw_digit(s[6]);
+	} else if (s[4] == 'T' || s[4] == 'N') {
+		*model = TPID3(s[0], s[1], s[2]);
+		*release = TPVER(s[5], s[6]);
+		return s[3];
+	}
+
+invalid:
+	return '\0';
 }
 
 /* returns 0 - probe ok, or < 0 - probe error.
@@ -9849,6 +9853,7 @@ static int __must_check __init get_thinkpad_model_data(
 	const struct dmi_device *dev = NULL;
 	char ec_fw_string[18];
 	char const *s;
+	char t;
 
 	if (!tp)
 		return -EINVAL;
@@ -9868,15 +9873,11 @@ static int __must_check __init get_thinkpad_model_data(
 		return -ENOMEM;
 
 	/* Really ancient ThinkPad 240X will fail this, which is fine */
-	if (!(tpacpi_is_valid_fw_id(tp->bios_version_str, 'E') ||
-	      tpacpi_is_valid_fw_id(tp->bios_version_str, 'C')))
+	t = tpacpi_parse_fw_id(tp->bios_version_str,
+			       &tp->bios_model, &tp->bios_release);
+	if (t != 'E' && t != 'C')
 		return 0;
 
-	tp->bios_model = tp->bios_version_str[0]
-			 | (tp->bios_version_str[1] << 8);
-	tp->bios_release = (tp->bios_version_str[4] << 8)
-			 | tp->bios_version_str[5];
-
 	/*
 	 * ThinkPad T23 or newer, A31 or newer, R50e or newer,
 	 * X32 or newer, all Z series;  Some models must have an
@@ -9895,12 +9896,9 @@ static int __must_check __init get_thinkpad_model_data(
 			if (!tp->ec_version_str)
 				return -ENOMEM;
 
-			if (tpacpi_is_valid_fw_id(ec_fw_string, 'H')) {
-				tp->ec_model = ec_fw_string[0]
-						| (ec_fw_string[1] << 8);
-				tp->ec_release = (ec_fw_string[4] << 8)
-						| ec_fw_string[5];
-			} else {
+			t = tpacpi_parse_fw_id(ec_fw_string,
+					       &tp->ec_model, &tp->ec_release);
+			if (t != 'H') {
 				pr_notice("ThinkPad firmware release %s doesn't match the known patterns\n",
 					  ec_fw_string);
 				pr_notice("please report this to %s\n",
-- 
2.18.0

Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: don't overwrite supported battery operations

[Thomas W. <li...@we...>]

Hi,

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.

> Signed-off-by: Thomas Weißschuh <li...@we...>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> [..]

Regards,
Thomas