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, 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.
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).
Regards,
Hans
