On Mon, Sep 16, 2024 at 03:50:59PM -0500, Wei Huang wrote: > --- 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? > --- 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. 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. > 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/ ? > --- /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@xxxxxxx> > + * Wei Huang <wei.huang2@xxxxxxx> > + */ > +#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
