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

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

[sashiko-bot]

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

Re: [PATCH 1/2] genirq: Add disable_irq_noflush()

[sashiko-bot]

> From: Keith Busch <kbusch@kernel.org>
> 
> Drivers that switch between interrupt and polled completions don't need
> the mask write flushed to the device; the disabled state already
> keeps their handler from running and a late interrupt is replayed on
> enable. Add an optional irq_chip::irq_mask_noflush() callback, falling
> back to irq_mask(), and disable_irq_noflush() to mask through it right
> away rather than lazily.
> 
> Suggested-by: Thomas Gleixner <tglx@kernel.org>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

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

[Keith Busch]

On Thu, Nov 05, 2026 at 06:36:52PM +0000, sashiko-bot@kernel.org wrote:
> 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;
> 	...
> }

Oh right. That check looks fairly recent, and I missed the merge from my
older version resolved this incorrectly. I'll respin it. 

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

[Bjorn Helgaas]

On Thu, Nov 05, 2026 at 10:27:06AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Skip the read-back that flushes the MSI-x mask write. Parent domains
> that mask on behalf of PCI/MSI keep using their own irq_mask().

When you respin it,

s/MSI-x/MSI-X/ to match spec usage

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

[Keith Busch]

On Thu, Nov 05, 2026 at 01:41:16PM -0600, Bjorn Helgaas wrote:
> On Thu, Nov 05, 2026 at 10:27:06AM -0800, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Skip the read-back that flushes the MSI-x mask write. Parent domains
> > that mask on behalf of PCI/MSI keep using their own irq_mask().
> 
> When you respin it,
> 
> s/MSI-x/MSI-X/ to match spec usage

Oh, sorry, the messages crosses on the wire so the v2 retains the MSI-x
syntax. I can respin it, but don't want to rapid fire another version.