Re: [PATCH] PCI: Call _REG when saving/restoring PCI state

[Rafael J. Wysocki]

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.

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

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?

> 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?

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

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

> 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?