Re: [PATCH V5 1/5] PCI: Add TLP Processing Hints (TPH) support

[Wei Huang <wei.huang2@amd.com>]

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@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