Re: [PATCH v20 10/16] PCI/TPH: Add pcie_tph_set_st_entries() batch ST entry programming helper

[sashiko-bot@kernel.org]

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

New issues:
- [High] API return value collision: PCIBIOS error codes masquerade as successful batch writes.
- [High] Partial success return value hides fatal global TPH disablement.
- [Medium] Calling the batch function with count=0 disrupts hardware TPH capability state.

Pre-existing issues:
- [High] Integer overflow in bounds check allows out-of-bounds config space writes.
- [Medium] Lockless data races on PCI_TPH_CTRL during concurrent updates.
--

--- Patch [10]: [PATCH v20 10/16] PCI/TPH: Add pcie_tph_set_st_entries() batch ST entry programming helper ---
commit ef45067b72f46179b7e7c22243bc81d01093a5bd
Author: Chengwen Feng <fengchengwen@huawei.com>
Subject: PCI/TPH: Add pcie_tph_set_st_entries() batch ST entry programming helper

This patch introduces a batch variant for programming contiguous Steering Tag
(ST) table entries. It modifies the existing pcie_tph_set_st_entry to act as a
wrapper around the new pcie_tph_set_st_entries helper. The batch helper
toggles TPH once per batch for better performance.

> diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
> index 633dc6308b68c..8194f8b45e3c6 100644
> --- a/drivers/pci/tph.c
> +++ b/drivers/pci/tph.c

[ ... ]

> @@ -378,7 +376,7 @@ int pcie_tph_set_st_entry(struct pci_dev *pdev, unsigned int index, u16 tag)
>  
>  	/* No need to write tag if device is in "No ST Mode" */
>  	if (pdev->tph_mode == PCI_TPH_ST_NS_MODE)
> -		return 0;
> +		return count;
>  
>  	/*
>  	 * Disable TPH before updating ST to avoid potential instability as
>  	 * cautioned in PCIe r6.2, sec 6.17.3, "ST Modes of Operation"
>  	 */

[Severity: Medium]
Will this unnecessarily disrupt the hardware state if the function is called
with count = 0? Since the loop will be bypassed, TPH will be disabled and
immediately re-enabled without writing any tags.

>  	set_ctrl_reg_req_en(pdev, PCI_TPH_REQ_DISABLE);

[Severity: Medium]
This is a pre-existing issue, but can concurrent updates cause lockless data
races on the PCI_TPH_CTRL register?

If multiple threads attempt to program the ST table concurrently, they may
race in set_ctrl_reg_req_en() which performs a lockless read-modify-write.
Could one thread re-enable TPH while another is midway through writing its
entries?

>  
>  	loc = pcie_tph_get_st_table_loc(pdev);
> -
> -	switch (loc) {
> -	case PCI_TPH_LOC_MSIX:
> -		err = pci_msix_write_tph_tag(pdev, index, tag);
> -		break;
> -	case PCI_TPH_LOC_CAP:
> -		err = write_tag_to_st_table(pdev, index, tag);
> -		break;
> -	default:
> -		err = -EINVAL;
> -	}
> -
> -	if (err) {
> -		pcie_disable_tph(pdev);
> -		return err;
> +	for (i = 0; i < count; i++) {
> +		switch (loc) {
> +		case PCI_TPH_LOC_MSIX:
> +			err = pci_msix_write_tph_tag(pdev, start + i, tags[i]);
> +			break;
> +		case PCI_TPH_LOC_CAP:
> +			err = write_tag_to_st_table(pdev, start + i, tags[i]);

[Severity: High]
This is a pre-existing issue, but does this bounds check in
write_tag_to_st_table() have an integer overflow vulnerability?

write_tag_to_st_table(struct pci_dev *pdev, int index, u16 tag) {
    ...
    if (index >= st_table_size)
        return -ENXIO;
    ...
}

Since write_tag_to_st_table() takes a signed int for the index, and we are
passing unsigned int start + i, could a large start value overflow to a
negative integer? A negative index would bypass the bounds check and cause an
out-of-bounds config space write.

> +			break;
> +		default:
> +			err = -EINVAL;
> +		}
> +		if (err != 0) {
> +			pci_err(pdev, "failed to set steering tag: %s table, index=%u, tags=%u, ret=%d\n",
> +				(loc == PCI_TPH_LOC_MSIX) ? "MSI-X" : "ST",
> +				start + i, tags[i], err);
> +			pcie_disable_tph(pdev);
> +			return i > 0 ? i : err;

[Severity: High]
If write_tag_to_st_table() fails on the first iteration where i == 0, could it
return a positive error code from pci_write_config_word() such as
PCIBIOS_BAD_REGISTER_NUMBER?

Because the return statement yields err when i == 0, a positive error code
would be interpreted by the caller of pcie_tph_set_st_entries() as a
successful batch write count.

[Severity: High]
If the loop fails on an entry after the first, this calls
pcie_disable_tph(pdev) which disables TPH for the entire device. However,
returning i yields a positive partial success count to the caller.

Does this leave the caller assuming the first i entries are functional when
TPH has actually been completely disabled?

> +		}
>  	}

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