On Wed, Jul 27, 2016 at 7:09 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote: > On Monday, July 25, 2016 02:11:12 PM Aaron Durbin wrote: >> On Fri, Jul 22, 2016 at 4:04 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote: >> > On Friday, July 22, 2016 12:26:03 PM Aaron Durbin wrote: >> >> On Thu, Jul 21, 2016 at 7:55 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote: >> >> > On Wednesday, July 20, 2016 08:58:29 PM Aaron Durbin wrote: >> >> >> On Wed, Jul 20, 2016 at 7:40 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote: >> >> >> > On Wednesday, July 20, 2016 06:02:08 PM Aaron Durbin wrote: >> >> > >> >> > [...] >> >> > >> >> >> >> > 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. >> >> >> >> > >> >> >> > >> >> >> >> 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 >> >> > >> >> >> > >> >> >> > 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. >> >> > >> >> >> > >> >> >> >> > 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. >> >> >> >> >> >> > >> >> >> > 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. >> > >> >> PCI devices might be enumerated, but that doesn't mean their >> >> *resources* are added. >> > >> > Right. >> > >> >> 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. > >> > 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. -Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
