On Wed, Jan 23, 2019 at 11:42 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi, > > On 23-01-19 23:27, Rafael J. Wysocki wrote: > > On Wed, Jan 23, 2019 at 11:08 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > >> > >> Hi All, > >> > >> While working on this bug: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1665610 > >> > >> I encountered a DSDT which contains the bits: > >> > >> Starting at line 6271: > >> > >> Device (SDHB) { > >> ... > >> Name (_DDN, "Intel(R) SDIO Controller - 80862295") > >> ... > >> Device (RTLW) { > >> Method (_STA, 0, NotSerialized) // _STA: Status > >> { > >> Local0 = Zero > >> If ((^^^^GPO1.AMMR == One)) > >> { > >> If ((^^^^GPO1.WID0 == One)) > >> { > >> Local0 = One > >> } > >> > >> If ((^^^^GPO1.WID1 == One)) > >> { > >> Local0 += 0x02 > >> } > >> } > >> > >> If ((Local0 == Zero)) > >> { > >> Return (Zero) > >> } > >> ElseIf ((Local0 == One)) > >> { > >> Return (0x0F) > >> } > >> ElseIf ((Local0 == 0x02)) > >> { > >> Return (Zero) > >> } > >> Else > >> { > >> Return (Zero) > >> } > >> } > >> } > >> ... > >> } > >> > >> There are other subdevices for if the device has a Broadcom wifi module > >> instead of a Realtek one, which return 0xf from _STA when > >> Local0 == Zero. > >> > >> And then there is this bit of AML for the Bluetooth part of the > >> broadcom/wifi module: > >> > >> Starting at line 9779: > >> > >> Scope (URT1) > >> { > >> Device (BTH3) > >> { > >> Method (_HID, 0, NotSerialized) // _HID: Hardware ID > >> { > >> Local0 = Zero > >> If ((^^^^GPO1.AMMR == One)) > >> { > >> If ((^^^^GPO1.WID0 == One)) > >> { > >> Local0 = One > >> } > >> If ((^^^^GPO1.WID1 == One)) > >> { > >> Local0 += 0x02 > >> } > >> } > >> > >> If ((Local0 == Zero)) > >> { > >> Return ("BCM2E5B") > >> } > >> ElseIf ((Local0 == One)) > >> { > >> Return ("OBDA0623") > >> } > >> ElseIf ((Local0 == 0x02)) > >> { > >> Return ("BCM2E74") > >> } > >> Else > >> { > >> Return ("BCM2E5B") > >> } > >> } > >> } > >> } > >> > >> What is happening here is that two GPIOs are connected to > >> (solder?) jumpers which indicate which type of wifi module > >> is installed. And instead of the UEFI firmware reading this > >> at boot and then setting an ACPI variable accordingly as is > >> typically done for options configurable through the setup > >> menu, the reading of the GPIOs is done from the AML code. And if I'm not mistaken, in order to read these GPIOs, the AML needs an opregion that needs to be backed by a GPIO controller etc. If so, for this to work it is not sufficient to enumerate the GPIO devices themselves, but it may even be necessary to enumerate the PCI bus and probe some drivers and all of that needs to happen even before we try to evaluate _HID and _STA for the devices in question. > >> The problem here is that we evaluate the _STA and _HID > >> methods while enumerating the ACPI devices, which we do in > >> the order they are listed in the AML code. > >> > >> The GPIO devices start at line 14974, so these are not enumerated > >> yet when we execute the _STA and _HID methods listed above, so > >> they alwys take the Local0 == Zero option, while on the device from: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1665610 > >> The Local0 == One option is the right one. > >> > >> I've provided the user with a DSDT override which hardcodes the > >> Local0 == One results for these _STA and _HID methods and that > >> fixes wifi and bluetooth not working on this device. > > > > So presumably there should be a _DEP to tell us that these GPIOs will > > be needed to enumerate the devices in question and so it is not there > > or we don't use it. Which is the case? > > A bit of both, there are 4 child-devices to the Device (SDHB) > which all use a similar _STA method, 2 of those miss the _DEPs > note the SDHB device itself has the _DEP, so arguably the > childs inherit that. The device with the troublesome _HID > method does have the _DEPs. I gather from this that if the kernel took _DEP into account, that would help to address the problem at hand. > Independent of the _DEP being there or not we only ever honor the > _DEP dependencies for Battery devices, for *all* other devices they are > ignored because often Linux lacks a driver for one or the other device > and that would then block other devices from being enumerated. That's right. > Changing this would be a huge change and is something which various > ACPI people were strongly against in the past. The current enumeration is two steps already, the first one is to create ACPI device objects and the second one it to bind drivers to them and possibly create platform device objects, attach them to PCI device objects and so on. In particular, we need all of the ACPI device objects for PCI devices to be there before starting step 2 for the PCI host bridge object or things will break, so enumerating top-level devices first completely (both steps) won't work. Also I don't see how doing the GPIO devices first will help. Yes, we can create ACPI device objects for them upfront theoretically, but will that be sufficient? Moreover, even though GPIOs are relevant in the context of this particular bug, the _HID and _STA might as well depend on I2C or similar accesses, so it would be good to have all of those covered, not just GPIO. I wonder if we could just defer the enumeration of devices with _DEP underneath. That is, evaluate _DEP before calling acpi_bus_type_and_status() in acpi_bus_check_add() and, if present and non-empty, put the handle onto a "to be inspected later" list, skip it along with everything below it and only get back to it when we have enumerated all stuff without _DEP. [And don't do that for devices on the PCI bus.]
