On Wed, Jul 27, 2016 at 11:02 PM, Aaron Durbin <adurbin@xxxxxxxxxx> wrote: > 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. 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 -- 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
