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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The function pcie_tph_set_st_entry() lacks internal serialization, allowing concurrent modifications to the TPH Control Register and violating the PCIe specification.
- [High] Unprotected modifications to the pdev->tph_enabled bitfield can cause silent data corruption of adjacent pci_dev state flags.
--

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

PCI/TPH: Move tph_req_type initialization into pci_tph_init
    
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.

> diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
> index 95e2a95055eeb..3660ad5d36239 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);

[Severity: High]
This isn't a bug introduced by this patch, but does pcie_tph_set_st_entry()
also need internal serialization when writing to the TPH Control Register?

pcie_tph_set_st_entry() temporarily disables TPH via set_ctrl_reg_req_en(),
updates the ST table, and then re-enables TPH.

Network drivers like bnxt call this function from irq_affinity_notify
callbacks, which execute in multi-threaded workqueues.

If multiple IRQ affinities are changed simultaneously, these callbacks can run
concurrently on different CPUs for the same pdev.

Without a lock protecting this sequence, could one thread re-enable TPH while
another thread is still actively updating its ST table entry?

This would violate PCIe r6.2, sec 6.17.3 (which requires TPH to be disabled
during ST updates) and cause overlapping read-modify-write operations on the
TPH Control Register, potentially leading to device instability.

>  
>  	pdev->tph_mode = 0;
> -	pdev->tph_req_type = 0;
>  	pdev->tph_enabled = 0;

[Severity: High]
This is a pre-existing issue, but can unprotected modifications to the
pdev->tph_enabled bitfield cause silent data corruption of adjacent pci_dev
state flags?

tph_enabled is defined as an unsigned int bitfield in struct pci_dev, sharing
a memory word with critical flags like block_cfg_access, is_busmaster, and
msi_enabled.

Bitfield assignments compile to non-atomic read-modify-write (RMW) operations
on the surrounding word.

pcie_enable_tph() and pcie_disable_tph() modify tph_enabled locklessly from
the PCI subsystem's perspective (e.g., bnxt calls it during device open under
rtnl_lock).

If another subsystem concurrently modifies an adjacent bitfield (e.g.,
pci_cfg_access_lock() updating block_cfg_access while holding pci_lock), will
the overlapping RMW cycles cause one of the updates to be silently lost,
corrupting the overall PCI device state?

>  }

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