On Wed, Jul 27, 2016 at 11:02 PM, Aaron Durbin [off-list ref] wrote:

On Wed, Jul 27, 2016 at 7:09 PM, Rafael J. Wysocki [off-list ref] wrote:

quoted

On Monday, July 25, 2016 02:11:12 PM Aaron Durbin wrote:

quoted

On Fri, Jul 22, 2016 at 4:04 PM, Rafael J. Wysocki [off-list ref] wrote:

quoted

On Friday, July 22, 2016 12:26:03 PM Aaron Durbin wrote:

quoted

On Thu, Jul 21, 2016 at 7:55 PM, Rafael J. Wysocki [off-list ref] wrote:

quoted

On Wednesday, July 20, 2016 08:58:29 PM Aaron Durbin wrote:

quoted

On Wed, Jul 20, 2016 at 7:40 PM, Rafael J. Wysocki [off-list ref] wrote:

quoted

On Wednesday, July 20, 2016 06:02:08 PM Aaron Durbin wrote:

[...]

quoted

quoted

quoted

quoted

Isn't the problem a probe ordering issue, though?

No. Not just ordering. There's no parent-child relationship taken into
account when PCI devices are added to the resource tree. Deferring
adding an ACPI device's resources would likely fail similarly because
one doesn't take into account the proper parent. One lives in PCI land
-- the other in ACPI land.

What exactly do you mean by "PCI land" and "ACPI land"?

All PCI resources are derived from the host bridge ones that come from
ACPI anyway.

Except that the BARs are queried in the pci devices themselves and
those resources are added w/o any communication to/from ACPI. That's
all done in the pci subsystem internally. Only if

Well, not quite.

It gets to host bridge resources via pcibios_bus_to_resource() AFAICS and
those come from ACPI on ACPI-based systems.

From what I can tell that's just updating resource values that have a
translation across a bridge. For a 1:1 mapping nothing is different
from the BAR encoded in the PCI device itself. Those windows are just
encoded in the bridge device. The only thing adjusted on the resources
is an offset. The BAR values are read from the device. That path is
just adjusting resources -- it's not adding those individual resources
into the resource tree for pci devices. See below.

quoted

quoted

quoted

quoted

Currently, the issue is manifesting that the ACPI device's resources
are added into the resource tree. PCI devices the follow trying to add
their own resources. Conflicts ensue without taking into account the
proper hierarchy. To make matters worse, struct resource doesn't have
a struct device -- only a name. That means there's no way of knowing
who owns what resource. All that information is unavailable. That's
why the semi-hacky proposal was to see if there is an ACPI companion
device and see if it has children. If it's children's names match the
conflicting resource it's a good bet that the PCI device's resources
should be placed as the parent in the resource tree.

That really looks like a broken hierarchy of devices somewhere.

Why do you think it's a broken hierarchy? This CL has the heirarchy:
https://chromium-review.googlesource.com/#/c/359432/

There is a device, P2S, which would result in an ACPI companion device
when the pci_dev is added to the device infrastructure by way of
acpi_platform_notify() which does the ACPI companion binding.
device_add() is the caller of platform_notify() which is set to
acpi_platform_notify() in init_acpi_device_notify() by way of
acpi_init().

There are 4 children of the P2S device: GPO0, GPO1, GPO2, GPO3.

Those children devices utilize a sub-resource of the P2S device which
has a pci_dev companion. It's BAR0 is the resource.

OK

quoted

quoted

And one more thing: a struct acpi_device object is not a device in general
(althogh some pieces of code in the kernel, arguably incorrectly, treat it
this way).  It is an abstract representation of a firmware entity (a node
in the ACPI namespace).

There really should be a "physical device" thing corresponding to it and
that should be a child of the PCI device object in question.

I thought I established that by the sysfs readlink I output I
previously provided?

# ls /sys/devices/platform/| grep INT3452
INT3452:00
INT3452:01
INT3452:02
INT3452:03

# ls /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:11 | grep INT3452
INT3452:00
INT3452:01
INT3452:02
INT3452:03

# readlink -f /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:11/INT3452:00/physical_node
/sys/devices/platform/INT3452:00

OK

I must have missed that part.

quoted

quoted

quoted

quoted

Do we assign BARs for endpoints during enumeration or only when we find a
matching driver?

BARs look to be assigned during enumeration. There's where conflicts
are found and resources reassigned in the current failing case.

OK

You seem to be saying that we insert some resources for ACPI device objects
that correspond to the children of ACPI companions of PCI devices before we
assign the BARs for their parent devices.

The pci_dev is the one owning the parent resource. Even if there is a
companion ACPI device with a resource it doesn't matter because there
will still be a conflict because there is no checking against the
companion. There are 2 parallel entities without any communication
between the PCI subsystem and the ACPI subsystem when resources are
being added.

quoted

To me, that sounds very suspicious.

Where in the code are the ACPI resources inserted, exactly?

I believe it's through this mechanism:
acpi_bus_attach() -> acpi_default_enumeration() ->
acpi_create_platform_device() -> platform_device_register_full()

I still would like to understand how and why that is called for child devices
before the (physical) parent is enumerated.

The theory is that when the PCI host bridge is found in the ACPI namespace,
it will be enumerated along with the entire PCI bus below it before enumerating
any _HID devices in the ACPI namespace below the host bridge object.

Therefore all PCI devices should be enumerated before any children of any of
them that aren't PCI devices themselves.  If BARs are assigned during
enumeration, they all should be assigned before inserting any resources
for any non-PCI children of any PCI devices.

If that doesn't happen, the practice doesn't match the theory and that very
well may be the source of the problem you're seeing.

I'll sprinkle some WARN_ON()s in the resource code to track down when
things are being added to provide a definitive answer to when these
resources are being inserted. The current issue is still stands:
there is nothing properly tying the 2 sources of resource information
together. And ACPI resources are added prior to PCI resources.

OK, I see what's going on.

quoted

PCI devices might be enumerated, but that doesn't mean their
*resources* are added.

Right.

quoted

Back to the subsys_initcall topic:

pci_subsys_init() is called *after* acpi_init().

pci_subsys_init() -> pcibios_init() -> pcibios_resource_survey() ->
pcibios_allocate_resources() -> pcibios_allocate_dev_resources() ->
pci_claim_resource()

So first, the PCI resources are allocated in an additional pass after bus
enumeration and after enumerating platform devices based on the ACPI namespace.
That causes resources of the (non-PCI) children to be inserted before the
resources of the (PCI) parents.

What is the proposal for fixing this issue?

I can think about a couple of ways, but I'm not sure which one is the best
candidate from the regression risk perspective etc.

I need to talk about that to some people who are on vacation ATM.

quoted

quoted

But the problem seems to be that acpi_create_platform_device() doesn't
check parent resources at all.  Namely, when pdevinfo.parent is set, the
function should look at the resources of the parent and possibly populate
the parent field of the new device's resources before passing them to
platform_device_register_full().

In the particular issue I reported how would the parent device have
any resources if it's a PCI device?

If we can fix the ordering (so that the resources of parents are always
inserted before the resources of their children), then it shouldn't
matter whether or not it is a PCI device.

In any case, though, its ACPI companion will have information on the
resources the device is using.

Thanks. Let me know if you want me to do anything.. validate
something. Take a stab at something, etc.

Was anyone able to take a look into a solution for the current
problem?  Again, please feel free to ask if anyone would like help
testing potential solutions.

Thanks.

-Aaron