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: "Joao.Pinto@synopsys.com" <Joao.Pinto@synopsys.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"gustavo.pimentel@synopsys.com" <gustavo.pimentel@synopsys.com>,
	"vigneshr@ti.com" <vigneshr@ti.com>,
	"helgaas@google.com" <helgaas@google.com>
Subject: Re: [PATCH 0/3] PCI: designware: Fixing MSI handling flow
Date: Mon, 19 Nov 2018 20:37:18 +0000	[thread overview]
Message-ID: <1542659838.30311.561.camel@impinj.com> (raw)
In-Reply-To: <86sh0348tu.wl-marc.zyngier@arm.com>

On Wed, 2018-11-14 at 22:01 +0000, Marc Zyngier wrote:
> On Wed, 14 Nov 2018 19:19:27 +0000,
> 
> > It looks to me that the previous code was using whatever MSIs were
> > already enabled when the driver is initialized as the set to leave
> > enabled.
> 
> And I claim that this is a gross bug. We don't want to inherit
> anything at all, rather start from a fresh start.
> 
> > This looks like it changing that behavior, and instead enabling all
> > MSIs on initialization.
> > 
> > I would think the default value for a MSI should be disabled until
> > something enables it.
> 
> Sure, that's no big deal. We can plug in the enable/disable callbacks
> to that effect, although we'll end-up with similar result, I'd expect.
> 
> > I speculated that the previous behavior was trying to work with an MSI
> > enabled by the bootloader, ACPI firmware, etc. that should be left
> > alone.  Or perhaps there was no good reason not to disable everything
> > on initialization and that code just got copied from somewhere else and
> > no one thought about it.  There's certainly evidence of that in this
> > driver.
> 
> As you said, you're speculating. Nonetheless, there is no reason to
> start with anything enabled the first place.

First introduction of this concept explicitly appears to be by Gustavo
in commit 7c5925afbc.  It added irq_status to the driver state and
initialized it from the existing value of the enable register.  Prior
to this the bits were always set in a RMW operation I don't think
initialization was considered.

I don't see any note about why.  All disabled makes far more sense to
me.  You get hard to track down bugs with unexpected interrupts during
kernel boot, but only a soft reboot, because the irq is enabled before
the drivers expected it to be.