Hi,
On 30-01-19 11:04, Rafael J. Wysocki wrote:
On Wed, Jan 30, 2019 at 9:33 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Hi,
On 29-01-19 23:41, Rafael J. Wysocki wrote:
On Fri, Jan 25, 2019 at 11:47 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
<big snip>
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.
Nitpick, I assume that enumerated above includes having acpi_bus_attach()
called on them.
Yes, it does.
Ah, rereading your original proposal I understand what you want to do now.
Yes I think that will work for the problematic cases I am aware of
(which is mostly the one from this thread).
But I think if we're going to do this, it would be better to have
something like this:
walk-tree, adding devices where all _DEP dependencies are met (or there are none),
add devices with missing deps to a to-be-enumerated list.
acpi_bus_attach(acpi_root)
while (1) {
number_of_enumerated_devices = 0;
for-each-handle-on-to-be-enumerated-list {
dep_ok = check_DEP()
That would return 'false' until the acpi_bus_attach() below runs,
however, because that's when the drivers that install opregion
handlers are attached to devices. That's one of the reasons why the
problem is kind of tricky.
Right but only for nested DEPs, which is why the while(1) is around
this, so that we keep looking for (deeper nesting levels) devices
which have their DEPs satisfied after resolving the first level
of deps.
I expect that we will do 1 effective run (and 1 ineffective run)
through the loop on most devices, matching your proposal, and 2
effective runs on some devices (see below).
if (dep_ok) {
add-device()
number_of_enumerated_devices++
}
}
if (number_of_enumerated_devices == 0)
break;
acpi_bus_attach(acpi_root)
}
number_of_enumerated_devices = 0;
for-each-handle-on-to-be-enumerated-list {
add-device()
number_of_enumerated_devices++
}
if (number_of_enumerated_devices)
acpi_bus_attach(acpi_root)
I believe that this is pretty close to your proposal, except that
it tries to also honor nested _DEP-s, while still adding all
devices independent of _DEP at the end.
Nested _DEPs could be addressed in a different way, I think, but I'm
not sure if they are a concern in practice.
I've seen nested DEPs where the PMIC has a DEP on the I2C controller
and some other I2C, SPI or MMC child devices have a DEP on the PMIC
because the _PS0 / _PS3 makes PMIC opregion accesses.
We've been getting away with not properly dealing with this scenario
so far by building the I2C + PMIC drivers into the kernel and
having the MMC host driver be a module.
If we are going to tackle this, it would be good to deal with the
nested case too, I believe that will not be a lot of extra work.
A substantial amount of the effort for this change is going to be
to have a long testing period to make sure the new enumeration order
does not cause regressions, which we would need to re-do if we later
add support for the nested cases, so IMHO it would be best to get
this right from the start.
Regards,
Hans
