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

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

[sashiko-bot]

> 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().
> 
> 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/20261106173448.34794-1-kbusch@meta.com?part=2

Re: [PATCH v3 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/20261106173448.34794-1-kbusch@meta.com?part=1

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

[Thomas Gleixner]

On Fri, Nov 06 2026 at 09:34, Keith Busch wrote:
> -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;
> +	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);

That's before the offline check now, which looks more than wrong.

>  	if (pci_channel_offline(msi_desc_to_pci_dev(desc)))
>  		return;
>  
> -	pci_msix_write_vector_ctrl(desc, desc->pci.msix_ctrl);
>  	/* Flush write to device */
>  	readl(desc->pci.mask_base);
>  }

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

[Keith Busch]

On Fri, Nov 06, 2026 at 09:53:06PM +0100, Thomas Gleixner wrote:
> On Fri, Nov 06 2026 at 09:34, Keith Busch wrote:
> > -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;
> > +	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);
> 
> That's before the offline check now, which looks more than wrong.

This is done on purpose: pci_msix_write_vector_ctrl() already has the
same offline check, so nothing actually gets written, same as before.

And regardless of whether it's offline or not, we have to set the
PCI_MSIX_ENTRY_CTRL_MASKBIT in the cached msxi_ctrl. That's common to
both paths, so it lives in the noflush helper to avoid duplicating it.

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

[Thomas Gleixner]

On Sat, Nov 07 2026 at 13:00, Keith Busch wrote:
> On Fri, Nov 06, 2026 at 09:53:06PM +0100, Thomas Gleixner wrote:
>> On Fri, Nov 06 2026 at 09:34, Keith Busch wrote:
>> > -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;
>> > +	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);
>> 
>> That's before the offline check now, which looks more than wrong.
>
> This is done on purpose: pci_msix_write_vector_ctrl() already has the
> same offline check, so nothing actually gets written, same as before.
>
> And regardless of whether it's offline or not, we have to set the
> PCI_MSIX_ENTRY_CTRL_MASKBIT in the cached msxi_ctrl. That's common to
> both paths, so it lives in the noflush helper to avoid duplicating it.

Indeed, but that's an unreadable mess TBH.

Can we please change that to have two functions here?

    pci_msix_write_vector_ctrl()
plus    
    pci_msix_write_vector_ctrl_and_flush()

and fixup the related call sites?

Thanks,

        tglx

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

[Keith Busch]

On Sat, Nov 07, 2026 at 10:53:47PM +0100, Thomas Gleixner wrote:
> On Sat, Nov 07 2026 at 13:00, Keith Busch wrote:
> > This is done on purpose: pci_msix_write_vector_ctrl() already has the
> > same offline check, so nothing actually gets written, same as before.
> >
> > And regardless of whether it's offline or not, we have to set the
> > PCI_MSIX_ENTRY_CTRL_MASKBIT in the cached msxi_ctrl. That's common to
> > both paths, so it lives in the noflush helper to avoid duplicating it.
> 
> Indeed, but that's an unreadable mess TBH.

No disagreement there! I was just going for a minimal diff, but we can
refactor a bit on the way. I'll take a stab at it.

I also don't really get the rationale for putting the check in
write_vector. The commit message says the checks are to avoid having to
deal with errors, but posted writes don't produce errors, so not sure
what it's actually guarding against.

> Can we please change that to have two functions here?
> 
>     pci_msix_write_vector_ctrl()
> plus    
>     pci_msix_write_vector_ctrl_and_flush()
> 
> and fixup the related call sites?

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

[Thomas Gleixner]

On Mon, Nov 09 2026 at 09:14, Keith Busch wrote:

> On Sat, Nov 07, 2026 at 10:53:47PM +0100, Thomas Gleixner wrote:
>> On Sat, Nov 07 2026 at 13:00, Keith Busch wrote:
>> > This is done on purpose: pci_msix_write_vector_ctrl() already has the
>> > same offline check, so nothing actually gets written, same as before.
>> >
>> > And regardless of whether it's offline or not, we have to set the
>> > PCI_MSIX_ENTRY_CTRL_MASKBIT in the cached msxi_ctrl. That's common to
>> > both paths, so it lives in the noflush helper to avoid duplicating it.
>> 
>> Indeed, but that's an unreadable mess TBH.
>
> No disagreement there! I was just going for a minimal diff, but we can
> refactor a bit on the way. I'll take a stab at it.

Yes, please. The minimal diff approch is what creates the most problems
down the road.

> I also don't really get the rationale for putting the check in
> write_vector. The commit message says the checks are to avoid having to
> deal with errors, but posted writes don't produce errors, so not sure
> what it's actually guarding against.

To me the explaination made sense and there were no objections within
month from any of the people involved. I'm not a DPC (or whatever is
involved) expert. I just work here.

I've Cc'ed the author of the patch for you as that might be a better way
to answer your question, no?

Thanks,

        tglx