Date: Mon, 4 May 2009 15:51:16 -0400 From: Jung-uk Kim <jkim@FreeBSD.org> To: freebsd-acpi@FreeBSD.org Cc: "Moore, Robert" <robert.moore@intel.com> Subject: Re: [Fwd: kern/134192: [patch] [acpi] don't probe acpi(4) children that are aliases of other nodes] Message-ID: <200905041551.19904.jkim@FreeBSD.org> In-Reply-To: <49FE2452.4000401@root.org> References: <49FE2452.4000401@root.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@freebsd.org (GNATS Filer) > Resent-To: freebsd-bugs@FreeBSD.org > Resent-CC: jkim@freebsd.org, njl@freebsd.org > Date: Sun, 3 May 2009 23:41:28 +0400 (MSD) > From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> > Reply-To: Eygene Ryabinkin <rea-fbsd@codelabs.ru> > To: FreeBSD-gnats-submit@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@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?200905041551.19904.jkim>