Re: [PATCH v12 4/6] PCI/TPH: Move tph_req_type initialization into pci_tph_init

[Alex Williamson <alex@shazbot.org>]

On Tue, 26 May 2026 12:08:28 +0800
Chengwen Feng <fengchengwen@huawei.com> wrote:

> Relocate tph_req_type resolution logic from pcie_enable_tph() to
> pci_tph_init(). The request type is fixed per device and root port topology
> at probe time, no need recalculation on each TPH enable.
> 
> Also drop redundant tph_req_type reset in pcie_disable_tph(), the value
> remains valid across disable/enable cycles.
> 
> This change allows pcie_tph_get_cpu_st() to work properly and retrieve
> valid steering tag values even when TPH is not enabled on the device.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  drivers/pci/tph.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
> index 95e2a95055ee..3660ad5d3623 100644
> --- a/drivers/pci/tph.c
> +++ b/drivers/pci/tph.c
> @@ -371,7 +371,6 @@ void pcie_disable_tph(struct pci_dev *pdev)
>  	pci_write_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, 0);
>  
>  	pdev->tph_mode = 0;
> -	pdev->tph_req_type = 0;
>  	pdev->tph_enabled = 0;
>  }
>  EXPORT_SYMBOL(pcie_disable_tph);
> @@ -396,7 +395,6 @@ int pcie_enable_tph(struct pci_dev *pdev, int mode)
>  {
>  	u32 reg;
>  	u8 dev_modes;
> -	u8 rp_req_type;
>  
>  	/* Honor "notph" kernel parameter */
>  	if (pci_tph_disabled)
> @@ -416,21 +414,6 @@ int pcie_enable_tph(struct pci_dev *pdev, int mode)
>  
>  	pdev->tph_mode = mode;
>  
> -	/* Get req_type supported by device and its Root Port */
> -	pci_read_config_dword(pdev, pdev->tph_cap + PCI_TPH_CAP, &reg);
> -	if (FIELD_GET(PCI_TPH_CAP_EXT_TPH, reg))
> -		pdev->tph_req_type = PCI_TPH_REQ_EXT_TPH;
> -	else
> -		pdev->tph_req_type = PCI_TPH_REQ_TPH_ONLY;
> -
> -	/* Check if the device is behind a Root Port */
> -	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END) {
> -		rp_req_type = get_rp_completer_type(pdev);
> -
> -		/* Final req_type is the smallest value of two */
> -		pdev->tph_req_type = min(pdev->tph_req_type, rp_req_type);
> -	}
> -
>  	if (pdev->tph_req_type == PCI_TPH_REQ_DISABLE)
>  		return -EINVAL;
>  
> @@ -538,11 +521,27 @@ void pci_tph_init(struct pci_dev *pdev)
>  {
>  	int num_entries;
>  	u32 save_size;
> +	u8 rp_req_type;
> +	u32 reg = 0;
>  
>  	pdev->tph_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_TPH);
>  	if (!pdev->tph_cap)
>  		return;
>  
> +	/* Get req_type supported by device and its Root Port */
> +	pci_read_config_dword(pdev, pdev->tph_cap + PCI_TPH_CAP, &reg);
> +	if (FIELD_GET(PCI_TPH_CAP_EXT_TPH, reg))
> +		pdev->tph_req_type = PCI_TPH_REQ_EXT_TPH;
> +	else
> +		pdev->tph_req_type = PCI_TPH_REQ_TPH_ONLY;
> +
> +	/* Check if the device is behind a Root Port */
> +	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END) {
> +		rp_req_type = get_rp_completer_type(pdev);
> +		/* Final req_type is the smallest value of two */
> +		pdev->tph_req_type = min(pdev->tph_req_type, rp_req_type);
> +	}
> +
>  	num_entries = pcie_tph_get_st_table_size(pdev);
>  	save_size = sizeof(u32) + num_entries * sizeof(u16);
>  	pci_add_ext_cap_save_buffer(pdev, PCI_EXT_CAP_ID_TPH, save_size);


There's a virtualization problem hidden here that we haven't discussed
yet.  tph_req_type goes on to define how pcie_enable_tph(),
pcie_tph_get_cpu_st(), and pcie_tph_set_st_entry work relative to
standard or extended TPH.  The user has no visibility or control of
whether these interfaces enable extended TPH mode or get/set extended
TPH values.

That not only means that the virtualization of the TPH capability is
broken (user writes one value to TPH Requester Enable, sees another),
but I think it also breaks DS mode if the device implementation that
depends on the width, and also breaks interoperability with Zhiping's
series.  The user doesn't know whether to take the 8-bit or 16-bit
steering tags.  In fact, the kernel's mode selection only considers the
path to the root port completer and not to other devices and could
enable TPH between devices in incompatible modes.

Minimally it seems the Extended TPH Requester Supported register needs
to be virtualized, restricting it based on the topology support, and
the user written operating mode needs to be honored, but that also
presents a challenge in how we represent/interpret the steering tags
to/from the user.  Thanks,

Alex