From rea-fbsd@codelabs.ru Sun May 3 19:41:31 2009 Return-Path: Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 894B61065670 for ; Sun, 3 May 2009 19:41:31 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) Received: from 0.mx.codelabs.ru (0.mx.codelabs.ru [144.206.177.45]) by mx1.freebsd.org (Postfix) with ESMTP id 9E70E8FC0C for ; Sun, 3 May 2009 19:41:30 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) Received: from phoenix.codelabs.ru (ppp85-141-64-167.pppoe.mtu-net.ru [85.141.64.167]) by 0.mx.codelabs.ru with esmtps (TLSv1:CAMELLIA256-SHA:256) id 1M0hZ3-000C4G-8h for FreeBSD-gnats-submit@freebsd.org; Sun, 03 May 2009 23:41:29 +0400 Message-Id: <20090503194128.944E2B8031@phoenix.codelabs.ru> Date: Sun, 3 May 2009 23:41:28 +0400 (MSD) From: Eygene Ryabinkin Reply-To: Eygene Ryabinkin To: FreeBSD-gnats-submit@freebsd.org Cc: Subject: [patch] [acpi] don't probe acpi(4) children that are aliases of other nodes X-Send-Pr-Version: 3.113 X-GNATS-Notify: jkim@freebsd.org, njl@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: closed >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Sun May 03 19:50:00 UTC 2009 >Closed-Date: Mon Jun 08 15:35:37 UTC 2009 >Last-Modified: Mon Jun 08 15:35:37 UTC 2009 >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 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 --- 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 #include +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: From: Robert Noland To: bug-followup@FreeBSD.org, rea-fbsd@codelabs.ru Cc: Subject: Re: kern/134192: [patch] [acpi] don't probe acpi(4) children that are aliases of other nodes Date: Thu, 07 May 2009 16:12:37 -0500 --=-P9/sGAzxXN8kL1udtkql Content-Type: text/plain Content-Transfer-Encoding: quoted-printable I have this applied to my Asus P5Q-E with an e7400 core2duo and it has been running now for a few days. est is attaching to both cores now and I have powerd running now (which used to cause periodic lockups) and everything seems to be good. robert. --=20 Robert Noland FreeBSD --=-P9/sGAzxXN8kL1udtkql Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.11 (FreeBSD) iEYEABECAAYFAkoDTsUACgkQM4TrQ4qfROP2QACfXFWDovmnogW8Yb/h3Glo1GPK F/YAnAq/XCNXBYMi7JHXyANZrZsqiEVl =sV+N -----END PGP SIGNATURE----- --=-P9/sGAzxXN8kL1udtkql-- From: Eygene Ryabinkin To: bug-followup@freebsd.org Cc: Subject: Re: kern/134192: [patch] [acpi] don't probe acpi(4) children that are aliases of other nodes Date: Mon, 8 Jun 2009 10:33:17 +0400 With the import of ACPI-CA 20090521 [1] the original issue is gone, so this PR can be closed. [1] http://lists.freebsd.org/pipermail/freebsd-acpi/2009-June/005780.html -- Eygene _ ___ _.--. # \`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard / ' ` , __.--' # to read the on-line manual )/' _/ \ `-_, / # while single-stepping the kernel. `-'" `"\_ ,_.-;_.-\_ ', fsc/as # _.-'_./ {_.' ; / # -- FreeBSD Developers handbook {_.-``-' {_/ # State-Changed-From-To: open->closed State-Changed-By: linimon State-Changed-When: Mon Jun 8 15:35:27 UTC 2009 State-Changed-Why: Apparently OBE. http://www.freebsd.org/cgi/query-pr.cgi?pr=134192 >Unformatted: