Re: [PATCH 2/2] PCI/MSI: Implement irq_mask_noflush() for MSI-x

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] PCI/MSI: pci_msix_mask() reads MMIO when pci_channel_offline() is true
--

commit b206f920f881524539101689cd7270ceba32c679
Author: Keith Busch <kbusch@kernel.org>

PCI/MSI: Implement irq_mask_noflush() for MSI-x

This commit implements the irq_mask_noflush() callback for PCI MSI-X by
splitting pci_msix_mask_noflush() out of pci_msix_mask() to skip the MMIO
read-back that flushes the mask write. Parent domains that mask on behalf of
PCI/MSI clear irq_mask_noflush so they continue using their own irq_mask().

> diff --git a/drivers/pci/msi/msi.h b/drivers/pci/msi/msi.h
> index f987cf897264e..db4dc37888f73 100644
> --- a/drivers/pci/msi/msi.h
> +++ b/drivers/pci/msi/msi.h
> @@ -44,7 +44,8 @@ static inline void pci_msix_write_vector_ctrl(struct msi_desc *desc, u32 ctrl)
>  		writel(ctrl, desc_addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>  }
>  
> -static inline void pci_msix_mask(struct msi_desc *desc)
> +/* Posted write only: the device may still send an interrupt after return */
> +static inline void pci_msix_mask_noflush(struct msi_desc *desc)
>  {
>  	desc->pci.msix_ctrl |= PCI_MSIX_ENTRY_CTRL_MASKBIT;
>  
> @@ -52,6 +53,11 @@ static inline void pci_msix_mask(struct msi_desc *desc)
>  		return;
>  
>  	pci_msix_write_vector_ctrl(desc, desc->pci.msix_ctrl);
> +}
> +
> +static inline void pci_msix_mask(struct msi_desc *desc)
> +{
> +	pci_msix_mask_noflush(desc);
>  	/* Flush write to device */
>  	readl(desc->pci.mask_base);
>  }

[Severity: High]
When pci_channel_offline(msi_desc_to_pci_dev(desc)) is true, doesn't
pci_msix_mask() now unconditionally execute readl(desc->pci.mask_base) after
pci_msix_mask_noflush() returns early?

Prior to this change, commit 2e2f69fc8ae2 ("PCI/MSI: Skip MSI/MSI-X
programming while the channel is offline") added the pci_channel_offline()
check to both pci_msix_write_vector_ctrl() and pci_msix_mask() so that
pci_msix_mask() would return before the flushing readl(desc->pci.mask_base):

drivers/pci/msi/msi.h:pci_msix_write_vector_ctrl() {
	/* The Table is unreachable while the Link is down */
	if (pci_channel_offline(msi_desc_to_pci_dev(desc)))
		return;
	...
}

With the early return moved into pci_msix_mask_noflush(), masking or tearing
down MSI-X vectors during PCIe error recovery, Downstream Port Containment,
or surprise removal (for example via pci_free_irq_vectors() ->
pci_msi_teardown_msi_irqs() -> pci_irq_shutdown_msix() -> pci_msix_mask() or
mask_irq() -> pci_irq_mask_msix() -> pci_msix_mask()) will still issue the
MMIO flush read to the unreachable MSI-X table BAR, which can trigger a
secondary Root Port containment event.

Should the pci_channel_offline() check remain in pci_msix_mask() before
readl(desc->pci.mask_base) instead of in pci_msix_mask_noflush() (since
pci_msix_write_vector_ctrl() already checks pci_channel_offline())?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105182706.3700908-1-kbusch@meta.com?part=2