Re: [PATCH v10 1/5] PCI/TPH: Fix pcie_tph_get_st_table_loc() field extraction

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `pdev->tph_cap` validation leads to out-of-bounds configuration space reads and bogus capability reporting.
- [High] Data race on the `PCI_TPH_CTRL` register during concurrent ST entry updates.
--

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

PCI/TPH: Fix pcie_tph_get_st_table_loc() field extraction

This commit fixes ST table location detection by replacing an incorrect
FIELD_GET() usage with a bitwise AND in pcie_tph_get_st_table_loc(). It
also removes the now-redundant FIELD_PREP() conversions in the callers.

> diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
> index 91145e8d9d951..877cf556242b4 100644
> --- a/drivers/pci/tph.c
> +++ b/drivers/pci/tph.c
> @@ -170,7 +170,7 @@ u32 pcie_tph_get_st_table_loc(struct pci_dev *pdev)
>  
>  	pci_read_config_dword(pdev, pdev->tph_cap + PCI_TPH_CAP, &reg);

This problem wasn't introduced by this patch, but if a device doesn't
support TPH and pdev->tph_cap is 0, will this read from the standard PCI
Command and Status register at offset 0x04 instead?

Bits 9 and 10 of the Command register overlap exactly with
PCI_TPH_CAP_LOC_MASK (0x0600). If those bits are set, this could incorrectly
return PCI_TPH_LOC_CAP or PCI_TPH_LOC_MSIX, leading to bogus ST table size
extractions in pcie_tph_get_st_table_size().

Should this check if pdev->tph_cap is valid before reading, and return
PCI_TPH_LOC_NONE otherwise?

> -	return FIELD_GET(PCI_TPH_CAP_LOC_MASK, reg);
> +	return reg & PCI_TPH_CAP_LOC_MASK;
>  }
>  EXPORT_SYMBOL(pcie_tph_get_st_table_loc);

[ ... ]

> @@ -316,8 +313,6 @@ int pcie_tph_set_st_entry(struct pci_dev *pdev, unsigned int index, u16 tag)
>  	set_ctrl_reg_req_en(pdev, PCI_TPH_REQ_DISABLE);

This wasn't introduced by this patch, but is there a potential data race here
if multiple threads configure MSI-X vectors concurrently?

When set_ctrl_reg_req_en() is called, it performs an unlocked
read-modify-write on the PCI_TPH_CTRL register:

static void set_ctrl_reg_req_en(struct pci_dev *pdev, u8 req_type)
{
        u32 reg;
        pci_read_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, &reg);
        reg &= ~PCI_TPH_CTRL_REQ_EN_MASK;
        reg |= FIELD_PREP(PCI_TPH_CTRL_REQ_EN_MASK, req_type);
        pci_write_config_dword(pdev, pdev->tph_cap + PCI_TPH_CTRL, reg);
}

Could this cause unpredictable toggling of the device-wide TPH state while
tags are actively being written? Would it be safer to serialize these
modifications using a lock like pdev->pcie_cap_lock?

>  	loc = pcie_tph_get_st_table_loc(pdev);
> -	/* Convert loc to match with PCI_TPH_LOC_* */
> -	loc = FIELD_PREP(PCI_TPH_CAP_LOC_MASK, loc);
>  
>  	switch (loc) {
>  	case PCI_TPH_LOC_MSIX:

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