Re: [PATCH v6 00/10] PCI: Fix unhandled interrupt on shutdown

[Bjorn Helgaas <bhelgaas@google.com>]

On Wed, May 06, 2015 at 07:53:48PM -0500, Eric W. Biederman wrote:
> Bjorn Helgaas <bhelgaas@google.com> writes:
> 
> > On Sun, Apr 26, 2015 at 08:50:06AM +0200, Michael S. Tsirkin wrote:
> >> On Fri, Apr 10, 2015 at 05:54:19PM -0500, Bjorn Helgaas wrote:
> >> > Hi Michael,
> >> > 
> >> > I put your patches on my pci/msi branch and I hope to merge them for v4.1.
> >> > I didn't apply the acks from Fam and Eric because I made changes to those
> >> > patches that weren't completely trivial.  I think the end result is
> >> > equivalent, though.  The diff attached to this cover letter is the
> >> > difference between your v5 series and this v6 series.
> >> > 
> >> > As far as I'm concerned, this is ready to go except that I would like a
> >> > little more info about the virtio kernel hang to include in the changelog
> >> > for "PCI/MSI: Don't disable MSI/MSI-X at shutdown".
> >> 
> >> 
> >> Hi Bjorn,
> >> do you have eveything you need to merge this?
> >
> > No.  I made the minor changelog edits you suggested and the result is on
> > my pci/msi-v7 branch.  But I still have these open issues:
> >
> >   - The last thing I heard from Eric was that "not disabling MSI/MSI-X at
> >     shutdown is the wrong fix, and someone needs to fix a buggy driver."
> >     I want to hear Eric say "OK, we need to leave MSI/MSI-X enabled at
> >     shutdown for this case."
> 
> So far this just sounds like a device that needs a shutdown method.
> 
> >   - One changelog says "Stop disabling MSIs at shutdown to avoid
> >     conflicting with drivers."  But I don't know what the conflict is.
> >
> >   - The bugzilla has no dmesg log or detailed analysis.  Fam said the
> >     scenario I came up with
> >     (http://lkml.kernel.org/r/20150416194245.GB20701@google.com)
> >     was fairly close, but it took me a lot of work to derive that.  Fixing
> >     any errors in it and putting it in the bugzilla would be a big step.
> >     The bugzilla should have the raw data and the analysis, so someone else
> >     can validate the analysis and conclude that this patch is a reasonable
> >     fix for it.  That's currently impossible because the bugzilla really
> >     only contains the fix as a fait accompli.
> 
> What I saw in the bugzilla was:
> 
> An interrupt was stuck on, and being reasserted as quickly as we could
> call iret for that interrupt.
> 
> We did not disable that interrupt because irq debugging was explicitly
> disabled on the kernel command line.
> 
> The irq was asserted because the the device did not have a shutdown
> method to stop the device doing things.
> 
> There is an argument that disabling bus mastering should have disabled
> whatever the interrupting condition was (and thus there may also be a
> bug in the qemu device emulation).
> 
> So I read this as the driver and maybe the "hardware" is buggy not that
> the linux pci layer is buggy.

OK, it sounds like we don't have consensus on this issue yet.

The rest of the series, which disable MSIs at boot-time even without
CONFIG_PCI_MSI=y in the new kernel and does some cleanup, seems worthwhile
and non-controversial, so I reordered it and applied the following patches
to pci/msi for v4.2:

  PCI/MSI: Rename msi_set_enable(), msix_clear_and_set_ctrl()
  PCI/MSI: Export pci_msi_set_enable(), pci_msix_clear_and_set_ctrl()
  PCI/MSI: Disable MSI at enumeration even if kernel doesn't support MSI
  virtio_pci: drop pci_msi_off() call during probe
  ntb: Drop pci_msi_off() call during probe
  PCI/MSI: Drop pci_msi_off() calls from quirks
  PCI/MSI: Remove unused pci_msi_off()

I removed the "stable" annotations because I don't have a clear report of
a bug that this fixes.  If it does fix a bug, please point me to a
bugzilla, and I can add the stable annotations back.

That leaves these two:

  PCI/MSI: Don't disable MSI/MSI-X at shutdown
  PCI/MSI: Make pci_msi_shutdown(), pci_msix_shutdown() static

I'm going to ignore them until you guys figure out what to do.

Bjorn