[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-pci
Subject:    Re: ACPI device using sub-resource of PCI device
From:       Aaron Durbin <adurbin () google ! com>
Date:       2016-07-28 4:02:22
Message-ID: CAE2855sk2M_77W7dPUb-G43zaDgiNvH2fwpEUvZKOZCBZ-1UpQ () mail ! gmail ! com
[Download RAW message or body]

On Wed, Jul 27, 2016 at 7:09 PM, Rafael J. Wysocki <rjw@rjwysocki.net> 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@rjwysocki.net> 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@rjwysocki.net> 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@rjwysocki.net> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]