Bug 1766 - passing argument by reference
Summary: passing argument by reference
Status: CLOSED CODE_FIX
Alias: None
Product: ACPI
Classification: Unclassified
Component: ACPICA-Core (show other bugs)
Hardware: i386 Linux
: P2 normal
Assignee: Robert Moore
URL:
Keywords:
: 1759 1776 (view as bug list)
Depends on:
Blocks: 1728
  Show dependency tree
 
Reported: 2003-12-30 22:48 UTC by Luming Yu
Modified: 2006-09-28 13:14 UTC (History)
5 users (show)

See Also:
Kernel Version: 2.4, 2.6
Subsystem:
Regression: ---
Bisected commit-id:


Attachments
a proposal (982 bytes, patch)
2003-12-30 22:57 UTC, Luming Yu
Details | Diff
ACPI-CA formatted version of the same patch (822 bytes, patch)
2004-01-01 23:22 UTC, Nate Lawson
Details | Diff
IBM T23 ASL (see _BST and GBST for ref count problems) (318.31 KB, application/octet-stream)
2004-01-04 20:18 UTC, Nate Lawson
Details
Bob's suggested fix as a diff vs. ACPICA 20040116 (672 bytes, patch)
2004-01-29 22:27 UTC, Len Brown
Details | Diff
Bob's suggested fix as a diff vs. ACPICA 20040116 (Linux) (765 bytes, patch)
2004-01-29 22:35 UTC, Len Brown
Details | Diff
Revert Luming's workaround and Bob's patch against acpi-ca (1.37 KB, patch)
2004-01-30 10:02 UTC, Nate Lawson
Details | Diff

Description Luming Yu 2003-12-30 22:48:14 UTC
Below test cases shows that current ACPI CA has bug in passing argument by
reference.

                        Name (SXX0, Buffer (0x0100) {})

                        Method (SXX5, 2, NotSerialized)
                        {
                            Store ("sizeof arg0", Debug)
                            Store (SizeOf (Arg0), Debug)
                            Store ("arg1 = ", Debug)
                            Store (Arg1, Debug)
                            If (LLess (Arg1, SizeOf (Arg0)))
                            {
                                CreateByteField (Arg0, Arg1, SX20)
                                Store(6, SX20)
                                Store("SX20", Debug)
                                Store(SX20, Debug)
                            }
                        }

                        Method (_STA, 0, NotSerialized)
                        {
                            Name (SX24, Buffer (0x07) {})
                            Store ("1111111", Debug)
                            Store (Zero, Local0)
                            Store (0x06, SXX2)
                            CreateByteField (SXX0, 0, SX80)
                            Store ("SXX5", Debug)
                            SXX5 (SXX0, Local0)
                            Store ("SX80", Debug)
                            Store (SX80, Debug)



[ACPI Debug] String: 1111111
[ACPI Debug] String: SXX5
[ACPI Debug] String: sizeof arg0
[ACPI Debug] Integer: 0000000000000100
[ACPI Debug] String: arg1 =
[ACPI Debug] Integer: 0000000000000000
[ACPI Debug] String: SX20
[ACPI Debug] Integer: 0000000000000006
[ACPI Debug] String: SX80
[ACPI Debug] Integer: 0000000000000000


                        Name (SXX0, Buffer (0x0100) {})

                        Method (SXX5, 2, NotSerialized)
                        {
                            Store ("sizeof arg0", Debug)
                            Store (SizeOf (Derefof(Arg0)), Debug)
                            Store ("arg1 = ", Debug)
                            Store (Arg1, Debug)
                            If (LLess (Arg1, SizeOf (Derefof(Arg0))))
                            {
                                CreateByteField (Derefof(Arg0), Arg1, SX20)
                                Store(6, SX20)
                                Store("SX20", Debug)
                                Store(SX20, Debug)
                            }
                        }

                        Method (_STA, 0, NotSerialized)
                        {
                            Name (SX24, Buffer (0x07) {})
                            Store ("1111111", Debug)
                            Store (Zero, Local0)
                            Store (0x06, SXX2)
                            CreateByteField (SXX0, 0, SX80)
                            Store ("SXX5", Debug)
                            SXX5 (Refof(SXX0), Local0)
                            Store ("SX80", Debug)
                            Store (SX80, Debug)



[ACPI Debug] String: 1111111
[ACPI Debug] String: SXX5
[ACPI Debug] String: sizeof arg0
[ACPI Debug] Integer: 0000000000000100
[ACPI Debug] String: arg1 =
[ACPI Debug] Integer: 0000000000000000
[ACPI Debug] String: SX20
[ACPI Debug] Integer: 0000000000000006
[ACPI Debug] String: SX80
[ACPI Debug] Integer: 0000000000000006
Comment 1 Luming Yu 2003-12-30 22:57:07 UTC
Created attachment 1774 [details]
a proposal

  Since reference of buffer could increase due to buffer filed . (Please
reference
acpi/dispatcher/dsopcode.c : 546). 
  But this patch is not good, just a proposal.
Comment 2 Nate Lawson 2004-01-01 23:22:01 UTC
Created attachment 1777 [details]
ACPI-CA formatted version of the same patch

The attached patch fixes the problem for several FreeBSD Dell users and is
simply a reformatted version of Luming's patch.
Comment 3 Nate Lawson 2004-01-03 22:30:13 UTC
Are you sure this patch is the final version that will be in a future ACPI-CA
dist?  It seems like a workaround, not a fix.
Comment 4 Nate Lawson 2004-01-04 20:16:41 UTC
This patch breaks battery evaluation on my IBM T23.  It doesn't get the
reference count right for my _BST method (actually GBST).  Please see the ASL I
will attach next to see how GBST works.

Here is the console prints:
utobject-0537 [39] UtGetSimpleObjectSize : Unsupported type=ca in object 0xc38b0f28
acpi_cmbat0: error fetching current battery status -- AE_TYPE
utobject-0537 [42] UtGetSimpleObjectSize : Unsupported type=ca in object 0xc38b0ca8
acpi_cmbat0: error fetching current battery status -- AE_TYPE
utobject-0537 [44] UtGetSimpleObjectSize : Unsupported type=ca in object 0xc38b0f28
acpi_cmbat0: error fetching current battery status -- AE_TYPE
utobject-0537 [46] UtGetSimpleObjectSize : Unsupported type=ca in object 0xc38b0f28
acpi_cmbat0: error fetching current battery status -- AE_TYPE
Comment 5 Nate Lawson 2004-01-04 20:17:26 UTC
Add Bob as CC since he did the original fix for GBST for thinkpads.
Comment 6 Nate Lawson 2004-01-04 20:18:50 UTC
Created attachment 1794 [details]
IBM T23 ASL (see _BST and GBST for ref count problems)
Comment 7 Luming Yu 2004-01-05 05:18:50 UTC
I'm not sure your problem is caused by this patch. But from you DSDT, I DID 
find out some error. Please reference 
http://bugzilla.kernel.org/show_bug.cgi?id=1791
Comment 8 Luming Yu 2004-01-06 21:46:41 UTC
*** Bug 1759 has been marked as a duplicate of this bug. ***
Comment 9 Luming Yu 2004-01-06 21:49:17 UTC
*** Bug 1776 has been marked as a duplicate of this bug. ***
Comment 10 Robert Moore 2004-01-29 14:12:54 UTC
I think that this may be the correct fix for these problems.  It directly 
addresses the call-by-value versus call-by-reference issue by not copying the 
method arguments to new objects.

ACPI_STATUS
AcpiDsMethodDataInitArgs (
    ACPI_OPERAND_OBJECT     **Params,
    UINT32                  MaxParamCount,
    ACPI_WALK_STATE         *WalkState)
{
    ACPI_STATUS             Status;
    UINT32                  Index = 0;


    ACPI_FUNCTION_TRACE_PTR ("DsMethodDataInitArgs", Params);


    if (!Params)
    {
        ACPI_DEBUG_PRINT ((ACPI_DB_EXEC, "No param list passed to method\n"));
        return_ACPI_STATUS (AE_OK);
    }

    /* Copy passed parameters into the new method stack frame  */

    while ((Index < ACPI_METHOD_NUM_ARGS) && (Index < MaxParamCount) && Params
[Index])
    {
        /*
         * A valid parameter.
         * Store the argument in the method/walk descriptor.
         * Do not copy the arg in order to implement call by reference
         */
        Status = AcpiDsMethodDataSetValue (AML_ARG_OP, Index, Params[Index], 
WalkState);
        if (ACPI_FAILURE (Status))
        {
            return_ACPI_STATUS (Status);
        }

        Index++;
    }

    ACPI_DEBUG_PRINT ((ACPI_DB_EXEC, "%d args passed to method\n", Index));
    return_ACPI_STATUS (AE_OK);
}

Comment 11 Len Brown 2004-01-29 22:27:01 UTC
Created attachment 1964 [details]
Bob's suggested fix as a diff vs. ACPICA 20040116
Comment 12 Len Brown 2004-01-29 22:35:39 UTC
Created attachment 1965 [details]
Bob's suggested fix as a diff vs. ACPICA 20040116 (Linux)
Comment 13 Nate Lawson 2004-01-30 10:02:49 UTC
Created attachment 1971 [details]
Revert Luming's workaround and Bob's patch against acpi-ca
Comment 14 Nate Lawson 2004-01-30 10:04:43 UTC
Bob's patch doesn't work, unfortunately.  Here is the error dump:

acpi_cmbat0: error fetching current battery status -- AE_TYPE

**** Exception AE_AML_INTERNAL during execution of method
[\\_SB_.PCI0.LPC_.EC__.GBST] (Node 0xc37ad028)

Method Execution Stack:
    Method [GBST] executing:    af #%4.4hX Store (Local0, -Return Value- ())
    Method [_BST] executing: Call to method [\\_SB_.PCI0.LPC_.EC__.GBST] (Node
0xc37ad028)

Local Variables for method [GBST]:
    Local0: 0xc37f49a8 <Obj>             Integer        0       1
    Local1: 0xc38b0ea8 <Obj>             Integer        0    41cc
    Local2: 0xc37a8428 <Obj>             Integer        0    3e1c
    Local3: 0xc38b1028 <Obj>             Integer        0    2cda
    Local4: 0 <NullObj>
    Local5: 0 <NullObj>
    Local6: 0 <NullObj>
    Local7: 0xc38b0e28 <Obj>             Integer        0      5e

Arguments for Method [GBST]:  (4 arguments defined, max concurrency = ff)
    Arg0:   0xc38b0d28 <Obj>             Integer        0       0
    Arg1:   0xc38b0da8 <Obj>             Integer        0      42
    Arg2:   0xc37f45a8 <Obj>             Integer        0       1
    Arg3:   0xc37da528 <Obj>             Package 0xc37da528
    Arg4:   0 <NullObj>
    Arg5:   0 <NullObj>
    Arg6:   0 <NullObj>

 psparse-1287: *** Error: Method execution failed [\\_SB_.PCI0.LPC_.EC__.GBST]
(Node 0xc37ad028), AE_AML_INTERNAL
 psparse-1287: *** Error: Method execution failed
[\\_SB_.PCI0.LPC_.EC__.BAT0._BST] (Node 0xc37ac928), AE_AML_INTERNAL
Comment 15 Robert Moore 2004-01-30 14:28:54 UTC
The problem above is a problem with doing an Index on a package that is an 
Arg.  I don't think that we've gotten this far before, so this is a new issue.
Bob
Comment 16 Len Brown 2004-01-30 14:38:26 UTC
Nate, the exstore.c part of your revert patch is actually from bug 1791, not 
this one.
Comment 17 Alessandro Suardi 2004-01-30 15:48:43 UTC
As per Len's request, the 2004-01-29 @ 22:35 patch successfully fixes my oops
 on boot with a div-by-zero error in cpufreq code; I'm now running that patch
 on top of 2.6.2-rc2-bk4.

For reference, I originally notified the lkml with this message:

http://www.ussg.iu.edu/hypermail/linux/kernel/0312.3/0442.html

Thanks !
Comment 18 Valdis Kletnieks 2004-02-02 10:57:32 UTC
The patch in http://bugzilla.kernel.org/attachment.cgi?id=1774&action=view is
sufficient to fix the incorrect values for batteries on my Dell Latitude C840
(BIOS A12, if it matters).
Comment 19 Bill Chmura 2004-02-02 15:57:34 UTC
I tried this patch on 2.6.2-rc3 as requested to resolve my problem originally
reported under #1759.  This did not fix my problem, or change the error message
I am recieving under bootup.  I tried patch number 5 in the above list (Bobs
suggested fix (linux)).
Comment 20 Joseph 2004-02-02 16:12:45 UTC
Patch (http://bugzilla.kernel.org/attachment.cgi?id=1965&action=view) works to
make 2.6.2-rc3 recognize my battery status (Dell Inspiron 8600).  Thought
posting should go here too (has been sent to LKML).
Comment 21 Nate Lawson 2004-02-02 16:14:06 UTC
Len, you are correct.  The workaround in this bug proposed by Luming is for
Dells.  However, that workaround broke GBST on my Thinkpad.  So I thought that
Bob's fix would work on both Dells and Thinkpads without needing the workaround
in bug 1791.  However, I was not correct.  It appears Bob's fix still needs the
workaround in bug 1791 to work on Thinkpads.  I will forward this bug to some
Dell users and see if it works for them.
Comment 22 Rudolf Koenig 2004-02-03 02:04:50 UTC
Patch (http://bugzilla.kernel.org/attachment.cgi?id=1965&action=view) works for
2.4.25-pre8 on a Dell Inspiron 8600 also. Whithout it you get battery and
processor files full of 0's.

Comment 23 Len Brown 2004-02-03 09:36:31 UTC
From: 	Valdis.Kletnieks@vt.edu 
 
> Any chance you can test the later patch in this bug? 
> http://bugzilla.kernel.org/attachment.cgi?id=1965&action=view 
 
The patch in 1965 produces sane results in /proc/acpi/battery/BAT?/* as 
well.  I looked around in /proc/acpi/* and the dmesg output and found 
no major regressions. 
Comment 24 Robert Moore 2004-02-11 15:08:09 UTC
Patch integrated into CA version 20040211

Note You need to log in before you can comment on or make changes to this bug.