On 9/23/24 02:43, Lukas Wunner wrote:
> 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?
>
>
Will fix
>> --- 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,
|
"
>
>
>> 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?
>
>
>> --- /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
