Date: Sun, 3 May 2009 23:41:28 +0400 (MSD) From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: FreeBSD-gnats-submit@freebsd.org Subject: kern/134192: [patch] [acpi] don't probe acpi(4) children that are aliases of other nodes Message-ID: <20090503194128.944E2B8031@phoenix.codelabs.ru> Resent-Message-ID: <200905031950.n43Jo1Pi052621@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>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@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@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:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090503194128.944E2B8031>