Bug 106231 - Call-By-Reference-Constant - battery and power adapter not working, MSHW0011 driver needed, even in windows - Surface 3
Summary: Call-By-Reference-Constant - battery and power adapter not working, MSHW0011...
Status: CLOSED CODE_FIX
Alias: None
Product: ACPI
Classification: Unclassified
Component: Power-Battery (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Lv Zheng
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-19 11:10 UTC by Bastien Nocera
Modified: 2017-10-16 07:51 UTC (History)
6 users (show)

See Also:
Kernel Version: 4.3 rc5
Tree: Mainline
Regression: No


Attachments
Surface-3-ACPI-tables.tar.gz (24.33 KB, application/octet-stream)
2015-10-22 10:23 UTC, Bastien Nocera
Details
acpidump -c off output (330.65 KB, text/plain)
2015-10-23 11:28 UTC, Bastien Nocera
Details
acpidump -c on output (328.46 KB, text/plain)
2015-10-23 11:29 UTC, Bastien Nocera
Details
[PATCH] ACPICA: Namespace: Add scope information to the simple object repair mechanism (4.66 KB, patch)
2015-10-28 06:53 UTC, Lv Zheng
Details | Diff
[PATCH] ACPICA: Namespace: Add String -> object_reference conversion support (4.79 KB, patch)
2015-10-28 06:53 UTC, Lv Zheng
Details | Diff
dmesg output (228.90 KB, text/plain)
2015-10-28 10:53 UTC, Bastien Nocera
Details
0001-WIP-MSHW0011-rev-eng-implementation.patch (11.67 KB, patch)
2016-05-19 16:52 UTC, Benjamin Tissoires
Details | Diff
0001-power-MSHW0011-rev-eng-implementation.patch (15.33 KB, patch)
2016-05-24 20:36 UTC, Benjamin Tissoires
Details | Diff
0001-power-MSHW0011-rev-eng-implementation-v2.patch (18.77 KB, patch)
2016-06-07 11:32 UTC, Benjamin Tissoires
Details | Diff
0002-power-surface3_power-Improve-battery-capacity-report.patch (4.21 KB, patch)
2016-06-29 22:33 UTC, Stephen Just
Details | Diff

Description Bastien Nocera 2015-10-19 11:10:49 UTC
attachment 187171 [details] contains the decompiled DSDT.

Oct 19 15:00:15 localhost.localdomain kernel: ACPI Warning: \_SB_.ADP1._DEP: Return Package type mismatch at index 0 - found String, expected Reference (20150818/nspredef-297)
Oct 19 15:00:15 localhost.localdomain kernel: ACPI Warning: \_SB_.BAT0._DEP: Return Package type mismatch at index 0 - found String, expected Reference (20150818/nspredef-297)
Comment 1 Aaron Lu 2015-10-21 02:11:14 UTC
Lv,

For your reference, here is the ASL code:
        Device (BAT0)
        {
            Name (_HID, EisaId ("PNP0C0A") /* Control Method Battery */)  // _HID: Hardware ID
            Name (_UID, One)  // _UID: Unique ID
            Name (_DEP, Package (0x02)  // _DEP: Dependencies
            {
                "\\_SB.PCI0.I2C1",
                I2Z1
            })


And ACPI spec said for _DEP:
Return Value:
A variable-length Package containing object references.
Comment 2 Lv Zheng 2015-10-22 02:14:52 UTC
Hi, Bastien

Could you upload full acpidump instead of the decompiled DSDT here?
It looks there might be a problem in ACPICA parser, failed to parse an absolute path in "object reference".
Only with the acpidump file, we can confirm if there is such a problem.

If this is not an ACPICA parser problem, then this is surely a BIOS issue.
What we need to do in ACPICA core is to prepare a workaround to convert string into object reference.

So since it can lead to different code for fixing different issues, we really need the acpidump to confirm.

Thanks and best regards
-Lv
Comment 3 Bastien Nocera 2015-10-22 10:23:52 UTC
Created attachment 190881 [details]
Surface-3-ACPI-tables.tar.gz

Tarball of the files in /sys/firmware/acpi/tables/*
Comment 4 Bastien Nocera 2015-10-22 10:24:57 UTC
If you actually wanted something else, let me know exactly what, the acpidump output looks like an hexdump on the binary file, not sure it's too useful.
Comment 5 Lv Zheng 2015-10-23 01:01:07 UTC
I can work with the sysfs tables.

Just for your information.

The only problem is I don't know if the sysfs tables contain customized stuffs.
Using the acpidump shipped in the kernel tools/power/acpi folder,
you can also execute "acpidump -c on" to dump the customized tables (exactly as from sysfs) and you can execute "acpidump -c off" to dump the non-customized tables.

hexdump style acpidump output can also be useful.
acpixtract can extract binary tables from this single output.
Using the acpidump shipped in the kernel tools/power/acpi folder,
you can also execute "acpidump -b" to dump the binary tables.
Comment 6 Bastien Nocera 2015-10-23 11:28:39 UTC
Created attachment 190951 [details]
acpidump -c off output
Comment 7 Bastien Nocera 2015-10-23 11:29:09 UTC
Created attachment 190961 [details]
acpidump -c on output
Comment 8 Lv Zheng 2015-10-27 01:56:53 UTC
In _DEP which is defined as Package on this platform, data objects (integer string, buffer, package) and control method references are allowed.

Decoding the _DEP package, we can get 2 itme, 1 is a DataRefObject(string), the other is a NameString(object reference):
==============================================================================
0000abf0                                             08 5f  |              ._|
0000ac00  44 45 50                                          |DEP             |
 <-NameOp(0x08) _DEP
0000ac00           12 16 02                                 |   ...          |
 <-PackageOp(0x12) PkgLength(0x16) NumElements(0x02)
0000ac00                    0d 5c  5f 53 42 2e 50 43 49 30  |      .\_SB.PCI0|
0000ac10  2e 49 32 43 31 00                                 |.I2C1.          |
 <-StringPrefix(0x0D) \_SB.PCI0.I2C1, this is a DataRefObject
0000ac10                    49 32  5a 31                    |      I2Z1      |
 <-I2Z1, this is a NameString, indicating an object reference
==============================================================================
Thus the AML parser seems not to be wrong here.
The platform need a workaround implemented in ACPICA namespace repair module.
Let me post the workaround later.

Thanks and best regards
-Lv
Comment 9 Lv Zheng 2015-10-28 06:53:31 UTC
Created attachment 191341 [details]
[PATCH] ACPICA: Namespace: Add scope information to the simple object repair mechanism
Comment 10 Lv Zheng 2015-10-28 06:53:58 UTC
Created attachment 191351 [details]
[PATCH] ACPICA: Namespace: Add String -> object_reference conversion support
Comment 11 Lv Zheng 2015-10-28 06:57:36 UTC
Hi,

Please apply attachment 191341 [details]/attachment 191351 [details] and boot the built kernel to try again.
I can apply the 2 patches on top of the recent upstream kernel, some old versioned kernels should still be able to use them.

Thanks and best regards
-Lv
Comment 12 Lv Zheng 2015-10-28 06:59:33 UTC
Just for you reference.

I tested the 2 patches in the simulation environment.
Instead of the error, now we can see the following lines:
=====
 nsrepair-0400 [00] NsSimpleRepair        : \_SB.BAT0._DEP: Converted String to expected Reference at Package index 0
=====
And the fixed \_SB.BAT0._DEP is as follows:
=====
- dump \_SB.BAT0._DEP
Object (0255C668) Pathname:  \_SB.BAT0._DEP
  0000: 60 CB 55 02 0F 04 20 01 5F 44 45 50 58 C5 55 02  // `.U... ._DEPX.U.
  0010: 00 00 00 00 00 00 00 00                          // ........
                Name : _DEP
                Type : 04 [Package]
               Flags : 20
            Owner Id : 01
         Object List : 0255CB60(Package 04)
              Parent : 0255C558 [BAT0]
               Child : 00000000
                Peer : 00000000

Attached Object (0255CB60):
  0000: 00 00 00 00 0E 04 01 00 04 00 00 00 68 C6 55 02  // ............h.U.
  0010: A8 27 DE 02 84 19 55 02 17 00 00 00 02 00 00 00  // .'....U.........
  0020: 00 00 00 00 00 00 00 00 00 00 00 00              // ............
                Type : 04 [Package]
     Reference Count : 0001
               Flags : 04
         Object List : 00000000
         Parent Node : 0255C668 [_DEP]
               Flags : 04
            Elements : 00000002
        Element List : 02DE27A8

Package Contents:
0255CB60 [Package] Contains 2 Elements:
  [00] 02DE2CF8 [Object Reference] Type [Named Object] 05 0255C150 \_SB.PCI0.I2C1
  [01] 02DE2820 [Object Reference] Type [Named Object] 05 0255C448 \_SB.I2Z1
=====
Comment 13 Bastien Nocera 2015-10-28 10:52:34 UTC
(In reply to Lv Zheng from comment #11)
> Hi,
> 
> Please apply attachment 191341 [details]/attachment 191351 [details] and
> boot the built kernel to try again.
> I can apply the 2 patches on top of the recent upstream kernel, some old
> versioned kernels should still be able to use them.

What data was I supposed to get?

The only thing I got related to either the power adapter and the battery is:
Oct 28 12:10:19 surface kernel: ACPI: AC Adapter [ADP1] (off-line)

With the power adapter unplugged. Plugging in the power adapter doesn't show any more messages. No mentions of "BAT" in the output.
Comment 14 Bastien Nocera 2015-10-28 10:53:02 UTC
Created attachment 191391 [details]
dmesg output
Comment 15 Lv Zheng 2015-10-29 02:43:19 UTC
The bug report and the fixes are around the the warnings reported in comment 1:
Oct 19 15:00:15 localhost.localdomain kernel: ACPI Warning: \_SB_.ADP1._DEP: Return Package type mismatch at index 0 - found String, expected Reference (20150818/nspredef-297)
Oct 19 15:00:15 localhost.localdomain kernel: ACPI Warning: \_SB_.BAT0._DEP: Return Package type mismatch at index 0 - found String, expected Reference (20150818/nspredef-297)

I didn't see it shown up again in the log. So we could mark it as resolved.

Thanks and best regards
-Lv
Comment 16 Bastien Nocera 2015-10-29 09:25:22 UTC
(In reply to Lv Zheng from comment #15)
> The bug report and the fixes are around the the warnings reported in comment
> 1:

The bug report is actually about the battery and power adapter not working.
Comment 17 Aaron Lu 2015-10-30 01:18:40 UTC
Do I understand correctly that after you applied the two patches from Lv, the warning is gone but the battery and power adapter is still not working?
Comment 18 Lv Zheng 2015-10-30 07:21:59 UTC
What symptoms are related to the wording "not working"?
From comment 1, I can only learn the warning messages.

Can you try to do some plugging/unplugging of power cord/battery and obtain the status of the power cord/battery to see if they are working correctly?
To obtain the status of power cord, you can:
# cat /sys/class/power_supply/ADP1/online
To obtain the status of battery, you can:
# cat /sys/class/power_supply/BAT0/status

Thanks and best regards
-Lv
Comment 19 Bastien Nocera 2015-10-30 11:05:28 UTC
(In reply to Aaron Lu from comment #17)
> Do I understand correctly that after you applied the two patches from Lv,
> the warning is gone but the battery and power adapter is still not working?

Correct.

(In reply to Lv Zheng from comment #18)
> What symptoms are related to the wording "not working"?
> From comment 1, I can only learn the warning messages.
> 
> Can you try to do some plugging/unplugging of power cord/battery and obtain
> the status of the power cord/battery to see if they are working correctly?

The line power (ADP1) is always offline, unplugging and replugging the power cord doesn't change its status (offline).

> To obtain the status of power cord, you can:
> # cat /sys/class/power_supply/ADP1/online

0

> To obtain the status of battery, you can:
> # cat /sys/class/power_supply/BAT0/status

No such file or directory
Comment 20 Stephen Just 2015-11-12 05:51:38 UTC
I think this might need a driver written for it, in addition to the ACPI work. If you look at the DSDT, there is device I2Z1 (MSHW0011), which on Windows is identified as a Surface Platform Power Device, with a custom driver.

The AML does not provide any GPIO interrupt handlers for the pins mentioned there, so my guess is that we'd need to write a driver to handle those GpioInts, and perhaps call the _DSM to trigger ADP1 and BAT0 changes, or do that directly without the _DSM. Hopefully there's nothing interesting in those I2C devices listed, but my bet would be that those report the battery charge level.

Is that even possible?
Comment 21 Zhang Rui 2016-05-10 03:05:05 UTC
This looks strange to me.
1.
        Device (ADP1)
        {
            Name (_HID, "ACPI0003" /* Power Source Device */)  // _HID: Hardware ID
            Name (_DEP, Package (0x02)  // _DEP: Dependencies
            {
                "\\_SB.PCI0.I2C1",
                I2Z1
            })
            ...
        }

        Device (BAT0)
        {
            Name (_HID, EisaId ("PNP0C0A") /* Control Method Battery */)  // _HID: Hardware ID
            Name (_UID, One)  // _UID: Unique ID
            Name (_DEP, Package (0x02)  // _DEP: Dependencies
            {
                "\\_SB.PCI0.I2C1",
                I2Z1
            })
            ...
        }

Both AC adapter and Battery device depends on I2Z1 device.

2.
        Device (I2Z1)
        {
            Name (_HID, "MSHW0011")  // _HID: Hardware ID
            ...
        }
We don't have a Linux driver for this I2Z1 device.

So, in theory, neither AC driver nor battery driver should be probed at all, but apparently, at least the ACPI driver is probed successfully in your system.
Comment 22 Zhang Rui 2016-05-10 05:44:50 UTC
(In reply to Stephen Just from comment #20)
> I think this might need a driver written for it, in addition to the ACPI
> work. If you look at the DSDT, there is device I2Z1 (MSHW0011), which on
> Windows is identified as a Surface Platform Power Device, with a custom
> driver.
> 
This is a good information.
I don't know what MSHW0011 is and what it can do. It seems that there is no public documentation for this.
My question would be: With this MSHW0011 device implemented, will windows use the ACPI AC adapter and battery? If no, it is possible that this platform is shipped with a broken ACPI AC&Battery.

In order to verify this, 
1. Stephen, please check if windows can show AC status and Battery information well, without the custom driver.
2. Yu, remove the ACPI AC/Battery device in DSDT and see if Windows can still show the AC and battery information.
Comment 23 Stephen Just 2016-05-10 16:22:23 UTC
(In reply to Zhang Rui from comment #22)
> In order to verify this, 
> 1. Stephen, please check if windows can show AC status and Battery
> information well, without the custom driver.

On Windows, I disabled the "Surface Platform Power Driver" corresponding to MSHW0011, and consequently the battery and AC status disappeared under Windows, and I was unable to query battery/AC status. Upon re-enabling the driver, I was able to see battery and AC status again. The ACPI battery and power supply drivers were enabled at all times.
Comment 24 Zhang Rui 2016-05-11 01:26:15 UTC
(In reply to Stephen Just from comment #23)
> (In reply to Zhang Rui from comment #22)
> > In order to verify this, 
> > 1. Stephen, please check if windows can show AC status and Battery
> > information well, without the custom driver.
> 
> On Windows, I disabled the "Surface Platform Power Driver" corresponding to
> MSHW0011, and consequently the battery and AC status disappeared under
> Windows, and I was unable to query battery/AC status. Upon re-enabling the
> driver, I was able to see battery and AC status again. The ACPI battery and
> power supply drivers were enabled at all times.

This is valuable information.
Windows can only get AC/Battery information via MSHW0011 on Surface 3.
Linux does not have MSHW0011 driver, thus it can not provide the proper information.

To me, this is a vendor driver gap in Linux, rather than a kernel bug.
Until we can get detailed information about how MSHW0011 works and introduce a driver for surface 3, AC/Battery information is always missing in Linux.

As this is not a kernel bug, bug closed.
Comment 25 Benjamin Tissoires 2016-05-19 16:52:57 UTC
Created attachment 216861 [details]
0001-WIP-MSHW0011-rev-eng-implementation.patch

This is my current WIP (not sure I'll get much further):
- the ACPI BAT0 and ADP1 rely on MSHW0011 to provide the information through an ACPI operation Region.
- the current Operation Region needs a fix here as the buffer parameter (0x02 in the DSDT) is not enough to reply the whole information requested in _BIX for example
- I am currently stuck with the I2C part of the chip:
  * the one at addr 0x0022 seems to have a simple register type between 0x00 and 0xff
  * the one at addr 0x0055 has 2 read commands that works (0x00 and 0x40), the other seem to return a null buffer.

I was hoping to be able to get this working by poking at the chip, but I think it would be better to have the spec or find a way to dump the I2C communications made under Windows (which I can't figure out how to do those traces).
Comment 26 Bastien Nocera 2016-05-24 15:10:37 UTC
Reopening.
Comment 27 Benjamin Tissoires 2016-05-24 20:36:33 UTC
Created attachment 217301 [details]
0001-power-MSHW0011-rev-eng-implementation.patch

This is a rev-eng version of the driver from the various dumps I made on the device. I'd say it's better than nothing and shouldn't be worse than having no driver at all (in term of power management). The ACPICA change needs to be reviewed and validated, but the mshw0011 part is mostly working now.

For the good side, the driver returns sensible data and even manages to trigger the ACPI events on plugs/unplug.
Comment 28 Stephen Just 2016-05-24 22:26:40 UTC
(In reply to Benjamin Tissoires from comment #27)
> Created attachment 217301 [details]
> 0001-power-MSHW0011-rev-eng-implementation.patch

I've tested this patch, and I only noticed one very minor issue - after disconnecting the power supply for the first time after boot, the following dmesg log appears:

[Firmware bug]: battery: (dis)charge rate invalid.

I only see the message once, even if I reconnect and disconnect the power supply again. The issue is reproducible on reboot.

Other than this minor bit, the driver works fine on my tablet.
Comment 29 Lv Zheng 2016-05-25 00:48:44 UTC
For this:
424 			if (accessor_type == 0xf)
425 				length = source_desc->buffer.length;
Looks like a gap of "implicit conversion" in ACPICA.
Thus this looks more like a workaround to me.

Thanks and best regards
-Lv
Comment 30 Zhang Rui 2016-05-25 00:57:59 UTC
Benjamin, great work! does this patch target upstream? If yes, can you illustrate more about how the patch is written, and some reference documentation if there is any.
Bastien, this bug report was closed because it's a kernel gap/feature request rather than a bug. what do you think?
Comment 31 Zhang Rui 2016-05-25 01:01:46 UTC
(In reply to Lv Zheng from comment #29)
> For this:
> 424                   if (accessor_type == 0xf)
> 425                           length = source_desc->buffer.length;
> Looks like a gap of "implicit conversion" in ACPICA.
> Thus this looks more like a workaround to me.
> 
Just synced with Lv, and this should be an ACPICA bug, that may take some time to fix.
So, the solution would be
1. lv to file a bug report in acpica bugzilla, to track this ACPICA issue.
2. workarounds like the code in Benjamin' patch should be took, before we have a real fix in ACPICA.
Comment 32 Lv Zheng 2016-05-25 01:54:47 UTC
In dsfield.c, is the parsing for AML_INT_ACCESSFIELD_OP/AML_INT_EXTACCESSFIELD_OP correct?

Thanks
-Lv
Comment 33 Lv Zheng 2016-05-25 01:56:21 UTC
It looks, the parsing for the 2 opcodes should be different:

FieldElement := NamedField | ReservedField | AccessField | ExtendedAccessField | ConnectField

ExtendedAccessField := 0x03 AccessType ExtendedAccessAttrib AccessLength
ExtendedAccessAttrib := ByteData // 0x0B AttribBytes
// 0x0E AttribRawBytes
// 0x0F AttribRawProcess

AccessField := 0x01 AccessType AccessAttrib
AccessType := ByteData // Bits 0:3 - Same as AccessType bits of FieldFlags.
// Bits 4:5 - Reserved
// Bits 7:6 - 0 = AccessAttrib = Normal Access Attributes
// 1 = AccessAttrib = AttribBytes (x)
// 2 = AccessAttrib = AttribRawBytes (x)
// 3 = AccessAttrib = AttribRawProcessBytes (x)
//
// x' is encoded as bits 0:7 of the AccessAttrib byte.
AccessAttrib := ByteData // If AccessType is BufferAcc for the SMB or
// GPIO OpRegions, AccessAttrib can be one of
// the following values:
// 0x02 AttribQuick
// 0x04 AttribSendReceive
// 0x06 AttribByte
// 0x08 AttribWord
// 0x0A AttribBlock
// 0x0C AttribProcessCall
// 0x0D AttribBlockProcessCall

FieldFlags := ByteData // bit 0-3: AccessType
// 0 AnyAcc
// 1 ByteAcc
// 2 WordAcc
// 3 DWordAcc
// 4 QWordAcc
// 5 BufferAcc
// 6 Reserved
// 7-15 Reserved
// bit 4: LockRule
// 0 NoLock
// 1 Lock
// bit 5-6: UpdateRule
// 0 Preserve
// 1 WriteAsOnes
Comment 34 Lv Zheng 2016-05-25 04:45:07 UTC
(In reply to Zhang Rui from comment #31)
> (In reply to Lv Zheng from comment #29)
> > For this:
> > 424                         if (accessor_type == 0xf)
> > 425                                 length = source_desc->buffer.length;
> > Looks like a gap of "implicit conversion" in ACPICA.
> > Thus this looks more like a workaround to me.
> > 
> Just synced with Lv, and this should be an ACPICA bug, that may take some
> time to fix.
> So, the solution would be
> 1. lv to file a bug report in acpica bugzilla, to track this ACPICA issue.
> 2. workarounds like the code in Benjamin' patch should be took, before we
> have a real fix in ACPICA.

Hi, Benjamin

Could you give me some ASL examples used on this platform to explain why this workaround is needed?
So that we can determine if there is a real bug related to the ExtendedAccessField/AccessField parsing.
Thanks in advance.

Best regards
-Lv
Comment 35 Benjamin Tissoires 2016-05-25 09:38:06 UTC
(In reply to Zhang Rui from comment #30)
> Benjamin, great work! does this patch target upstream? If yes, can you

I hope being able to submit it soon, yes (after a split with the acpica bits and I have one other thing that bothers me in the _DSM function).

> illustrate more about how the patch is written, and some reference
> documentation if there is any.

You mean, in the patch, not here I suppose.
The thing is I have no docs whatsoever, but the DSDT of the device itself which explains a lot of mechanisms (how the operation region is supposed to be working, how the ACPI notifications are triggered). For the I2C part, that was just me staring at the dumps and trying to put sense in those.

When submitting I can add a little bit more description in the commit message if you need.
Comment 36 Benjamin Tissoires 2016-05-25 09:47:50 UTC
(In reply to Lv Zheng from comment #34)
> (In reply to Zhang Rui from comment #31)
> > (In reply to Lv Zheng from comment #29)
> > > For this:
> > > 424                       if (accessor_type == 0xf)
> > > 425                               length = source_desc->buffer.length;
> > > Looks like a gap of "implicit conversion" in ACPICA.
> > > Thus this looks more like a workaround to me.
> > > 
> > Just synced with Lv, and this should be an ACPICA bug, that may take some
> > time to fix.
> > So, the solution would be
> > 1. lv to file a bug report in acpica bugzilla, to track this ACPICA issue.
> > 2. workarounds like the code in Benjamin' patch should be took, before we
> > have a real fix in ACPICA.
> 
> Hi, Benjamin
> 
> Could you give me some ASL examples used on this platform to explain why
> this workaround is needed?

The DSDT is available in the various acpidump or in attachment 217301 [details] from bug 104291.

The important ASL are, IMO, when retrieving the _BIX data.
I2Z1 has the ACPI operation region AttribRawProcessBytes(0x02) but we need to access three parameters (RSPB which calls BB00 takes 3 parameters), and we can use up to 168 bytes of out data (like in _BIX):

        Device (I2Z1)
        {
            Name (_HID, "MSHW0011")  // _HID: Hardware ID
...
            Name (DVER, Zero)
            OperationRegion (OR02, GenericSerialBus, Zero, 0x0100)
            Field (OR02, BufferAcc, NoLock, Preserve)
            {
                Connection (BX00), 
                AccessAs (BufferAcc, AttribRawProcessBytes (0x02)), 
                BB00,   8
            }
        }

        Device (BAT0)
        {
            Name (_HID, EisaId ("PNP0C0A") /* Control Method Battery */)
...
            Name (BT1P, Zero) // <- set to One in _STA when MSHW0011 is present
...
            Name (SPB0, Buffer (0x80) {})
            CreateByteField (SPB0, Zero, STA2)
            CreateByteField (SPB0, One, LEN0)
            CreateByteField (SPB0, 0x02, CMD0)
            CreateByteField (SPB0, 0x03, DAT0)
            CreateByteField (SPB0, 0x04, DAT1)
            CreateDWordField (SPB0, 0x05, DAT2)
            CreateField (SPB0, 0x10, 0x0320, DAT3)
            Method (RSPB, 3, NotSerialized)
            {
                CMD0 = One
                LEN0 = 0x08
                DAT0 = Arg0
                DAT1 = Arg1
                DAT2 = Arg2
                Debug = "_RSPB#1 (ID, Command 1/_STA 2/_BIX 3/_BCT 4/_BTM 5/_BST 6/_BTP, Arg) :"
                Debug = Arg0
                Debug = Arg1
                Debug = Arg2
                SPB0 = ^^I2Z1.BB00 = SPB0 /* \_SB_.BAT0.SPB0 */
                Debug = "_RSPB#1 (Call Complete: Status) :"
                Debug = STA2 /* \_SB_.BAT0.STA2 */
                If ((STA2 != Zero))
                {
                    Debug = "RSPB(): I2C returned an error (STA2 != 0)."
                    Return (Zero)
                }

                Return (SPB0) /* \_SB_.BAT0.SPB0 */
            }
...
            Name (BIX0, Package (0x14)
            {
                Zero, 
                One, 
                0x1D00, 
                0x1D00, 
                One, 
                0x0ED8, 
                0x8F, 
                0x47, 
                0xFFFFFFFF, 
                0x00015F90, 
                0x03E8, 
                0x03E8, 
                0x03E8, 
                0x03E8, 
                0x45, 
                0x11, 
                "Model    ", 
                "Serial   ", 
                "LIONX    ", 
                "OEMx00000"
            })
            Method (_BIX, 0, NotSerialized)  // _BIX: Battery Information Extended
            {
                Acquire (BT1M, 0xFFFF)
                Debug = "_BIX#1"
                If ((BT1P == Zero))
                {
                    Release (BT1M)
                    Return (BIX0) /* \_SB_.BAT0.BIX0 */
                }

                Name (TBUF, Buffer (0x80) {})
                CreateByteField (TBUF, 0x02, STA4)
                CreateDWordField (TBUF, 0x03, VAL0)
                CreateDWordField (TBUF, 0x07, VAL1)
                CreateDWordField (TBUF, 0x0B, VAL2)
                CreateDWordField (TBUF, 0x0F, VAL3)
                CreateDWordField (TBUF, 0x13, VAL4)
                CreateDWordField (TBUF, 0x17, VAL5)
                CreateDWordField (TBUF, 0x1B, VAL6)
                CreateDWordField (TBUF, 0x1F, VAL7)
                CreateDWordField (TBUF, 0x23, VAL8)
                CreateDWordField (TBUF, 0x27, VAL9)
                CreateDWordField (TBUF, 0x2B, VALA)
                CreateDWordField (TBUF, 0x2F, VALB)
                CreateDWordField (TBUF, 0x33, VALC)
                CreateDWordField (TBUF, 0x37, VALD)
                CreateDWordField (TBUF, 0x3B, VALE)
                CreateDWordField (TBUF, 0x3F, VALF)
                CreateField (TBUF, 0x0218, 0x50, VALG)
                CreateField (TBUF, 0x0268, 0x50, VALH)
                CreateField (TBUF, 0x02B8, 0x50, VALI)
                CreateField (TBUF, 0x0308, 0x50, VALJ)
                TBUF = RSPB (One, 0x02, Zero)
                If ((STA4 == Zero))
                {
                    Release (BT1M)
                    Return (BIX0) /* \_SB_.BAT0.BIX0 */
                }

                If ((VAL1 == Zero))
                {
                    Local1 = (0x0A * VAL2) /* \_SB_.BAT0._BIX.VAL2 */
                    Local2 = (0x0A * VAL3) /* \_SB_.BAT0._BIX.VAL3 */
                    Local3 = (0x0A * VAL6) /* \_SB_.BAT0._BIX.VAL6 */
                    Local4 = (0x0A * VAL7) /* \_SB_.BAT0._BIX.VAL7 */
                    Local5 = (0x0A * VALE) /* \_SB_.BAT0._BIX.VALE */
                    Local6 = (0x0A * VALF) /* \_SB_.BAT0._BIX.VALF */
                }
                Else
                {
                    Local1 = VAL2 /* \_SB_.BAT0._BIX.VAL2 */
                    Local2 = VAL3 /* \_SB_.BAT0._BIX.VAL3 */
                    Local3 = VAL6 /* \_SB_.BAT0._BIX.VAL6 */
                    Local4 = VAL7 /* \_SB_.BAT0._BIX.VAL7 */
                    Local5 = VALE /* \_SB_.BAT0._BIX.VALE */
                    Local6 = VALF /* \_SB_.BAT0._BIX.VALF */
                }

                Index (BIX0, Zero) = VAL0 /* \_SB_.BAT0._BIX.VAL0 */
                Index (BIX0, One) = VAL1 /* \_SB_.BAT0._BIX.VAL1 */
                Index (BIX0, 0x02) = Local1
                Index (BIX0, 0x03) = Local2
                Index (BIX0, 0x04) = VAL4 /* \_SB_.BAT0._BIX.VAL4 */
                Index (BIX0, 0x05) = VAL5 /* \_SB_.BAT0._BIX.VAL5 */
                Index (BIX0, 0x06) = Local3
                Index (BIX0, 0x07) = Local4
                Index (BIX0, 0x08) = VAL8 /* \_SB_.BAT0._BIX.VAL8 */
                Index (BIX0, 0x09) = VAL9 /* \_SB_.BAT0._BIX.VAL9 */
                Index (BIX0, 0x0A) = VALA /* \_SB_.BAT0._BIX.VALA */
                Index (BIX0, 0x0B) = VALB /* \_SB_.BAT0._BIX.VALB */
                Index (BIX0, 0x0C) = VALC /* \_SB_.BAT0._BIX.VALC */
                Index (BIX0, 0x0D) = VALD /* \_SB_.BAT0._BIX.VALD */
                Index (BIX0, 0x0E) = Local5
                Index (BIX0, 0x0F) = Local6
                Index (BIX0, 0x10) = VALG /* \_SB_.BAT0._BIX.VALG */
                Index (BIX0, 0x11) = VALH /* \_SB_.BAT0._BIX.VALH */
                Index (BIX0, 0x12) = VALI /* \_SB_.BAT0._BIX.VALI */
                Index (BIX0, 0x13) = VALJ /* \_SB_.BAT0._BIX.VALJ */
                Release (BT1M)
                Debug = Index (BIX0, Zero)
                Debug = Index (BIX0, One)
                Debug = Index (BIX0, 0x02)
                Debug = Index (BIX0, 0x03)
                Debug = Index (BIX0, 0x04)
                Debug = Index (BIX0, 0x05)
                Debug = Index (BIX0, 0x06)
                Debug = Index (BIX0, 0x07)
                Debug = Index (BIX0, 0x08)
                Debug = Index (BIX0, 0x09)
                Debug = Index (BIX0, 0x0A)
                Debug = Index (BIX0, 0x0B)
                Debug = Index (BIX0, 0x0C)
                Debug = Index (BIX0, 0x0D)
                Debug = Index (BIX0, 0x0E)
                Debug = Index (BIX0, 0x0F)
                Debug = Index (BIX0, 0x10)
                Debug = Index (BIX0, 0x11)
                Debug = Index (BIX0, 0x12)
                Debug = Index (BIX0, 0x13)
                Return (BIX0) /* \_SB_.BAT0.BIX0 */
            }
Comment 37 Benjamin Tissoires 2016-05-25 09:49:22 UTC
(In reply to Benjamin Tissoires from comment #36)
> The DSDT is available in the various acpidump or in attachment 217301 [details]
> [details] from bug 104291.

I meant attachment 187171 [details], sorry
Comment 38 Lv Zheng 2016-05-26 01:01:23 UTC
Hi, Benjamin

Thanks for the details.
To clarify for the other developers, the code related is:
            OperationRegion (OR02, GenericSerialBus, Zero, 0x0100)
            Field (OR02, BufferAcc, NoLock, Preserve)
            {
                Connection (BX00), 
                AccessAs (BufferAcc, AttribRawProcessBytes (0x02)), 
                BB00,   8
            }
            Name (SPB0, Buffer (0x80) {})
            CreateByteField (SPB0, Zero, STA2)
            CreateByteField (SPB0, One, LEN0)
            CreateByteField (SPB0, 0x02, CMD0)
            CreateByteField (SPB0, 0x03, DAT0)
            CreateByteField (SPB0, 0x04, DAT1)
            CreateDWordField (SPB0, 0x05, DAT2)
            CreateField (SPB0, 0x10, 0x0320, DAT3)
            Method (RSPB, 3, NotSerialized)
            {
                CMD0 = One
                LEN0 = 0x08
                DAT0 = Arg0
                DAT1 = Arg1
                DAT2 = Arg2
                SPB0 = ^^I2Z1.BB00 = SPB0 /* \_SB_.BAT0.SPB0 */
                ...
            }
I'll dig into the DSDT binary to see if this is a correct parsing result:
AttribRawProcessBytes (0x02)

Thanks and best regards
-Lv
Comment 39 Benjamin Tissoires 2016-06-07 11:32:33 UTC
Created attachment 219301 [details]
0001-power-MSHW0011-rev-eng-implementation-v2.patch

Update of the patch:
- use of SMBus as it doesn't seem to erase values in the registers (thanks Stephen for pointing this out)
- serial and cycle count are now retrieved (thanks Stephen again)

The (dis)charging rate is still not good, but the driver looks in a better shape (I think).
Comment 40 Stephen Just 2016-06-29 22:33:52 UTC
Created attachment 221501 [details]
0002-power-surface3_power-Improve-battery-capacity-report.patch

I spent a little bit of time to try to make Benjamin's driver better report capacity. I have identified a couple more battery-capacity related registers, and I have used them to better report the battery's state. Now the battery properly reports when it is full, and the "Firmware Bug" warning is gone.

Feel free to incorporate the changes into any patch submissions.
Comment 41 Andrea Gottardo 2016-09-16 18:24:53 UTC
I've been using Stephen's patch for about two weeks on my Surface 3, and it seems to be working great.
Comment 42 Bastien Nocera 2016-09-16 23:04:42 UTC
And I've been using it for a couple of months. Benjamin, Stephen, does one of you want to send the squash patches up for review?
Comment 43 Stephen Just 2016-09-17 01:39:38 UTC
They got rejected by upstream a month ago based on the ACPI changes. I couldn't find a workaround that didn't require that change.
Comment 44 Stephen Just 2016-09-17 03:12:57 UTC
Upstream submission:
 https://patchwork.kernel.org/patch/9252633/
 https://patchwork.kernel.org/patch/9252643/


Lv, after reading through 5.5.2.4.5.3 of the ACPI spec several times, it appears that AccessLength is ignored for AttribRawProcessBytes accesses - the access length is always specified by the user - and in fact, the example omits the AccessLength argument entirely.

More specifically, comparing 5.5.2.4.5.3.9 and 5.5.2.4.5.3.10, it appears that the only difference between AttribRawProcessBytes and AttribRawBytes is that AttribRawProcessBytes does not seem to pay attention to AccessLength, and lets the driver deal with it.

Does this seem accurate to you?
Comment 45 Lv Zheng 2016-09-20 04:33:39 UTC
@Stephen

You seem to be right as there is no such description in AttribRawProcessBytes but in AttribRawBytes:
The actual number of bytes to read or write is specified as part of the AccessAs attribute.

However, I still have no idea how to correctly implement it:
SPB0 [#1] = ^^I2Z1.BB00 = SPB0 [#2]
It means the transaction includes reading #2 SPB0, and writing #1 SPB0. And the fix obtains the size of #2 SPB0.

What if the code is:
SPBW = ^^I2Z1.BB00 = SPBR
And SPBW / SPBR are not the sized same.

Due to the data type conversion requirement, when the i2c transaction happens, it seems not possible to obtain the size of SPBW in ACPICA.

This is my last comment.

Thanks
Lv
Comment 46 Bastien Nocera 2016-11-07 16:19:01 UTC
I have rebased patches in:
https://github.com/hadess/fedora-surface3-kernel
Comment 47 Lv Zheng 2016-12-19 05:35:46 UTC
(In reply to Bastien Nocera from comment #46)
> I have rebased patches in:
> https://github.com/hadess/fedora-surface3-kernel

For ACPICA part, you may need to clone https://github.com/acpica/acpica and submit a pull request.

At least there is a workaround for this bug. Let me mark it as RESOLVED.
As we don't track ACPICA bugs here.
Comment 48 Benjamin Tissoires 2017-06-20 14:26:25 UTC
Just a small FYI, I sent out a PR on ACPICA github with the patch:
https://github.com/acpica/acpica/pull/277

Let see how it goes :)
Comment 49 Lv Zheng 2017-06-21 01:53:50 UTC
Great!
Thanks for the submission.
Comment 50 Benjamin Tissoires 2017-06-29 12:12:48 UTC
Now that the ACPICA PR has been merged (and not reverted, yet), I also submitted the v2 of the MSHW0011 patch here: https://patchwork.kernel.org/patch/9816601/

Crossing fingers :)
Comment 51 Andrea Gottardo 2017-06-29 17:47:55 UTC
As a Surface 3 owner who has been trying to get Linux running without custom kernels or other hacks, many thanks! :-)
Comment 52 Lv Zheng 2017-07-04 01:17:55 UTC
Closing as workaround upstreamed.
Comment 53 Benjamin Tissoires 2017-10-16 07:51:07 UTC
For the record (I have been pinged regarding it), the ACPICA patch has been reverted, so we are still in the status quo here.

Note You need to log in before you can comment on or make changes to this bug.