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

[Limonciello, Mario]

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.