Re: [PATCH V5 1/5] PCI: Add TLP Processing Hints (TPH) support
From: Lukas Wunner <lukas@wunner.de>
Date: 2024-09-23 07:54:38
Also in:
linux-doc, linux-pci, lkml
On Mon, Sep 16, 2024 at 03:50:59PM -0500, Wei Huang wrote:
quoted hunk ↗ jump to hunk
--- 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?
quoted hunk ↗ jump to hunk
--- 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.
quoted hunk ↗ jump to hunk
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/ ?
quoted hunk ↗ jump to hunk
--- /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