Re: [PATCH v20 01/16] PCI/TPH: Add requester/completer type helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The internal get_rp_completer_type() function fails to fold the reserved 0b10 encoding into NONE, which can lead to writing a reserved value to the device's TPH Control Register and causes pcie_tph_enabled_req_type() to return an undocumented reserved value.
--

--- Patch [1]: [PATCH v20 01/16] PCI/TPH: Add requester/completer type helpers ---
commit f0ced626715c0e05a18f4a1a24c4e5840b72c4a0
Author: Zhiping Zhang <zhipingz@meta.com>

    PCI/TPH: Add requester/completer type helpers
    
    Add pcie_tph_enabled_req_type() so drivers can query the enabled TPH
    requester mode without reaching into pci_dev internals.
    
    Add pcie_tph_completer_type() so drivers that publish TPH metadata for
    a device acting as a completer can gate on the "TPH Completer
    Supported" field of Device Capabilities 2 (bits 13:12,
    PCI_EXP_DEVCAP2_TPH_COMP_MASK) rather than reusing requester-side
    state. Fold the reserved 0b10 encoding into NONE so callers only see
    the defined values.

> diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
> index 655ffd60e62f6..e7693fd9d6763 100644
> --- a/drivers/pci/tph.c
> +++ b/drivers/pci/tph.c
> @@ -173,6 +173,51 @@ u32 pcie_tph_get_st_table_loc(struct pci_dev *pdev)
>  }
>  EXPORT_SYMBOL(pcie_tph_get_st_table_loc);
>  
> +/**
> + * pcie_tph_enabled_req_type - Return the device's enabled TPH requester type
> + * @pdev: PCI device to query
> + *
> + * Return: PCI_TPH_REQ_DISABLE, PCI_TPH_REQ_TPH_ONLY or PCI_TPH_REQ_EXT_TPH.
> + */
> +u8 pcie_tph_enabled_req_type(struct pci_dev *pdev)
> +{
> +	return pdev->tph_req_type;
> +}
> +EXPORT_SYMBOL(pcie_tph_enabled_req_type);

[Severity: High]
This is a pre-existing issue, but does pdev->tph_req_type correctly avoid
returning the reserved 0b10 encoding?

While pcie_tph_completer_type() added in this patch explicitly folds the
reserved 0b10 encoding into NONE, the pre-existing internal
get_rp_completer_type() function in drivers/pci/tph.c directly returns
the field value without folding the reserved state:

drivers/pci/tph.c:get_rp_completer_type() {
    ...
    return FIELD_GET(PCI_EXP_DEVCAP2_TPH_COMP_MASK, reg);
}

If a Root Port returns the reserved value 2 (0b10), pcie_enable_tph() handles
it like this:

drivers/pci/tph.c:pcie_enable_tph() {
    ...
    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);
    }
    ...
    FIELD_MODIFY(PCI_TPH_CTRL_REQ_EN_MASK, &reg, pdev->tph_req_type);
}

If the endpoint supports PCI_TPH_REQ_EXT_TPH (3), pdev->tph_req_type becomes
min(3, 2) = 2.

This bypassed value 2 is then written directly to the endpoint's TPH Requester
Enable field, which violates the PCIe specification and could cause undefined
hardware behavior. Furthermore, it causes this new pcie_tph_enabled_req_type()
function to leak this undocumented reserved value to external callers, which
contradicts the function documentation.

Should get_rp_completer_type() be updated to handle the reserved 0b10 encoding
in the same way as pcie_tph_completer_type()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714104325.54444-1-fengchengwen@huawei.com?part=1