Patchwork PCI / ACPI: Rework ACPI device nodes lookup for the PCI bus type

login
register
mail settings
Submitter Rafael Wysocki
Date Dec. 28, 2012, 9:29 p.m.
Message ID <16168955.038G1kVfN2@vostro.rjw.lan>
Download mbox | patch
Permalink /patch/1917651/
State New, archived
Headers show

Comments

Rafael Wysocki - Dec. 28, 2012, 9:29 p.m.
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

As the kernel Bugzilla report #42696 indicates, it generally is not
sufficient to use _ADR to get an ACPI device node corresponding to
the given PCI device, because there may be multiple objects with
matching _ADR in the ACPI namespace (this probably is against the
spec, but it evidently happens in practice).

One possible way to improve the situation is to use the presence of
another ACPI method to distinguish between the matching namespace
nodes.  For example, if the presence of _INI is checked in addition
to the return value of _ADR, bug #42696 goes away on the affected
machines.  Of course, this is somewhat arbitrary, but it may be
argued that executing _INI for an ACPI device node kind of means that
we are going to use that device node going forward, so we should
generally prefer the nodes where we have executed _INI to "competing"
nodes without _INI.

In that case, though, we shouldn't take the nodes where we haven't
executed _INI into account, but that's quite straightforwad to
achieve.  Namely, we only need to check nodes that we created struct
acpi_device objects for.  This also makes sense for a different
reason, which is that the result of acpi_pci_find_device() is used
to get a struct acpi_device object (not just an ACPI handle)
corresponding to the given PCI device.

Accordingly, introduce acpi_get_child_device() that finds a struct
acpi_device corresponding to the given address by walking the
children of the ACPI device node whose handle is its first argument.
The lookup is carried out by evaluating _ADR for every child and
comparing the result with the given address.  If there's a match and
that child has _INI defined, it is returned as a result.  If _INI is
not present, the search continues until (a) there are no more matches
or (b) there is another matching child whose _INI is present (in
which case that child is returned instead of the first matching one).

The walk of the list of children is done in the reverse direction for
two reasons.  The first reason is for compatibility with
acpi_get_child() that returns the handle of the last matching child
of the given parent.  The second one is to get the last device whose
_INI was executed first (that _INI might have overriden whatever _INI
for the other matching device nodes had done).

To fix bug #42696, modify acpi_pci_find_device() to use
acpi_get_child_device() instead of acpi_get_child() for ACPI device
node lookup.

References: https://bugzilla.kernel.org/show_bug.cgi?id=42696
Reported-by: Peter Wu <lekensteyn@gmail.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/glue.c     |   45 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h |    4 ++++
 drivers/acpi/proc.c     |    1 +
 drivers/acpi/sleep.h    |    1 -
 drivers/pci/pci-acpi.c  |    7 +++++--
 include/acpi/acpi_bus.h |    1 +
 6 files changed, 56 insertions(+), 3 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Bjorn Helgaas - Jan. 3, 2013, 3:16 p.m.
On Fri, Dec 28, 2012 at 2:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> As the kernel Bugzilla report #42696 indicates, it generally is not
> sufficient to use _ADR to get an ACPI device node corresponding to
> the given PCI device, because there may be multiple objects with
> matching _ADR in the ACPI namespace (this probably is against the
> spec, but it evidently happens in practice).

I don't see anything in sec 6.1.1 (_ADR) that precludes having
multiple objects that contain the same _ADR.  Do you have any other
pointers?

> One possible way to improve the situation is to use the presence of
> another ACPI method to distinguish between the matching namespace
> nodes.  For example, if the presence of _INI is checked in addition
> to the return value of _ADR, bug #42696 goes away on the affected
> machines.  Of course, this is somewhat arbitrary, but it may be
> argued that executing _INI for an ACPI device node kind of means that
> we are going to use that device node going forward, so we should
> generally prefer the nodes where we have executed _INI to "competing"
> nodes without _INI.

I consider this a purely ACPI issue, and hence something that you own
completely.

That said, my opinion is that this heuristic doesn't sound reliable to
me.  It feels like an ad hoc solution that works for the case at hand,
but I don't have any reason to think BIOS writers will unconsciously
make the same assumptions or that other OSes will contain the same
algorithm.

The existence of acpi_get_child_device() means we're assuming there
can only be a single child with matching _ADR.  Since that assumption
turned out to be false, maybe we need a way to deal with several
children.  Maybe we need a list of matching children, or maybe we
search matching children for a method at the time we need it instead
of trying to pick one child up front.

> In that case, though, we shouldn't take the nodes where we haven't
> executed _INI into account, but that's quite straightforwad to
> achieve.  Namely, we only need to check nodes that we created struct
> acpi_device objects for.  This also makes sense for a different
> reason, which is that the result of acpi_pci_find_device() is used
> to get a struct acpi_device object (not just an ACPI handle)
> corresponding to the given PCI device.
>
> Accordingly, introduce acpi_get_child_device() that finds a struct
> acpi_device corresponding to the given address by walking the
> children of the ACPI device node whose handle is its first argument.
> The lookup is carried out by evaluating _ADR for every child and
> comparing the result with the given address.  If there's a match and
> that child has _INI defined, it is returned as a result.  If _INI is
> not present, the search continues until (a) there are no more matches
> or (b) there is another matching child whose _INI is present (in
> which case that child is returned instead of the first matching one).
>
> The walk of the list of children is done in the reverse direction for
> two reasons.  The first reason is for compatibility with
> acpi_get_child() that returns the handle of the last matching child
> of the given parent.  The second one is to get the last device whose
> _INI was executed first (that _INI might have overriden whatever _INI
> for the other matching device nodes had done).
>
> To fix bug #42696, modify acpi_pci_find_device() to use
> acpi_get_child_device() instead of acpi_get_child() for ACPI device
> node lookup.
>
> References: https://bugzilla.kernel.org/show_bug.cgi?id=42696
> Reported-by: Peter Wu <lekensteyn@gmail.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/glue.c     |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/internal.h |    4 ++++
>  drivers/acpi/proc.c     |    1 +
>  drivers/acpi/sleep.h    |    1 -
>  drivers/pci/pci-acpi.c  |    7 +++++--
>  include/acpi/acpi_bus.h |    1 +
>  6 files changed, 56 insertions(+), 3 deletions(-)
>
> Index: linux/drivers/acpi/glue.c
> ===================================================================
> --- linux.orig/drivers/acpi/glue.c
> +++ linux/drivers/acpi/glue.c
> @@ -93,6 +93,51 @@ static int acpi_find_bridge_device(struc
>         return ret;
>  }
>
> +/**
> + * acpi_get_child_device - Find specific child of an ACPI device.
> + * @phandle: ACPI handle of the parent device to find a child of.
> + * @address: Address of the child to find (as returned by _ADR).
> + *
> + * Find the child of the ACPI device node represented by @phandle whose _ADR
> + * method's return value is equal to @address.  If there are more children with
> + * matching _ADR return values, take the (last) one having _INI defined.
> + */
> +struct acpi_device *acpi_get_child_device(acpi_handle phandle, u64 address)
> +{
> +       struct acpi_device *parent, *adev, *ret = NULL;
> +
> +       if (acpi_bus_get_device(phandle, &parent))
> +               return NULL;
> +
> +       mutex_lock(&acpi_device_lock);
> +       /* Use reverse direction for compatibility with acpi_get_child(). */
> +       list_for_each_entry_reverse(adev, &parent->children, node) {
> +               unsigned long long addr;
> +               acpi_status status;
> +               acpi_handle out;
> +
> +               status = acpi_evaluate_integer(adev->handle, METHOD_NAME__ADR,
> +                                              NULL, &addr);
> +               if (ACPI_FAILURE(status) || addr != address)
> +                       continue;
> +
> +               if (ret)
> +                       acpi_handle_warn(adev->handle,
> +                                        "_ADR conflict with device %s\n",
> +                                        dev_name(&ret->dev));
> +
> +               status = acpi_get_handle(adev->handle, "_INI", &out);
> +               if (ACPI_SUCCESS(status)) {
> +                       ret = adev;
> +                       break;
> +               } else if (!ret) {
> +                       ret = adev;
> +               }
> +       }
> +       mutex_unlock(&acpi_device_lock);
> +       return ret;
> +}
> +
>  /* Get device's handler per its address under its parent */
>  struct acpi_find_child {
>         acpi_handle handle;
> Index: linux/drivers/acpi/internal.h
> ===================================================================
> --- linux.orig/drivers/acpi/internal.h
> +++ linux/drivers/acpi/internal.h
> @@ -21,8 +21,12 @@
>  #ifndef _ACPI_INTERNAL_H_
>  #define _ACPI_INTERNAL_H_
>
> +#include <linux/mutex.h>
> +
>  #define PREFIX "ACPI: "
>
> +extern struct mutex acpi_device_lock;
> +
>  int init_acpi_device_notify(void);
>  int acpi_scan_init(void);
>  int acpi_sysfs_init(void);
> Index: linux/drivers/acpi/sleep.h
> ===================================================================
> --- linux.orig/drivers/acpi/sleep.h
> +++ linux/drivers/acpi/sleep.h
> @@ -5,4 +5,3 @@ extern void acpi_enable_wakeup_devices(u
>  extern void acpi_disable_wakeup_devices(u8 sleep_state);
>
>  extern struct list_head acpi_wakeup_device_list;
> -extern struct mutex acpi_device_lock;
> Index: linux/drivers/acpi/proc.c
> ===================================================================
> --- linux.orig/drivers/acpi/proc.c
> +++ linux/drivers/acpi/proc.c
> @@ -12,6 +12,7 @@
>  #include <linux/mc146818rtc.h>
>  #endif
>
> +#include "internal.h"
>  #include "sleep.h"
>
>  #define _COMPONENT             ACPI_SYSTEM_COMPONENT
> Index: linux/include/acpi/acpi_bus.h
> ===================================================================
> --- linux.orig/include/acpi/acpi_bus.h
> +++ linux/include/acpi/acpi_bus.h
> @@ -400,6 +400,7 @@ struct acpi_pci_root {
>  };
>
>  /* helper */
> +struct acpi_device *acpi_get_child_device(acpi_handle phandle, u64 address);
>  acpi_handle acpi_get_child(acpi_handle, u64);
>  int acpi_is_root_bridge(acpi_handle);
>  struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle);
> Index: linux/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux.orig/drivers/pci/pci-acpi.c
> +++ linux/drivers/pci/pci-acpi.c
> @@ -291,14 +291,17 @@ static struct pci_platform_pm_ops acpi_p
>  static int acpi_pci_find_device(struct device *dev, acpi_handle *handle)
>  {
>         struct pci_dev * pci_dev;
> +       struct acpi_device *adev;
>         u64     addr;
>
>         pci_dev = to_pci_dev(dev);
>         /* Please ref to ACPI spec for the syntax of _ADR */
>         addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
> -       *handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr);
> -       if (!*handle)
> +       adev = acpi_get_child_device(ACPI_HANDLE(dev->parent), addr);
> +       if (!adev)
>                 return -ENODEV;
> +
> +       *handle = adev->handle;
>         return 0;
>  }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael Wysocki - Jan. 3, 2013, 8:17 p.m.
On Thursday, January 03, 2013 08:16:26 AM Bjorn Helgaas wrote:
> On Fri, Dec 28, 2012 at 2:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > As the kernel Bugzilla report #42696 indicates, it generally is not
> > sufficient to use _ADR to get an ACPI device node corresponding to
> > the given PCI device, because there may be multiple objects with
> > matching _ADR in the ACPI namespace (this probably is against the
> > spec, but it evidently happens in practice).
> 
> I don't see anything in sec 6.1.1 (_ADR) that precludes having
> multiple objects that contain the same _ADR. Do you have any other
> pointers?

Section 6.1 implicitly means that.  It says that for PCI devices _ADR
must be present to identify which device is represented by the given
ACPI node.  Next, Section 6.1.1 says that the parent bus should be inferred
from the location of the _ADR object's device package in the ACPI namespace,
so clearly, if that's under the PCI root bridge ACPI node, the _ADR
corresponds to a PCI device's bus address.

Then, Table 6-139 specifies the format of _ADR for PCI devices as being
euqivalent to devfn, which means that if two nodes with the same _ADR are
present in one scope (under one parent), then it is impossible to distinguish
between them and that's against Section 6.1.

So I really think it *is* against the spec - not because _ADR is generally
required to be unique, but because _ADR *is* required to be sufficient for
matching ACPI nodes under a PCI root bridge's node with PCI devices.

> > One possible way to improve the situation is to use the presence of
> > another ACPI method to distinguish between the matching namespace
> > nodes.  For example, if the presence of _INI is checked in addition
> > to the return value of _ADR, bug #42696 goes away on the affected
> > machines.  Of course, this is somewhat arbitrary, but it may be
> > argued that executing _INI for an ACPI device node kind of means that
> > we are going to use that device node going forward, so we should
> > generally prefer the nodes where we have executed _INI to "competing"
> > nodes without _INI.
> 
> I consider this a purely ACPI issue, and hence something that you own
> completely.

OK

> That said, my opinion is that this heuristic doesn't sound reliable to
> me.  It feels like an ad hoc solution that works for the case at hand,
> but I don't have any reason to think BIOS writers will unconsciously
> make the same assumptions or that other OSes will contain the same
> algorithm.

It's supposed to be a heuristic that is less likely to break things. :-)

I have a reason to believe that other OSes simply happen to work with
the broken machines in question, because they match _ADR in direct order,
while we match them in reverse order.  The original proposal was to
change our code to match _ADR in direct order, but Len was afraid that it
could break systems that we happen to handle correctly (but then they would
not be handled correctly by other OSes, so perhaps it's what we should do
after all).

> The existence of acpi_get_child_device() means we're assuming there
> can only be a single child with matching _ADR.  Since that assumption
> turned out to be false,

For PCI devices this is required by the spec, AFAICS.

> maybe we need a way to deal with several children.
>
> Maybe we need a list of matching children, or maybe we
> search matching children for a method at the time we need it instead
> of trying to pick one child up front.

It may not be entirely clear from the changelog, but for PCI devices we use
acpi_get_child_device(), or acpi_get_child(), to find the ACPI "companion" node
for a given PCI device, so we have to pick one.  Now, the spec is pretty clear
in that _ADR is the only thing we can use to do that.

If _ADR doesn't work, we have to do strange things to work around breakage,
unless we want to use blacklists.

Thanks,
Rafael
Bjorn Helgaas - Jan. 3, 2013, 9:44 p.m.
On Thu, Jan 3, 2013 at 1:17 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 03, 2013 08:16:26 AM Bjorn Helgaas wrote:
>> On Fri, Dec 28, 2012 at 2:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >
>> > As the kernel Bugzilla report #42696 indicates, it generally is not
>> > sufficient to use _ADR to get an ACPI device node corresponding to
>> > the given PCI device, because there may be multiple objects with
>> > matching _ADR in the ACPI namespace (this probably is against the
>> > spec, but it evidently happens in practice).
>>
>> I don't see anything in sec 6.1.1 (_ADR) that precludes having
>> multiple objects that contain the same _ADR. Do you have any other
>> pointers?
>
> Section 6.1 implicitly means that.  It says that for PCI devices _ADR
> must be present to identify which device is represented by the given
> ACPI node.  Next, Section 6.1.1 says that the parent bus should be inferred
> from the location of the _ADR object's device package in the ACPI namespace,
> so clearly, if that's under the PCI root bridge ACPI node, the _ADR
> corresponds to a PCI device's bus address.

I agree that for namespace Devices below a PCI host bridge, the _ADR
value and its position in the hierarchy is required to be sufficient
to identify a PCI device and function (or the set of all functions on
a device #).

> Then, Table 6-139 specifies the format of _ADR for PCI devices as being
> euqivalent to devfn, which means that if two nodes with the same _ADR are
> present in one scope (under one parent), then it is impossible to distinguish
> between them and that's against Section 6.1.

This is the bit I don't understand.  Where's the requirement that we
be able to distinguish between two namespace nodes with the same _ADR?

Linux assumes we can start from a PCI device and identify a single
related ACPI namespace node, e.g., in acpi_pci_find_device().  But all
I see in the spec is a requirement that we can start from an ACPI
namespace node and find a PCI device.  So I'm not sure
acpi_pci_find_device() is based on a valid assumption.

Let's say we want to provide _SUN and _UID.  _SUN is a slot number
that may apply to several PCI functions, while _UID probably refers to
a single PCI function.  Is it legal to provide two namespace objects,
one with _ADR 0x0003ffff and _SUN, and another with _ADR 0x00030000
and _UID?  If so, which node should acpi_pci_find_device() return?

> So I really think it *is* against the spec - not because _ADR is generally
> required to be unique, but because _ADR *is* required to be sufficient for
> matching ACPI nodes under a PCI root bridge's node with PCI devices.

>> > One possible way to improve the situation is to use the presence of
>> > another ACPI method to distinguish between the matching namespace
>> > nodes.  For example, if the presence of _INI is checked in addition
>> > to the return value of _ADR, bug #42696 goes away on the affected
>> > machines.  Of course, this is somewhat arbitrary, but it may be
>> > argued that executing _INI for an ACPI device node kind of means that
>> > we are going to use that device node going forward, so we should
>> > generally prefer the nodes where we have executed _INI to "competing"
>> > nodes without _INI.
>>
>> I consider this a purely ACPI issue, and hence something that you own
>> completely.
>
> OK
>
>> That said, my opinion is that this heuristic doesn't sound reliable to
>> me.  It feels like an ad hoc solution that works for the case at hand,
>> but I don't have any reason to think BIOS writers will unconsciously
>> make the same assumptions or that other OSes will contain the same
>> algorithm.
>
> It's supposed to be a heuristic that is less likely to break things. :-)
>
> I have a reason to believe that other OSes simply happen to work with
> the broken machines in question, because they match _ADR in direct order,
> while we match them in reverse order.  The original proposal was to
> change our code to match _ADR in direct order, but Len was afraid that it
> could break systems that we happen to handle correctly (but then they would
> not be handled correctly by other OSes, so perhaps it's what we should do
> after all).
>
>> The existence of acpi_get_child_device() means we're assuming there
>> can only be a single child with matching _ADR.  Since that assumption
>> turned out to be false,
>
> For PCI devices this is required by the spec, AFAICS.
>
>> maybe we need a way to deal with several children.
>>
>> Maybe we need a list of matching children, or maybe we
>> search matching children for a method at the time we need it instead
>> of trying to pick one child up front.
>
> It may not be entirely clear from the changelog, but for PCI devices we use
> acpi_get_child_device(), or acpi_get_child(), to find the ACPI "companion" node
> for a given PCI device, so we have to pick one.  Now, the spec is pretty clear
> in that _ADR is the only thing we can use to do that.
>
> If _ADR doesn't work, we have to do strange things to work around breakage,
> unless we want to use blacklists.
>
> Thanks,
> Rafael
>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael Wysocki - Jan. 3, 2013, 10:38 p.m.
On Thursday, January 03, 2013 02:44:32 PM Bjorn Helgaas wrote:
> On Thu, Jan 3, 2013 at 1:17 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 03, 2013 08:16:26 AM Bjorn Helgaas wrote:
> >> On Fri, Dec 28, 2012 at 2:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> >
> >> > As the kernel Bugzilla report #42696 indicates, it generally is not
> >> > sufficient to use _ADR to get an ACPI device node corresponding to
> >> > the given PCI device, because there may be multiple objects with
> >> > matching _ADR in the ACPI namespace (this probably is against the
> >> > spec, but it evidently happens in practice).
> >>
> >> I don't see anything in sec 6.1.1 (_ADR) that precludes having
> >> multiple objects that contain the same _ADR. Do you have any other
> >> pointers?
> >
> > Section 6.1 implicitly means that.  It says that for PCI devices _ADR
> > must be present to identify which device is represented by the given
> > ACPI node.  Next, Section 6.1.1 says that the parent bus should be inferred
> > from the location of the _ADR object's device package in the ACPI namespace,
> > so clearly, if that's under the PCI root bridge ACPI node, the _ADR
> > corresponds to a PCI device's bus address.
> 
> I agree that for namespace Devices below a PCI host bridge, the _ADR
> value and its position in the hierarchy is required to be sufficient
> to identify a PCI device and function (or the set of all functions on
> a device #).
> 
> > Then, Table 6-139 specifies the format of _ADR for PCI devices as being
> > euqivalent to devfn, which means that if two nodes with the same _ADR are
> > present in one scope (under one parent), then it is impossible to distinguish
> > between them and that's against Section 6.1.
> 
> This is the bit I don't understand.  Where's the requirement that we
> be able to distinguish between two namespace nodes with the same _ADR?

According to the spec we can't (if they are under the same parent) and that's
the whole problem.

> Linux assumes we can start from a PCI device and identify a single
> related ACPI namespace node, e.g., in acpi_pci_find_device().  But all
> I see in the spec is a requirement that we can start from an ACPI
> namespace node and find a PCI device.  So I'm not sure
> acpi_pci_find_device() is based on a valid assumption.

I think it is.

Suppose that we have two namespace nodes with the same _ADR under one parent
(PCI bridge ACPI node) and they both contain things like _PS0 and _PS3.  Which
one of these are we supposed to use for the power management of the
corresponding PCI device?  Because they both would point to the same device,
right?

> Let's say we want to provide _SUN and _UID.  _SUN is a slot number
> that may apply to several PCI functions, while _UID probably refers to
> a single PCI function.  Is it legal to provide two namespace objects,
> one with _ADR 0x0003ffff and _SUN, and another with _ADR 0x00030000
> and _UID?

I don't think it is valid to do that.

Thanks,
Rafael
Bjorn Helgaas - Jan. 3, 2013, 11 p.m.
On Thu, Jan 3, 2013 at 3:38 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, January 03, 2013 02:44:32 PM Bjorn Helgaas wrote:
>> On Thu, Jan 3, 2013 at 1:17 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Thursday, January 03, 2013 08:16:26 AM Bjorn Helgaas wrote:
>> >> On Fri, Dec 28, 2012 at 2:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >> >
>> >> > As the kernel Bugzilla report #42696 indicates, it generally is not
>> >> > sufficient to use _ADR to get an ACPI device node corresponding to
>> >> > the given PCI device, because there may be multiple objects with
>> >> > matching _ADR in the ACPI namespace (this probably is against the
>> >> > spec, but it evidently happens in practice).
>> >>
>> >> I don't see anything in sec 6.1.1 (_ADR) that precludes having
>> >> multiple objects that contain the same _ADR. Do you have any other
>> >> pointers?
>> >
>> > Section 6.1 implicitly means that.  It says that for PCI devices _ADR
>> > must be present to identify which device is represented by the given
>> > ACPI node.  Next, Section 6.1.1 says that the parent bus should be inferred
>> > from the location of the _ADR object's device package in the ACPI namespace,
>> > so clearly, if that's under the PCI root bridge ACPI node, the _ADR
>> > corresponds to a PCI device's bus address.
>>
>> I agree that for namespace Devices below a PCI host bridge, the _ADR
>> value and its position in the hierarchy is required to be sufficient
>> to identify a PCI device and function (or the set of all functions on
>> a device #).
>>
>> > Then, Table 6-139 specifies the format of _ADR for PCI devices as being
>> > euqivalent to devfn, which means that if two nodes with the same _ADR are
>> > present in one scope (under one parent), then it is impossible to distinguish
>> > between them and that's against Section 6.1.
>>
>> This is the bit I don't understand.  Where's the requirement that we
>> be able to distinguish between two namespace nodes with the same _ADR?
>
> According to the spec we can't (if they are under the same parent) and that's
> the whole problem.

It's only a problem if you make the assumptions Linux does.  I can
imagine a system with different assumptions.  For example, an OS could
start with PCI device X and ask "please run any _PS0 method that
matches X."  In that case, you don't care how many objects have an
_ADR that matches X; you merely find *any* matching object that
contains _PS0.

>> Linux assumes we can start from a PCI device and identify a single
>> related ACPI namespace node, e.g., in acpi_pci_find_device().  But all
>> I see in the spec is a requirement that we can start from an ACPI
>> namespace node and find a PCI device.  So I'm not sure
>> acpi_pci_find_device() is based on a valid assumption.
>
> I think it is.
>
> Suppose that we have two namespace nodes with the same _ADR under one parent
> (PCI bridge ACPI node) and they both contain things like _PS0 and _PS3.  Which
> one of these are we supposed to use for the power management of the
> corresponding PCI device?  Because they both would point to the same device,
> right?

That's a good question.  It's more complicated if two objects supply
the same method.

>> Let's say we want to provide _SUN and _UID.  _SUN is a slot number
>> that may apply to several PCI functions, while _UID probably refers to
>> a single PCI function.  Is it legal to provide two namespace objects,
>> one with _ADR 0x0003ffff and _SUN, and another with _ADR 0x00030000
>> and _UID?
>
> I don't think it is valid to do that.

Is there something in the spec that says you can't?  I can imagine a
BIOS writer doing that, and I don't know how I could convince him that
it's illegal.

It would be really interesting to try some of these scenarios on
Windows with qemu.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael Wysocki - Jan. 3, 2013, 11:44 p.m.
On Thursday, January 03, 2013 04:00:55 PM Bjorn Helgaas wrote:
> On Thu, Jan 3, 2013 at 3:38 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, January 03, 2013 02:44:32 PM Bjorn Helgaas wrote:
> >> On Thu, Jan 3, 2013 at 1:17 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > On Thursday, January 03, 2013 08:16:26 AM Bjorn Helgaas wrote:
> >> >> On Fri, Dec 28, 2012 at 2:29 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> >> >
> >> >> > As the kernel Bugzilla report #42696 indicates, it generally is not
> >> >> > sufficient to use _ADR to get an ACPI device node corresponding to
> >> >> > the given PCI device, because there may be multiple objects with
> >> >> > matching _ADR in the ACPI namespace (this probably is against the
> >> >> > spec, but it evidently happens in practice).
> >> >>
> >> >> I don't see anything in sec 6.1.1 (_ADR) that precludes having
> >> >> multiple objects that contain the same _ADR. Do you have any other
> >> >> pointers?
> >> >
> >> > Section 6.1 implicitly means that.  It says that for PCI devices _ADR
> >> > must be present to identify which device is represented by the given
> >> > ACPI node.  Next, Section 6.1.1 says that the parent bus should be inferred
> >> > from the location of the _ADR object's device package in the ACPI namespace,
> >> > so clearly, if that's under the PCI root bridge ACPI node, the _ADR
> >> > corresponds to a PCI device's bus address.
> >>
> >> I agree that for namespace Devices below a PCI host bridge, the _ADR
> >> value and its position in the hierarchy is required to be sufficient
> >> to identify a PCI device and function (or the set of all functions on
> >> a device #).
> >>
> >> > Then, Table 6-139 specifies the format of _ADR for PCI devices as being
> >> > euqivalent to devfn, which means that if two nodes with the same _ADR are
> >> > present in one scope (under one parent), then it is impossible to distinguish
> >> > between them and that's against Section 6.1.
> >>
> >> This is the bit I don't understand.  Where's the requirement that we
> >> be able to distinguish between two namespace nodes with the same _ADR?
> >
> > According to the spec we can't (if they are under the same parent) and that's
> > the whole problem.
> 
> It's only a problem if you make the assumptions Linux does.  I can
> imagine a system with different assumptions.  For example, an OS could
> start with PCI device X and ask "please run any _PS0 method that
> matches X."  In that case, you don't care how many objects have an
> _ADR that matches X; you merely find *any* matching object that
> contains _PS0.

Well, except when there are multiple matching objects having _PS0.
Which actually happens in the failing case in bug #42696.

Our assumptions work pretty well on other systems and I don't quite see the
reason to change them entirely.

Moreover, Section 19.5.30 of the spec says that "Device object [...] represents
either a bus or a device or any other similar hardware".  That implies that if
there are two objects with the same _ADR matching the same single devfn of a
PCI device, that will mean that there are _two_ different PCI devices under the
same parent that have the same devfn.  In that case PCI config space accesses
wouldn't work for those devices, though.

> >> Linux assumes we can start from a PCI device and identify a single
> >> related ACPI namespace node, e.g., in acpi_pci_find_device().  But all
> >> I see in the spec is a requirement that we can start from an ACPI
> >> namespace node and find a PCI device.  So I'm not sure
> >> acpi_pci_find_device() is based on a valid assumption.
> >
> > I think it is.
> >
> > Suppose that we have two namespace nodes with the same _ADR under one parent
> > (PCI bridge ACPI node) and they both contain things like _PS0 and _PS3.  Which
> > one of these are we supposed to use for the power management of the
> > corresponding PCI device?  Because they both would point to the same device,
> > right?
> 
> That's a good question.  It's more complicated if two objects supply
> the same method.

Well it is and they do.

> >> Let's say we want to provide _SUN and _UID.  _SUN is a slot number
> >> that may apply to several PCI functions, while _UID probably refers to
> >> a single PCI function.  Is it legal to provide two namespace objects,
> >> one with _ADR 0x0003ffff and _SUN, and another with _ADR 0x00030000
> >> and _UID?
> >
> > I don't think it is valid to do that.
> 
> Is there something in the spec that says you can't?  I can imagine a
> BIOS writer doing that, and I don't know how I could convince him that
> it's illegal.

Well, OK.

> It would be really interesting to try some of these scenarios on
> Windows with qemu.

That's interesting theoretically, but doesn't directly relate to the case at
hand.  The case at hand is that for a given PCI device we want to find the ACPI
namespace node that can be used for things like power management, if one
exists.  While it may be valid to specify _ADR of type 0x0003ffff for some
namespace nodes, I don't really think it is valid to specify two objects with
the same _ADR matching a specific devfn that both provide the same methods
(like _PSx or _CRS).

And the question we need to answer is not "I have a namespace node, so which
device it represents?", but "I have a device, so which namespace node provides
methods I'm supposed to use for it?"

So I think we make the right assumptions, but there are broken BIOSes that
don't follow them and I'm trying to find out how to handle them without
blacklisting etc.

Questioning the validity of everything we're doing doesn't really help, mind
you.

Thanks,
Rafael
Peter Wu - Jan. 23, 2013, 7 p.m.
Hi,

Any progress on this one? I guess it won't make into 3.8, perhaps 3.9?

On Friday 04 January 2013 00:44:16 Rafael J. Wysocki wrote:
> On Thursday, January 03, 2013 04:00:55 PM Bjorn Helgaas wrote:
> > On Thu, Jan 3, 2013 at 3:38 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Thursday, January 03, 2013 02:44:32 PM Bjorn Helgaas wrote:
> > >> On Thu, Jan 3, 2013 at 1:17 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > >> > On Thursday, January 03, 2013 08:16:26 AM Bjorn Helgaas wrote:
> > >> >> On Fri, Dec 28, 2012 at 2:29 PM, Rafael J. Wysocki <rjw@sisk.pl> 
wrote:
> > >> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >> >> > 
> > >> >> > As the kernel Bugzilla report #42696 indicates, it generally is
> > >> >> > not
> > >> >> > sufficient to use _ADR to get an ACPI device node corresponding to
> > >> >> > the given PCI device, because there may be multiple objects with
> > >> >> > matching _ADR in the ACPI namespace (this probably is against the
> > >> >> > spec, but it evidently happens in practice).
> > >> >> 
> > >> >> I don't see anything in sec 6.1.1 (_ADR) that precludes having
> > >> >> multiple objects that contain the same _ADR. Do you have any other
> > >> >> pointers?
> > >> > 
> > >> > Section 6.1 implicitly means that.  It says that for PCI devices _ADR
> > >> > must be present to identify which device is represented by the given
> > >> > ACPI node.  Next, Section 6.1.1 says that the parent bus should be
> > >> > inferred
> > >> > from the location of the _ADR object's device package in the ACPI
> > >> > namespace, so clearly, if that's under the PCI root bridge ACPI
> > >> > node, the _ADR corresponds to a PCI device's bus address.
> > >> 
> > >> I agree that for namespace Devices below a PCI host bridge, the _ADR
> > >> value and its position in the hierarchy is required to be sufficient
> > >> to identify a PCI device and function (or the set of all functions on
> > >> a device #).
> > >> 
> > >> > Then, Table 6-139 specifies the format of _ADR for PCI devices as
> > >> > being
> > >> > euqivalent to devfn, which means that if two nodes with the same _ADR
> > >> > are
> > >> > present in one scope (under one parent), then it is impossible to
> > >> > distinguish between them and that's against Section 6.1.
> > >> 
> > >> This is the bit I don't understand.  Where's the requirement that we
> > >> be able to distinguish between two namespace nodes with the same _ADR?
> > > 
> > > According to the spec we can't (if they are under the same parent) and
> > > that's the whole problem.
> > 
> > It's only a problem if you make the assumptions Linux does.  I can
> > imagine a system with different assumptions.  For example, an OS could
> > start with PCI device X and ask "please run any _PS0 method that
> > matches X."  In that case, you don't care how many objects have an
> > _ADR that matches X; you merely find *any* matching object that
> > contains _PS0.
> 
> Well, except when there are multiple matching objects having _PS0.
> Which actually happens in the failing case in bug #42696.
> 
> Our assumptions work pretty well on other systems and I don't quite see the
> reason to change them entirely.
> 
> Moreover, Section 19.5.30 of the spec says that "Device object [...]
> represents either a bus or a device or any other similar hardware".  That
> implies that if there are two objects with the same _ADR matching the same
> single devfn of a PCI device, that will mean that there are _two_ different
> PCI devices under the same parent that have the same devfn.  In that case
> PCI config space accesses wouldn't work for those devices, though.
> 
> > >> Linux assumes we can start from a PCI device and identify a single
> > >> related ACPI namespace node, e.g., in acpi_pci_find_device().  But all
> > >> I see in the spec is a requirement that we can start from an ACPI
> > >> namespace node and find a PCI device.  So I'm not sure
> > >> acpi_pci_find_device() is based on a valid assumption.
> > > 
> > > I think it is.
> > > 
> > > Suppose that we have two namespace nodes with the same _ADR under one
> > > parent (PCI bridge ACPI node) and they both contain things like _PS0
> > > and _PS3.  Which one of these are we supposed to use for the power
> > > management of the corresponding PCI device?  Because they both would
> > > point to the same device, right?
> > 
> > That's a good question.  It's more complicated if two objects supply
> > the same method.
> 
> Well it is and they do.
> 
> > >> Let's say we want to provide _SUN and _UID.  _SUN is a slot number
> > >> that may apply to several PCI functions, while _UID probably refers to
> > >> a single PCI function.  Is it legal to provide two namespace objects,
> > >> one with _ADR 0x0003ffff and _SUN, and another with _ADR 0x00030000
> > >> and _UID?
> > > 
> > > I don't think it is valid to do that.
> > 
> > Is there something in the spec that says you can't?  I can imagine a
> > BIOS writer doing that, and I don't know how I could convince him that
> > it's illegal.
> 
> Well, OK.
> 
> > It would be really interesting to try some of these scenarios on
> > Windows with qemu.
> 
> That's interesting theoretically, but doesn't directly relate to the case at
> hand.  The case at hand is that for a given PCI device we want to find the
> ACPI namespace node that can be used for things like power management, if
> one exists.  While it may be valid to specify _ADR of type 0x0003ffff for
> some namespace nodes, I don't really think it is valid to specify two
> objects with the same _ADR matching a specific devfn that both provide the
> same methods (like _PSx or _CRS).
> 
> And the question we need to answer is not "I have a namespace node, so which
> device it represents?", but "I have a device, so which namespace node
> provides methods I'm supposed to use for it?"
> 
> So I think we make the right assumptions, but there are broken BIOSes that
> don't follow them and I'm trying to find out how to handle them without
> blacklisting etc.
> 
> Questioning the validity of everything we're doing doesn't really help, mind
> you.
> 
> Thanks,
> Rafael

Regards,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael Wysocki - Jan. 23, 2013, 7:12 p.m.
On Wednesday, January 23, 2013 08:00:31 PM Peter Wu wrote:
> Hi,
> 
> Any progress on this one? I guess it won't make into 3.8, perhaps 3.9?

No, that doesn't go anywhere for now.

In fact, I need to discuss that with Len.

Thanks,
Rafael


> On Friday 04 January 2013 00:44:16 Rafael J. Wysocki wrote:
> > On Thursday, January 03, 2013 04:00:55 PM Bjorn Helgaas wrote:
> > > On Thu, Jan 3, 2013 at 3:38 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > On Thursday, January 03, 2013 02:44:32 PM Bjorn Helgaas wrote:
> > > >> On Thu, Jan 3, 2013 at 1:17 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > >> > On Thursday, January 03, 2013 08:16:26 AM Bjorn Helgaas wrote:
> > > >> >> On Fri, Dec 28, 2012 at 2:29 PM, Rafael J. Wysocki <rjw@sisk.pl> 
> wrote:
> > > >> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >> >> > 
> > > >> >> > As the kernel Bugzilla report #42696 indicates, it generally is
> > > >> >> > not
> > > >> >> > sufficient to use _ADR to get an ACPI device node corresponding to
> > > >> >> > the given PCI device, because there may be multiple objects with
> > > >> >> > matching _ADR in the ACPI namespace (this probably is against the
> > > >> >> > spec, but it evidently happens in practice).
> > > >> >> 
> > > >> >> I don't see anything in sec 6.1.1 (_ADR) that precludes having
> > > >> >> multiple objects that contain the same _ADR. Do you have any other
> > > >> >> pointers?
> > > >> > 
> > > >> > Section 6.1 implicitly means that.  It says that for PCI devices _ADR
> > > >> > must be present to identify which device is represented by the given
> > > >> > ACPI node.  Next, Section 6.1.1 says that the parent bus should be
> > > >> > inferred
> > > >> > from the location of the _ADR object's device package in the ACPI
> > > >> > namespace, so clearly, if that's under the PCI root bridge ACPI
> > > >> > node, the _ADR corresponds to a PCI device's bus address.
> > > >> 
> > > >> I agree that for namespace Devices below a PCI host bridge, the _ADR
> > > >> value and its position in the hierarchy is required to be sufficient
> > > >> to identify a PCI device and function (or the set of all functions on
> > > >> a device #).
> > > >> 
> > > >> > Then, Table 6-139 specifies the format of _ADR for PCI devices as
> > > >> > being
> > > >> > euqivalent to devfn, which means that if two nodes with the same _ADR
> > > >> > are
> > > >> > present in one scope (under one parent), then it is impossible to
> > > >> > distinguish between them and that's against Section 6.1.
> > > >> 
> > > >> This is the bit I don't understand.  Where's the requirement that we
> > > >> be able to distinguish between two namespace nodes with the same _ADR?
> > > > 
> > > > According to the spec we can't (if they are under the same parent) and
> > > > that's the whole problem.
> > > 
> > > It's only a problem if you make the assumptions Linux does.  I can
> > > imagine a system with different assumptions.  For example, an OS could
> > > start with PCI device X and ask "please run any _PS0 method that
> > > matches X."  In that case, you don't care how many objects have an
> > > _ADR that matches X; you merely find *any* matching object that
> > > contains _PS0.
> > 
> > Well, except when there are multiple matching objects having _PS0.
> > Which actually happens in the failing case in bug #42696.
> > 
> > Our assumptions work pretty well on other systems and I don't quite see the
> > reason to change them entirely.
> > 
> > Moreover, Section 19.5.30 of the spec says that "Device object [...]
> > represents either a bus or a device or any other similar hardware".  That
> > implies that if there are two objects with the same _ADR matching the same
> > single devfn of a PCI device, that will mean that there are _two_ different
> > PCI devices under the same parent that have the same devfn.  In that case
> > PCI config space accesses wouldn't work for those devices, though.
> > 
> > > >> Linux assumes we can start from a PCI device and identify a single
> > > >> related ACPI namespace node, e.g., in acpi_pci_find_device().  But all
> > > >> I see in the spec is a requirement that we can start from an ACPI
> > > >> namespace node and find a PCI device.  So I'm not sure
> > > >> acpi_pci_find_device() is based on a valid assumption.
> > > > 
> > > > I think it is.
> > > > 
> > > > Suppose that we have two namespace nodes with the same _ADR under one
> > > > parent (PCI bridge ACPI node) and they both contain things like _PS0
> > > > and _PS3.  Which one of these are we supposed to use for the power
> > > > management of the corresponding PCI device?  Because they both would
> > > > point to the same device, right?
> > > 
> > > That's a good question.  It's more complicated if two objects supply
> > > the same method.
> > 
> > Well it is and they do.
> > 
> > > >> Let's say we want to provide _SUN and _UID.  _SUN is a slot number
> > > >> that may apply to several PCI functions, while _UID probably refers to
> > > >> a single PCI function.  Is it legal to provide two namespace objects,
> > > >> one with _ADR 0x0003ffff and _SUN, and another with _ADR 0x00030000
> > > >> and _UID?
> > > > 
> > > > I don't think it is valid to do that.
> > > 
> > > Is there something in the spec that says you can't?  I can imagine a
> > > BIOS writer doing that, and I don't know how I could convince him that
> > > it's illegal.
> > 
> > Well, OK.
> > 
> > > It would be really interesting to try some of these scenarios on
> > > Windows with qemu.
> > 
> > That's interesting theoretically, but doesn't directly relate to the case at
> > hand.  The case at hand is that for a given PCI device we want to find the
> > ACPI namespace node that can be used for things like power management, if
> > one exists.  While it may be valid to specify _ADR of type 0x0003ffff for
> > some namespace nodes, I don't really think it is valid to specify two
> > objects with the same _ADR matching a specific devfn that both provide the
> > same methods (like _PSx or _CRS).
> > 
> > And the question we need to answer is not "I have a namespace node, so which
> > device it represents?", but "I have a device, so which namespace node
> > provides methods I'm supposed to use for it?"
> > 
> > So I think we make the right assumptions, but there are broken BIOSes that
> > don't follow them and I'm trying to find out how to handle them without
> > blacklisting etc.
> > 
> > Questioning the validity of everything we're doing doesn't really help, mind
> > you.
> > 
> > Thanks,
> > Rafael
> 
> Regards,
> Peter
Peter Wu - March 4, 2013, 3:56 p.m.
On Wednesday 23 January 2013 20:12:59 Rafael J. Wysocki wrote:
> On Wednesday, January 23, 2013 08:00:31 PM Peter Wu wrote:
> > Hi,
> > 
> > Any progress on this one? I guess it won't make into 3.8, perhaps 3.9?
> 
> No, that doesn't go anywhere for now.
> 
> In fact, I need to discuss that with Len.
Is this patch still valid? If not, what patch do you suggest for users of the 
affected Lenovo machines? (please CC me in a related discussion)

Regards,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch

Index: linux/drivers/acpi/glue.c
===================================================================
--- linux.orig/drivers/acpi/glue.c
+++ linux/drivers/acpi/glue.c
@@ -93,6 +93,51 @@  static int acpi_find_bridge_device(struc
 	return ret;
 }
 
+/**
+ * acpi_get_child_device - Find specific child of an ACPI device.
+ * @phandle: ACPI handle of the parent device to find a child of.
+ * @address: Address of the child to find (as returned by _ADR).
+ *
+ * Find the child of the ACPI device node represented by @phandle whose _ADR
+ * method's return value is equal to @address.  If there are more children with
+ * matching _ADR return values, take the (last) one having _INI defined.
+ */
+struct acpi_device *acpi_get_child_device(acpi_handle phandle, u64 address)
+{
+	struct acpi_device *parent, *adev, *ret = NULL;
+
+	if (acpi_bus_get_device(phandle, &parent))
+		return NULL;
+
+	mutex_lock(&acpi_device_lock);
+	/* Use reverse direction for compatibility with acpi_get_child(). */
+	list_for_each_entry_reverse(adev, &parent->children, node) {
+		unsigned long long addr;
+		acpi_status status;
+		acpi_handle out;
+
+		status = acpi_evaluate_integer(adev->handle, METHOD_NAME__ADR,
+					       NULL, &addr);
+		if (ACPI_FAILURE(status) || addr != address)
+			continue;
+
+		if (ret)
+			acpi_handle_warn(adev->handle,
+					 "_ADR conflict with device %s\n",
+					 dev_name(&ret->dev));
+
+		status = acpi_get_handle(adev->handle, "_INI", &out);
+		if (ACPI_SUCCESS(status)) {
+			ret = adev;
+			break;
+		} else if (!ret) {
+			ret = adev;
+		}
+	}
+	mutex_unlock(&acpi_device_lock);
+	return ret;
+}
+
 /* Get device's handler per its address under its parent */
 struct acpi_find_child {
 	acpi_handle handle;
Index: linux/drivers/acpi/internal.h
===================================================================
--- linux.orig/drivers/acpi/internal.h
+++ linux/drivers/acpi/internal.h
@@ -21,8 +21,12 @@ 
 #ifndef _ACPI_INTERNAL_H_
 #define _ACPI_INTERNAL_H_
 
+#include <linux/mutex.h>
+
 #define PREFIX "ACPI: "
 
+extern struct mutex acpi_device_lock;
+
 int init_acpi_device_notify(void);
 int acpi_scan_init(void);
 int acpi_sysfs_init(void);
Index: linux/drivers/acpi/sleep.h
===================================================================
--- linux.orig/drivers/acpi/sleep.h
+++ linux/drivers/acpi/sleep.h
@@ -5,4 +5,3 @@  extern void acpi_enable_wakeup_devices(u
 extern void acpi_disable_wakeup_devices(u8 sleep_state);
 
 extern struct list_head acpi_wakeup_device_list;
-extern struct mutex acpi_device_lock;
Index: linux/drivers/acpi/proc.c
===================================================================
--- linux.orig/drivers/acpi/proc.c
+++ linux/drivers/acpi/proc.c
@@ -12,6 +12,7 @@ 
 #include <linux/mc146818rtc.h>
 #endif
 
+#include "internal.h"
 #include "sleep.h"
 
 #define _COMPONENT		ACPI_SYSTEM_COMPONENT
Index: linux/include/acpi/acpi_bus.h
===================================================================
--- linux.orig/include/acpi/acpi_bus.h
+++ linux/include/acpi/acpi_bus.h
@@ -400,6 +400,7 @@  struct acpi_pci_root {
 };
 
 /* helper */
+struct acpi_device *acpi_get_child_device(acpi_handle phandle, u64 address);
 acpi_handle acpi_get_child(acpi_handle, u64);
 int acpi_is_root_bridge(acpi_handle);
 struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle);
Index: linux/drivers/pci/pci-acpi.c
===================================================================
--- linux.orig/drivers/pci/pci-acpi.c
+++ linux/drivers/pci/pci-acpi.c
@@ -291,14 +291,17 @@  static struct pci_platform_pm_ops acpi_p
 static int acpi_pci_find_device(struct device *dev, acpi_handle *handle)
 {
 	struct pci_dev * pci_dev;
+	struct acpi_device *adev;
 	u64	addr;
 
 	pci_dev = to_pci_dev(dev);
 	/* Please ref to ACPI spec for the syntax of _ADR */
 	addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn);
-	*handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr);
-	if (!*handle)
+	adev = acpi_get_child_device(ACPI_HANDLE(dev->parent), addr);
+	if (!adev)
 		return -ENODEV;
+
+	*handle = adev->handle;
 	return 0;
 }