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, Rafael -- 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
