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

[Rafael J. Wysocki]

On Tue, Jun 6, 2023 at 6:23 PM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
>
> ASMedia PCIe GPIO controllers fail functional tests after returning from
> suspend (S3 or 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.
>
> As described in 6.5.4 in the APCI spec, `_REG` is used to inform the AML
> code on the availability of an operation region.
>
> To fix this issue, call acpi_evaluate_reg() when saving and restoring the
> state of PCI devices.
>
> 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>
> ---
> v1->v2:
>  * Handle case of no CONFIG_ACPI
>  * Rename function
>  * Update commit message
>  * Move ACPI calling code into pci-acpi.c instead
>  * Cite the ACPI spec
> ---
>  drivers/pci/pci-acpi.c | 10 ++++++++++
>  drivers/pci/pci.c      | 14 ++++++++++++++
>  drivers/pci/pci.h      |  2 ++
>  3 files changed, 26 insertions(+)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 1698205dd73c..abc8bcfc2c71 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1209,6 +1209,16 @@ void acpi_pci_remove_bus(struct pci_bus *bus)
>         acpi_pci_slot_remove(bus);
>  }
>
> +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable)
> +{
> +       int val = enable ? ACPI_REG_CONNECT : ACPI_REG_DISCONNECT;
> +       int ret = acpi_evaluate_reg(ACPI_HANDLE(&dev->dev),
> +                                   ACPI_ADR_SPACE_PCI_CONFIG, val);
> +       if (ret)
> +               pci_dbg(dev, "ACPI _REG %s evaluation failed (%d)\n",
> +                       val ? "connect" : "disconnect", ret);

s/val/enable/ ?

Then I don't have to remember that ACPI_REG_DISCONNECT is 0.

<bikeshedding>

I would call this function something like
acpi_pci_config_space_access(), because technically it is about
allowing AML to access the PCI configuration space.

</bikeshedding>

> +}
> +
>  /* ACPI bus type */
>
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e38c2f6eebd4..b2f1f603ec62 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1068,6 +1068,14 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
>         return acpi_pci_bridge_d3(dev);
>  }
>
> +static inline void platform_set_register_access(struct pci_dev *dev, bool en)
> +{
> +       if (pci_use_mid_pm())
> +               return;
> +
> +       acpi_pci_set_register_access(dev, en);
> +}
> +
>  /**
>   * pci_update_current_state - Read power state of given device and cache it
>   * @dev: PCI device to handle.
> @@ -1645,6 +1653,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev)
>  int pci_save_state(struct pci_dev *dev)
>  {
>         int i;
> +
> +       platform_set_register_access(dev, false);
> +
>         /* 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 +1801,8 @@ void pci_restore_state(struct pci_dev *dev)
>         pci_enable_acs(dev);
>         pci_restore_iov_state(dev);
>
> +       platform_set_register_access(dev, true);
> +
>         dev->state_saved = false;
>  }
>  EXPORT_SYMBOL(pci_restore_state);
> @@ -3203,6 +3216,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_set_register_access(dev, true);
>  }
>
>  static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop)
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index ffccb03933e2..78961505aae2 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -703,6 +703,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev);
>  int acpi_pci_wakeup(struct pci_dev *dev, bool enable);
>  bool acpi_pci_need_resume(struct pci_dev *dev);
>  pci_power_t acpi_pci_choose_state(struct pci_dev *pdev);
> +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable);
>  #else
>  static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe)
>  {
> @@ -742,6 +743,7 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
>  {
>         return PCI_POWER_ERROR;
>  }
> +static inline void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) {}
>  #endif
>
>  #ifdef CONFIG_PCIEASPM
> --
> 2.34.1
>