Date:   Tue, 6 Jun 2023 10:27:38 -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/5/2023 1:33 PM, Limonciello, Mario wrote:
>
> 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.
I did confirm with internal team that this issue also reproduces on S3 and
this patch fixes S3 case 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