* 5.6 regression caused by "ACPICA: Dispatcher: always generate buffer objects for ASL create_field() operator" @ 2020-05-05 12:55 Hans de Goede 2020-05-05 13:50 ` Maximilian Luz 0 siblings, 1 reply; 5+ messages in thread From: Hans de Goede @ 2020-05-05 12:55 UTC (permalink / raw) To: Maximilian Luz, Rafael J. Wysocki, Bob Moore, Erik Kaneda, Mattia Dongili, William Bader, Dominik Mierzejewski Cc: linux-acpi, Darren Hart, Andy Shevchenko, platform-driver-x86 Hi All, Commit 6d232b29cfce ("ACPICA: Dispatcher: always generate buffer objects for ASL create_field() operator") has dropped the automatic conversion of small buffers into integers. But some drivers relied on this automatic conversion, these drivers have checks like this: if (object->type != ACPI_TYPE_INTEGER) { pr_warn("Invalid acpi_object: expected 0x%x got 0x%x\n", ACPI_TYPE_INTEGER, object->type); kfree(object); return -EINVAL; } This specific bit comes from the sony-laptop (platform/x86) code, the ACPICA change has broken this code, causing systems using this driver to hang on resume from suspend. We have multiple bug-reports open for this already: https://bugzilla.kernel.org/show_bug.cgi?id=207491 https://bugzilla.redhat.com/show_bug.cgi?id=1829096 https://bugzilla.redhat.com/show_bug.cgi?id=1830150 Mattia Dongili, the sony-laptop driver has already submitted a fix for this upstream, adjusting the sony-laptop driver to deal with the returned object type now being a BUFFER. The goal of this email is to: 1. Make everyone involved aware of this breakage as we may see similar breakage elsewhere. 2. Discuss if we should maybe revert the ACPICA change. If you are reading this then 1. has been accomplished :) Which leaves us with 2. I'm tending towards keeping the change, since it seems the right thing to do and dealing with the fallout. But since there is fallout we should also at least consider reverting the ACPICA change. So ACPI maintainers what is you take on this ? Regards, Hans ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: 5.6 regression caused by "ACPICA: Dispatcher: always generate buffer objects for ASL create_field() operator" 2020-05-05 12:55 5.6 regression caused by "ACPICA: Dispatcher: always generate buffer objects for ASL create_field() operator" Hans de Goede @ 2020-05-05 13:50 ` Maximilian Luz 2020-05-05 22:11 ` Kaneda, Erik 0 siblings, 1 reply; 5+ messages in thread From: Maximilian Luz @ 2020-05-05 13:50 UTC (permalink / raw) To: Hans de Goede, Rafael J. Wysocki, Bob Moore, Erik Kaneda, Mattia Dongili, William Bader, Dominik Mierzejewski Cc: linux-acpi, Darren Hart, Andy Shevchenko, platform-driver-x86 On 5/5/20 2:55 PM, Hans de Goede wrote: > Hi All, > > Commit 6d232b29cfce ("ACPICA: Dispatcher: always generate buffer > objects for ASL create_field() operator") has dropped the > automatic conversion of small buffers into integers. > > But some drivers relied on this automatic conversion, these > drivers have checks like this: > > if (object->type != ACPI_TYPE_INTEGER) { > pr_warn("Invalid acpi_object: expected 0x%x got 0x%x\n", > ACPI_TYPE_INTEGER, object->type); > kfree(object); > return -EINVAL; > } > > This specific bit comes from the sony-laptop (platform/x86) code, > the ACPICA change has broken this code, causing systems using this > driver to hang on resume from suspend. > > We have multiple bug-reports open for this already: > > https://bugzilla.kernel.org/show_bug.cgi?id=207491 > https://bugzilla.redhat.com/show_bug.cgi?id=1829096 > https://bugzilla.redhat.com/show_bug.cgi?id=1830150 > > Mattia Dongili, the sony-laptop driver has already submitted > a fix for this upstream, adjusting the sony-laptop driver > to deal with the returned object type now being a BUFFER. > > The goal of this email is to: > > 1. Make everyone involved aware of this breakage as we > may see similar breakage elsewhere. > > 2. Discuss if we should maybe revert the ACPICA change. > > If you are reading this then 1. has been accomplished :) > > Which leaves us with 2. I'm tending towards keeping the > change, since it seems the right thing to do and dealing > with the fallout. But since there is fallout we should > also at least consider reverting the ACPICA change. > > So ACPI maintainers what is you take on this ? > > Regards, > > Hans Hi, I'd like to advise against reverting the commit. Quite honestly, I don't think reverting the commit is a good idea. This will break things for devices that assume Microsoft-like AML interpreter behavior _inside_ the DSDT. Such as the MS Surface devices for example, which, as stated in the commit message, depend on the type being Buffer via a check on ObjectType(...). There is no fix for those devices other than a) accepting MS behavior in ACPICA, b) introducing a quirk in ACPICA which switches between behaviors on a device basis, or c) patching the DSDT of those devices specifically for Linux. I'd also argue that since this is MS behavior, this is the behavior that almost all consumer electronics devices with ACPI will expect. Case in point, the DSDT of one of the affected Sony laptops, which contains the following code: CreateField (SNBF, Zero, 0x20, SNBD) CreateWordField (SNBF, 0x02, CPW0) CreateWordField (SNBF, 0x04, CPW1) CreateWordField (SNBF, Zero, RCW0) CreateWordField (SNBF, 0x02, RCW1) They explicitly create a Buffer field of 32 bits via CreateField and not a 32 bit Integer field via CreateDWordField. I'd argue that if they wanted this field to be an Integer, CreateDWordField would be the straight-forward approach. Unfortunately, I also don't see a way to identify all affected calls to ACPI functions automatically or easily, as this requires to look at the DSDTs and the code behind those calls. If you have DSDTs, here's a way to identify if that particular DSDT/driver combo is affected: If a call to an ACPI function expects an Integer and the ACPI function returns a field created with CreateField(...) and the field is smaller than 64 bits (on 64bit machines), then this call is affected. The only semi-reasonable way, as far as I can see, to identify this on a broad scale is to get this information out to the respective maintainers of drivers with apci_evaluate_{integer,object,dsm,..?} calls and ask them to check those calls against DSDTs. Also maybe help them by introducing a function which does Buffer-to-Integer conversion. Regards, Maximilian ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: 5.6 regression caused by "ACPICA: Dispatcher: always generate buffer objects for ASL create_field() operator" 2020-05-05 13:50 ` Maximilian Luz @ 2020-05-05 22:11 ` Kaneda, Erik 2020-05-06 17:21 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Kaneda, Erik @ 2020-05-05 22:11 UTC (permalink / raw) To: Maximilian Luz, Hans de Goede, Wysocki, Rafael J, Moore, Robert, Mattia Dongili, William Bader, Dominik Mierzejewski Cc: linux-acpi, Darren Hart, Andy Shevchenko, platform-driver-x86 > -----Original Message----- > From: linux-acpi-owner@vger.kernel.org <linux-acpi- > owner@vger.kernel.org> On Behalf Of Maximilian Luz > Sent: Tuesday, May 5, 2020 6:50 AM > To: Hans de Goede <hdegoede@redhat.com>; Wysocki, Rafael J > <rafael.j.wysocki@intel.com>; Moore, Robert <robert.moore@intel.com>; > Kaneda, Erik <erik.kaneda@intel.com>; Mattia Dongili <malattia@linux.it>; > William Bader <williambader@hotmail.com>; Dominik Mierzejewski > <dominik@greysector.net> > Cc: linux-acpi <linux-acpi@vger.kernel.org>; Darren Hart > <dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>; > platform-driver-x86@vger.kernel.org > Subject: Re: 5.6 regression caused by "ACPICA: Dispatcher: always generate > buffer objects for ASL create_field() operator" > > On 5/5/20 2:55 PM, Hans de Goede wrote: > > Hi All, > > > > Commit 6d232b29cfce ("ACPICA: Dispatcher: always generate buffer > > objects for ASL create_field() operator") has dropped the automatic > > conversion of small buffers into integers. > > > > But some drivers relied on this automatic conversion, these drivers > > have checks like this: > > > > if (object->type != ACPI_TYPE_INTEGER) { > > pr_warn("Invalid acpi_object: expected 0x%x got 0x%x\n", > > ACPI_TYPE_INTEGER, object->type); > > kfree(object); > > return -EINVAL; > > } > > > > This specific bit comes from the sony-laptop (platform/x86) code, the > > ACPICA change has broken this code, causing systems using this driver > > to hang on resume from suspend. > > > > We have multiple bug-reports open for this already: > > > > https://bugzilla.kernel.org/show_bug.cgi?id=207491 > > https://bugzilla.redhat.com/show_bug.cgi?id=1829096 > > https://bugzilla.redhat.com/show_bug.cgi?id=1830150 > > > > Mattia Dongili, the sony-laptop driver has already submitted a fix for > > this upstream, adjusting the sony-laptop driver to deal with the > > returned object type now being a BUFFER. > > > > The goal of this email is to: > > > > 1. Make everyone involved aware of this breakage as we may see similar > > breakage elsewhere. > > > > 2. Discuss if we should maybe revert the ACPICA change. > > > > If you are reading this then 1. has been accomplished :) > > > > Which leaves us with 2. I'm tending towards keeping the change, since > > it seems the right thing to do and dealing with the fallout. But since > > there is fallout we should also at least consider reverting the ACPICA > > change. > > > > So ACPI maintainers what is you take on this ? What a mess. Several thoughts... I think we should keep the internal AML interpreter behavior as is because it solves a problem. The ACPI spec says that buffers that are small enough to fit inside of integers should be treated as integers. However, we found that this is not the case for CreateField operators. The internal representation of this needs to be a buffer to match MS AML interpreter. SNC is not a pre-defined method so we actually don't know what the return type is supposed to be. At some point during the development of this driver, it's likely that someone made an observation about what "SNC" returned at some point in time and continued to assume that it will use the same type. For methods that are not a part of the ACPI specification, buffers less than or equal to 8 bytes and integers could be used interchangeably. I don't see a reason why the data inside the buffer should be invalid. Erik > > > > Regards, > > > > Hans > > Hi, > > I'd like to advise against reverting the commit. Quite honestly, I don't think > reverting the commit is a good idea. This will break things for devices that > assume Microsoft-like AML interpreter behavior _inside_ the DSDT. Such as > the MS Surface devices for example, which, as stated in the commit message, > depend on the type being Buffer via a check on ObjectType(...). There is no > fix for those devices other than a) accepting MS behavior in ACPICA, b) > introducing a quirk in ACPICA which switches between behaviors on a device > basis, or c) patching the DSDT of those devices specifically for Linux. > > I'd also argue that since this is MS behavior, this is the behavior that almost all > consumer electronics devices with ACPI will expect. Case in point, the DSDT > of one of the affected Sony laptops, which contains the following code: > > CreateField (SNBF, Zero, 0x20, SNBD) > CreateWordField (SNBF, 0x02, CPW0) > CreateWordField (SNBF, 0x04, CPW1) > CreateWordField (SNBF, Zero, RCW0) > CreateWordField (SNBF, 0x02, RCW1) > > They explicitly create a Buffer field of 32 bits via CreateField and not a 32 bit > Integer field via CreateDWordField. I'd argue that if they wanted this field to > be an Integer, CreateDWordField would be the straight-forward approach. > > Unfortunately, I also don't see a way to identify all affected calls to ACPI > functions automatically or easily, as this requires to look at the DSDTs and the > code behind those calls. If you have DSDTs, here's a way to identify if that > particular DSDT/driver combo is affected: > > If a call to an ACPI function expects an Integer and the ACPI function returns > a field created with CreateField(...) and the field is smaller than 64 bits (on > 64bit machines), then this call is affected. > > The only semi-reasonable way, as far as I can see, to identify this on a broad > scale is to get this information out to the respective maintainers of drivers > with apci_evaluate_{integer,object,dsm,..?} calls and ask them to check > those calls against DSDTs. Also maybe help them by introducing a function > which does Buffer-to-Integer conversion. > > Regards, > Maximilian > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: 5.6 regression caused by "ACPICA: Dispatcher: always generate buffer objects for ASL create_field() operator" 2020-05-05 22:11 ` Kaneda, Erik @ 2020-05-06 17:21 ` Rafael J. Wysocki 2020-05-07 7:34 ` Hans de Goede 0 siblings, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2020-05-06 17:21 UTC (permalink / raw) To: Kaneda, Erik Cc: Maximilian Luz, Hans de Goede, Moore, Robert, Mattia Dongili, William Bader, Dominik Mierzejewski, linux-acpi, Darren Hart, Andy Shevchenko, platform-driver-x86 On 5/6/2020 12:11 AM, Kaneda, Erik wrote: > >> -----Original Message----- >> From: linux-acpi-owner@vger.kernel.org <linux-acpi- >> owner@vger.kernel.org> On Behalf Of Maximilian Luz >> Sent: Tuesday, May 5, 2020 6:50 AM >> To: Hans de Goede <hdegoede@redhat.com>; Wysocki, Rafael J >> <rafael.j.wysocki@intel.com>; Moore, Robert <robert.moore@intel.com>; >> Kaneda, Erik <erik.kaneda@intel.com>; Mattia Dongili <malattia@linux.it>; >> William Bader <williambader@hotmail.com>; Dominik Mierzejewski >> <dominik@greysector.net> >> Cc: linux-acpi <linux-acpi@vger.kernel.org>; Darren Hart >> <dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>; >> platform-driver-x86@vger.kernel.org >> Subject: Re: 5.6 regression caused by "ACPICA: Dispatcher: always generate >> buffer objects for ASL create_field() operator" >> >> On 5/5/20 2:55 PM, Hans de Goede wrote: >>> Hi All, >>> >>> Commit 6d232b29cfce ("ACPICA: Dispatcher: always generate buffer >>> objects for ASL create_field() operator") has dropped the automatic >>> conversion of small buffers into integers. >>> >>> But some drivers relied on this automatic conversion, these drivers >>> have checks like this: >>> >>> if (object->type != ACPI_TYPE_INTEGER) { >>> pr_warn("Invalid acpi_object: expected 0x%x got 0x%x\n", >>> ACPI_TYPE_INTEGER, object->type); >>> kfree(object); >>> return -EINVAL; >>> } >>> >>> This specific bit comes from the sony-laptop (platform/x86) code, the >>> ACPICA change has broken this code, causing systems using this driver >>> to hang on resume from suspend. >>> >>> We have multiple bug-reports open for this already: >>> >>> https://bugzilla.kernel.org/show_bug.cgi?id=207491 >>> https://bugzilla.redhat.com/show_bug.cgi?id=1829096 >>> https://bugzilla.redhat.com/show_bug.cgi?id=1830150 >>> >>> Mattia Dongili, the sony-laptop driver has already submitted a fix for >>> this upstream, adjusting the sony-laptop driver to deal with the >>> returned object type now being a BUFFER. >>> >>> The goal of this email is to: >>> >>> 1. Make everyone involved aware of this breakage as we may see similar >>> breakage elsewhere. >>> >>> 2. Discuss if we should maybe revert the ACPICA change. >>> >>> If you are reading this then 1. has been accomplished :) >>> >>> Which leaves us with 2. I'm tending towards keeping the change, since >>> it seems the right thing to do and dealing with the fallout. But since >>> there is fallout we should also at least consider reverting the ACPICA >>> change. >>> >>> So ACPI maintainers what is you take on this ? > What a mess. Several thoughts... > > I think we should keep the internal AML interpreter behavior as is because it solves a problem. I also would prefer to deal with the fallout rather than to revert the fix. I think that the potential damage is limited and the affected driver code should be fixed anyway which may not happen if the change in question is reverted. > The ACPI spec says that buffers that are small enough to fit inside of integers should be treated as > integers. However, we found that this is not the case for CreateField operators. The internal > representation of this needs to be a buffer to match MS AML interpreter. > > SNC is not a pre-defined method so we actually don't know what the return type is supposed > to be. At some point during the development of this driver, it's likely that someone made an observation > about what "SNC" returned at some point in time and continued to assume that it will use the same > type. > > For methods that are not a part of the ACPI specification, buffers less than or equal to 8 bytes and > integers could be used interchangeably. I don't see a reason why the data inside the buffer should be > invalid. > > Erik >>> Regards, >>> >>> Hans >> Hi, >> >> I'd like to advise against reverting the commit. Quite honestly, I don't think >> reverting the commit is a good idea. This will break things for devices that >> assume Microsoft-like AML interpreter behavior _inside_ the DSDT. Such as >> the MS Surface devices for example, which, as stated in the commit message, >> depend on the type being Buffer via a check on ObjectType(...). There is no >> fix for those devices other than a) accepting MS behavior in ACPICA, b) >> introducing a quirk in ACPICA which switches between behaviors on a device >> basis, or c) patching the DSDT of those devices specifically for Linux. >> >> I'd also argue that since this is MS behavior, this is the behavior that almost all >> consumer electronics devices with ACPI will expect. Case in point, the DSDT >> of one of the affected Sony laptops, which contains the following code: >> >> CreateField (SNBF, Zero, 0x20, SNBD) >> CreateWordField (SNBF, 0x02, CPW0) >> CreateWordField (SNBF, 0x04, CPW1) >> CreateWordField (SNBF, Zero, RCW0) >> CreateWordField (SNBF, 0x02, RCW1) >> >> They explicitly create a Buffer field of 32 bits via CreateField and not a 32 bit >> Integer field via CreateDWordField. I'd argue that if they wanted this field to >> be an Integer, CreateDWordField would be the straight-forward approach. >> >> Unfortunately, I also don't see a way to identify all affected calls to ACPI >> functions automatically or easily, as this requires to look at the DSDTs and the >> code behind those calls. If you have DSDTs, here's a way to identify if that >> particular DSDT/driver combo is affected: >> >> If a call to an ACPI function expects an Integer and the ACPI function returns >> a field created with CreateField(...) and the field is smaller than 64 bits (on >> 64bit machines), then this call is affected. >> >> The only semi-reasonable way, as far as I can see, to identify this on a broad >> scale is to get this information out to the respective maintainers of drivers >> with apci_evaluate_{integer,object,dsm,..?} calls and ask them to check >> those calls against DSDTs. Also maybe help them by introducing a function >> which does Buffer-to-Integer conversion. >> >> Regards, >> Maximilian >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: 5.6 regression caused by "ACPICA: Dispatcher: always generate buffer objects for ASL create_field() operator" 2020-05-06 17:21 ` Rafael J. Wysocki @ 2020-05-07 7:34 ` Hans de Goede 0 siblings, 0 replies; 5+ messages in thread From: Hans de Goede @ 2020-05-07 7:34 UTC (permalink / raw) To: Rafael J. Wysocki, Kaneda, Erik Cc: Maximilian Luz, Moore, Robert, Mattia Dongili, William Bader, Dominik Mierzejewski, linux-acpi, Darren Hart, Andy Shevchenko, platform-driver-x86 Hi, On 5/6/20 7:21 PM, Rafael J. Wysocki wrote: > On 5/6/2020 12:11 AM, Kaneda, Erik wrote: >> >>> -----Original Message----- >>> From: linux-acpi-owner@vger.kernel.org <linux-acpi- >>> owner@vger.kernel.org> On Behalf Of Maximilian Luz >>> Sent: Tuesday, May 5, 2020 6:50 AM >>> To: Hans de Goede <hdegoede@redhat.com>; Wysocki, Rafael J >>> <rafael.j.wysocki@intel.com>; Moore, Robert <robert.moore@intel.com>; >>> Kaneda, Erik <erik.kaneda@intel.com>; Mattia Dongili <malattia@linux.it>; >>> William Bader <williambader@hotmail.com>; Dominik Mierzejewski >>> <dominik@greysector.net> >>> Cc: linux-acpi <linux-acpi@vger.kernel.org>; Darren Hart >>> <dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>; >>> platform-driver-x86@vger.kernel.org >>> Subject: Re: 5.6 regression caused by "ACPICA: Dispatcher: always generate >>> buffer objects for ASL create_field() operator" >>> >>> On 5/5/20 2:55 PM, Hans de Goede wrote: >>>> Hi All, >>>> >>>> Commit 6d232b29cfce ("ACPICA: Dispatcher: always generate buffer >>>> objects for ASL create_field() operator") has dropped the automatic >>>> conversion of small buffers into integers. >>>> >>>> But some drivers relied on this automatic conversion, these drivers >>>> have checks like this: >>>> >>>> if (object->type != ACPI_TYPE_INTEGER) { >>>> pr_warn("Invalid acpi_object: expected 0x%x got 0x%x\n", >>>> ACPI_TYPE_INTEGER, object->type); >>>> kfree(object); >>>> return -EINVAL; >>>> } >>>> >>>> This specific bit comes from the sony-laptop (platform/x86) code, the >>>> ACPICA change has broken this code, causing systems using this driver >>>> to hang on resume from suspend. >>>> >>>> We have multiple bug-reports open for this already: >>>> >>>> https://bugzilla.kernel.org/show_bug.cgi?id=207491 >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1829096 >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1830150 >>>> >>>> Mattia Dongili, the sony-laptop driver has already submitted a fix for >>>> this upstream, adjusting the sony-laptop driver to deal with the >>>> returned object type now being a BUFFER. >>>> >>>> The goal of this email is to: >>>> >>>> 1. Make everyone involved aware of this breakage as we may see similar >>>> breakage elsewhere. >>>> >>>> 2. Discuss if we should maybe revert the ACPICA change. >>>> >>>> If you are reading this then 1. has been accomplished :) >>>> >>>> Which leaves us with 2. I'm tending towards keeping the change, since >>>> it seems the right thing to do and dealing with the fallout. But since >>>> there is fallout we should also at least consider reverting the ACPICA >>>> change. >>>> >>>> So ACPI maintainers what is you take on this ? >> What a mess. Several thoughts... >> >> I think we should keep the internal AML interpreter behavior as is because it solves a problem. > > I also would prefer to deal with the fallout rather than to revert the fix. > > I think that the potential damage is limited and the affected driver code should be fixed anyway which may not happen if the change in question is reverted. Ok, so it seems that we are all in agreement to NOT revert and to deal with any (other) fallout instead. Andy / Darren can you get the sony-laptop fix merged and on its way to Linus as a fix please? Regards, Hans ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-05-07 7:34 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-05 12:55 5.6 regression caused by "ACPICA: Dispatcher: always generate buffer objects for ASL create_field() operator" Hans de Goede 2020-05-05 13:50 ` Maximilian Luz 2020-05-05 22:11 ` Kaneda, Erik 2020-05-06 17:21 ` Rafael J. Wysocki 2020-05-07 7:34 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).