ACPI device using sub-resource of PCI device

ACPI device using sub-resource of PCI device

[Aaron Durbin]

Hi,

We're currently running into a problem of resource conflicts with a
PCI device and ACPI devices.

[    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
0xd0c00000-0xd0c03fff]

The PCI BAR covers a large amount mmio resources, however, there are
ACPI devices with their own HID (for probing) which uses resources
that are a subset of the PCI BAR.

Short of re-structuring the linux driver is there anything that can be
done with ASL to properly have the ACPI device use a sub-resource of
the PCI device during the ACPI/PCI probing?

Thanks for the help/insight.

-Aaron

Re: ACPI device using sub-resource of PCI device

[Rafael J. Wysocki]

On Wed, May 18, 2016 at 5:54 PM, Aaron Durbin <adurbin@google.com> wrote:
> Hi,
>
> We're currently running into a problem of resource conflicts with a
> PCI device and ACPI devices.
>
> [    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
> 0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
> 0xd0c00000-0xd0c03fff]
>
> The PCI BAR covers a large amount mmio resources, however, there are
> ACPI devices with their own HID (for probing) which uses resources
> that are a subset of the PCI BAR.
>
> Short of re-structuring the linux driver is there anything that can be
> done with ASL to properly have the ACPI device use a sub-resource of
> the PCI device during the ACPI/PCI probing?

Do you have an ACPI device object corresponding to the PCI device?

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

On Wed, May 18, 2016 at 4:25 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, May 18, 2016 at 5:54 PM, Aaron Durbin <adurbin@google.com> wrote:
>> Hi,
>>
>> We're currently running into a problem of resource conflicts with a
>> PCI device and ACPI devices.
>>
>> [    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
>> 0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
>> 0xd0c00000-0xd0c03fff]
>>
>> The PCI BAR covers a large amount mmio resources, however, there are
>> ACPI devices with their own HID (for probing) which uses resources
>> that are a subset of the PCI BAR.
>>
>> Short of re-structuring the linux driver is there anything that can be
>> done with ASL to properly have the ACPI device use a sub-resource of
>> the PCI device during the ACPI/PCI probing?
>
> Do you have an ACPI device object corresponding to the PCI device?

I've been debugging this by proxy, and I did request that test. The
following is the overall structure:

scope (\_SB.PCI0) {

Device (P2S)
{
        Name (_ADR, 0x000D0000)
        Device (GPO0)
        {
                Name (_ADR, 0)
                Name (_HID, "INT3452")
                Name (_CID, "INT3452")
        }
}
}

There are _STA methods in both Devices. The GP0 device has a _CRS
method which just returns a ResourceTemplate which is filled in with
static values. The PCI bar is at a fixed address from the firmware
which allows the fixed calculations. However there is no specific
reference to the P2S device's resources proper -- only the parent
child relationship within the ASL. I'm not sure how to directly say "I
want this sub-region of this other device's resource for my resource."
That seems like the right thing, but it's not clear if that's implied
by hierarchy of the devices.

Lastly, if it helps, the kernel being used is based on v4.4 (no core
patches on top).

Thanks.

-Aaron

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

On Wed, May 18, 2016 at 3:32 PM, Aaron Durbin <adurbin@google.com> wrote:
> On Wed, May 18, 2016 at 4:25 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, May 18, 2016 at 5:54 PM, Aaron Durbin <adurbin@google.com> wrote:
>>> Hi,
>>>
>>> We're currently running into a problem of resource conflicts with a
>>> PCI device and ACPI devices.
>>>
>>> [    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
>>> 0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
>>> 0xd0c00000-0xd0c03fff]
>>>
>>> The PCI BAR covers a large amount mmio resources, however, there are
>>> ACPI devices with their own HID (for probing) which uses resources
>>> that are a subset of the PCI BAR.
>>>
>>> Short of re-structuring the linux driver is there anything that can be
>>> done with ASL to properly have the ACPI device use a sub-resource of
>>> the PCI device during the ACPI/PCI probing?
>>
>> Do you have an ACPI device object corresponding to the PCI device?
>
> I've been debugging this by proxy, and I did request that test. The
> following is the overall structure:
>
> scope (\_SB.PCI0) {
>
> Device (P2S)
> {
>         Name (_ADR, 0x000D0000)
>         Device (GPO0)
>         {
>                 Name (_ADR, 0)
>                 Name (_HID, "INT3452")
>                 Name (_CID, "INT3452")
>         }
> }
> }
>
> There are _STA methods in both Devices. The GP0 device has a _CRS
> method which just returns a ResourceTemplate which is filled in with
> static values. The PCI bar is at a fixed address from the firmware
> which allows the fixed calculations. However there is no specific
> reference to the P2S device's resources proper -- only the parent
> child relationship within the ASL. I'm not sure how to directly say "I
> want this sub-region of this other device's resource for my resource."
> That seems like the right thing, but it's not clear if that's implied
> by hierarchy of the devices.
>
> Lastly, if it helps, the kernel being used is based on v4.4 (no core
> patches on top).
>

Hi Rafael,

I haven't tried a newer kernel yet, but are you of the opinion that
having the Devices as parent-child within the ASL should work? I'm
wondering if there's already a patch in newer kernels that doesn't
report the conflict and works as expected once there are child Devices
under the P2S device.

Thanks for the help.

-Aaron

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

On Tue, Jun 14, 2016 at 12:04 PM, Aaron Durbin <adurbin@google.com> wrote:
> On Wed, May 18, 2016 at 3:32 PM, Aaron Durbin <adurbin@google.com> wrote:
>> On Wed, May 18, 2016 at 4:25 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Wed, May 18, 2016 at 5:54 PM, Aaron Durbin <adurbin@google.com> wrote:
>>>> Hi,
>>>>
>>>> We're currently running into a problem of resource conflicts with a
>>>> PCI device and ACPI devices.
>>>>
>>>> [    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
>>>> 0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
>>>> 0xd0c00000-0xd0c03fff]
>>>>
>>>> The PCI BAR covers a large amount mmio resources, however, there are
>>>> ACPI devices with their own HID (for probing) which uses resources
>>>> that are a subset of the PCI BAR.
>>>>
>>>> Short of re-structuring the linux driver is there anything that can be
>>>> done with ASL to properly have the ACPI device use a sub-resource of
>>>> the PCI device during the ACPI/PCI probing?
>>>
>>> Do you have an ACPI device object corresponding to the PCI device?
>>
>> I've been debugging this by proxy, and I did request that test. The
>> following is the overall structure:
>>
>> scope (\_SB.PCI0) {
>>
>> Device (P2S)
>> {
>>         Name (_ADR, 0x000D0000)
>>         Device (GPO0)
>>         {
>>                 Name (_ADR, 0)
>>                 Name (_HID, "INT3452")
>>                 Name (_CID, "INT3452")
>>         }
>> }
>> }
>>
>> There are _STA methods in both Devices. The GP0 device has a _CRS
>> method which just returns a ResourceTemplate which is filled in with
>> static values. The PCI bar is at a fixed address from the firmware
>> which allows the fixed calculations. However there is no specific
>> reference to the P2S device's resources proper -- only the parent
>> child relationship within the ASL. I'm not sure how to directly say "I
>> want this sub-region of this other device's resource for my resource."
>> That seems like the right thing, but it's not clear if that's implied
>> by hierarchy of the devices.
>>
>> Lastly, if it helps, the kernel being used is based on v4.4 (no core
>> patches on top).
>>
>
> Hi Rafael,
>
> I haven't tried a newer kernel yet, but are you of the opinion that
> having the Devices as parent-child within the ASL should work? I'm
> wondering if there's already a patch in newer kernels that doesn't
> report the conflict and works as expected once there are child Devices
> under the P2S device.
>

I've been looking at this more closely. A child ACPI device under a
ACPI PCI device doesn't change the resource conflict even when a _CRS
method is added to the ACPI PCI device.  Below is my sleuthing which
is probably not a surprise to anyone here, but please correct me where
I am wrong.

acpi_init() and pci_subsys_init() are both subsys_initcalls during
boot up. I'm not sure if the ordering is dumb luck or not, but
acpi_init() is called prior to pci_subsys_init(). The conflict error
is spit out from pcibios_resource_survey() by way of pci_subsys_init()
 subsys_initcall. However, the PCI device scanning is kicked off prior
to this through acpi_scan_init() by way of acpi_init()
subsys_initcall.  The conflict error occurs because there's already
the child ACPI device in the resource tree. I'm not sure when/where
those ACPI devices' resources are added, but clearly they are sitting
in there since the conflict was found.

Somewhere along the way a PCI device from a scan is linked with the
ACPI device for that same PCI device in sysfs.  This is with me
putting a _HID and _CID in the PCI ACPI device.
# readlink -f /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00/physical_node
/sys/devices/pci0000:00/0000:00:0d.0
# readlink -f /sys/devices/pci0000\:00/0000\:00\:0d.0/firmware_node/
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00

So the hierarchy is known eventually, but it's clearly not honored
when adding resources. The current ACPI support doesn't handle
PciBarTarget which initially sounds (from ACPI spec) like the way to
go for referencing a resource in a PCI device from an ACPI device. So
that's out of the question currently, but maybe someone has a patch
for that? I don't think reordering the acpi_init() and
pci_subsys_init() would do anything different except change which
device discovers the conflict.

Is there a way to honor the ACPI device hierarchy during resource
addition for the PCI devices? The conflict is found because of the
presence of a child device claiming resources through _CRS.
Alternatively, is there a good way to defer the probing of an ACPI
device until one knows PCI resources have been added?

Any insights would be very helpful. Thank you.

-Aaron

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

On Fri, Jun 24, 2016 at 12:34 PM, Aaron Durbin <adurbin@google.com> wrote:
> On Tue, Jun 14, 2016 at 12:04 PM, Aaron Durbin <adurbin@google.com> wrote:
>> On Wed, May 18, 2016 at 3:32 PM, Aaron Durbin <adurbin@google.com> wrote:
>>> On Wed, May 18, 2016 at 4:25 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>> On Wed, May 18, 2016 at 5:54 PM, Aaron Durbin <adurbin@google.com> wrote:
>>>>> Hi,
>>>>>
>>>>> We're currently running into a problem of resource conflicts with a
>>>>> PCI device and ACPI devices.
>>>>>
>>>>> [    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
>>>>> 0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
>>>>> 0xd0c00000-0xd0c03fff]
>>>>>
>>>>> The PCI BAR covers a large amount mmio resources, however, there are
>>>>> ACPI devices with their own HID (for probing) which uses resources
>>>>> that are a subset of the PCI BAR.
>>>>>
>>>>> Short of re-structuring the linux driver is there anything that can be
>>>>> done with ASL to properly have the ACPI device use a sub-resource of
>>>>> the PCI device during the ACPI/PCI probing?
>>>>
>>>> Do you have an ACPI device object corresponding to the PCI device?
>>>
>>> I've been debugging this by proxy, and I did request that test. The
>>> following is the overall structure:
>>>
>>> scope (\_SB.PCI0) {
>>>
>>> Device (P2S)
>>> {
>>>         Name (_ADR, 0x000D0000)
>>>         Device (GPO0)
>>>         {
>>>                 Name (_ADR, 0)
>>>                 Name (_HID, "INT3452")
>>>                 Name (_CID, "INT3452")
>>>         }
>>> }
>>> }
>>>
>>> There are _STA methods in both Devices. The GP0 device has a _CRS
>>> method which just returns a ResourceTemplate which is filled in with
>>> static values. The PCI bar is at a fixed address from the firmware
>>> which allows the fixed calculations. However there is no specific
>>> reference to the P2S device's resources proper -- only the parent
>>> child relationship within the ASL. I'm not sure how to directly say "I
>>> want this sub-region of this other device's resource for my resource."
>>> That seems like the right thing, but it's not clear if that's implied
>>> by hierarchy of the devices.
>>>
>>> Lastly, if it helps, the kernel being used is based on v4.4 (no core
>>> patches on top).
>>>
>>
>> Hi Rafael,
>>
>> I haven't tried a newer kernel yet, but are you of the opinion that
>> having the Devices as parent-child within the ASL should work? I'm
>> wondering if there's already a patch in newer kernels that doesn't
>> report the conflict and works as expected once there are child Devices
>> under the P2S device.
>>
>
> I've been looking at this more closely. A child ACPI device under a
> ACPI PCI device doesn't change the resource conflict even when a _CRS
> method is added to the ACPI PCI device.  Below is my sleuthing which
> is probably not a surprise to anyone here, but please correct me where
> I am wrong.
>
> acpi_init() and pci_subsys_init() are both subsys_initcalls during
> boot up. I'm not sure if the ordering is dumb luck or not, but
> acpi_init() is called prior to pci_subsys_init(). The conflict error
> is spit out from pcibios_resource_survey() by way of pci_subsys_init()
>  subsys_initcall. However, the PCI device scanning is kicked off prior
> to this through acpi_scan_init() by way of acpi_init()
> subsys_initcall.  The conflict error occurs because there's already
> the child ACPI device in the resource tree. I'm not sure when/where
> those ACPI devices' resources are added, but clearly they are sitting
> in there since the conflict was found.
>
> Somewhere along the way a PCI device from a scan is linked with the
> ACPI device for that same PCI device in sysfs.  This is with me
> putting a _HID and _CID in the PCI ACPI device.
> # readlink -f /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00/physical_node
> /sys/devices/pci0000:00/0000:00:0d.0
> # readlink -f /sys/devices/pci0000\:00/0000\:00\:0d.0/firmware_node/
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00
>
> So the hierarchy is known eventually, but it's clearly not honored
> when adding resources. The current ACPI support doesn't handle
> PciBarTarget which initially sounds (from ACPI spec) like the way to
> go for referencing a resource in a PCI device from an ACPI device. So
> that's out of the question currently, but maybe someone has a patch
> for that? I don't think reordering the acpi_init() and
> pci_subsys_init() would do anything different except change which
> device discovers the conflict.
>
> Is there a way to honor the ACPI device hierarchy during resource
> addition for the PCI devices? The conflict is found because of the
> presence of a child device claiming resources through _CRS.
> Alternatively, is there a good way to defer the probing of an ACPI
> device until one knows PCI resources have been added?
>
> Any insights would be very helpful. Thank you.

I stumbled upon the hierarchy connection. That's all handled with the
platform_notify() end of things when device_add() is done on the pci
device. I was thinking we could take advantage of this when adding
resources, but a struct resource has no struct device. It's just a
name description for the resource at hand. However, platform devices
are added when the ACPI tree is parsed along with adding the resources
associated with them (PciBarTarget would be helpful here) so those
resources are sitting in the resource tree when PCI BARs are added.

The following suggestion is sort of hacky, but it's the best I could
come up with provided the currently supported infrastructure. In
pci_claim_resource() do request_resource_conflict() as before. If it
fails do the following: 1. check if the device has an ACPI companion.
2. For any children hanging off the ACPI companion device. check if
that device's name matches the conflict resource's name. 3. If so,
insert_resource_conflict() to place the BAR within the tree itself.

Thoughts or suggestions are welcomed.

Thank you.

-Aaron

Re: ACPI device using sub-resource of PCI device

[Bjorn Helgaas]

On Tue, Jun 28, 2016 at 09:41:19PM -0700, Aaron Durbin wrote:
> On Fri, Jun 24, 2016 at 12:34 PM, Aaron Durbin <adurbin@google.com> wrote:
> > On Tue, Jun 14, 2016 at 12:04 PM, Aaron Durbin <adurbin@google.com> wrote:
> >> On Wed, May 18, 2016 at 3:32 PM, Aaron Durbin <adurbin@google.com> wrote:
> >>> On Wed, May 18, 2016 at 4:25 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>>> On Wed, May 18, 2016 at 5:54 PM, Aaron Durbin <adurbin@google.com> wrote:
> >>>>> Hi,
> >>>>>
> >>>>> We're currently running into a problem of resource conflicts with a
> >>>>> PCI device and ACPI devices.
> >>>>>
> >>>>> [    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
> >>>>> 0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
> >>>>> 0xd0c00000-0xd0c03fff]
> >>>>>
> >>>>> The PCI BAR covers a large amount mmio resources, however, there are
> >>>>> ACPI devices with their own HID (for probing) which uses resources
> >>>>> that are a subset of the PCI BAR.
> >>>>>
> >>>>> Short of re-structuring the linux driver is there anything that can be
> >>>>> done with ASL to properly have the ACPI device use a sub-resource of
> >>>>> the PCI device during the ACPI/PCI probing?
> >>>>
> >>>> Do you have an ACPI device object corresponding to the PCI device?
> >>>
> >>> I've been debugging this by proxy, and I did request that test. The
> >>> following is the overall structure:
> >>>
> >>> scope (\_SB.PCI0) {
> >>>
> >>> Device (P2S)
> >>> {
> >>>         Name (_ADR, 0x000D0000)
> >>>         Device (GPO0)
> >>>         {
> >>>                 Name (_ADR, 0)
> >>>                 Name (_HID, "INT3452")
> >>>                 Name (_CID, "INT3452")
> >>>         }
> >>> }
> >>> }
> >>>
> >>> There are _STA methods in both Devices. The GP0 device has a _CRS
> >>> method which just returns a ResourceTemplate which is filled in with
> >>> static values. The PCI bar is at a fixed address from the firmware
> >>> which allows the fixed calculations. However there is no specific
> >>> reference to the P2S device's resources proper -- only the parent
> >>> child relationship within the ASL. I'm not sure how to directly say "I
> >>> want this sub-region of this other device's resource for my resource."
> >>> That seems like the right thing, but it's not clear if that's implied
> >>> by hierarchy of the devices.
> >>>
> >>> Lastly, if it helps, the kernel being used is based on v4.4 (no core
> >>> patches on top).
> >>>
> >>
> >> Hi Rafael,
> >>
> >> I haven't tried a newer kernel yet, but are you of the opinion that
> >> having the Devices as parent-child within the ASL should work? I'm
> >> wondering if there's already a patch in newer kernels that doesn't
> >> report the conflict and works as expected once there are child Devices
> >> under the P2S device.
> >>
> >
> > I've been looking at this more closely. A child ACPI device under a
> > ACPI PCI device doesn't change the resource conflict even when a _CRS
> > method is added to the ACPI PCI device.  Below is my sleuthing which
> > is probably not a surprise to anyone here, but please correct me where
> > I am wrong.
> >
> > acpi_init() and pci_subsys_init() are both subsys_initcalls during
> > boot up. I'm not sure if the ordering is dumb luck or not, but
> > acpi_init() is called prior to pci_subsys_init(). The conflict error
> > is spit out from pcibios_resource_survey() by way of pci_subsys_init()
> >  subsys_initcall. However, the PCI device scanning is kicked off prior
> > to this through acpi_scan_init() by way of acpi_init()
> > subsys_initcall.  The conflict error occurs because there's already
> > the child ACPI device in the resource tree. I'm not sure when/where
> > those ACPI devices' resources are added, but clearly they are sitting
> > in there since the conflict was found.

I think the acpi_init()/pci_subsys_init() ordering is correct.  The
ACPI namespace is primary.  A PCI hierarchy originates at a PCI host
bridge in the ACPI namespace, so we should enumerate the ACPI
namespace first, and when we find a PCI host bridge, we should
enumerate the PCI devices below it.

That said, I think it is correct mostly by accident and it would be
nice if it were more explicit.

> > Somewhere along the way a PCI device from a scan is linked with the
> > ACPI device for that same PCI device in sysfs.  This is with me
> > putting a _HID and _CID in the PCI ACPI device.
> > # readlink -f /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00/physical_node
> > /sys/devices/pci0000:00/0000:00:0d.0
> > # readlink -f /sys/devices/pci0000\:00/0000\:00\:0d.0/firmware_node/
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00
> >
> > So the hierarchy is known eventually, but it's clearly not honored
> > when adding resources. The current ACPI support doesn't handle
> > PciBarTarget which initially sounds (from ACPI spec) like the way to
> > go for referencing a resource in a PCI device from an ACPI device.

Yes, I think PCIBARTarget looks like the right way to do this.  It
doesn't *seem* like it'd be that hard to implement; have you looked
into that at all?

Without PCIBARTarget, the AML contains fixed register addresses, so it
will break if Linux reassigns the BAR.

> > So
> > that's out of the question currently, but maybe someone has a patch
> > for that? I don't think reordering the acpi_init() and
> > pci_subsys_init() would do anything different except change which
> > device discovers the conflict.
> >
> > Is there a way to honor the ACPI device hierarchy during resource
> > addition for the PCI devices? The conflict is found because of the
> > presence of a child device claiming resources through _CRS.
> > Alternatively, is there a good way to defer the probing of an ACPI
> > device until one knows PCI resources have been added?
> >
> > Any insights would be very helpful. Thank you.
> 
> I stumbled upon the hierarchy connection. That's all handled with the
> platform_notify() end of things when device_add() is done on the pci
> device. I was thinking we could take advantage of this when adding
> resources, but a struct resource has no struct device. It's just a
> name description for the resource at hand. However, platform devices
> are added when the ACPI tree is parsed along with adding the resources
> associated with them (PciBarTarget would be helpful here) so those
> resources are sitting in the resource tree when PCI BARs are added.
> 
> The following suggestion is sort of hacky, but it's the best I could
> come up with provided the currently supported infrastructure. In
> pci_claim_resource() do request_resource_conflict() as before. If it
> fails do the following: 1. check if the device has an ACPI companion.
> 2. For any children hanging off the ACPI companion device. check if
> that device's name matches the conflict resource's name. 3. If so,
> insert_resource_conflict() to place the BAR within the tree itself.

I think the best solution would be to implement PCIBARTarget, but if
that's impossible, this seems like a plausible workaround.

I don't know if the conflict would necessarily have to be with the
ACPI companion itself.  It seems like you could have some hierarchy of
ACPI devices where the leaf conflicts with a PCI BAR.  Maybe if a
resource of *any* ACPI device below a PCI host bridge conflicts with a
PCI BAR, we should insert the PCI BAR as a parent?

And since moving that BAR would break the AML, we should probably mark
the BAR as IORESOURCE_PCI_FIXED.

Bjorn

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

On Wed, Jul 20, 2016 at 2:35 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Jun 28, 2016 at 09:41:19PM -0700, Aaron Durbin wrote:
>> On Fri, Jun 24, 2016 at 12:34 PM, Aaron Durbin <adurbin@google.com> wrote:
>> > On Tue, Jun 14, 2016 at 12:04 PM, Aaron Durbin <adurbin@google.com> wrote:
>> >> On Wed, May 18, 2016 at 3:32 PM, Aaron Durbin <adurbin@google.com> wrote:
>> >>> On Wed, May 18, 2016 at 4:25 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> >>>> On Wed, May 18, 2016 at 5:54 PM, Aaron Durbin <adurbin@google.com> wrote:
>> >>>>> Hi,
>> >>>>>
>> >>>>> We're currently running into a problem of resource conflicts with a
>> >>>>> PCI device and ACPI devices.
>> >>>>>
>> >>>>> [    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
>> >>>>> 0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
>> >>>>> 0xd0c00000-0xd0c03fff]
>> >>>>>
>> >>>>> The PCI BAR covers a large amount mmio resources, however, there are
>> >>>>> ACPI devices with their own HID (for probing) which uses resources
>> >>>>> that are a subset of the PCI BAR.
>> >>>>>
>> >>>>> Short of re-structuring the linux driver is there anything that can be
>> >>>>> done with ASL to properly have the ACPI device use a sub-resource of
>> >>>>> the PCI device during the ACPI/PCI probing?
>> >>>>
>> >>>> Do you have an ACPI device object corresponding to the PCI device?
>> >>>
>> >>> I've been debugging this by proxy, and I did request that test. The
>> >>> following is the overall structure:
>> >>>
>> >>> scope (\_SB.PCI0) {
>> >>>
>> >>> Device (P2S)
>> >>> {
>> >>>         Name (_ADR, 0x000D0000)
>> >>>         Device (GPO0)
>> >>>         {
>> >>>                 Name (_ADR, 0)
>> >>>                 Name (_HID, "INT3452")
>> >>>                 Name (_CID, "INT3452")
>> >>>         }
>> >>> }
>> >>> }
>> >>>
>> >>> There are _STA methods in both Devices. The GP0 device has a _CRS
>> >>> method which just returns a ResourceTemplate which is filled in with
>> >>> static values. The PCI bar is at a fixed address from the firmware
>> >>> which allows the fixed calculations. However there is no specific
>> >>> reference to the P2S device's resources proper -- only the parent
>> >>> child relationship within the ASL. I'm not sure how to directly say "I
>> >>> want this sub-region of this other device's resource for my resource."
>> >>> That seems like the right thing, but it's not clear if that's implied
>> >>> by hierarchy of the devices.
>> >>>
>> >>> Lastly, if it helps, the kernel being used is based on v4.4 (no core
>> >>> patches on top).
>> >>>
>> >>
>> >> Hi Rafael,
>> >>
>> >> I haven't tried a newer kernel yet, but are you of the opinion that
>> >> having the Devices as parent-child within the ASL should work? I'm
>> >> wondering if there's already a patch in newer kernels that doesn't
>> >> report the conflict and works as expected once there are child Devices
>> >> under the P2S device.
>> >>
>> >
>> > I've been looking at this more closely. A child ACPI device under a
>> > ACPI PCI device doesn't change the resource conflict even when a _CRS
>> > method is added to the ACPI PCI device.  Below is my sleuthing which
>> > is probably not a surprise to anyone here, but please correct me where
>> > I am wrong.
>> >
>> > acpi_init() and pci_subsys_init() are both subsys_initcalls during
>> > boot up. I'm not sure if the ordering is dumb luck or not, but
>> > acpi_init() is called prior to pci_subsys_init(). The conflict error
>> > is spit out from pcibios_resource_survey() by way of pci_subsys_init()
>> >  subsys_initcall. However, the PCI device scanning is kicked off prior
>> > to this through acpi_scan_init() by way of acpi_init()
>> > subsys_initcall.  The conflict error occurs because there's already
>> > the child ACPI device in the resource tree. I'm not sure when/where
>> > those ACPI devices' resources are added, but clearly they are sitting
>> > in there since the conflict was found.
>
> I think the acpi_init()/pci_subsys_init() ordering is correct.  The
> ACPI namespace is primary.  A PCI hierarchy originates at a PCI host
> bridge in the ACPI namespace, so we should enumerate the ACPI
> namespace first, and when we find a PCI host bridge, we should
> enumerate the PCI devices below it.
>
> That said, I think it is correct mostly by accident and it would be
> nice if it were more explicit.
>
>> > Somewhere along the way a PCI device from a scan is linked with the
>> > ACPI device for that same PCI device in sysfs.  This is with me
>> > putting a _HID and _CID in the PCI ACPI device.
>> > # readlink -f /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00/physical_node
>> > /sys/devices/pci0000:00/0000:00:0d.0
>> > # readlink -f /sys/devices/pci0000\:00/0000\:00\:0d.0/firmware_node/
>> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00
>> >
>> > So the hierarchy is known eventually, but it's clearly not honored
>> > when adding resources. The current ACPI support doesn't handle
>> > PciBarTarget which initially sounds (from ACPI spec) like the way to
>> > go for referencing a resource in a PCI device from an ACPI device.
>
> Yes, I think PCIBARTarget looks like the right way to do this.  It
> doesn't *seem* like it'd be that hard to implement; have you looked
> into that at all?
>

I just noticed that linux has no support for it. With the ability of
the BAR to be reassigned it sounds way more complicated unless you
were considering the support to be one-shot. During ACPI scan one
would have to see if the _CRS is using PciBarTarget and defer resource
probing until PCI devices' resources were added. However, that
deferral should likely happen before the device is added to the system
such that driver probing doesn't occur on a partially resolved ACPI
device (we need the PCI device and its resources before a driver
should engage it).

> Without PCIBARTarget, the AML contains fixed register addresses, so it
> will break if Linux reassigns the BAR.

Understood. Do we need to worry about reassignment when a driver is
bound to a device? If so that complicates everything. I'm also not
sure how one would go about suspending the dependent child while a BAR
is being reassigned -- resources need to be removed and re-added for
both the PCI parent and the ACPI child device.

>
>> > So
>> > that's out of the question currently, but maybe someone has a patch
>> > for that? I don't think reordering the acpi_init() and
>> > pci_subsys_init() would do anything different except change which
>> > device discovers the conflict.
>> >
>> > Is there a way to honor the ACPI device hierarchy during resource
>> > addition for the PCI devices? The conflict is found because of the
>> > presence of a child device claiming resources through _CRS.
>> > Alternatively, is there a good way to defer the probing of an ACPI
>> > device until one knows PCI resources have been added?
>> >
>> > Any insights would be very helpful. Thank you.
>>
>> I stumbled upon the hierarchy connection. That's all handled with the
>> platform_notify() end of things when device_add() is done on the pci
>> device. I was thinking we could take advantage of this when adding
>> resources, but a struct resource has no struct device. It's just a
>> name description for the resource at hand. However, platform devices
>> are added when the ACPI tree is parsed along with adding the resources
>> associated with them (PciBarTarget would be helpful here) so those
>> resources are sitting in the resource tree when PCI BARs are added.
>>
>> The following suggestion is sort of hacky, but it's the best I could
>> come up with provided the currently supported infrastructure. In
>> pci_claim_resource() do request_resource_conflict() as before. If it
>> fails do the following: 1. check if the device has an ACPI companion.
>> 2. For any children hanging off the ACPI companion device. check if
>> that device's name matches the conflict resource's name. 3. If so,
>> insert_resource_conflict() to place the BAR within the tree itself.
>
> I think the best solution would be to implement PCIBARTarget, but if
> that's impossible, this seems like a plausible workaround.

I'm not sure I'm in a position to do it correctly as I'm not an ACPI
subystem expert. I was hoping someone already had this on their radar
for such support. I could take a stab at it, but the nuances listed
above makes me think it's beyond me currently w/ my current ACPI
subsystem knowledge. Perhaps there's an easy way to deal w/ deferrals
and removal of devices from the system to handle resource updates?
What am I missing w.r.t. your comment about not seeming that hard?

>
> I don't know if the conflict would necessarily have to be with the
> ACPI companion itself.  It seems like you could have some hierarchy of
> ACPI devices where the leaf conflicts with a PCI BAR.  Maybe if a
> resource of *any* ACPI device below a PCI host bridge conflicts with a
> PCI BAR, we should insert the PCI BAR as a parent?

I was going to attempt to see if the resource name matches a child.
Your suggestion seems to allow any conflict -- even if it's a true
conflict. That's why I specifically created an ACPI device for the PCI
one with a true ACPI device nested under that. There's a parent-child
relationship which is explicit that we can leverage in the kernel.

>
> And since moving that BAR would break the AML, we should probably mark
> the BAR as IORESOURCE_PCI_FIXED.
>
> Bjorn

Re: ACPI device using sub-resource of PCI device

[Rafael J. Wysocki]

On Wednesday, July 20, 2016 02:35:43 PM Bjorn Helgaas wrote:
> On Tue, Jun 28, 2016 at 09:41:19PM -0700, Aaron Durbin wrote:
> > On Fri, Jun 24, 2016 at 12:34 PM, Aaron Durbin <adurbin@google.com> wrote:
> > > On Tue, Jun 14, 2016 at 12:04 PM, Aaron Durbin <adurbin@google.com> wrote:
> > >> On Wed, May 18, 2016 at 3:32 PM, Aaron Durbin <adurbin@google.com> wrote:
> > >>> On Wed, May 18, 2016 at 4:25 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >>>> On Wed, May 18, 2016 at 5:54 PM, Aaron Durbin <adurbin@google.com> wrote:
> > >>>>> Hi,
> > >>>>>
> > >>>>> We're currently running into a problem of resource conflicts with a
> > >>>>> PCI device and ACPI devices.
> > >>>>>
> > >>>>> [    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
> > >>>>> 0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
> > >>>>> 0xd0c00000-0xd0c03fff]
> > >>>>>
> > >>>>> The PCI BAR covers a large amount mmio resources, however, there are
> > >>>>> ACPI devices with their own HID (for probing) which uses resources
> > >>>>> that are a subset of the PCI BAR.
> > >>>>>
> > >>>>> Short of re-structuring the linux driver is there anything that can be
> > >>>>> done with ASL to properly have the ACPI device use a sub-resource of
> > >>>>> the PCI device during the ACPI/PCI probing?
> > >>>>
> > >>>> Do you have an ACPI device object corresponding to the PCI device?
> > >>>
> > >>> I've been debugging this by proxy, and I did request that test. The
> > >>> following is the overall structure:
> > >>>
> > >>> scope (\_SB.PCI0) {
> > >>>
> > >>> Device (P2S)
> > >>> {
> > >>>         Name (_ADR, 0x000D0000)
> > >>>         Device (GPO0)
> > >>>         {
> > >>>                 Name (_ADR, 0)
> > >>>                 Name (_HID, "INT3452")
> > >>>                 Name (_CID, "INT3452")
> > >>>         }
> > >>> }
> > >>> }
> > >>>
> > >>> There are _STA methods in both Devices. The GP0 device has a _CRS
> > >>> method which just returns a ResourceTemplate which is filled in with
> > >>> static values. The PCI bar is at a fixed address from the firmware
> > >>> which allows the fixed calculations. However there is no specific
> > >>> reference to the P2S device's resources proper -- only the parent
> > >>> child relationship within the ASL. I'm not sure how to directly say "I
> > >>> want this sub-region of this other device's resource for my resource."
> > >>> That seems like the right thing, but it's not clear if that's implied
> > >>> by hierarchy of the devices.
> > >>>
> > >>> Lastly, if it helps, the kernel being used is based on v4.4 (no core
> > >>> patches on top).
> > >>>
> > >>
> > >> Hi Rafael,
> > >>
> > >> I haven't tried a newer kernel yet, but are you of the opinion that
> > >> having the Devices as parent-child within the ASL should work? I'm
> > >> wondering if there's already a patch in newer kernels that doesn't
> > >> report the conflict and works as expected once there are child Devices
> > >> under the P2S device.
> > >>
> > >
> > > I've been looking at this more closely. A child ACPI device under a
> > > ACPI PCI device doesn't change the resource conflict even when a _CRS
> > > method is added to the ACPI PCI device.  Below is my sleuthing which
> > > is probably not a surprise to anyone here, but please correct me where
> > > I am wrong.
> > >
> > > acpi_init() and pci_subsys_init() are both subsys_initcalls during
> > > boot up. I'm not sure if the ordering is dumb luck or not, but
> > > acpi_init() is called prior to pci_subsys_init(). The conflict error
> > > is spit out from pcibios_resource_survey() by way of pci_subsys_init()
> > >  subsys_initcall. However, the PCI device scanning is kicked off prior
> > > to this through acpi_scan_init() by way of acpi_init()
> > > subsys_initcall.  The conflict error occurs because there's already
> > > the child ACPI device in the resource tree. I'm not sure when/where
> > > those ACPI devices' resources are added, but clearly they are sitting
> > > in there since the conflict was found.
> 
> I think the acpi_init()/pci_subsys_init() ordering is correct.  The
> ACPI namespace is primary.  A PCI hierarchy originates at a PCI host
> bridge in the ACPI namespace, so we should enumerate the ACPI
> namespace first, and when we find a PCI host bridge, we should
> enumerate the PCI devices below it.
> 
> That said, I think it is correct mostly by accident and it would be
> nice if it were more explicit.

No, it isn't by accident.

The enumeration of PCI devices under a PCI host bridge discovered via ACPI
starts in acpi_pci_root_add() which quite explicitly is only called after
enumerating the ACPI namespace entirely.

acpi_bus_scan() has two passes now, one is to call acpi_bus_check_add() for
all namespace objects and the other is the acpi_bus_attach() pass where
all things like acpi_pci_root_add() are called.

> > > Somewhere along the way a PCI device from a scan is linked with the
> > > ACPI device for that same PCI device in sysfs.  This is with me
> > > putting a _HID and _CID in the PCI ACPI device.
> > > # readlink -f /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00/physical_node
> > > /sys/devices/pci0000:00/0000:00:0d.0
> > > # readlink -f /sys/devices/pci0000\:00/0000\:00\:0d.0/firmware_node/
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00
> > >
> > > So the hierarchy is known eventually, but it's clearly not honored
> > > when adding resources. The current ACPI support doesn't handle
> > > PciBarTarget which initially sounds (from ACPI spec) like the way to
> > > go for referencing a resource in a PCI device from an ACPI device.
> 
> Yes, I think PCIBARTarget looks like the right way to do this.  It
> doesn't *seem* like it'd be that hard to implement; have you looked
> into that at all?
> 
> Without PCIBARTarget, the AML contains fixed register addresses, so it
> will break if Linux reassigns the BAR.

Right.

> > > So
> > > that's out of the question currently, but maybe someone has a patch
> > > for that? I don't think reordering the acpi_init() and
> > > pci_subsys_init() would do anything different except change which
> > > device discovers the conflict.
> > >
> > > Is there a way to honor the ACPI device hierarchy during resource
> > > addition for the PCI devices? The conflict is found because of the
> > > presence of a child device claiming resources through _CRS.
> > > Alternatively, is there a good way to defer the probing of an ACPI
> > > device until one knows PCI resources have been added?
> > >
> > > Any insights would be very helpful. Thank you.
> > 
> > I stumbled upon the hierarchy connection. That's all handled with the
> > platform_notify() end of things when device_add() is done on the pci
> > device. I was thinking we could take advantage of this when adding
> > resources, but a struct resource has no struct device. It's just a
> > name description for the resource at hand. However, platform devices
> > are added when the ACPI tree is parsed along with adding the resources
> > associated with them (PciBarTarget would be helpful here) so those
> > resources are sitting in the resource tree when PCI BARs are added.
> > 
> > The following suggestion is sort of hacky, but it's the best I could
> > come up with provided the currently supported infrastructure. In
> > pci_claim_resource() do request_resource_conflict() as before. If it
> > fails do the following: 1. check if the device has an ACPI companion.
> > 2. For any children hanging off the ACPI companion device. check if
> > that device's name matches the conflict resource's name. 3. If so,
> > insert_resource_conflict() to place the BAR within the tree itself.
> 
> I think the best solution would be to implement PCIBARTarget, but if
> that's impossible, this seems like a plausible workaround.
> 
> I don't know if the conflict would necessarily have to be with the
> ACPI companion itself.  It seems like you could have some hierarchy of
> ACPI devices where the leaf conflicts with a PCI BAR.  Maybe if a
> resource of *any* ACPI device below a PCI host bridge conflicts with a
> PCI BAR, we should insert the PCI BAR as a parent?
> 
> And since moving that BAR would break the AML, we should probably mark
> the BAR as IORESOURCE_PCI_FIXED.

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

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

Thanks,
Rafael

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

On Wed, Jul 20, 2016 at 5:46 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, July 20, 2016 02:35:43 PM Bjorn Helgaas wrote:
>> On Tue, Jun 28, 2016 at 09:41:19PM -0700, Aaron Durbin wrote:
>> > On Fri, Jun 24, 2016 at 12:34 PM, Aaron Durbin <adurbin@google.com> wrote:
>> > > On Tue, Jun 14, 2016 at 12:04 PM, Aaron Durbin <adurbin@google.com> wrote:
>> > >> On Wed, May 18, 2016 at 3:32 PM, Aaron Durbin <adurbin@google.com> wrote:
>> > >>> On Wed, May 18, 2016 at 4:25 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > >>>> On Wed, May 18, 2016 at 5:54 PM, Aaron Durbin <adurbin@google.com> wrote:
>> > >>>>> Hi,
>> > >>>>>
>> > >>>>> We're currently running into a problem of resource conflicts with a
>> > >>>>> PCI device and ACPI devices.
>> > >>>>>
>> > >>>>> [    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
>> > >>>>> 0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
>> > >>>>> 0xd0c00000-0xd0c03fff]
>> > >>>>>
>> > >>>>> The PCI BAR covers a large amount mmio resources, however, there are
>> > >>>>> ACPI devices with their own HID (for probing) which uses resources
>> > >>>>> that are a subset of the PCI BAR.
>> > >>>>>
>> > >>>>> Short of re-structuring the linux driver is there anything that can be
>> > >>>>> done with ASL to properly have the ACPI device use a sub-resource of
>> > >>>>> the PCI device during the ACPI/PCI probing?
>> > >>>>
>> > >>>> Do you have an ACPI device object corresponding to the PCI device?
>> > >>>
>> > >>> I've been debugging this by proxy, and I did request that test. The
>> > >>> following is the overall structure:
>> > >>>
>> > >>> scope (\_SB.PCI0) {
>> > >>>
>> > >>> Device (P2S)
>> > >>> {
>> > >>>         Name (_ADR, 0x000D0000)
>> > >>>         Device (GPO0)
>> > >>>         {
>> > >>>                 Name (_ADR, 0)
>> > >>>                 Name (_HID, "INT3452")
>> > >>>                 Name (_CID, "INT3452")
>> > >>>         }
>> > >>> }
>> > >>> }
>> > >>>
>> > >>> There are _STA methods in both Devices. The GP0 device has a _CRS
>> > >>> method which just returns a ResourceTemplate which is filled in with
>> > >>> static values. The PCI bar is at a fixed address from the firmware
>> > >>> which allows the fixed calculations. However there is no specific
>> > >>> reference to the P2S device's resources proper -- only the parent
>> > >>> child relationship within the ASL. I'm not sure how to directly say "I
>> > >>> want this sub-region of this other device's resource for my resource."
>> > >>> That seems like the right thing, but it's not clear if that's implied
>> > >>> by hierarchy of the devices.
>> > >>>
>> > >>> Lastly, if it helps, the kernel being used is based on v4.4 (no core
>> > >>> patches on top).
>> > >>>
>> > >>
>> > >> Hi Rafael,
>> > >>
>> > >> I haven't tried a newer kernel yet, but are you of the opinion that
>> > >> having the Devices as parent-child within the ASL should work? I'm
>> > >> wondering if there's already a patch in newer kernels that doesn't
>> > >> report the conflict and works as expected once there are child Devices
>> > >> under the P2S device.
>> > >>
>> > >
>> > > I've been looking at this more closely. A child ACPI device under a
>> > > ACPI PCI device doesn't change the resource conflict even when a _CRS
>> > > method is added to the ACPI PCI device.  Below is my sleuthing which
>> > > is probably not a surprise to anyone here, but please correct me where
>> > > I am wrong.
>> > >
>> > > acpi_init() and pci_subsys_init() are both subsys_initcalls during
>> > > boot up. I'm not sure if the ordering is dumb luck or not, but
>> > > acpi_init() is called prior to pci_subsys_init(). The conflict error
>> > > is spit out from pcibios_resource_survey() by way of pci_subsys_init()
>> > >  subsys_initcall. However, the PCI device scanning is kicked off prior
>> > > to this through acpi_scan_init() by way of acpi_init()
>> > > subsys_initcall.  The conflict error occurs because there's already
>> > > the child ACPI device in the resource tree. I'm not sure when/where
>> > > those ACPI devices' resources are added, but clearly they are sitting
>> > > in there since the conflict was found.
>>
>> I think the acpi_init()/pci_subsys_init() ordering is correct.  The
>> ACPI namespace is primary.  A PCI hierarchy originates at a PCI host
>> bridge in the ACPI namespace, so we should enumerate the ACPI
>> namespace first, and when we find a PCI host bridge, we should
>> enumerate the PCI devices below it.
>>
>> That said, I think it is correct mostly by accident and it would be
>> nice if it were more explicit.
>
> No, it isn't by accident.
>
> The enumeration of PCI devices under a PCI host bridge discovered via ACPI
> starts in acpi_pci_root_add() which quite explicitly is only called after
> enumerating the ACPI namespace entirely.
>
> acpi_bus_scan() has two passes now, one is to call acpi_bus_check_add() for
> all namespace objects and the other is the acpi_bus_attach() pass where
> all things like acpi_pci_root_add() are called.
>
>> > > Somewhere along the way a PCI device from a scan is linked with the
>> > > ACPI device for that same PCI device in sysfs.  This is with me
>> > > putting a _HID and _CID in the PCI ACPI device.
>> > > # readlink -f /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00/physical_node
>> > > /sys/devices/pci0000:00/0000:00:0d.0
>> > > # readlink -f /sys/devices/pci0000\:00/0000\:00\:0d.0/firmware_node/
>> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00
>> > >
>> > > So the hierarchy is known eventually, but it's clearly not honored
>> > > when adding resources. The current ACPI support doesn't handle
>> > > PciBarTarget which initially sounds (from ACPI spec) like the way to
>> > > go for referencing a resource in a PCI device from an ACPI device.
>>
>> Yes, I think PCIBARTarget looks like the right way to do this.  It
>> doesn't *seem* like it'd be that hard to implement; have you looked
>> into that at all?
>>
>> Without PCIBARTarget, the AML contains fixed register addresses, so it
>> will break if Linux reassigns the BAR.
>
> Right.
>
>> > > So
>> > > that's out of the question currently, but maybe someone has a patch
>> > > for that? I don't think reordering the acpi_init() and
>> > > pci_subsys_init() would do anything different except change which
>> > > device discovers the conflict.
>> > >
>> > > Is there a way to honor the ACPI device hierarchy during resource
>> > > addition for the PCI devices? The conflict is found because of the
>> > > presence of a child device claiming resources through _CRS.
>> > > Alternatively, is there a good way to defer the probing of an ACPI
>> > > device until one knows PCI resources have been added?
>> > >
>> > > Any insights would be very helpful. Thank you.
>> >
>> > I stumbled upon the hierarchy connection. That's all handled with the
>> > platform_notify() end of things when device_add() is done on the pci
>> > device. I was thinking we could take advantage of this when adding
>> > resources, but a struct resource has no struct device. It's just a
>> > name description for the resource at hand. However, platform devices
>> > are added when the ACPI tree is parsed along with adding the resources
>> > associated with them (PciBarTarget would be helpful here) so those
>> > resources are sitting in the resource tree when PCI BARs are added.
>> >
>> > The following suggestion is sort of hacky, but it's the best I could
>> > come up with provided the currently supported infrastructure. In
>> > pci_claim_resource() do request_resource_conflict() as before. If it
>> > fails do the following: 1. check if the device has an ACPI companion.
>> > 2. For any children hanging off the ACPI companion device. check if
>> > that device's name matches the conflict resource's name. 3. If so,
>> > insert_resource_conflict() to place the BAR within the tree itself.
>>
>> I think the best solution would be to implement PCIBARTarget, but if
>> that's impossible, this seems like a plausible workaround.
>>
>> I don't know if the conflict would necessarily have to be with the
>> ACPI companion itself.  It seems like you could have some hierarchy of
>> ACPI devices where the leaf conflicts with a PCI BAR.  Maybe if a
>> resource of *any* ACPI device below a PCI host bridge conflicts with a
>> PCI BAR, we should insert the PCI BAR as a parent?
>>
>> And since moving that BAR would break the AML, we should probably mark
>> the BAR as IORESOURCE_PCI_FIXED.
>
> 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.

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.

>
> 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.

>
> Thanks,
> Rafael
>

Re: ACPI device using sub-resource of PCI device

[Rafael J. Wysocki]

On Wednesday, July 20, 2016 06:02:08 PM Aaron Durbin wrote:
> On Wed, Jul 20, 2016 at 5:46 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Wednesday, July 20, 2016 02:35:43 PM Bjorn Helgaas wrote:
> >> On Tue, Jun 28, 2016 at 09:41:19PM -0700, Aaron Durbin wrote:
> >> > On Fri, Jun 24, 2016 at 12:34 PM, Aaron Durbin <adurbin@google.com> wrote:
> >> > > On Tue, Jun 14, 2016 at 12:04 PM, Aaron Durbin <adurbin@google.com> wrote:
> >> > >> On Wed, May 18, 2016 at 3:32 PM, Aaron Durbin <adurbin@google.com> wrote:
> >> > >>> On Wed, May 18, 2016 at 4:25 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >> > >>>> On Wed, May 18, 2016 at 5:54 PM, Aaron Durbin <adurbin@google.com> wrote:
> >> > >>>>> Hi,
> >> > >>>>>
> >> > >>>>> We're currently running into a problem of resource conflicts with a
> >> > >>>>> PCI device and ACPI devices.
> >> > >>>>>
> >> > >>>>> [    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
> >> > >>>>> 0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
> >> > >>>>> 0xd0c00000-0xd0c03fff]
> >> > >>>>>
> >> > >>>>> The PCI BAR covers a large amount mmio resources, however, there are
> >> > >>>>> ACPI devices with their own HID (for probing) which uses resources
> >> > >>>>> that are a subset of the PCI BAR.
> >> > >>>>>
> >> > >>>>> Short of re-structuring the linux driver is there anything that can be
> >> > >>>>> done with ASL to properly have the ACPI device use a sub-resource of
> >> > >>>>> the PCI device during the ACPI/PCI probing?
> >> > >>>>
> >> > >>>> Do you have an ACPI device object corresponding to the PCI device?
> >> > >>>
> >> > >>> I've been debugging this by proxy, and I did request that test. The
> >> > >>> following is the overall structure:
> >> > >>>
> >> > >>> scope (\_SB.PCI0) {
> >> > >>>
> >> > >>> Device (P2S)
> >> > >>> {
> >> > >>>         Name (_ADR, 0x000D0000)
> >> > >>>         Device (GPO0)
> >> > >>>         {
> >> > >>>                 Name (_ADR, 0)
> >> > >>>                 Name (_HID, "INT3452")
> >> > >>>                 Name (_CID, "INT3452")
> >> > >>>         }
> >> > >>> }
> >> > >>> }
> >> > >>>
> >> > >>> There are _STA methods in both Devices. The GP0 device has a _CRS
> >> > >>> method which just returns a ResourceTemplate which is filled in with
> >> > >>> static values. The PCI bar is at a fixed address from the firmware
> >> > >>> which allows the fixed calculations. However there is no specific
> >> > >>> reference to the P2S device's resources proper -- only the parent
> >> > >>> child relationship within the ASL. I'm not sure how to directly say "I
> >> > >>> want this sub-region of this other device's resource for my resource."
> >> > >>> That seems like the right thing, but it's not clear if that's implied
> >> > >>> by hierarchy of the devices.
> >> > >>>
> >> > >>> Lastly, if it helps, the kernel being used is based on v4.4 (no core
> >> > >>> patches on top).
> >> > >>>
> >> > >>
> >> > >> Hi Rafael,
> >> > >>
> >> > >> I haven't tried a newer kernel yet, but are you of the opinion that
> >> > >> having the Devices as parent-child within the ASL should work? I'm
> >> > >> wondering if there's already a patch in newer kernels that doesn't
> >> > >> report the conflict and works as expected once there are child Devices
> >> > >> under the P2S device.
> >> > >>
> >> > >
> >> > > I've been looking at this more closely. A child ACPI device under a
> >> > > ACPI PCI device doesn't change the resource conflict even when a _CRS
> >> > > method is added to the ACPI PCI device.  Below is my sleuthing which
> >> > > is probably not a surprise to anyone here, but please correct me where
> >> > > I am wrong.
> >> > >
> >> > > acpi_init() and pci_subsys_init() are both subsys_initcalls during
> >> > > boot up. I'm not sure if the ordering is dumb luck or not, but
> >> > > acpi_init() is called prior to pci_subsys_init(). The conflict error
> >> > > is spit out from pcibios_resource_survey() by way of pci_subsys_init()
> >> > >  subsys_initcall. However, the PCI device scanning is kicked off prior
> >> > > to this through acpi_scan_init() by way of acpi_init()
> >> > > subsys_initcall.  The conflict error occurs because there's already
> >> > > the child ACPI device in the resource tree. I'm not sure when/where
> >> > > those ACPI devices' resources are added, but clearly they are sitting
> >> > > in there since the conflict was found.
> >>
> >> I think the acpi_init()/pci_subsys_init() ordering is correct.  The
> >> ACPI namespace is primary.  A PCI hierarchy originates at a PCI host
> >> bridge in the ACPI namespace, so we should enumerate the ACPI
> >> namespace first, and when we find a PCI host bridge, we should
> >> enumerate the PCI devices below it.
> >>
> >> That said, I think it is correct mostly by accident and it would be
> >> nice if it were more explicit.
> >
> > No, it isn't by accident.
> >
> > The enumeration of PCI devices under a PCI host bridge discovered via ACPI
> > starts in acpi_pci_root_add() which quite explicitly is only called after
> > enumerating the ACPI namespace entirely.
> >
> > acpi_bus_scan() has two passes now, one is to call acpi_bus_check_add() for
> > all namespace objects and the other is the acpi_bus_attach() pass where
> > all things like acpi_pci_root_add() are called.
> >
> >> > > Somewhere along the way a PCI device from a scan is linked with the
> >> > > ACPI device for that same PCI device in sysfs.  This is with me
> >> > > putting a _HID and _CID in the PCI ACPI device.
> >> > > # readlink -f /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00/physical_node
> >> > > /sys/devices/pci0000:00/0000:00:0d.0
> >> > > # readlink -f /sys/devices/pci0000\:00/0000\:00\:0d.0/firmware_node/
> >> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00
> >> > >
> >> > > So the hierarchy is known eventually, but it's clearly not honored
> >> > > when adding resources. The current ACPI support doesn't handle
> >> > > PciBarTarget which initially sounds (from ACPI spec) like the way to
> >> > > go for referencing a resource in a PCI device from an ACPI device.
> >>
> >> Yes, I think PCIBARTarget looks like the right way to do this.  It
> >> doesn't *seem* like it'd be that hard to implement; have you looked
> >> into that at all?
> >>
> >> Without PCIBARTarget, the AML contains fixed register addresses, so it
> >> will break if Linux reassigns the BAR.
> >
> > Right.
> >
> >> > > So
> >> > > that's out of the question currently, but maybe someone has a patch
> >> > > for that? I don't think reordering the acpi_init() and
> >> > > pci_subsys_init() would do anything different except change which
> >> > > device discovers the conflict.
> >> > >
> >> > > Is there a way to honor the ACPI device hierarchy during resource
> >> > > addition for the PCI devices? The conflict is found because of the
> >> > > presence of a child device claiming resources through _CRS.
> >> > > Alternatively, is there a good way to defer the probing of an ACPI
> >> > > device until one knows PCI resources have been added?
> >> > >
> >> > > Any insights would be very helpful. Thank you.
> >> >
> >> > I stumbled upon the hierarchy connection. That's all handled with the
> >> > platform_notify() end of things when device_add() is done on the pci
> >> > device. I was thinking we could take advantage of this when adding
> >> > resources, but a struct resource has no struct device. It's just a
> >> > name description for the resource at hand. However, platform devices
> >> > are added when the ACPI tree is parsed along with adding the resources
> >> > associated with them (PciBarTarget would be helpful here) so those
> >> > resources are sitting in the resource tree when PCI BARs are added.
> >> >
> >> > The following suggestion is sort of hacky, but it's the best I could
> >> > come up with provided the currently supported infrastructure. In
> >> > pci_claim_resource() do request_resource_conflict() as before. If it
> >> > fails do the following: 1. check if the device has an ACPI companion.
> >> > 2. For any children hanging off the ACPI companion device. check if
> >> > that device's name matches the conflict resource's name. 3. If so,
> >> > insert_resource_conflict() to place the BAR within the tree itself.
> >>
> >> I think the best solution would be to implement PCIBARTarget, but if
> >> that's impossible, this seems like a plausible workaround.
> >>
> >> I don't know if the conflict would necessarily have to be with the
> >> ACPI companion itself.  It seems like you could have some hierarchy of
> >> ACPI devices where the leaf conflicts with a PCI BAR.  Maybe if a
> >> resource of *any* ACPI device below a PCI host bridge conflicts with a
> >> PCI BAR, we should insert the PCI BAR as a parent?
> >>
> >> And since moving that BAR would break the AML, we should probably mark
> >> the BAR as IORESOURCE_PCI_FIXED.
> >
> > 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.

> 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.

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.

> > 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.

To me, that sounds very suspicious.

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

Thanks,
Rafael

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

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:
>> On Wed, Jul 20, 2016 at 5:46 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Wednesday, July 20, 2016 02:35:43 PM Bjorn Helgaas wrote:
>> >> On Tue, Jun 28, 2016 at 09:41:19PM -0700, Aaron Durbin wrote:
>> >> > On Fri, Jun 24, 2016 at 12:34 PM, Aaron Durbin <adurbin@google.com> wrote:
>> >> > > On Tue, Jun 14, 2016 at 12:04 PM, Aaron Durbin <adurbin@google.com> wrote:
>> >> > >> On Wed, May 18, 2016 at 3:32 PM, Aaron Durbin <adurbin@google.com> wrote:
>> >> > >>> On Wed, May 18, 2016 at 4:25 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> >> > >>>> On Wed, May 18, 2016 at 5:54 PM, Aaron Durbin <adurbin@google.com> wrote:
>> >> > >>>>> Hi,
>> >> > >>>>>
>> >> > >>>>> We're currently running into a problem of resource conflicts with a
>> >> > >>>>> PCI device and ACPI devices.
>> >> > >>>>>
>> >> > >>>>> [    0.243534] pci 0000:00:0d.0: can't claim BAR 0 [mem
>> >> > >>>>> 0xd0000000-0xd0ffffff 64bit]: address conflict with INT3452:03 [mem
>> >> > >>>>> 0xd0c00000-0xd0c03fff]
>> >> > >>>>>
>> >> > >>>>> The PCI BAR covers a large amount mmio resources, however, there are
>> >> > >>>>> ACPI devices with their own HID (for probing) which uses resources
>> >> > >>>>> that are a subset of the PCI BAR.
>> >> > >>>>>
>> >> > >>>>> Short of re-structuring the linux driver is there anything that can be
>> >> > >>>>> done with ASL to properly have the ACPI device use a sub-resource of
>> >> > >>>>> the PCI device during the ACPI/PCI probing?
>> >> > >>>>
>> >> > >>>> Do you have an ACPI device object corresponding to the PCI device?
>> >> > >>>
>> >> > >>> I've been debugging this by proxy, and I did request that test. The
>> >> > >>> following is the overall structure:
>> >> > >>>
>> >> > >>> scope (\_SB.PCI0) {
>> >> > >>>
>> >> > >>> Device (P2S)
>> >> > >>> {
>> >> > >>>         Name (_ADR, 0x000D0000)
>> >> > >>>         Device (GPO0)
>> >> > >>>         {
>> >> > >>>                 Name (_ADR, 0)
>> >> > >>>                 Name (_HID, "INT3452")
>> >> > >>>                 Name (_CID, "INT3452")
>> >> > >>>         }
>> >> > >>> }
>> >> > >>> }
>> >> > >>>
>> >> > >>> There are _STA methods in both Devices. The GP0 device has a _CRS
>> >> > >>> method which just returns a ResourceTemplate which is filled in with
>> >> > >>> static values. The PCI bar is at a fixed address from the firmware
>> >> > >>> which allows the fixed calculations. However there is no specific
>> >> > >>> reference to the P2S device's resources proper -- only the parent
>> >> > >>> child relationship within the ASL. I'm not sure how to directly say "I
>> >> > >>> want this sub-region of this other device's resource for my resource."
>> >> > >>> That seems like the right thing, but it's not clear if that's implied
>> >> > >>> by hierarchy of the devices.
>> >> > >>>
>> >> > >>> Lastly, if it helps, the kernel being used is based on v4.4 (no core
>> >> > >>> patches on top).
>> >> > >>>
>> >> > >>
>> >> > >> Hi Rafael,
>> >> > >>
>> >> > >> I haven't tried a newer kernel yet, but are you of the opinion that
>> >> > >> having the Devices as parent-child within the ASL should work? I'm
>> >> > >> wondering if there's already a patch in newer kernels that doesn't
>> >> > >> report the conflict and works as expected once there are child Devices
>> >> > >> under the P2S device.
>> >> > >>
>> >> > >
>> >> > > I've been looking at this more closely. A child ACPI device under a
>> >> > > ACPI PCI device doesn't change the resource conflict even when a _CRS
>> >> > > method is added to the ACPI PCI device.  Below is my sleuthing which
>> >> > > is probably not a surprise to anyone here, but please correct me where
>> >> > > I am wrong.
>> >> > >
>> >> > > acpi_init() and pci_subsys_init() are both subsys_initcalls during
>> >> > > boot up. I'm not sure if the ordering is dumb luck or not, but
>> >> > > acpi_init() is called prior to pci_subsys_init(). The conflict error
>> >> > > is spit out from pcibios_resource_survey() by way of pci_subsys_init()
>> >> > >  subsys_initcall. However, the PCI device scanning is kicked off prior
>> >> > > to this through acpi_scan_init() by way of acpi_init()
>> >> > > subsys_initcall.  The conflict error occurs because there's already
>> >> > > the child ACPI device in the resource tree. I'm not sure when/where
>> >> > > those ACPI devices' resources are added, but clearly they are sitting
>> >> > > in there since the conflict was found.
>> >>
>> >> I think the acpi_init()/pci_subsys_init() ordering is correct.  The
>> >> ACPI namespace is primary.  A PCI hierarchy originates at a PCI host
>> >> bridge in the ACPI namespace, so we should enumerate the ACPI
>> >> namespace first, and when we find a PCI host bridge, we should
>> >> enumerate the PCI devices below it.
>> >>
>> >> That said, I think it is correct mostly by accident and it would be
>> >> nice if it were more explicit.
>> >
>> > No, it isn't by accident.
>> >
>> > The enumeration of PCI devices under a PCI host bridge discovered via ACPI
>> > starts in acpi_pci_root_add() which quite explicitly is only called after
>> > enumerating the ACPI namespace entirely.
>> >
>> > acpi_bus_scan() has two passes now, one is to call acpi_bus_check_add() for
>> > all namespace objects and the other is the acpi_bus_attach() pass where
>> > all things like acpi_pci_root_add() are called.
>> >
>> >> > > Somewhere along the way a PCI device from a scan is linked with the
>> >> > > ACPI device for that same PCI device in sysfs.  This is with me
>> >> > > putting a _HID and _CID in the PCI ACPI device.
>> >> > > # readlink -f /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00/physical_node
>> >> > > /sys/devices/pci0000:00/0000:00:0d.0
>> >> > > # readlink -f /sys/devices/pci0000\:00/0000\:00\:0d.0/firmware_node/
>> >> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT5A92:00
>> >> > >
>> >> > > So the hierarchy is known eventually, but it's clearly not honored
>> >> > > when adding resources. The current ACPI support doesn't handle
>> >> > > PciBarTarget which initially sounds (from ACPI spec) like the way to
>> >> > > go for referencing a resource in a PCI device from an ACPI device.
>> >>
>> >> Yes, I think PCIBARTarget looks like the right way to do this.  It
>> >> doesn't *seem* like it'd be that hard to implement; have you looked
>> >> into that at all?
>> >>
>> >> Without PCIBARTarget, the AML contains fixed register addresses, so it
>> >> will break if Linux reassigns the BAR.
>> >
>> > Right.
>> >
>> >> > > So
>> >> > > that's out of the question currently, but maybe someone has a patch
>> >> > > for that? I don't think reordering the acpi_init() and
>> >> > > pci_subsys_init() would do anything different except change which
>> >> > > device discovers the conflict.
>> >> > >
>> >> > > Is there a way to honor the ACPI device hierarchy during resource
>> >> > > addition for the PCI devices? The conflict is found because of the
>> >> > > presence of a child device claiming resources through _CRS.
>> >> > > Alternatively, is there a good way to defer the probing of an ACPI
>> >> > > device until one knows PCI resources have been added?
>> >> > >
>> >> > > Any insights would be very helpful. Thank you.
>> >> >
>> >> > I stumbled upon the hierarchy connection. That's all handled with the
>> >> > platform_notify() end of things when device_add() is done on the pci
>> >> > device. I was thinking we could take advantage of this when adding
>> >> > resources, but a struct resource has no struct device. It's just a
>> >> > name description for the resource at hand. However, platform devices
>> >> > are added when the ACPI tree is parsed along with adding the resources
>> >> > associated with them (PciBarTarget would be helpful here) so those
>> >> > resources are sitting in the resource tree when PCI BARs are added.
>> >> >
>> >> > The following suggestion is sort of hacky, but it's the best I could
>> >> > come up with provided the currently supported infrastructure. In
>> >> > pci_claim_resource() do request_resource_conflict() as before. If it
>> >> > fails do the following: 1. check if the device has an ACPI companion.
>> >> > 2. For any children hanging off the ACPI companion device. check if
>> >> > that device's name matches the conflict resource's name. 3. If so,
>> >> > insert_resource_conflict() to place the BAR within the tree itself.
>> >>
>> >> I think the best solution would be to implement PCIBARTarget, but if
>> >> that's impossible, this seems like a plausible workaround.
>> >>
>> >> I don't know if the conflict would necessarily have to be with the
>> >> ACPI companion itself.  It seems like you could have some hierarchy of
>> >> ACPI devices where the leaf conflicts with a PCI BAR.  Maybe if a
>> >> resource of *any* ACPI device below a PCI host bridge conflicts with a
>> >> PCI BAR, we should insert the PCI BAR as a parent?
>> >>
>> >> And since moving that BAR would break the AML, we should probably mark
>> >> the BAR as IORESOURCE_PCI_FIXED.
>> >
>> > 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

>
>> 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.

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


>
>> > 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()


>
> Thanks,
> Rafael
>

Re: ACPI device using sub-resource of PCI device

[Rafael J. Wysocki]

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.

> >
> >> 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.

Thanks,
Rafael

Re: ACPI device using sub-resource of PCI device

[Bjorn Helgaas]

On Thu, Jul 21, 2016 at 12:46:03AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 20, 2016 02:35:43 PM Bjorn Helgaas wrote:
> > On Tue, Jun 28, 2016 at 09:41:19PM -0700, Aaron Durbin wrote:
> > > On Fri, Jun 24, 2016 at 12:34 PM, Aaron Durbin <adurbin@google.com> wrote:

> > > > acpi_init() and pci_subsys_init() are both subsys_initcalls during
> > > > boot up. I'm not sure if the ordering is dumb luck or not, but
> > > > acpi_init() is called prior to pci_subsys_init(). The conflict error
> > > > is spit out from pcibios_resource_survey() by way of pci_subsys_init()
> > > >  subsys_initcall. However, the PCI device scanning is kicked off prior
> > > > to this through acpi_scan_init() by way of acpi_init()
> > > > subsys_initcall.  The conflict error occurs because there's already
> > > > the child ACPI device in the resource tree. I'm not sure when/where
> > > > those ACPI devices' resources are added, but clearly they are sitting
> > > > in there since the conflict was found.
> > 
> > I think the acpi_init()/pci_subsys_init() ordering is correct.  The
> > ACPI namespace is primary.  A PCI hierarchy originates at a PCI host
> > bridge in the ACPI namespace, so we should enumerate the ACPI
> > namespace first, and when we find a PCI host bridge, we should
> > enumerate the PCI devices below it.
> > 
> > That said, I think it is correct mostly by accident and it would be
> > nice if it were more explicit.
> 
> No, it isn't by accident.
> 
> The enumeration of PCI devices under a PCI host bridge discovered via ACPI
> starts in acpi_pci_root_add() which quite explicitly is only called after
> enumerating the ACPI namespace entirely.
> 
> acpi_bus_scan() has two passes now, one is to call acpi_bus_check_add() for
> all namespace objects and the other is the acpi_bus_attach() pass where
> all things like acpi_pci_root_add() are called.

I meant the ordering between acpi_init() and pci_subsys_init().
They're both subsys_initcalls, so their ordering is determined by link
order, which is not at all obvious (at least to me), since acpi_init()
is in drivers/acpi and pci_subsys_init() is in arch/x86.  If they were
both in drivers, the drivers/Makefile would make it pretty obvious.
But finding the order of "arch" relative to "drivers" in Makefile is
less obvious.

acpi_init() has to be before pci_subsys_init(), and it is.  I
shouldn't have said it was by accident, just that it's non-obvious.

Bjorn

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

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.

PCI devices might be enumerated, but that doesn't mean their
*resources* are added.  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()


> Thanks,
> Rafael
>

Re: ACPI device using sub-resource of PCI device

[Rafael J. Wysocki]

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.

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().

Thanks,
Rafael

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

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?

>
> 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?

Re: ACPI device using sub-resource of PCI device

[Rafael J. Wysocki]

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

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

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

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

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

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

Re: ACPI device using sub-resource of PCI device

[Mika Westerberg]

On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote:
> 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.

Below is one proposal for fixing the issue. It is just a prototype and
I'm not sure if it takes everything needed into account. Would you be
able to try it out and let us know if it works for you?

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index 159f7f19abce..72068415f806 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
+#include <linux/pci.h>
 
 #include "internal.h"
 
@@ -30,6 +31,35 @@ static const struct acpi_device_id forbidden_id_list[] = {
 	{"", 0},
 };
 
+static struct resource *acpi_find_parent_resource(struct acpi_device *adev,
+						  struct resource *res)
+{
+	struct device *parent;
+
+	parent = acpi_get_first_physical_node(adev->parent);
+	if (!parent)
+		return NULL;
+
+#if IS_ENABLED(CONFIG_PCI)
+	if (dev_is_pci(parent)) {
+		struct pci_dev *pdev = to_pci_dev(parent);
+
+		if (!pci_is_bridge(pdev)) {
+			int i;
+
+			for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+				struct resource *r = &pdev->resource[i];
+
+				if (r->start && resource_contains(r, res))
+					return r;
+			}
+		}
+	}
+#endif
+
+	return NULL;
+}
+
 /**
  * acpi_create_platform_device - Create platform device for ACPI device node
  * @adev: ACPI device node to create a platform device for.
@@ -69,8 +99,17 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
 			return ERR_PTR(-ENOMEM);
 		}
 		count = 0;
-		list_for_each_entry(rentry, &resource_list, node)
-			resources[count++] = *rentry->res;
+		list_for_each_entry(rentry, &resource_list, node) {
+			struct resource *res = &resources[count++];
+
+			*res = *rentry->res;
+			/*
+			 * If the device has parent we need to take its
+			 * resources into account as well because this
+			 * device might consume part of those.
+			 */
+			res->parent = acpi_find_parent_resource(adev, res);
+		}
 
 		acpi_dev_free_resource_list(&resource_list);
 	}

Re: ACPI device using sub-resource of PCI device

[Andy Shevchenko]

On Tue, 2016-08-16 at 12:15 +0300, Mika Westerberg wrote:
> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote:
> > 
> > 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.
> 
> Below is one proposal for fixing the issue. It is just a prototype and
> I'm not sure if it takes everything needed into account. Would you be
> able to try it out and let us know if it works for you?

Just style comments below.

> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -18,6 +18,7 @@
>  #include <linux/module.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/platform_device.h>
> +#include <linux/pci.h>

I would keep trying to arrange them in alphabetical order, i.e. put
before platform_device.h.

>  
>  #include "internal.h"
>  
> @@ -30,6 +31,35 @@ static const struct acpi_device_id
> forbidden_id_list[] = {
>  	{"", 0},
>  };
>  
> +static struct resource *acpi_find_parent_resource(struct acpi_device
> *adev,
> +						  struct resource
> *res)
> +{
> +	struct device *parent;
> +
> +	parent = acpi_get_first_physical_node(adev->parent);
> +	if (!parent)
> +		return NULL;
> +
> +#if IS_ENABLED(CONFIG_PCI)

Oh.

> +	if (dev_is_pci(parent)) {

Has a stub.

> +		struct pci_dev *pdev = to_pci_dev(parent);

Not needed an ugly define.

> +
> +		if (!pci_is_bridge(pdev)) {

Ditto.

> +			int i;
> +
> +			for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {

Ditto.

> +				struct resource *r = &pdev-
> >resource[i];
> +
> +				if (r->start && resource_contains(r,
> res))
> +					return r;
> +			}
> +		}
> +	}
> +#endif
> +
> +	return NULL;
> +}
> +


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: ACPI device using sub-resource of PCI device

[Andy Shevchenko]

On Tue, 2016-08-16 at 12:15 +0300, Mika Westerberg wrote:
> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote:
> > 
> > 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.
> 
> Below is one proposal for fixing the issue. It is just a prototype and
> I'm not sure if it takes everything needed into account. Would you be
> able to try it out and let us know if it works for you?

Just style comments below.

> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -18,6 +18,7 @@
>  #include <linux/module.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/platform_device.h>
> +#include <linux/pci.h>

I would keep trying to arrange them in alphabetical order, i.e. put
before platform_device.h.

>  
>  #include "internal.h"
>  
> @@ -30,6 +31,35 @@ static const struct acpi_device_id
> forbidden_id_list[] = {
>  	{"", 0},
>  };
>  
> +static struct resource *acpi_find_parent_resource(struct acpi_device
> *adev,
> +						  struct resource
> *res)
> +{
> +	struct device *parent;
> +
> +	parent = acpi_get_first_physical_node(adev->parent);
> +	if (!parent)
> +		return NULL;
> +
> +#if IS_ENABLED(CONFIG_PCI)

Oh.

> +	if (dev_is_pci(parent)) {

Has a stub.

> +		struct pci_dev *pdev = to_pci_dev(parent);

Not needed an ugly define.

> +
> +		if (!pci_is_bridge(pdev)) {

Ditto.

> +			int i;
> +
> +			for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {

Ditto.

> +				struct resource *r = &pdev-
> >resource[i];
> +
> +				if (r->start && resource_contains(r,
> res))
> +					return r;
> +			}
> +		}
> +	}
> +#endif
> +
> +	return NULL;
> +}
> +


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: ACPI device using sub-resource of PCI device

[Rafael J. Wysocki]

On Tue, Aug 16, 2016 at 1:23 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2016-08-16 at 12:15 +0300, Mika Westerberg wrote:
>> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote:
>> >
>> > 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.
>>
>> Below is one proposal for fixing the issue. It is just a prototype and
>> I'm not sure if it takes everything needed into account. Would you be
>> able to try it out and let us know if it works for you?
>
> Just style comments below.

Didn't Mika say it was a prototype?  Meaning that it would be cleaned
up (among other things)?

Why do you have to confuse things by posting style comments on patches
clearly described as prototypes?  Is there not enough noise on the
mailing lists already?

Thanks,
Rafael

Re: ACPI device using sub-resource of PCI device

[Mika Westerberg]

On Tue, Aug 16, 2016 at 02:23:24PM +0300, Andy Shevchenko wrote:
> On Tue, 2016-08-16 at 12:15 +0300, Mika Westerberg wrote:
> > On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote:
> > > 
> > > 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.
> > 
> > Below is one proposal for fixing the issue. It is just a prototype and
> > I'm not sure if it takes everything needed into account. Would you be
> > able to try it out and let us know if it works for you?
> 
> Just style comments below.

Thanks for the comments. As Rafael already mentioned, this is just a
prototype. We don't even know yet if it fixes the issue.

If it turns out to be working and we decide to go with this approach, I
will clean up the code accordingly and submit it as a proper patch.

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

On Tue, Aug 16, 2016 at 2:15 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote:
>> 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.
>
> Below is one proposal for fixing the issue. It is just a prototype and
> I'm not sure if it takes everything needed into account. Would you be
> able to try it out and let us know if it works for you?

Yes. I'll give it a go. I'm traveling this week so it won't likely be
til early next week. Thanks for the proposed fix. I'll report back on
my findings.

>
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index 159f7f19abce..72068415f806 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -18,6 +18,7 @@
>  #include <linux/module.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/platform_device.h>
> +#include <linux/pci.h>
>
>  #include "internal.h"
>
> @@ -30,6 +31,35 @@ static const struct acpi_device_id forbidden_id_list[] = {
>         {"", 0},
>  };
>
> +static struct resource *acpi_find_parent_resource(struct acpi_device *adev,
> +                                                 struct resource *res)
> +{
> +       struct device *parent;
> +
> +       parent = acpi_get_first_physical_node(adev->parent);
> +       if (!parent)
> +               return NULL;
> +
> +#if IS_ENABLED(CONFIG_PCI)
> +       if (dev_is_pci(parent)) {
> +               struct pci_dev *pdev = to_pci_dev(parent);
> +
> +               if (!pci_is_bridge(pdev)) {
> +                       int i;
> +
> +                       for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> +                               struct resource *r = &pdev->resource[i];
> +
> +                               if (r->start && resource_contains(r, res))
> +                                       return r;
> +                       }
> +               }
> +       }
> +#endif
> +
> +       return NULL;
> +}
> +
>  /**
>   * acpi_create_platform_device - Create platform device for ACPI device node
>   * @adev: ACPI device node to create a platform device for.
> @@ -69,8 +99,17 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
>                         return ERR_PTR(-ENOMEM);
>                 }
>                 count = 0;
> -               list_for_each_entry(rentry, &resource_list, node)
> -                       resources[count++] = *rentry->res;
> +               list_for_each_entry(rentry, &resource_list, node) {
> +                       struct resource *res = &resources[count++];
> +
> +                       *res = *rentry->res;
> +                       /*
> +                        * If the device has parent we need to take its
> +                        * resources into account as well because this
> +                        * device might consume part of those.
> +                        */
> +                       res->parent = acpi_find_parent_resource(adev, res);
> +               }
>
>                 acpi_dev_free_resource_list(&resource_list);
>         }

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

On Wed, Aug 17, 2016 at 6:02 PM, Aaron Durbin <adurbin@google.com> wrote:
> On Tue, Aug 16, 2016 at 2:15 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
>> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote:
>>> 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.
>>
>> Below is one proposal for fixing the issue. It is just a prototype and
>> I'm not sure if it takes everything needed into account. Would you be
>> able to try it out and let us know if it works for you?
>
> Yes. I'll give it a go. I'm traveling this week so it won't likely be
> til early next week. Thanks for the proposed fix. I'll report back on
> my findings.
>

Sorry for the late response. I tried the patch and it works with ACPI
devices in a hierarchy below PCI devices. I didn't have any ACPI
drivers probed but I was able to hang 2 ACPI devices off of a BAR that
had a driver for the PCI device without any resource conflicts
(snippet from /proc/iomem):

    c2a43000-c2a43fff : 0000:00:1b.0
      c2a43000-c2a437ff : GOOG1234:00
      c2a43800-c2a43fff : GOOG1235:00

One observation that I don't think matters much, but I wanted to write
it down in case anyone pulls this thread up again. In the case of
finding the parent resource, the ACPI devcie's resources won't show up
in the resource tree until the pci parent devices' resources are
inserted.


>>
>> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
>> index 159f7f19abce..72068415f806 100644
>> --- a/drivers/acpi/acpi_platform.c
>> +++ b/drivers/acpi/acpi_platform.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/module.h>
>>  #include <linux/dma-mapping.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pci.h>
>>
>>  #include "internal.h"
>>
>> @@ -30,6 +31,35 @@ static const struct acpi_device_id forbidden_id_list[] = {
>>         {"", 0},
>>  };
>>
>> +static struct resource *acpi_find_parent_resource(struct acpi_device *adev,
>> +                                                 struct resource *res)
>> +{
>> +       struct device *parent;
>> +
>> +       parent = acpi_get_first_physical_node(adev->parent);
>> +       if (!parent)
>> +               return NULL;
>> +
>> +#if IS_ENABLED(CONFIG_PCI)
>> +       if (dev_is_pci(parent)) {
>> +               struct pci_dev *pdev = to_pci_dev(parent);
>> +
>> +               if (!pci_is_bridge(pdev)) {
>> +                       int i;
>> +
>> +                       for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>> +                               struct resource *r = &pdev->resource[i];
>> +
>> +                               if (r->start && resource_contains(r, res))
>> +                                       return r;
>> +                       }
>> +               }
>> +       }
>> +#endif
>> +
>> +       return NULL;
>> +}
>> +
>>  /**
>>   * acpi_create_platform_device - Create platform device for ACPI device node
>>   * @adev: ACPI device node to create a platform device for.
>> @@ -69,8 +99,17 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
>>                         return ERR_PTR(-ENOMEM);
>>                 }
>>                 count = 0;
>> -               list_for_each_entry(rentry, &resource_list, node)
>> -                       resources[count++] = *rentry->res;
>> +               list_for_each_entry(rentry, &resource_list, node) {
>> +                       struct resource *res = &resources[count++];
>> +
>> +                       *res = *rentry->res;
>> +                       /*
>> +                        * If the device has parent we need to take its
>> +                        * resources into account as well because this
>> +                        * device might consume part of those.
>> +                        */
>> +                       res->parent = acpi_find_parent_resource(adev, res);
>> +               }
>>
>>                 acpi_dev_free_resource_list(&resource_list);
>>         }

Re: ACPI device using sub-resource of PCI device

[Aaron Durbin]

On Fri, Sep 9, 2016 at 9:12 AM, Aaron Durbin <adurbin@google.com> wrote:
> On Wed, Aug 17, 2016 at 6:02 PM, Aaron Durbin <adurbin@google.com> wrote:
>> On Tue, Aug 16, 2016 at 2:15 AM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>>> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote:
>>>> 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.
>>>
>>> Below is one proposal for fixing the issue. It is just a prototype and
>>> I'm not sure if it takes everything needed into account. Would you be
>>> able to try it out and let us know if it works for you?
>>
>> Yes. I'll give it a go. I'm traveling this week so it won't likely be
>> til early next week. Thanks for the proposed fix. I'll report back on
>> my findings.
>>
>
> Sorry for the late response. I tried the patch and it works with ACPI
> devices in a hierarchy below PCI devices. I didn't have any ACPI
> drivers probed but I was able to hang 2 ACPI devices off of a BAR that
> had a driver for the PCI device without any resource conflicts
> (snippet from /proc/iomem):
>
>     c2a43000-c2a43fff : 0000:00:1b.0
>       c2a43000-c2a437ff : GOOG1234:00
>       c2a43800-c2a43fff : GOOG1235:00
>
> One observation that I don't think matters much, but I wanted to write
> it down in case anyone pulls this thread up again. In the case of
> finding the parent resource, the ACPI devcie's resources won't show up
> in the resource tree until the pci parent devices' resources are
> inserted.
>

I forgot to say thanks for helping in sorting this issue out. It's
very much appreciated.

-Aaron

>
>>>
>>> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
>>> index 159f7f19abce..72068415f806 100644
>>> --- a/drivers/acpi/acpi_platform.c
>>> +++ b/drivers/acpi/acpi_platform.c
>>> @@ -18,6 +18,7 @@
>>>  #include <linux/module.h>
>>>  #include <linux/dma-mapping.h>
>>>  #include <linux/platform_device.h>
>>> +#include <linux/pci.h>
>>>
>>>  #include "internal.h"
>>>
>>> @@ -30,6 +31,35 @@ static const struct acpi_device_id forbidden_id_list[] = {
>>>         {"", 0},
>>>  };
>>>
>>> +static struct resource *acpi_find_parent_resource(struct acpi_device *adev,
>>> +                                                 struct resource *res)
>>> +{
>>> +       struct device *parent;
>>> +
>>> +       parent = acpi_get_first_physical_node(adev->parent);
>>> +       if (!parent)
>>> +               return NULL;
>>> +
>>> +#if IS_ENABLED(CONFIG_PCI)
>>> +       if (dev_is_pci(parent)) {
>>> +               struct pci_dev *pdev = to_pci_dev(parent);
>>> +
>>> +               if (!pci_is_bridge(pdev)) {
>>> +                       int i;
>>> +
>>> +                       for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>>> +                               struct resource *r = &pdev->resource[i];
>>> +
>>> +                               if (r->start && resource_contains(r, res))
>>> +                                       return r;
>>> +                       }
>>> +               }
>>> +       }
>>> +#endif
>>> +
>>> +       return NULL;
>>> +}
>>> +
>>>  /**
>>>   * acpi_create_platform_device - Create platform device for ACPI device node
>>>   * @adev: ACPI device node to create a platform device for.
>>> @@ -69,8 +99,17 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
>>>                         return ERR_PTR(-ENOMEM);
>>>                 }
>>>                 count = 0;
>>> -               list_for_each_entry(rentry, &resource_list, node)
>>> -                       resources[count++] = *rentry->res;
>>> +               list_for_each_entry(rentry, &resource_list, node) {
>>> +                       struct resource *res = &resources[count++];
>>> +
>>> +                       *res = *rentry->res;
>>> +                       /*
>>> +                        * If the device has parent we need to take its
>>> +                        * resources into account as well because this
>>> +                        * device might consume part of those.
>>> +                        */
>>> +                       res->parent = acpi_find_parent_resource(adev, res);
>>> +               }
>>>
>>>                 acpi_dev_free_resource_list(&resource_list);
>>>         }

Re: ACPI device using sub-resource of PCI device

[Mika Westerberg]

On Fri, Sep 09, 2016 at 09:12:49AM -0500, Aaron Durbin wrote:
> On Wed, Aug 17, 2016 at 6:02 PM, Aaron Durbin <adurbin@google.com> wrote:
> > On Tue, Aug 16, 2016 at 2:15 AM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> >> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote:
> >>> 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.
> >>
> >> Below is one proposal for fixing the issue. It is just a prototype and
> >> I'm not sure if it takes everything needed into account. Would you be
> >> able to try it out and let us know if it works for you?
> >
> > Yes. I'll give it a go. I'm traveling this week so it won't likely be
> > til early next week. Thanks for the proposed fix. I'll report back on
> > my findings.
> >
> 
> Sorry for the late response. I tried the patch and it works with ACPI
> devices in a hierarchy below PCI devices. I didn't have any ACPI
> drivers probed but I was able to hang 2 ACPI devices off of a BAR that
> had a driver for the PCI device without any resource conflicts
> (snippet from /proc/iomem):
> 
>     c2a43000-c2a43fff : 0000:00:1b.0
>       c2a43000-c2a437ff : GOOG1234:00
>       c2a43800-c2a43fff : GOOG1235:00

Thanks for testing.

> One observation that I don't think matters much, but I wanted to write
> it down in case anyone pulls this thread up again. In the case of
> finding the parent resource, the ACPI devcie's resources won't show up
> in the resource tree until the pci parent devices' resources are
> inserted.

Right, that's expected as the parent PCI device resources will be
allocated a bit later when pcibios_resource_survey_bus() is called.

Rafael, Bjorn,

What do you think? Should I clean up the code and send it out as a
formal patch?

Re: ACPI device using sub-resource of PCI device

[Rafael J. Wysocki]

On Mon, Sep 12, 2016 at 10:10 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Sep 09, 2016 at 09:12:49AM -0500, Aaron Durbin wrote:
>> On Wed, Aug 17, 2016 at 6:02 PM, Aaron Durbin <adurbin@google.com> wrote:
>> > On Tue, Aug 16, 2016 at 2:15 AM, Mika Westerberg
>> > <mika.westerberg@linux.intel.com> wrote:
>> >> On Fri, Aug 12, 2016 at 11:45:30AM -0500, Aaron Durbin wrote:
>> >>> 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.
>> >>
>> >> Below is one proposal for fixing the issue. It is just a prototype and
>> >> I'm not sure if it takes everything needed into account. Would you be
>> >> able to try it out and let us know if it works for you?
>> >
>> > Yes. I'll give it a go. I'm traveling this week so it won't likely be
>> > til early next week. Thanks for the proposed fix. I'll report back on
>> > my findings.
>> >
>>
>> Sorry for the late response. I tried the patch and it works with ACPI
>> devices in a hierarchy below PCI devices. I didn't have any ACPI
>> drivers probed but I was able to hang 2 ACPI devices off of a BAR that
>> had a driver for the PCI device without any resource conflicts
>> (snippet from /proc/iomem):
>>
>>     c2a43000-c2a43fff : 0000:00:1b.0
>>       c2a43000-c2a437ff : GOOG1234:00
>>       c2a43800-c2a43fff : GOOG1235:00
>
> Thanks for testing.
>
>> One observation that I don't think matters much, but I wanted to write
>> it down in case anyone pulls this thread up again. In the case of
>> finding the parent resource, the ACPI devcie's resources won't show up
>> in the resource tree until the pci parent devices' resources are
>> inserted.
>
> Right, that's expected as the parent PCI device resources will be
> allocated a bit later when pcibios_resource_survey_bus() is called.
>
> Rafael, Bjorn,
>
> What do you think? Should I clean up the code and send it out as a
> formal patch?

Yes, please, unless Bjorn disagrees. :-)

Thanks,
Rafael

[PATCH 1/2] PCI: Add pci_find_resource()

[Mika Westerberg]

Add a new helper function pci_find_resource() that can be used to find out
whether a given resource (for example from a child device) is contained
within given PCI device's standard resources.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pci.c   | 27 +++++++++++++++++++++++++++
 include/linux/pci.h |  4 ++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aab9d5115a5f..491f879f34cb 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -480,6 +480,33 @@ struct resource *pci_find_parent_resource(const struct pci_dev *dev,
 EXPORT_SYMBOL(pci_find_parent_resource);
 
 /**
+ * pci_find_resource - Return matching PCI device resource
+ * @dev: PCI device to query
+ * @res: Resource to look for
+ *
+ * Goes over standard PCI resources (BARs) and checks if the given resource
+ * is partially or fully contained in any of them. In that case the
+ * matching resource is returned, %NULL otherwise.
+ */
+struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res)
+{
+	int i;
+
+	if (!res)
+		return NULL;
+
+	for (i = 0; i < PCI_ROM_RESOURCE; i++) {
+		struct resource *r = &dev->resource[i];
+
+		if (r->start && resource_contains(r, res))
+			return r;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(pci_find_resource);
+
+/**
  * pci_find_pcie_root_port - return PCIe Root Port
  * @dev: PCI device to query
  *
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0ab835965669..a917d4b20554 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1126,6 +1126,7 @@ void pdev_enable_device(struct pci_dev *);
 int pci_enable_resources(struct pci_dev *, int mask);
 void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
 		    int (*)(const struct pci_dev *, u8, u8));
+struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res);
 #define HAVE_PCI_REQ_REGIONS	2
 int __must_check pci_request_regions(struct pci_dev *, const char *);
 int __must_check pci_request_regions_exclusive(struct pci_dev *, const char *);
@@ -1542,6 +1543,9 @@ static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 				  int enable)
 { return 0; }
 
+static inline struct resource *pci_find_resource(struct pci_dev *dev,
+						 struct resource *res)
+{ return NULL; }
 static inline int pci_request_regions(struct pci_dev *dev, const char *res_name)
 { return -EIO; }
 static inline void pci_release_regions(struct pci_dev *dev) { }
-- 
2.9.3

[PATCH 2/2] ACPI / platform: Pay attention to parent device's resources

[Mika Westerberg]

Given following simplified device hierarchy:

  // PCI device having BAR0 (RMEM) split between 4 GPIO devices.
  Device (P2S)
  {
      Name (_ADR, 0x000d0000)

      Device (GPO0)
      {
          Name (_HID, "INT3452")
          Name (_UID, 1)
          Name (_CRS, ResourceTemplate () {
              Memory32Fixed (ReadWrite, 0, 0x4000, RMEM + 0x0000)
          })
      }

      Device (GPO1)
      {
          Name (_HID, "INT3452")
          Name (_UID, 2)
          Name (_CRS, ResourceTemplate () {
              Memory32Fixed (ReadWrite, 0, 0x4000, RMEM + 0x4000)
          })
      }

      Device (GPO2)
      {
          Name (_HID, "INT3452")
          Name (_UID, 3)
          Name (_CRS, ResourceTemplate () {
              Memory32Fixed (ReadWrite, 0, 0x4000, RMEM + 0x8000)
          })
      }

      Device (GPO3)
      {
          Name (_HID, "INT3452")
          Name (_UID, 4)
          Name (_CRS, ResourceTemplate () {
              Memory32Fixed (ReadWrite, 0, 0x4000, RMEM + 0xc000)
          })
      }
  }

The current ACPI platform enumeration code allocates resources from the
global MMIO resource pool (/proc/iomem) for all the four GPIO devices.
After this PCI core calls pcibios_resource_survey() to allocate resources
for all PCI devices including the parent device for these GPIO devices
(P2S). Since that resource range has already been reserved the allocation
fails.

The reason for this is that we never bother with parent device's resources
when ACPI platform devices are created.

Fix this by checking whether there is a parent device and in that case make
sure we assign correct parent resource to the resources for the child ACPI
platform device. Currently we only deal with parent devices if they are PCI
devices but we may expand this later to cover other bus types as well.

Reported-by: Aaron Durbin <adurbin@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/acpi_platform.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index 159f7f19abce..b200ae1f3c6f 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/dma-mapping.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 
 #include "internal.h"
@@ -30,6 +31,22 @@ static const struct acpi_device_id forbidden_id_list[] = {
 	{"", 0},
 };
 
+static void acpi_platform_fill_resource(struct acpi_device *adev,
+	const struct resource *src, struct resource *dest)
+{
+	struct device *parent;
+
+	*dest = *src;
+
+	/*
+	 * If the device has parent we need to take its resources into
+	 * account as well because this device might consume part of those.
+	 */
+	parent = acpi_get_first_physical_node(adev->parent);
+	if (parent && dev_is_pci(parent))
+		dest->parent = pci_find_resource(to_pci_dev(parent), dest);
+}
+
 /**
  * acpi_create_platform_device - Create platform device for ACPI device node
  * @adev: ACPI device node to create a platform device for.
@@ -70,7 +87,8 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
 		}
 		count = 0;
 		list_for_each_entry(rentry, &resource_list, node)
-			resources[count++] = *rentry->res;
+			acpi_platform_fill_resource(adev, rentry->res,
+						    &resources[count++]);
 
 		acpi_dev_free_resource_list(&resource_list);
 	}
-- 
2.9.3

Re: [PATCH 1/2] PCI: Add pci_find_resource()

[Andy Shevchenko]

On Tue, 2016-09-13 at 15:19 +0300, Mika Westerberg wrote:
> Add a new helper function pci_find_resource() that can be used to find
> out
> whether a given resource (for example from a child device) is
> contained
> within given PCI device's standard resources.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pci/pci.c   | 27 +++++++++++++++++++++++++++
>  include/linux/pci.h |  4 ++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aab9d5115a5f..491f879f34cb 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -480,6 +480,33 @@ struct resource *pci_find_parent_resource(const
> struct pci_dev *dev,
>  EXPORT_SYMBOL(pci_find_parent_resource);
>  
>  /**
> + * pci_find_resource - Return matching PCI device resource
> + * @dev: PCI device to query
> + * @res: Resource to look for
> + *
> + * Goes over standard PCI resources (BARs) and checks if the given
> resource
> + * is partially or fully contained in any of them. In that case the
> + * matching resource is returned, %NULL otherwise.
> + */
> +struct resource *pci_find_resource(struct pci_dev *dev, struct
> resource *res)
> +{
> +	int i;
> +

> +	if (!res)
> +		return NULL;

Shouldn't it be a problem of caller to supply valid pointer?
Seems other function(s) has(ve) this assumption.

> +
> +	for (i = 0; i < PCI_ROM_RESOURCE; i++) {
> +		struct resource *r = &dev->resource[i];
> +
> +		if (r->start && resource_contains(r, res))
> +			return r;

I'm not sure what we have to return in case of
1) r->start == 0, r->end > 0
2) r->start > 0, r->end == 0

assuming that all previous checks are positive.

> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(pci_find_resource);
> +
> +/**
>   * pci_find_pcie_root_port - return PCIe Root Port
>   * @dev: PCI device to query
>   *
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 0ab835965669..a917d4b20554 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1126,6 +1126,7 @@ void pdev_enable_device(struct pci_dev *);
>  int pci_enable_resources(struct pci_dev *, int mask);
>  void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
>  		    int (*)(const struct pci_dev *, u8, u8));
> +struct resource *pci_find_resource(struct pci_dev *dev, struct
> resource *res);
>  #define HAVE_PCI_REQ_REGIONS	2
>  int __must_check pci_request_regions(struct pci_dev *, const char *);
>  int __must_check pci_request_regions_exclusive(struct pci_dev *,
> const char *);
> @@ -1542,6 +1543,9 @@ static inline int pci_enable_wake(struct pci_dev
> *dev, pci_power_t state,
>  				  int enable)
>  { return 0; }
>  
> +static inline struct resource *pci_find_resource(struct pci_dev *dev,
> +						 struct resource
> *res)
> +{ return NULL; }
>  static inline int pci_request_regions(struct pci_dev *dev, const char
> *res_name)
>  { return -EIO; }
>  static inline void pci_release_regions(struct pci_dev *dev) { }

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/2] PCI: Add pci_find_resource()

[Mika Westerberg]

On Tue, Sep 13, 2016 at 05:11:49PM +0300, Andy Shevchenko wrote:
> On Tue, 2016-09-13 at 15:19 +0300, Mika Westerberg wrote:
> > Add a new helper function pci_find_resource() that can be used to find
> > out
> > whether a given resource (for example from a child device) is
> > contained
> > within given PCI device's standard resources.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/pci/pci.c   | 27 +++++++++++++++++++++++++++
> >  include/linux/pci.h |  4 ++++
> >  2 files changed, 31 insertions(+)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index aab9d5115a5f..491f879f34cb 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -480,6 +480,33 @@ struct resource *pci_find_parent_resource(const
> > struct pci_dev *dev,
> >  EXPORT_SYMBOL(pci_find_parent_resource);
> >  
> >  /**
> > + * pci_find_resource - Return matching PCI device resource
> > + * @dev: PCI device to query
> > + * @res: Resource to look for
> > + *
> > + * Goes over standard PCI resources (BARs) and checks if the given
> > resource
> > + * is partially or fully contained in any of them. In that case the
> > + * matching resource is returned, %NULL otherwise.
> > + */
> > +struct resource *pci_find_resource(struct pci_dev *dev, struct
> > resource *res)
> > +{
> > +	int i;
> > +
> 
> > +	if (!res)
> > +		return NULL;
> 
> Shouldn't it be a problem of caller to supply valid pointer?
> Seems other function(s) has(ve) this assumption.

No, I can drop the check.

> > +
> > +	for (i = 0; i < PCI_ROM_RESOURCE; i++) {
> > +		struct resource *r = &dev->resource[i];
> > +
> > +		if (r->start && resource_contains(r, res))
> > +			return r;
> 
> I'm not sure what we have to return in case of
> 1) r->start == 0, r->end > 0
> 2) r->start > 0, r->end == 0
> 
> assuming that all previous checks are positive.

These are PCI BARs so if they are not populated (r->start == 0) we skip
them. PCI core should have filled those already with correct values (and
resource_contains() should deal with everything that does not match
anyway).

Re: [PATCH 1/2] PCI: Add pci_find_resource()

[Bjorn Helgaas]

On Tue, Sep 13, 2016 at 03:19:41PM +0300, Mika Westerberg wrote:
> Add a new helper function pci_find_resource() that can be used to find out
> whether a given resource (for example from a child device) is contained
> within given PCI device's standard resources.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I think the ACPI part of this series is more interesting than the PCI
part, so I propose that Rafael merge the series if he likes it.

> ---
>  drivers/pci/pci.c   | 27 +++++++++++++++++++++++++++
>  include/linux/pci.h |  4 ++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aab9d5115a5f..491f879f34cb 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -480,6 +480,33 @@ struct resource *pci_find_parent_resource(const struct pci_dev *dev,
>  EXPORT_SYMBOL(pci_find_parent_resource);
>  
>  /**
> + * pci_find_resource - Return matching PCI device resource
> + * @dev: PCI device to query
> + * @res: Resource to look for
> + *
> + * Goes over standard PCI resources (BARs) and checks if the given resource
> + * is partially or fully contained in any of them. In that case the
> + * matching resource is returned, %NULL otherwise.
> + */
> +struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res)
> +{
> +	int i;
> +
> +	if (!res)
> +		return NULL;

I agree that this test for !res is unnecessary.

> +
> +	for (i = 0; i < PCI_ROM_RESOURCE; i++) {
> +		struct resource *r = &dev->resource[i];
> +
> +		if (r->start && resource_contains(r, res))
> +			return r;
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(pci_find_resource);
> +
> +/**
>   * pci_find_pcie_root_port - return PCIe Root Port
>   * @dev: PCI device to query
>   *
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 0ab835965669..a917d4b20554 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1126,6 +1126,7 @@ void pdev_enable_device(struct pci_dev *);
>  int pci_enable_resources(struct pci_dev *, int mask);
>  void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
>  		    int (*)(const struct pci_dev *, u8, u8));
> +struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res);
>  #define HAVE_PCI_REQ_REGIONS	2
>  int __must_check pci_request_regions(struct pci_dev *, const char *);
>  int __must_check pci_request_regions_exclusive(struct pci_dev *, const char *);
> @@ -1542,6 +1543,9 @@ static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
>  				  int enable)
>  { return 0; }
>  
> +static inline struct resource *pci_find_resource(struct pci_dev *dev,
> +						 struct resource *res)
> +{ return NULL; }
>  static inline int pci_request_regions(struct pci_dev *dev, const char *res_name)
>  { return -EIO; }
>  static inline void pci_release_regions(struct pci_dev *dev) { }
> -- 
> 2.9.3
> 

Re: [PATCH 1/2] PCI: Add pci_find_resource()

[Rafael J. Wysocki]

On Thu, Sep 15, 2016 at 12:16 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Sep 13, 2016 at 03:19:41PM +0300, Mika Westerberg wrote:
>> Add a new helper function pci_find_resource() that can be used to find out
>> whether a given resource (for example from a child device) is contained
>> within given PCI device's standard resources.
>>
>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> I think the ACPI part of this series is more interesting than the PCI
> part, so I propose that Rafael merge the series if he likes it.

I do, thanks!

>
>> ---
>>  drivers/pci/pci.c   | 27 +++++++++++++++++++++++++++
>>  include/linux/pci.h |  4 ++++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index aab9d5115a5f..491f879f34cb 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -480,6 +480,33 @@ struct resource *pci_find_parent_resource(const struct pci_dev *dev,
>>  EXPORT_SYMBOL(pci_find_parent_resource);
>>
>>  /**
>> + * pci_find_resource - Return matching PCI device resource
>> + * @dev: PCI device to query
>> + * @res: Resource to look for
>> + *
>> + * Goes over standard PCI resources (BARs) and checks if the given resource
>> + * is partially or fully contained in any of them. In that case the
>> + * matching resource is returned, %NULL otherwise.
>> + */
>> +struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res)
>> +{
>> +     int i;
>> +
>> +     if (!res)
>> +             return NULL;
>
> I agree that this test for !res is unnecessary.

Right.

I'm assuming the patch will be updated to drop this check.

Thanks,
Rafael

[PATCH v2 1/2] PCI: Add pci_find_resource()

[Mika Westerberg]

Add a new helper function pci_find_resource() that can be used to find out
whether a given resource (for example from a child device) is contained
within given PCI device's standard resources.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
---
Changes from v1:
  - Removed check for NULL res
  - Added Bjorn's ACK

 drivers/pci/pci.c   | 24 ++++++++++++++++++++++++
 include/linux/pci.h |  4 ++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aab9d5115a5f..415956c5c593 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -480,6 +480,30 @@ struct resource *pci_find_parent_resource(const struct pci_dev *dev,
 EXPORT_SYMBOL(pci_find_parent_resource);
 
 /**
+ * pci_find_resource - Return matching PCI device resource
+ * @dev: PCI device to query
+ * @res: Resource to look for
+ *
+ * Goes over standard PCI resources (BARs) and checks if the given resource
+ * is partially or fully contained in any of them. In that case the
+ * matching resource is returned, %NULL otherwise.
+ */
+struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res)
+{
+	int i;
+
+	for (i = 0; i < PCI_ROM_RESOURCE; i++) {
+		struct resource *r = &dev->resource[i];
+
+		if (r->start && resource_contains(r, res))
+			return r;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(pci_find_resource);
+
+/**
  * pci_find_pcie_root_port - return PCIe Root Port
  * @dev: PCI device to query
  *
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0ab835965669..a917d4b20554 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1126,6 +1126,7 @@ void pdev_enable_device(struct pci_dev *);
 int pci_enable_resources(struct pci_dev *, int mask);
 void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
 		    int (*)(const struct pci_dev *, u8, u8));
+struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res);
 #define HAVE_PCI_REQ_REGIONS	2
 int __must_check pci_request_regions(struct pci_dev *, const char *);
 int __must_check pci_request_regions_exclusive(struct pci_dev *, const char *);
@@ -1542,6 +1543,9 @@ static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
 				  int enable)
 { return 0; }
 
+static inline struct resource *pci_find_resource(struct pci_dev *dev,
+						 struct resource *res)
+{ return NULL; }
 static inline int pci_request_regions(struct pci_dev *dev, const char *res_name)
 { return -EIO; }
 static inline void pci_release_regions(struct pci_dev *dev) { }
-- 
2.9.3

[PATCH v2 2/2] ACPI / platform: Pay attention to parent device's resources

[Mika Westerberg]

Given following simplified device hierarchy:

  // PCI device having BAR0 (RMEM) split between 4 GPIO devices.
  Device (P2S)
  {
      Name (_ADR, 0x000d0000)

      Device (GPO0)
      {
          Name (_HID, "INT3452")
          Name (_UID, 1)
          Name (_CRS, ResourceTemplate () {
              Memory32Fixed (ReadWrite, 0, 0x4000, RMEM + 0x0000)
          })
      }

      Device (GPO1)
      {
          Name (_HID, "INT3452")
          Name (_UID, 2)
          Name (_CRS, ResourceTemplate () {
              Memory32Fixed (ReadWrite, 0, 0x4000, RMEM + 0x4000)
          })
      }

      Device (GPO2)
      {
          Name (_HID, "INT3452")
          Name (_UID, 3)
          Name (_CRS, ResourceTemplate () {
              Memory32Fixed (ReadWrite, 0, 0x4000, RMEM + 0x8000)
          })
      }

      Device (GPO3)
      {
          Name (_HID, "INT3452")
          Name (_UID, 4)
          Name (_CRS, ResourceTemplate () {
              Memory32Fixed (ReadWrite, 0, 0x4000, RMEM + 0xc000)
          })
      }
  }

The current ACPI platform enumeration code allocates resources from the
global MMIO resource pool (/proc/iomem) for all the four GPIO devices.
After this PCI core calls pcibios_resource_survey() to allocate resources
for all PCI devices including the parent device for these GPIO devices
(P2S). Since that resource range has already been reserved the allocation
fails.

The reason for this is that we never bother with parent device's resources
when ACPI platform devices are created.

Fix this by checking whether there is a parent device and in that case make
sure we assign correct parent resource to the resources for the child ACPI
platform device. Currently we only deal with parent devices if they are PCI
devices but we may expand this later to cover other bus types as well.

Reported-by: Aaron Durbin <adurbin@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
No changes from v1.

 drivers/acpi/acpi_platform.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index 159f7f19abce..b200ae1f3c6f 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/dma-mapping.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 
 #include "internal.h"
@@ -30,6 +31,22 @@ static const struct acpi_device_id forbidden_id_list[] = {
 	{"", 0},
 };
 
+static void acpi_platform_fill_resource(struct acpi_device *adev,
+	const struct resource *src, struct resource *dest)
+{
+	struct device *parent;
+
+	*dest = *src;
+
+	/*
+	 * If the device has parent we need to take its resources into
+	 * account as well because this device might consume part of those.
+	 */
+	parent = acpi_get_first_physical_node(adev->parent);
+	if (parent && dev_is_pci(parent))
+		dest->parent = pci_find_resource(to_pci_dev(parent), dest);
+}
+
 /**
  * acpi_create_platform_device - Create platform device for ACPI device node
  * @adev: ACPI device node to create a platform device for.
@@ -70,7 +87,8 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
 		}
 		count = 0;
 		list_for_each_entry(rentry, &resource_list, node)
-			resources[count++] = *rentry->res;
+			acpi_platform_fill_resource(adev, rentry->res,
+						    &resources[count++]);
 
 		acpi_dev_free_resource_list(&resource_list);
 	}
-- 
2.9.3