Re: [PATCH v19 04/18] PCI/TPH: Refactor pcie_enable_tph & add explicit requester variant

[Alex Williamson <alex@shazbot.org>]

On Thu, 2 Jul 2026 20:42:10 +0800
Chengwen Feng <fengchengwen@huawei.com> wrote:

> Refactor pcie_enable_tph implementation: extract core logic into static
> internal enable_tph() helper accepting explicit requester type.
> 
> - Preserve original pcie_enable_tph() unchanged as auto wrapper; it
>   auto-selects EXT/standard TPH requester per device capability, existing
>   bnxt/mlx5 callers require zero modification.
> - Add exported pcie_enable_tph_explicit() with bool 'extended' parameter
>   for explicit STD/EXT selection, used by upcoming VFIO TPH support.
> 
> Input validation for EXT_TPH availability is retained inside helper to
> reject invalid explicit EXT request if hardware does not support extended
> requester.

I'll leave final judgment on the APi to Bjorn, but I'd tend towards
defining tph_mode and tph_req_type as enums, store the max available
tph_req_type on the pci_dev.tph_max_type, and have the base function
take exactly those enums as args, get rid of the bool wrapper.  Then the
existing function just becomes:

int pcie_enable_tph(struct pci_dev *pdev, int mode)
{
	return pcie_enable_tph_type(pdev, (enum tph_mode)mode,
				    pdev->tph_max_type);
}

(Given the 2 existing callers, this could also be easily changed to
take enum tph_mode as the arg as well)

This might also disambiguate how this series conflates tph_cap to mean
TPH capability is not present or not available to instead retain
tph_cap as simply the offset of the capability and (tph_max_type ==
PCI_TPH_REQ_DISABLE) means it's not available.

Validation in the base function is also simplified:

	if (tph_req_type == PCI_TPH_REQ_DISABLE ||
	    tph_req_type > pdev->tph_max_type)
		return -EINVAL;

Same comment for patch 5, expose the base function as "_type" with the
enum tph_req_type (the precedent is already there for enum
tph_mem_type).  The wrapper for the existing interface is the same as
you have, the base function adds the same test as above, modulo
s/tph_max_type/tph_req_type/.  Thanks,

Alex
 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  drivers/pci/tph.c       | 68 +++++++++++++++++++++++++++++------------
>  include/linux/pci-tph.h |  4 +++
>  2 files changed, 52 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
> index 4097787ea98a..c22eb7f4b0bd 100644
> --- a/drivers/pci/tph.c
> +++ b/drivers/pci/tph.c
> @@ -363,23 +363,7 @@ void pcie_disable_tph(struct pci_dev *pdev)
>  }
>  EXPORT_SYMBOL(pcie_disable_tph);
>  
> -/**
> - * pcie_enable_tph - Enable TPH support for device using a specific ST mode
> - * @pdev: PCI device
> - * @mode: ST mode to enable. Current supported modes include:
> - *
> - *   - PCI_TPH_ST_NS_MODE: NO ST Mode
> - *   - PCI_TPH_ST_IV_MODE: Interrupt Vector Mode
> - *   - PCI_TPH_ST_DS_MODE: Device Specific Mode
> - *
> - * Check whether the mode is actually supported by the device before enabling
> - * and return an error if not. Additionally determine what types of requests,
> - * TPH or extended TPH, can be issued by the device based on its TPH requester
> - * capability and the Root Port's completer capability.
> - *
> - * Return: 0 on success, otherwise negative value (-errno)
> - */
> -int pcie_enable_tph(struct pci_dev *pdev, int mode)
> +static int enable_tph(struct pci_dev *pdev, int mode, u8 req_type)
>  {
>  	u32 reg;
>  	u8 dev_modes;
> @@ -400,10 +384,11 @@ int pcie_enable_tph(struct pci_dev *pdev, int mode)
>  	if (!((1 << mode) & dev_modes))
>  		return -EINVAL;
>  
> -	pdev->tph_mode = mode;
> +	if (req_type == PCI_TPH_REQ_EXT_TPH && !pdev->tph_ext_support)
> +		return -EINVAL;
>  
> -	pdev->tph_req_type = pdev->tph_ext_support ? PCI_TPH_REQ_EXT_TPH :
> -						     PCI_TPH_REQ_TPH_ONLY;
> +	pdev->tph_mode = mode;
> +	pdev->tph_req_type = req_type;
>  
>  	/* Write them into TPH control register */
>  	pci_read_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, &reg);
> @@ -417,8 +402,51 @@ int pcie_enable_tph(struct pci_dev *pdev, int mode)
>  
>  	return 0;
>  }
> +
> +/**
> + * pcie_enable_tph - Enable TPH support for device using a specific ST mode
> + * @pdev: PCI device
> + * @mode: ST mode to enable. Current supported modes include:
> + *
> + *   - PCI_TPH_ST_NS_MODE: NO ST Mode
> + *   - PCI_TPH_ST_IV_MODE: Interrupt Vector Mode
> + *   - PCI_TPH_ST_DS_MODE: Device Specific Mode
> + *
> + * Check whether the mode is actually supported by the device before enabling
> + * and return an error if not. Additionally determine what types of requests,
> + * TPH or extended TPH, can be issued by the device based on its TPH requester
> + * capability and the Root Port's completer capability.
> + *
> + * Return: 0 on success, otherwise negative value (-errno)
> + */
> +int pcie_enable_tph(struct pci_dev *pdev, int mode)
> +{
> +	u8 req_type = pdev->tph_ext_support ? PCI_TPH_REQ_EXT_TPH :
> +					      PCI_TPH_REQ_TPH_ONLY;
> +	return enable_tph(pdev, mode, req_type);
> +}
>  EXPORT_SYMBOL(pcie_enable_tph);
>  
> +/**
> + * pcie_enable_tph_explicit - Enable TPH with explicit requester selection
> + * @pdev: PCI device to operate
> + * @mode: ST table operating mode (NS/IV/DS)
> + * @extended: true = EXT_TPH, false = standard TPH only
> + *
> + * Unlike auto-detecting pcie_enable_tph(), caller selects requester type
> + * manually instead of hardware auto-selection. Rejects EXT_TPH request
> + * if device lacks extended requester capability.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int pcie_enable_tph_explicit(struct pci_dev *pdev, int mode, bool extended)
> +{
> +	u8 req_type = extended ? PCI_TPH_REQ_EXT_TPH : PCI_TPH_REQ_TPH_ONLY;
> +
> +	return enable_tph(pdev, mode, req_type);
> +}
> +EXPORT_SYMBOL(pcie_enable_tph_explicit);
> +
>  void pci_restore_tph_state(struct pci_dev *pdev)
>  {
>  	struct pci_cap_saved_state *save_state;
> diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
> index 6f02b020d7d7..ca0faa98afac 100644
> --- a/include/linux/pci-tph.h
> +++ b/include/linux/pci-tph.h
> @@ -29,6 +29,7 @@ int pcie_tph_get_cpu_st(struct pci_dev *dev,
>  			unsigned int cpu, u16 *tag);
>  void pcie_disable_tph(struct pci_dev *pdev);
>  int pcie_enable_tph(struct pci_dev *pdev, int mode);
> +int pcie_enable_tph_explicit(struct pci_dev *pdev, int mode, bool extended);
>  u16 pcie_tph_get_st_table_size(struct pci_dev *pdev);
>  u32 pcie_tph_get_st_table_loc(struct pci_dev *pdev);
>  #else
> @@ -42,6 +43,9 @@ static inline int pcie_tph_get_cpu_st(struct pci_dev *dev,
>  static inline void pcie_disable_tph(struct pci_dev *pdev) { }
>  static inline int pcie_enable_tph(struct pci_dev *pdev, int mode)
>  { return -EINVAL; }
> +static inline int pcie_enable_tph_explicit(struct pci_dev *pdev, int mode,
> +					   bool extended)
> +{ return -EINVAL; }
>  static inline u16 pcie_tph_get_st_table_size(struct pci_dev *pdev)
>  { return 0; }
>  static inline u32 pcie_tph_get_st_table_loc(struct pci_dev *pdev)