On Tue, 2018-11-27 at 07:50 +0000, Marc Zyngier wrote: > On Mon, 26 Nov 2018 15:52:42 +0000, > Trent Piepho <tpiepho@xxxxxxxxxx> 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.
