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.
