On 9/23/24 02:43, Lukas Wunner wrote:

On Mon, Sep 16, 2024 at 03:50:59PM -0500, Wei Huang wrote:

quoted

--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c

@@ -1813,6 +1813,7 @@ int pci_save_state(struct pci_dev *dev)
 	pci_save_dpc_state(dev);
 	pci_save_aer_state(dev);
 	pci_save_ptm_state(dev);
+	pci_save_tph_state(dev);
 	return pci_save_vc_state(dev);
 }
 EXPORT_SYMBOL(pci_save_state);

@@ -1917,6 +1918,7 @@ void pci_restore_state(struct pci_dev *dev)
 	pci_restore_vc_state(dev);
 	pci_restore_rebar_state(dev);
 	pci_restore_dpc_state(dev);
+	pci_restore_tph_state(dev);
 	pci_restore_ptm_state(dev);
 
 	pci_aer_clear_status(dev);

I'm wondering if there's a reason to use a different order on save versus
restore?  E.g. does PTM need to be restored last?

Will fix

quoted

--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig

@@ -155,3 +155,14 @@ config PCIE_EDR
 	  the PCI Firmware Specification r3.2.  Enable this if you want to
 	  support hybrid DPC model which uses both firmware and OS to
 	  implement DPC.
+
+config PCIE_TPH
+	bool "TLP Processing Hints"
+	depends on ACPI

TPH isn't really an ACPI-specific feature, it could exist on
devicetree-based platforms as well.  I think there could be valid
use cases for enabling TPH support on such platforms:

E.g. the platform firmware or bootloader might set up the TPH Extended
Capability in a specific way and the kernel would have to save/restore
it on system sleep.

So I'd recommend removing this dependency.

This is reasonable - I can remove this dependency.

Note that there's a static inline for acpi_check_dsm() which returns
false if CONFIG_ACPI=n, so tph_invoke_dsm() returns AE_ERROR and
pcie_tph_get_cpu_st() returns -EINVAL.  It thus looks like you may not
even need an #ifdef.

We might have to add #ifdef around the ACPI related functions. The
reason is not because of acpi_evaluate_dsm() or acpi_evaluate_dsm().
Instead the compilation will fail due to "pci_acpi_dsm_guid". In TPH V2
series, somebody reported the following error:

"
This seems to break builds on ARM (32bit) with multi_v7_defconfig.

  .../tph.c:221:39: error: use of undeclared identifier 'pci_acpi_dsm_guid'
  221 |         out_obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid,
MIN_ST_DSM_REV,
      |
"

quoted

diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
new file mode 100644

The PCIe features added most recently (such as DOE) have been placed
directly in drivers/pci/ instead of the pcie/ subdirectory.
The pcie/ subdirectory mostly deals with port drivers.
So perhaps tph.c should likewise be placed in drivers/pci/ ?

I am OK with it. Some extended features, such as ATS, are indeed
implemented in drivers/pci/.

Bjorn: Any comments on this idea?

quoted

--- /dev/null
+++ b/drivers/pci/pcie/tph.c

@@ -0,0 +1,199 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TPH (TLP Processing Hints) support
+ *
+ * Copyright (C) 2024 Advanced Micro Devices, Inc.
+ *     Eric Van Tassell <Eric.VanTassell@amd.com>
+ *     Wei Huang <wei.huang2@amd.com>
+ */
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>

This patch doesn't seem to use any of the symbols defined in pci-acpi.h,
or did I miss anything?  I'd move the inclusion of pci-acpi.h to the patch
that actually uses its symbols.

Thanks,

Lukas