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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
