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

[Rafael J. Wysocki]

On Fri, Jun 2, 2023 at 10:21 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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.

>From the ACPI spec viewpoint, s2idle is S0, so this means that _REG
needs to be called under specific conditions when the system is in S0.
I'm not sure if this is really ACPI-compliant.

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

That's true, but it has been promised to us that these URLs will be
stable for the foreseeable future.

Besides, the spec version and section information is there in the link itself.

> but the name/section number will not.
>
> > 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?
>
> The function actually doesn't *toggle*; it connects or disconnects
> based on "c".
>
> This looks like it only builds when CONFIG_ACPI=y?
>
> >  /**
> >   * 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.

Right, they are not PM-specific.

Also, it looks like this isn't really about system suspend, but about
the device (port?) going into D3(hot/cold?) and back into D0.

I'm not sure if the spec mandates the need to reevaluate _REG on
device power state transitions.

At least, it looks like the operation region becomes invalid when the
device goes into D3 and there is no provision in the spec I can recall
ATM to notify AML about that.

IOW, once _REG has been called, the operation region needs to stay
valid from the perspective of AML using it which I think is not the
case on the affected platform.

> >       /* XXX: 100% dword access ok here? */
> >       for (i = 0; i < 16; i++) {
> >               pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
> > @@ -1790,6 +1799,8 @@ void pci_restore_state(struct pci_dev *dev)
> >       pci_enable_acs(dev);
> >       pci_restore_iov_state(dev);
> >
> > +     platform_toggle_reg(dev, ACPI_REG_CONNECT);
> > +
> >       dev->state_saved = false;
> >  }
> >  EXPORT_SYMBOL(pci_restore_state);
> > @@ -3203,6 +3214,7 @@ void pci_pm_init(struct pci_dev *dev)
> >       pci_read_config_word(dev, PCI_STATUS, &status);
> >       if (status & PCI_STATUS_IMM_READY)
> >               dev->imm_ready = 1;
> > +     platform_toggle_reg(dev, ACPI_REG_CONNECT);
> >  }
> >
> >  static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop)
> > --