Re: [PATCH RESEND v18 04/12] PCI/TPH: Refactor pcie_enable_tph & add explicit requester variant

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent execution of `pcie_tph_set_st_entry` can violate the PCIe spec and corrupt hardware state by modifying the ST table while TPH is globally enabled.
--

commit e3b30f139c34eadf8039e394826ffa947655fe86
Author: Chengwen Feng <fengchengwen@huawei.com>

PCI/TPH: Refactor pcie_enable_tph & add explicit requester variant

This commit refactors the pcie_enable_tph implementation by extracting the
core logic into a static internal enable_tph helper that accepts an explicit
requester type. It preserves the original pcie_enable_tph behavior as an
auto-detecting wrapper and adds a new pcie_enable_tph_explicit function
for explicit standard or extended TPH selection.

> diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
> index 4097787ea98a1..c22eb7f4b0bd7 100644
> --- a/drivers/pci/tph.c
> +++ b/drivers/pci/tph.c

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but does the TPH subsystem lack necessary
locking around the TPH Control Register and ST table updates?

If multiple MSI-X vectors' SMP affinities are updated simultaneously by a
user or irqbalance, concurrent calls to pcie_tph_set_st_entry could happen.
Since pcie_tph_set_st_entry temporarily disables TPH system-wide:

pcie_tph_set_st_entry()
    set_ctrl_reg_req_en(pdev, PCI_TPH_REQ_DISABLE);
    ... 
    loc = pcie_tph_get_st_table_loc(pdev);
    ... write to ST table ...
    ...
    set_ctrl_reg_req_en(pdev, pdev->tph_req_type);

Would CPU2 re-enable TPH while CPU1 is still writing to the ST table,
violating the PCIe r6.2 sec 6.17.3 specification which cautions against
updating the ST table while TPH is enabled?

Additionally, set_ctrl_reg_req_en performs an unprotected read-modify-write:

set_ctrl_reg_req_en()
    pci_read_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, &reg);
    FIELD_MODIFY(PCI_TPH_CTRL_REQ_EN_MASK, &reg, req_type);
    pci_write_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, reg);

Could concurrent IRQ notifiers cause a data race here, corrupting the TPH
control register state?

>  	FIELD_MODIFY(PCI_TPH_CTRL_MODE_SEL_MASK, &reg, pdev->tph_mode);
>  	FIELD_MODIFY(PCI_TPH_CTRL_REQ_EN_MASK, &reg, pdev->tph_req_type);
>  
>  	pci_write_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, reg);
>

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