Re: [PATCH v2 2/2] powerpc/PCI: Disable MSI/MSI-X interrupts at PCI probe time in OF case

[Bjorn Helgaas <bhelgaas@google.com>]

On Mon, Sep 07, 2015 at 01:17:03PM +1000, Michael Ellerman wrote:
> On Sun, 2015-09-06 at 17:44 +0300, Michael S. Tsirkin wrote:
> > On Fri, Sep 04, 2015 at 08:17:12PM -0300, Guilherme G. Piccoli wrote:
> > > Hello Bjorn,
> > > 
> > > >of_create_pci_dev() already has a lot of code that duplicates
> > > >pci_setup_device(), and it's a shame to add more.  There's also a sparc
> > > >version of of_create_pci_dev() that presumably has the same problem you're
> > > >fixing for powerpc.
> > > 
> > > Thanks for the information!
> > > 
> > > >Michael originally called pci_msi_setup_pci_dev() from
> > > >pci_init_capabilities() [1].  A subsequent patch moved the call
> > > >to pci_setup_device() [2] because an early quirk (called from
> > > >pci_setup_device()) used pci_msi_off(), which depended on
> > > >pci_msi_setup_pci_dev().
> > > >
> > > >But we later removed pci_msi_off() completely, so I think we probably
> > > >*could* call pci_msi_setup_pci_dev() from pci_init_capabilities().
> > > >
> > > >That would be much nicer because it makes more sense there, and it
> > > >would do the right thing for powerpc and sparc because they both
> > > >already use that path.
> > > >
> > > >Can you look into moving the call?
> > > 
> > > I might have misunderstood something here (sorry if it's the case), but
> > > moving the call to pci_init_capabilities() has the same practical
> > > implications than reverting my 2 commmits [1] [2] and Michael Tsirkin's
> > > commit [3], except when CONFIG_PCI_MSI is not set - in this case, moving the
> > > call would initialize MSI capabilities anyway, since pci_init_capabilities()
> > > executes even if CONFIG_PCI_MSI isn't set.
> > > 
> > > My question is: is necessary to initialize MSI capabilities even with
> > > CONFIG_PCI_MSI not set? In negative case, would be "cleaner" revert the 3
> > > commits, right?
> > > 
> > > On the other hand, if it's necessary to initialize MSI capabilities on
> > > devices anyway, we can change the call place.
> > 
> > I think the reason why it's necessary is explained in
> > commit log for commit 1851617cd2da9cc53cdc1738f4148f4f042c0e56 (that's
> > [3] below).
> 
> Well yes and no.
> 
> What we want to do when CONFIG_PCI_MSI=n is disable MSI on the device. In order
> to do that the code first initialises dev->msi[x]_cap.
> 
> But arguably that's wrong, ie. when CONFIG_PCI_MSI=n dev->msi[x]_cap *should*
> be zero so that any code which erroneously tries to use them will fail.

We could also argue that when CONFIG_PCI_MSI=n, dev->msi[x]_cap should not
even exist, so we could catch that a build-time instead of run-time.  My
personal opinion is that it's not a big deal, and the existing code that
includes dev->msi[x]_cap and initializes it even when CONFIG_PCI_MSI=n
allows some useful code sharing.

Bjorn