[Fwd: kern/134192: [patch] [acpi] don't probe acpi(4) children
that are aliases of other nodes]
Jung-uk Kim
jkim at FreeBSD.org
Mon May 4 19:51:33 UTC 2009
On Sunday 03 May 2009 07:10 pm, Nate Lawson wrote:
> Wonder what you thought of this patch to avoid Aliases in
> AcpiWalkNamespace()?
I believe it was fixed in the vendor source differently since ACPI-CA
20071019 if I am not mistaken:
http://git.moblin.org/cgit.cgi/acpica/commit/?id=1e8f03866122dc06146879c9d4d4ad8bb408b60e
Jung-uk Kim
> -------- Original Message --------
> Subject: kern/134192: [patch] [acpi] don't probe acpi(4) children
> that are aliases of other nodes
> Resent-Date: Sun, 3 May 2009 19:50:01 GMT
> Resent-From: FreeBSD-gnats-submit at freebsd.org (GNATS Filer)
> Resent-To: freebsd-bugs at FreeBSD.org
> Resent-CC: jkim at freebsd.org, njl at freebsd.org
> Date: Sun, 3 May 2009 23:41:28 +0400 (MSD)
> From: Eygene Ryabinkin <rea-fbsd at codelabs.ru>
> Reply-To: Eygene Ryabinkin <rea-fbsd at codelabs.ru>
> To: FreeBSD-gnats-submit at freebsd.org
>
> >Number: 134192
> >Category: kern
> >Synopsis: [patch] [acpi] don't probe acpi(4) children that
> > are aliases of other nodes Confidential: no
> >Severity: serious
> >Priority: medium
> >Responsible: freebsd-bugs
> >State: open
> >Quarter:
> >Keywords:
> >Date-Required:
> >Class: sw-bug
> >Submitter-Id: current-users
> >Arrival-Date: Sun May 03 19:50:00 UTC 2009
> >Closed-Date:
> >Last-Modified:
> >Originator: Eygene Ryabinkin
> >Release: FreeBSD 7.1-STABLE amd64
> >Organization:
>
> Code Labs
>
> >Environment:
>
> System: FreeBSD 7.1-STABLE amd64
>
> >Description:
>
> Some vendors (in my case, Asus) like to create aliases for node
> names in the DSDT table:
> -----
> Scope (_PR)
> {
> Processor (P001, 0x01, 0x00000810, 0x06) {}
> Alias (P001, CPU1)
> Processor (P002, 0x02, 0x00000000, 0x00) {}
> Alias (P002, CPU2)
> Processor (P003, 0x03, 0x00000000, 0x00) {}
> Alias (P003, CPU3)
> Processor (P004, 0x04, 0x00000000, 0x00) {}
> Alias (P004, CPU4)
> }
> -----
> Another example (from Asus system too) is presented in the patch
> below.
>
> Since AcpiWalkNamespace treats CPUX as the real Processor objects,
> the probe order for acpi(4) bus children will be P001, CPU1, P002,
> CPU2, P003, CPU3, P004, CPU4. This is very bad, because half of
> processors are attached twice and half -- aren't attached at all.
> Moreover, est (Enhanced SpeedStep cpufreq driver) isn't able to get
> _PSS for CPUX, so P-states will be missing for half of processors.
>
> >How-To-Repeat:
>
> Insert Alias() instruction to your custom DSDT and try to use it,
> enabling debug that will show what ACPI nodes are attached to cpuX
> device nodes.
>
> >Fix:
>
> The following patch adds ACPI namespace walking procedure that
> skips nodes that are results of the Alias() operator invocation.
> This new method is used for probing of acpi(4) bus members.
>
> The patch works on two machines of mine that have these annoying
> Alias() calls in the _PR namespace.
>
> I had tried this patch both on 7.x and 8-CURRENT.
>
> --- ACPI-attach-children-without-aliases.diff begins here ---
>
> >From f70d0d754f381735a67a42be91fa7253b19a5c8c Mon Sep 17 00:00:00
> > 2001
>
> From: Eygene Ryabinkin <rea-fbsd at codelabs.ru>
> Date: Sun, 3 May 2009 22:00:20 +0400
>
> ...and use this function to probe acpi(4) children.
>
> The rationale is very simple: some BIOS vendors, in my case it was
> Asus, like to create aliased nodes in the namespaces that will be
> walked by the acpi bus method that attaches children to the bus.
>
> In my particular case, the problem lied in the Processor objects:
> -----
> Scope (_PR)
> {
> Processor (P001, 0x01, 0x00000810, 0x06) {}
> Alias (P001, CPU1)
> }
>
> Scope (_PR)
> {
> Processor (P002, 0x02, 0x00000810, 0x06) {}
> Alias (P002, CPU2)
> }
> -----
> The thing is that the simple walk routine treated CPU1 and CPU2 as
> real processors and order of attachment was P001, CPU1, P002, CPU2.
> Thus, the second CPU was never attached, first was attached twice
> and est (Enhanced SpeedStep cpufreq(4) driver) was failing to get
> _PSS (processor P-states) from the CPU1.
>
> I greatly doubt that some real devices will be ever created as
> aliases, since they should be real objects and not modified copies
> of other device instances. And since ASL Alias() semantics is to
> provide pseudonym, this modification should be "The Good Thing
> (tm)".
>
> Signed-off-by: Eygene Ryabinkin <rea-fbsd at codelabs.ru>
> ---
> sys/contrib/dev/acpica/aclocal.h | 1 +
> sys/contrib/dev/acpica/acnamesp.h | 1 +
> sys/contrib/dev/acpica/acpixf.h | 9 +++++
> sys/contrib/dev/acpica/excreate.c | 3 ++
> sys/contrib/dev/acpica/nswalk.c | 10 ++++-
> sys/contrib/dev/acpica/nsxfeval.c | 69
> ++++++++++++++++++++++++++++++++++---
> sys/dev/acpica/acpi.c | 18 +++++----
> 7 files changed, 96 insertions(+), 15 deletions(-)
>
> diff --git a/sys/contrib/dev/acpica/aclocal.h
> b/sys/contrib/dev/acpica/aclocal.h
> index ba1145e..73e1568 100644
> --- a/sys/contrib/dev/acpica/aclocal.h
> +++ b/sys/contrib/dev/acpica/aclocal.h
> @@ -301,6 +301,7 @@ typedef struct acpi_namespace_node
> #define ANOBJ_METHOD_ARG 0x04 /* Node is a
> method argument */
> #define ANOBJ_METHOD_LOCAL 0x08 /* Node is a
> method local */
> #define ANOBJ_SUBTREE_HAS_INI 0x10 /* Used to
> optimize device initialization */
> +#define ANOBJ_IS_ALIAS 0x20 /* Node is an
> alias to another one */
>
> #define ANOBJ_IS_EXTERNAL 0x08 /* iASL only: This
> object created via External() */
> #define ANOBJ_METHOD_NO_RETVAL 0x10 /* iASL only:
> Method has no return value */
> diff --git a/sys/contrib/dev/acpica/acnamesp.h
> b/sys/contrib/dev/acpica/acnamesp.h
> index 8d07fb3..52a50bb 100644
> --- a/sys/contrib/dev/acpica/acnamesp.h
> +++ b/sys/contrib/dev/acpica/acnamesp.h
> @@ -146,6 +146,7 @@
> #define ACPI_NS_WALK_NO_UNLOCK 0
> #define ACPI_NS_WALK_UNLOCK 0x01
> #define ACPI_NS_WALK_TEMP_NODES 0x02
> +#define ACPI_NS_WALK_SKIP_ALIASES 0x04
>
>
> /*
> diff --git a/sys/contrib/dev/acpica/acpixf.h
> b/sys/contrib/dev/acpica/acpixf.h
> index f85fd67..3757ad0 100644
> --- a/sys/contrib/dev/acpica/acpixf.h
> +++ b/sys/contrib/dev/acpica/acpixf.h
> @@ -238,6 +238,15 @@ AcpiWalkNamespace (
> void **ReturnValue);
>
> ACPI_STATUS
> +AcpiWalkNamespaceNoAliases (
> + ACPI_OBJECT_TYPE Type,
> + ACPI_HANDLE StartObject,
> + UINT32 MaxDepth,
> + ACPI_WALK_CALLBACK UserFunction,
> + void *Context,
> + void **ReturnValue);
> +
> +ACPI_STATUS
> AcpiGetDevices (
> char *HID,
> ACPI_WALK_CALLBACK UserFunction,
> diff --git a/sys/contrib/dev/acpica/excreate.c
> b/sys/contrib/dev/acpica/excreate.c
> index d237dab..dba8312 100644
> --- a/sys/contrib/dev/acpica/excreate.c
> +++ b/sys/contrib/dev/acpica/excreate.c
> @@ -221,6 +221,9 @@ AcpiExCreateAlias (
> break;
> }
>
> + /* Mark object as alias, so alias creation could be detected
> */ + AliasNode->Flags |= ANOBJ_IS_ALIAS;
> +
> /* Since both operands are Nodes, we don't need to delete them
> */
>
> return_ACPI_STATUS (Status);
> diff --git a/sys/contrib/dev/acpica/nswalk.c
> b/sys/contrib/dev/acpica/nswalk.c
> index a3ac86c..9cbc56d 100644
> --- a/sys/contrib/dev/acpica/nswalk.c
> +++ b/sys/contrib/dev/acpica/nswalk.c
> @@ -208,8 +208,7 @@ AcpiNsGetNextNode (
> * PARAMETERS: Type - ACPI_OBJECT_TYPE to search
> for * StartNode - Handle in namespace where
> search begins
> * MaxDepth - Depth to which search is to
> reach - * Flags - Whether to unlock the
> NS before invoking
> - * the callback routine
> + * Flags - Flags that modify walk
> parameters * UserFunction - Called when an
> object of "Type" is found
> * Context - Passed to user function
> * ReturnValue - from the UserFunction if
> terminated early.
> @@ -300,6 +299,13 @@ AcpiNsWalkNamespace (
> Status = AE_CTRL_DEPTH;
> }
>
> + /* Skip nodes created by Alias() routines if it was
> requested */
> + else if ((ChildNode->Flags & ANOBJ_IS_ALIAS) &&
> + (Flags & ACPI_NS_WALK_SKIP_ALIASES))
> + {
> + Status = AE_CTRL_DEPTH;
> + }
> +
> /* Type must match requested type */
>
> else if (ChildType == Type)
> diff --git a/sys/contrib/dev/acpica/nsxfeval.c
> b/sys/contrib/dev/acpica/nsxfeval.c
> index 617002c..662a9df 100644
> --- a/sys/contrib/dev/acpica/nsxfeval.c
> +++ b/sys/contrib/dev/acpica/nsxfeval.c
> @@ -122,6 +122,16 @@
> #include <contrib/dev/acpica/acnamesp.h>
> #include <contrib/dev/acpica/acinterp.h>
>
> +static ACPI_STATUS
> +_AcpiWalkNamespace (
> + ACPI_OBJECT_TYPE Type,
> + ACPI_HANDLE StartObject,
> + UINT32 MaxDepth,
> + ACPI_WALK_CALLBACK UserFunction,
> + void *Context,
> + void **ReturnValue,
> + UINT32 AddFlags);
> +
>
> #define _COMPONENT ACPI_NAMESPACE
> ACPI_MODULE_NAME ("nsxfeval")
> @@ -491,11 +501,62 @@ AcpiWalkNamespace (
> void *Context,
> void **ReturnValue)
> {
> - ACPI_STATUS Status;
> + ACPI_FUNCTION_TRACE (AcpiWalkNamespace);
>
> + return _AcpiWalkNamespace(Type, StartObject, MaxDepth,
> + UserFunction, Context, ReturnValue, 0);
> +}
>
> - ACPI_FUNCTION_TRACE (AcpiWalkNamespace);
> +ACPI_EXPORT_SYMBOL (AcpiWalkNamespace)
>
> +/*****************************************************************
>************** + *
> + * FUNCTION: AcpiWalkNamespaceNoAliases
> + *
> + * DESCRIPTION: The same as AcpiWalkNamespace, but skips nodes
> that were + * created as the result of an Alias
> function. + *
> + * NOTE: for parameters/semantics see AcpiWalkNamespace.
> + *
> +
> *******************************************************************
>***********/ +
> +ACPI_STATUS
> +AcpiWalkNamespaceNoAliases (
> + ACPI_OBJECT_TYPE Type,
> + ACPI_HANDLE StartObject,
> + UINT32 MaxDepth,
> + ACPI_WALK_CALLBACK UserFunction,
> + void *Context,
> + void **ReturnValue)
> +{
> + ACPI_FUNCTION_TRACE (AcpiWalkNamespaceNoAliases);
> +
> + return _AcpiWalkNamespace(Type, StartObject, MaxDepth,
> + UserFunction, Context, ReturnValue,
> ACPI_NS_WALK_SKIP_ALIASES); +}
> +
> +ACPI_EXPORT_SYMBOL (AcpiWalkNamespaceNoAliases)
> +
> +/*****************************************************************
>************** + *
> + * FUNCTION: _AcpiWalkNamespace
> + *
> + * DESCRIPTION: Internal helper for AcpiWalkNamespace and
> + * AcpiWalkNamespaceNoAliases.
> + *
> +
> *******************************************************************
>***********/ +
> +static ACPI_STATUS
> +_AcpiWalkNamespace (
> + ACPI_OBJECT_TYPE Type,
> + ACPI_HANDLE StartObject,
> + UINT32 MaxDepth,
> + ACPI_WALK_CALLBACK UserFunction,
> + void *Context,
> + void **ReturnValue,
> + UINT32 AddFlags)
> +{
> + ACPI_STATUS Status;
>
> /* Parameter validation */
>
> @@ -519,15 +580,13 @@ AcpiWalkNamespace (
> }
>
> Status = AcpiNsWalkNamespace (Type, StartObject, MaxDepth,
> - ACPI_NS_WALK_UNLOCK,
> + AddFlags | ACPI_NS_WALK_UNLOCK,
> UserFunction, Context, ReturnValue);
>
> (void) AcpiUtReleaseMutex (ACPI_MTX_NAMESPACE);
> return_ACPI_STATUS (Status);
> }
>
> -ACPI_EXPORT_SYMBOL (AcpiWalkNamespace)
> -
>
>
> /******************************************************************
>************* *
> diff --git a/sys/dev/acpica/acpi.c b/sys/dev/acpica/acpi.c
> index d79b413..18a9800 100644
> --- a/sys/dev/acpica/acpi.c
> +++ b/sys/dev/acpica/acpi.c
> @@ -1528,7 +1528,7 @@ acpi_device_scan_children(device_t bus,
> device_t dev, int max_depth,
> ctx.user_fn = user_fn;
> ctx.arg = arg;
> ctx.parent = h;
> - return (AcpiWalkNamespace(ACPI_TYPE_ANY, h, max_depth,
> + return (AcpiWalkNamespaceNoAliases(ACPI_TYPE_ANY, h,
> max_depth, acpi_device_scan_cb, &ctx, NULL));
> }
>
> @@ -1648,14 +1648,16 @@ acpi_probe_children(device_t bus)
> * Scan the namespace and insert placeholders for all the
> devices that * we find. We also probe/attach any early devices.
> *
> - * Note that we use AcpiWalkNamespace rather than
> AcpiGetDevices because
> - * we want to create nodes for all devices, not just those
> that are - * currently present. (This assumes that we don't
> want to create/remove - * devices as they appear, which might
> be smarter.)
> + * Note that we use AcpiWalkNamespaceNoAliases rather than
> + * AcpiGetDevices because we want to create nodes for all
> devices, + * not just those that are currently present. (This
> assumes that we + * don't want to create/remove devices as they
> appear, which might + * be smarter.) We avoid counting aliased
> nodes, because we generally + * want to attach only to a
> "genuine" devices.
> */
> ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "namespace scan\n"));
> - AcpiWalkNamespace(ACPI_TYPE_ANY, ACPI_ROOT_OBJECT, 100,
> acpi_probe_child,
> - bus, NULL);
> + AcpiWalkNamespaceNoAliases(ACPI_TYPE_ANY, ACPI_ROOT_OBJECT,
> 100, + acpi_probe_child, bus, NULL);
>
> /* Pre-allocate resources for our rman from any sysresource
> devices. */ acpi_sysres_alloc(bus);
> @@ -2794,7 +2796,7 @@ acpi_wake_prep_walk(int sstate)
> ACPI_HANDLE sb_handle;
>
> if (ACPI_SUCCESS(AcpiGetHandle(ACPI_ROOT_OBJECT, "\\_SB_",
> &sb_handle)))
> - AcpiWalkNamespace(ACPI_TYPE_DEVICE, sb_handle, 100,
> + AcpiWalkNamespaceNoAliases(ACPI_TYPE_DEVICE, sb_handle, 100,
> acpi_wake_prep, &sstate, NULL);
> return (0);
> }
> --
> 1.6.2.4
> --- ACPI-attach-children-without-aliases.diff ends here ---
>
> >Release-Note:
> >Audit-Trail:
> >Unformatted:
More information about the freebsd-acpi
mailing list