Re: [PATCH V2] PCI/MSI: Don't touch the MSI-X table while the Link is contained

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The early `pci_msi_dev_inaccessible()` check in `pci_msix_write_tph_tag()` leaves a large race window across lock acquisitions, leading to an unconditional flush read on an inaccessible device.
- [High] Silently skipping writes in `pci_msix_write_vector_ctrl()` exposes callers like `pci_write_msg_msix()` to partial MMIO updates and unconditional flush reads.
--

--- Patch [1]: [PATCH V2] PCI/MSI: Don't touch the MSI-X table while the Link is contained ---
commit 170aedad1f36f8c51175a3888ba22b67ee4a98ef
Author: Vidya Sagar <vidyas@nvidia.com>

PCI/MSI: Don't touch the MSI-X table while the Link is contained

The MSI-X Table lives in device MMIO space behind a BAR, so it is only
reachable while the Link is up. This patch skips hardware accesses to
the MSI-X table when the device is not in pci_channel_io_normal to prevent
DPC escalations caused by Unsupported Requests. It also modifies
pci_msix_write_tph_tag() to return -EIO to let the caller disable TPH.

> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index 80a9db417dc87..0ebe153937ccc 100644
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c
[ ... ]
> @@ -954,6 +954,14 @@ int pci_msix_write_tph_tag(struct pci_dev *pdev, unsigned int index, u16 tag)
>  	if (!pdev->msix_enabled)
>  		return -ENXIO;
>  
> +	/*
> +	 * The tag update below is a write to the MSI-X Table followed by a
> +	 * flush read, neither of which can be completed while the Link is
> +	 * contained. Let the caller disable TPH instead.
> +	 */
> +	if (pci_msi_dev_inaccessible(pdev))
> +		return -EIO;
> +

[Severity: High]
Since this check happens before acquiring msi_descs_lock and irq_desc->lock
in pci_msix_write_tph_tag(), does it leave a race window where the device
could become inaccessible after the check?

If a DPC event occurs while waiting for the locks, it looks like the
subsequent unconditional flush read:

drivers/pci/msi/msi.c:pci_msix_write_tph_tag() {
    ...
	/* Flush the write */
	readl(pci_msix_desc_addr(msi_desc));
    ...
}

would still execute on an inaccessible device, potentially triggering the
exact DPC escalation this patch aims to prevent.

>  	virq = msi_get_virq(&pdev->dev, index);
>  	if (!virq)
>  		return -ENXIO;
> diff --git a/drivers/pci/msi/msi.h b/drivers/pci/msi/msi.h
> index 0b420b319f50f..c3194d8425c88 100644
> --- a/drivers/pci/msi/msi.h
> +++ b/drivers/pci/msi/msi.h
[ ... ]
> @@ -36,6 +50,9 @@ static inline void pci_msix_write_vector_ctrl(struct msi_desc *desc, u32 ctrl)
>  {
>  	void __iomem *desc_addr = pci_msix_desc_addr(desc);
>  
> +	if (pci_msi_dev_inaccessible(msi_desc_to_pci_dev(desc)))
> +		return;
> +

[Severity: High]
If this silently skips the vector control write, does it expose callers like
pci_write_msg_msix() to partial MMIO updates and unconditional flush reads?

Looking at pci_write_msg_msix(), if the mask write is silently skipped,
would the subsequent address and data writes, as well as the flush read,
still execute unconditionally:

drivers/pci/msi/msi.c:pci_write_msg_msix() {
    ...
	writel(msg->address_lo, base + PCI_MSIX_ENTRY_LOWER_ADDR);
	writel(msg->address_hi, base + PCI_MSIX_ENTRY_UPPER_ADDR);
	writel(msg->data, base + PCI_MSIX_ENTRY_DATA);

	if (unmasked)
		pci_msix_write_vector_ctrl(desc, ctrl);

	/* Ensure that the writes are visible in the device */
	readl(base + PCI_MSIX_ENTRY_DATA);
    ...
}

Could this trigger an Unsupported Request and the DPC escalation if the link
is contained?

>  	if (desc->pci.msi_attrib.can_mask)
>  		writel(ctrl, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825140952.4066140-1-vidyas@nvidia.com?part=1