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

[Trent Piepho <tpiepho@impinj.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.