Re: [PATCH 0/3] PCI: designware: Fixing MSI handling flow

From: Trent Piepho <tpiepho@impinj.com>
To: "marc.zyngier@arm.com" <marc.zyngier@arm.com>
Cc: "jingoohan1@gmail.com" <jingoohan1@gmail.com>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"gustavo.pimentel@synopsys.com" <gustavo.pimentel@synopsys.com>,
	"faiz_abbas@ti.com" <faiz_abbas@ti.com>,
	"Joao.Pinto@synopsys.com" <Joao.Pinto@synopsys.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"helgaas@google.com" <helgaas@google.com>,
	"vigneshr@ti.com" <vigneshr@ti.com>
Subject: Re: [PATCH 0/3] PCI: designware: Fixing MSI handling flow
Date: Mon, 19 Nov 2018 20:14:55 +0000	[thread overview]
Message-ID: <1542658494.30311.552.camel@impinj.com> (raw)
In-Reply-To: <865zwyw351.wl-marc.zyngier@arm.com>

On Thu, 2018-11-15 at 19:29 +0000, Marc Zyngier wrote:
> On Thu, 15 Nov 2018 18:37:33 +0000,
> Trent Piepho <tpiepho@impinj.com> wrote:
> > 
> > On Thu, 2018-11-15 at 15:22 +0000, Gustavo Pimentel wrote:
> > > On 14/11/2018 18:28, Trent Piepho wrote:
> > > > On Tue, 2018-11-13 at 22:57 +0000, Marc Zyngier wrote:
> > > > > 
> > > > 
> > > > Now it works again.  Race still present.  I don't see the
> > > > dw_pci_msi_bottom_(un)mask methods ever get called.  I seem to recall
> > > > that they are called as a substitute if enable/disable are not present,
> > > > but haven't confirmed that, which would explain why they are not called
> > > > after I added enable.
> > > 
> > > Hum, this probably is correlated with [1] where on the describition the this
> > > enumerator says that "One shot does not require mask/unmask" see [2]
> > 
> > That seems reasonable then.  I've said before that while I think
> > masking could be added in the interrupt flow without breaking anything,
> > I think it's redundant to do it at this layer.
> > 
> > I suspect it might be possible mask/unmask the interrupt "manually",
> > e.g. UIO does this to allow handling a level interrupt from userspace,
> > and that would be a path to reach the mask methods.
> 
> The real use case is that a driver can perfectly disable (mask) an
> interrupt while being in the interrupt handler, and enable it again at
> a later time.

That's what I was thinking of with UIO, but uio_dmem_genirq_handler()
actually calls disable_irq_nosync().  I had thought it masked the irq
rather than disabled it.

I wonder if this is a UIO bug.  The distinction between disable and
mask is usually that the masked interrupt is not lost while the
disabled interrupt is.  But there are irq chips (like dwc!) that do not
 implement enable/disable methods and use mask methods instead.