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: Tue, 27 Nov 2018 18:12:02 +0000	[thread overview]
Message-ID: <1543342322.18519.64.camel@impinj.com> (raw)
In-Reply-To: <86d0qrrmbc.wl-marc.zyngier@arm.com>

On Tue, 2018-11-27 at 07:50 +0000, Marc Zyngier wrote:
> On Mon, 26 Nov 2018 15:52:42 +0000,
> Trent Piepho <tpiepho@impinj.com> wrote:
> > 
> > > I used your patch and made it more perceptible in my opinion, (basically I
> > > transformed the variable irq_mask (previous irq_status) into a mirror of MASK
> > > registers, which removed the need for the *NOT* operation and forced the mask
> > > operation swap in the callbacks)
> > 
> > This would be the patch that enables all MSI interrupts on driver
> > initialization?
> > 
> > I don't think that's a good idea.  I was under the impression Marc
> > thought that as well.  It would be better to enable them when they are
> > enabled, via enable and disable methods.
> 
> What gain does this bring? Frankly, I see exactly zero advantage of
> doing so. It may look cosmetically appealing in the sense that it
> makes use of of a register that the IP offers, but for Linux the
> advantage is basically null.

Here's why:

1. It's a big change in driver behavior to enable all MSIs.  There will
certainly be hardware that writes to an MSI-X address, perhaps to
generate a MSI, perhaps not, where that MSI is disabled.  Now that
hardware will start generating interrupts.  That could be a big deal. 
The MSI might well have been not enabled very intentionally!  No reason
to create that change in behavior and also very much not a good idea to
backport to stable kernels.

2. Existing code is not clear about the difference between mask and
disable, getting it wrong in some places and causing bugs.  Creating
both mask and disable will make it clear.  It's the same reasoning you
use to reject my simple patch to put the irq ack at the correct time
and instead also put it in the semantically correct location.

3. A platform, keystone, has provided enable/disable methods for the
dwc driver.  But they are (now) called from the mask/unmask routines?!
That's not right; it'll drop irqs if it's really an enable/disable
feature in keystone.  Without dwc enable/disable methods, where will
the platform enable/disable methods be called from?  These methods are
getting randomly moved from mask to disable and back, like the ack
getting moved around, and clear distinction between disable and mask
will help stop that.