Message-ID: <f8b5ed80-62b2-3a00-d036-08bf1ef6a7dd@amd.com>
Date:   Mon, 5 Jun 2023 13:33:03 -0500
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Bjorn Helgaas <helgaas@...nel.org>, bhelgaas@...gle.com,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org
Subject: Re: [PATCH] PCI: Call _REG when saving/restoring PCI state


On 6/4/2023 6:30 AM, Rafael J. Wysocki wrote:
> On Fri, Jun 2, 2023 at 11:57 PM Limonciello, Mario
> <mario.limonciello@....com> wrote:
>>
>> On 6/2/2023 3:21 PM, Bjorn Helgaas wrote:
>>> [+cc Rafael, Len, linux-acpi]
>>>
>>> Hi Mario,
>>>
>>> On Thu, Jun 01, 2023 at 10:11:22PM -0500, Mario Limonciello wrote:
>>>> ASMedia PCIe GPIO controllers connected to AMD SOC fail functional tests
>>>> after returning from s2idle. This is because the BIOS checks whether the
>>>> OSPM has called the _REG method to determine whether it can interact with
>>>> the OperationRegion assigned to the device.
>>> "s2idle" is a Linux term; I'd prefer something that we can relate to
>>> the ACPI spec.
>> It's important for the symptoms of this issue though, this
>> problem doesn't trigger "just" by moving D-states.
>>
>> It happens as a result of system suspend.
> As I said in my response to Bjorn, s2idle is D0 from the ACPI
> standpoint.  It is not a system sleep and it has no special meaning in
> ACPI.
>
> The problem seems to be related to the low-power S0 idle _DSM calls to me.

This particular hardware that triggered this patch can do S3
or s2idle.

Let me confirm with internal guys whether this can reproduce
with BIOS configured to S3 as well.
>>> Maybe a pointer to the specific function in the driver that has a
>>> problem?  Based on the patch, I assume the driver uses some control
>>> method that looks at PCI config space?
>> The issue isn't in anything Linux code "does"; it's in the "lack"
>> of Linux code doing what it needs to IE using _REG.
> So the argument seems to be that under certain conditions the PCI
> config space becomes unavailable and so _REG(dev, 0) needs to be
> called when this is about to happen and _REG(dev, 1) needs to be
> called when the config space becomes available again.  Fair enough,
> but I'm not sure why this is limited to system suspend and resume.
I didn't think it should be limited to suspend/resume
either.

That's why I had put it in save state/restore state.

> Moreover, "PCI_Config operation regions on a PCI root bus containing a
> _BBN object" are specifically mentioned as one of the cases when _REG
> need not be evaluated at all.  I guess the operation region in
> question doesn't fall into that category?

Yes; that's right.  _BBN is only present on \_SB_.PCI0
and the problematic device is on \_SB_.PCI0.GPP5.

>> At least for this issue _REG is treated like a lock mechanism.
>> In the spec it says specifically:
>>
>> "When an operation region handler is unavailable, AML cannot access
>> data fields in that region".
>>
>> That is it's to ensure that OSPM and AML don't both simultaneously
>> access the same region.
>>
>> What happens is that AML normally wants to access this region during
>> suspend, but without the sequence of calling _REG it can't.
> Is this about being unable to access the opregion or racing with
> concurrent accesses on the OS side?
Access.
>
>>>> To fix this issue, call acpi_evaluate_reg() when saving and restoring the
>>>> state of PCI devices.
>>> Please include the spec citation: ACPI r6.5, sec 6.5.4.  The URL has
>>> changed in the past and may change in the future, but the name/section
>>> number will not.
>> Sure.
>>>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>>> ---
>>>>    drivers/pci/pci.c | 12 ++++++++++++
>>>>    1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index e38c2f6eebd4..071ecba548b0 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -1068,6 +1068,12 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
>>>>       return acpi_pci_bridge_d3(dev);
>>>>    }
>>>>
>>>> +static inline int platform_toggle_reg(struct pci_dev *dev, int c)
>>>> +{
>>>> +    return acpi_evaluate_reg(ACPI_HANDLE(&dev->dev),
>>>> +                             ACPI_ADR_SPACE_PCI_CONFIG, c);
>>>> +}
>>> You never check the return value, so why return it?
>> _REG isn't mandatory for any of these uses, and I wanted to make
>> sure that if it does end up being mandatory in a future use that
>> the return code wasn't thrown away.  If you think it's better to
>> just throw it away now, I have no qualms making it a void instead.
> I don't think it can reasonably become mandatory without adding a
> specific _OSC bit for that.
OK.
>
>>> The function actually doesn't *toggle*; it connects or disconnects
>>> based on "c".
>> Can you suggest a better function name?
>>> This looks like it only builds when CONFIG_ACPI=y?
>> The prototype for acpi_evaluate_reg isn't guarded by CONFIG_ACPI
>> so I figured it worked both ways.
>>
>> But looking again I don't see a dummy implementation version for
>> the lack of CONFIG_ACPI, so I'll double check it.
>>
>>>>    /**
>>>>     * pci_update_current_state - Read power state of given device and cache it
>>>>     * @dev: PCI device to handle.
>>>> @@ -1645,6 +1651,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev)
>>>>    int pci_save_state(struct pci_dev *dev)
>>>>    {
>>>>       int i;
>>>> +
>>>> +    platform_toggle_reg(dev, ACPI_REG_DISCONNECT);
>>> I would expect these to be in the PM code near the power state
>>> transitions, not in the state save/restore code.  These functions
>>> *are* used during suspend/resume, but are used in other places as
>>> well, where we probably don't want _REG executed.
>>>
>>> Cc'd Rafael and PM folks, who can give much better feedback.
>> My knee jerk reaction when we found the root cause for this issue
>> was to put the code right around the D-state transitions, but I
>> decided against this.
>>
>> I put it in save/restore intentionally because
>> like I mentioned above it's treated like a locking mechanism between
>> OSPM and AML and it's not functionally tied to a D-state transition.
>>
>> When the state is saved it's like Linux says
>> "I'm done using this region, go ahead and touch it firmware".
>> When it's restored it's like Linux says
>> "Don't use that region, I'm claiming it for now".
> So it looks like you want to use _REG for protecting PCI config space
> against concurrent accesses from AML and the OS.
Yeah.  When I discussed it with BIOS guys they
explained to me that the BIOS will typically save/restore the
PCIe device BAR when _REG is called (depending on the argument
to _REG).

They'll only operate on the region when it's in the right
state, and they'll restore it as necessary when OSPM would use
it again.

This is also how Windows works.

>> Think about that other patch I wrote recently that controls D3
>> availability [1].  If it was only run in the D-state transitions and
>> the root port stays in D0 but has a _REG method it would never get
>> called.
> And why should it be evaluated in that case?
No matter what the actual D-state is the OSPM isn't
accessing it anymore, so AML should be able to.

Powered by blists - more mailing lists