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@xxxxxxx> 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@xxxxxxx>
---
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.
