On Fri, Jan 25, 2019 at 11:47 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi, > > On 24-01-19 19:12, Rafael J. Wysocki wrote: > > On Wed, Jan 23, 2019 at 11:42 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > <snip> > > >>>> 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. > > Right. > > > 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. > > Well in *this* case the GPIO devices are seen by Linux as platform devices, > so we don't need to touch the PCI subsystem. > > >>>> 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. > > Yes I believe it will. > > >> 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? > > No your right, I thought just finding the GPIO controllers and calling > acpi_add_single_object() would be enough, but we also need to > call acpi_bus_attach on them. > > Still I think this could work as a quick fix, Maybe, but rather a dirty one IMO. > but if we're going to rework things to take _DEP into account for enumeration > ordering then I won't bother with this. > > > 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 agree that that would be good, but currently I'm tempted to look > for a minimal solution, rather then completely reworking how we > enumerate ACPI devices. > > > 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.] > > I agree with you that ideally we would honor _DEP during enumeration > and skip enumeration of devices (and their children) with a _DEP which > lists unmet dependencies. This is not exactly what I'm talking about, however. The idea is to defer the enumeration of devices with non-empty _DEP (that is, non-empty after taking some known pathological cases into account) and their children until all of the devices without _DEP have been enumerated. > But this is a huge change. The only way I can see this happening is if > we initially keep both the old and new methods, with some kernel commandline > to switch and then blog, etc. about this and ask people to test the new > way as soon as a kernel with the new way is actually available in distros > (Fedora gets new kernel releases in a month or so from the .0 release). > > If you feel this is the way to go and you can make some time to actually > implement this this year, then I will happily help with patch-review and > trying the new option on all my hardware. I'm afraid I don't have time > to do such an overhaul myself, but I'm willing to commit to reviewing > the patches (and testing). Fair enough. I may be able to find some time to do that, so we'll see.
