Bug 106231

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.

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

Created attachment 190881 [details]
Surface-3-ACPI-tables.tar.gz

Tarball of the files in /sys/firmware/acpi/tables/*

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.

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.

Created attachment 190951 [details]
acpidump -c off output

Created attachment 190961 [details]
acpidump -c on output

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

Created attachment 191341 [details]
[PATCH] ACPICA: Namespace: Add scope information to the simple object repair mechanism

Created attachment 191351 [details]
[PATCH] ACPICA: Namespace: Add String -> object_reference conversion support

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

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

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

Created attachment 191391 [details]
dmesg output

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

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

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?

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

(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

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?

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.

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

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

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

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

Reopening.

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.

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

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

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?

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

In dsfield.c, is the parsing for AML_INT_ACCESSFIELD_OP/AML_INT_EXTACCESSFIELD_OP correct?

Thanks
-Lv

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

(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

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

(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 */
            }

(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

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

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

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.

I've been using Stephen's patch for about two weeks on my Surface 3, and it seems to be working great.

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?

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.

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?

@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

I have rebased patches in:
https://github.com/hadess/fedora-surface3-kernel

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

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

Great!
Thanks for the submission.

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

As a Surface 3 owner who has been trying to get Linux running without custom kernels or other hacks, many thanks! :-)

Closing as workaround upstreamed.

For the record (I have been pinged regarding it), the ACPICA patch has been reverted, so we are still in the status quo here.

The patch got upstreamed in 2020, after changes were made to the ACPI implementation.

https://github.com/torvalds/linux/blob/master/drivers/platform/x86/surface3_power.c

See also:
https://github.com/linux-surface/linux-surface/wiki/Surface-3#battery-status-reading