Bug 206859 - pci_acpi_optimize_delay() doesn't update d3 delay, unexpected result from acpi_evaluate_dsm()
Summary: pci_acpi_optimize_delay() doesn't update d3 delay, unexpected result from acp...
Status: CLOSED INVALID
Alias: None
Product: ACPI
Classification: Unclassified
Component: Other (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Zhang Rui
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-16 07:02 UTC by Kai-Heng Feng
Modified: 2021-03-22 12:31 UTC (History)
4 users (show)

See Also:
Kernel Version: mainline
Subsystem:
Regression: No
Bisected commit-id:


Attachments
dmesg with debug message (88.54 KB, text/plain)
2020-03-16 07:04 UTC, Kai-Heng Feng
Details
acpidump (2.19 MB, text/plain)
2020-03-16 07:06 UTC, Kai-Heng Feng
Details

Description Kai-Heng Feng 2020-03-16 07:02:06 UTC
To avoid D3cold delay after S3 resume, we can use the _DSM value to achieve faster resume time.

However the _DSM doesn't seem to be evaluated correctly.
Comment 1 Kai-Heng Feng 2020-03-16 07:04:56 UTC
Created attachment 287943 [details]
dmesg with debug message

logged with the following diff:

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 0c02d500158f..b76daa330178 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1197,6 +1197,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
        if (!obj)
                return;
 
+       pci_info(pdev, "DEBUG: %s %d %d\n", __func__, obj->type, obj->package.count);
        if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
                elements = obj->package.elements;
                if (elements[0].type == ACPI_TYPE_INTEGER) {
Comment 2 Kai-Heng Feng 2020-03-16 07:06:30 UTC
Created attachment 287945 [details]
acpidump
Comment 3 Kai-Heng Feng 2020-03-16 07:10:47 UTC
Take 00:01.0 as example, the _DSM looks good however we doesn't get what we expected from the method:
dsdt.dsl:
                Device (PEG0)
                {
                    Name (_ADR, 0x00010000)  // _ADR: Address
                    Device (PEGP)
                    {
                        Name (_ADR, Zero)  // _ADR: Address
                    }
                }


ssdt10.dsl:
    Scope (\_SB.PCI0.PEG0)
    {
        Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
        {
            If ((Arg0 == ToUUID ("e5c937d0-3553-4d7a-9117-ea4d19c3434d") /* Device Labeling Interface */))
            {
                Switch (ToInteger (Arg2))
                {
                    Case (Zero)
                    {
                        Name (DSMF, Buffer (0x02)
                        {
                             0x00, 0x00                                       // ..
                        })
                        CreateBitField (DSMF, Zero, FUN0)
                        CreateBitField (DSMF, 0x04, FUN4)
                        CreateBitField (DSMF, 0x06, FUN6)
                        CreateBitField (DSMF, 0x08, FUN8)
                        CreateBitField (DSMF, 0x09, FUN9)
                        CreateBitField (DSMF, 0x0A, FUNA)
                        CreateBitField (DSMF, 0x0B, FUNB)
                        FUN0 = One
                        If ((Arg1 >= 0x02))
                        {
                            If (LTRS)
                            {
                                FUN6 = One
                            }

                            If (OBFS)
                            {
                                FUN4 = One
                            }
                        }

                        If ((Arg1 >= 0x03))
                        {
                            If (ECR1)
                            {
                                FUN8 = One
                            }

                            If (ECR1)
                            {
                                FUN9 = One
                            }
                        }

                        If ((Arg1 >= 0x04))
                        {
                            If (CondRefOf (PPBA))
                            {
                                FUNA = One
                            }

                            FUNB = One
                            FUNB = One
                        }

                        Return (DSMF) /* \_SB_.PCI0.PEG0._DSM.DSMF */
                    }
                    Case (0x04)
                    {
                        If ((Arg1 >= 0x02))
                        {
                            If (OBFS)
                            {
                                Return (Buffer (0x10)
                                {
                                    /* 0000 */  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,  // ........
                                    /* 0008 */  0x00, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00   // ........
                                })
                            }
                            Else
                            {
                                Return (Buffer (0x10)
                                {
                                    /* 0000 */  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,  // ........
                                    /* 0008 */  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00   // ........
                                })
                            }
                        }
                    }
                    Case (0x06)
                    {
                        If ((Arg1 >= 0x02))
                        {
                            If (LTRS)
                            {
                                LTRV [Zero] = ((SMSL >> 0x0A) & 0x07)
                                LTRV [One] = (SMSL & 0x03FF)
                                LTRV [0x02] = ((SNSL >> 0x0A) & 0x07)
                                LTRV [0x03] = (SNSL & 0x03FF)
                                Return (LTRV) /* \_SB_.PCI0.PEG0.LTRV */
                            }
                            Else
                            {
                                Return (Zero)
                            }
                        }
                    }
                    Case (0x08)
                    {
                        If ((ECR1 == One))
                        {
                            If ((Arg1 >= 0x03))
                            {
                                Return (One)
                            }
                        }
                    }
                    Case (0x09)
                    {
                        If ((ECR1 == One))
                        {
                            If ((Arg1 >= 0x03))
                            {
                                Return (Package (0x05)
                                {
                                    0xC350,
                                    Ones,
                                    Ones,
                                    0xC350,
                                    Ones
                                })
                            }
                        }
                    }
                    Case (0x0A)
                    {
                        If (CondRefOf (PPBA))
                        {
                            Return (PPBA (Arg3))
                        }
                    }
                    Case (0x0B)
                    {
                        Return (UPRD (Arg3))
                    }

                }
            }

            Return (Buffer (One)
            {
                 0x00                                             // .
            })
        }
Comment 4 Zhang Rui 2020-11-19 04:13:16 UTC
I think PCIC and PCID are key ACPI methods to handle this, on this platform.
If PCID is invoked, it seems that we will return a package with 5 elements, because
Arg0 is the UUID, Arg1 is 3, Arg2 is DSM_PCI_DEVICE_READINESS_DURATIONS, which is 9, right?
So can we check if PCID is invoked or not?
If not, it is really possible that ECR1 is not set in this BIOS, which results in PCIC returns false, thus PCID will not be invoked. And this also applies to the _DSM method for device 00:01.0.

            Method (PCID, 4, Serialized)
            {
                If ((Arg0 == PCIG))
                {
                    If ((Arg1 >= 0x03))
                    {
                        If ((Arg2 == Zero))
                        {
                            Return (Buffer (0x02)
                            {
                                 0x01, 0x03                                       // ..
                            })
                        }

                        If ((Arg2 == 0x08))
                        {
                            Return (One)
                        }

                        If ((Arg2 == 0x09))
                        {
                            Return (Package (0x05)
                            {
                                0xC350, 
                                Ones, 
                                Ones, 
                                0xC350, 
                                Ones
                            })
                        }
                    }
                }

                Return (Buffer (One)
                {
                     0x00                                             // .
                })
            }
Comment 5 Zhang Rui 2021-03-19 07:27:22 UTC
kai-heng, is this still a problem?
Comment 6 Zhang Rui 2021-03-19 07:27:53 UTC
and it would be good to know what model this platform is.
Comment 7 Kai-Heng Feng 2021-03-19 07:29:52 UTC
Sorry for the belated response.

Yes I think it's still is. Let me get back to you when I find the platform in our lab.
Comment 8 Kai-Heng Feng 2021-03-22 12:31:39 UTC
Ok, I think that platform is designed that way, because I found another platform that has the right value from _DSM. Let's close it.

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