Hi,
On 27-11-18 17:40, Rafael J. Wysocki wrote:
On Tue, Nov 27, 2018 at 9:39 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Hi,
On 27-11-18 04:11, Moore, Robert wrote:
Also, please send the acpidump for whatever machine is showing a problem.
We want to look closely at the surface 3 code vs. the code for this machine.
Bob, I know you are unhappy with / frustrated about this part of the ACPI
specification (*), but please read the !@#$ patch description / commit
message.
The current problem has very little to do with the specification being
ambiguous, the code currently in 4.20 is using a struct field which is
*clearly* marked in the specification as "reserved" as length argument
to a memcpy without checking that either the source or the destination
buffer is big enough.
Nor does it guarantee that we copy *enough* data when doing a write to
an i2c device through the GSB code, resulting in the OpRegion handler
writing whatever was in the tmp-buffer when we got it from
acpi_ut_create_buffer_object to the i2c-device, crashing the machine.
The description of the problem is clear to me and and the patch does
make it go away.
Question is if the patch as is clashes with any future ACPICA work,
because if it does, it may be better to do something else in
principle. So far this is still unclear, but this regression needs to
go away before final 4.20 and the patch still is there in my
linux-next branch.
I can give you the acpidump of the non-booting device but it is not doing
anything special,
Sending it over wouldn't hurt though IMO, would it?
True, here it is: https://fedorapeople.org/~jwrdegoede/acpidump.acer-One-S1003
it is only using AttribByte accesses, the problem is
not the DSDT, the problem is the 4.20 code clearly, *obviously*, being
buggy, so please review the patch first.
*) IMHO this mostly is about the Surface line of products abusing the
specification, all other uses I've seen are very straight forward
So until recently it has not been clear to Bob/Erik if there are any
other production users of GenericSerialBus AFAICS and it would help to
give them ASL with it from whatever machines you have access to.
I do not think that is the case. A lot of laptops out there use
the GenericSerialBus interface in the form of an I2cSerialBus for
Battery monitoring, but all these laptops pretty much exclusively
use GenericSerialBus OpRegion accesses with AccessAs set to either
AttribByte or AttribBytes and that has been working fine for years
and the spec is clear enough on how to interpret these (esp. in
combination with the DSTDs using them which make the intention clear).
The problem with the Surface line of devices is that the use
AccessAs AttribRawProcessBytes on which the spec is a bit handwavy,
the patch in 4.20 to fix AttribRawProcessBytes breaks
AttribByte and AttribBytes accesses, because for those the
length field of the ACPI buffer passed when writing the field is
reserved, so we should not use it.
Frankly given that using GenericSerialBus OpRegion accesses with
AccessAs set to either AttribByte or AttribBytes is very common,
I'm amazed that there have not been more regressions reported against
4.20. Note these do not necessarily have to end up as system not
booting, but could also e.g. be battery monitoring behaving weirdly.
Note I was involved in the acpica discussion surrounding the patches
to deal with AttribRawProcessBytes, only I was so focused on
AttribRawProcessBytes that I failed to see they would break
AttribByte and AttribBytes accesses at that time, only to have it
poetically bite myself in the ass and at the end of a long bisect
end up at the very patch I was sideways involved with.
Regards,
Hans
I would suggest opening a BZ for that and attaching some relevant
acpidump files in there, for future reference if nothing else.
If you make a claim like the above, it's rather good to support it
with some evidence. :-)
-----Original Message-----
From: Moore, Robert
Sent: Monday, November 26, 2018 2:33 PM
To: Schmauss, Erik <erik.schmauss@xxxxxxxxx>; Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>; Hans de Goede <hdegoede@xxxxxxxxxx>
Cc: Len Brown <lenb@xxxxxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx
Subject: RE: [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()
Looks like another round of issues with the completely ill-defined generic serial bus. Below is a summary of the problems we have seen with the Length field.
I have not had time to completely read through this thread yet, so I'm posting some text that I wrote a month or two ago.
Let me say this, however:
The biggest (and it is very big) issue is that the GenericSerialBus interfaces are so poorly defined in the ACPI specification that any implementation simply must guess at the proper behavior (Both the ACPI implementation and any related drivers).
Bob
Problems with these ACPI chapters:
5.5.2.4.6.2 Declaring and Using a GenericSerialBus Data Buffer
5.5.2.4.6.3 Using the GenericSerialBus Protocols
1) All of the ASL code examples do not compile due to syntax errors. There are
10 such examples, often with multiple syntax errors.
2) The basic/common data buffer format is not fully defined:
typedef struct
{
BYTE Status; // Byte 0 of the data buffer
BYTE Length; // Byte 1 of the data buffer
BYTE[x-1] Data; // Bytes 2-x of the arbitrary length data buffer,
} // where x is the last index of the overall buffer
The "Length" field is not defined. Is it the length of "Data" or the Length of the entire structure (Status + Length + Data)?
The example code from the spec illustrates how the "Length" term in the structure above is undefined. Again, is it the "length of entire structure" or is it the "length of the Data element"?. The example code makes it appear that it is the length of the entire structure, but it is not fully clear because it is wrong in either case:
Name(BUFF, Buffer(34)()) // Create GenericSerialBus data buffer
as BUFF
CreateByteField(BUFF, 0x00, STAT) // STAT = Status (Byte)
CreateByteField(BUFF, 0x01, LEN) // LEN = Length (Byte)
CreateField(BUFF, 0x10, 256, DATA) // Data (Block)
Store("ACPI", DATA) // Fill in outgoing data
Store(8, LEN) // Length of the valid data
(Actually should be 6? Or should it be 4?)
