| Message ID | 1440010450-4549-1-git-send-email-gpiccoli@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
[+cc Fam, Yinghai, Yijing, Eric (reviewers of MST's original series), Dave] Hi Guilherme, On Wed, Aug 19, 2015 at 03:54:10PM -0300, Guilherme G. Piccoli wrote: > Changes since v2: > * Added "Fixes" line > * Improved commit reference by using 12 first chars of SHA > > >8----------8< > > Since the commit 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even > if kernel doesn't support MSI"), MSI/MSI-X interrupts aren't being > disabled at PCI probe time, as the logic responsible for this was moved > in the aforementioned commit from pci_device_add() to pci_setup_device(). > The latter function is not reachable on PowerPC pSeries platform during > Open Firmware PCI probing time. > > This patch calls pci_msi_setup_pci_dev() explicitly to disable MSI/MSI-X > during PCI probe time on pSeries platform. > > Fixes: 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel > doesn't support MSI") > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> > --- > arch/powerpc/kernel/pci_of_scan.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c > index 42e02a2..0e920f3 100644 > --- a/arch/powerpc/kernel/pci_of_scan.c > +++ b/arch/powerpc/kernel/pci_of_scan.c > @@ -191,6 +191,9 @@ struct pci_dev *of_create_pci_dev(struct device_node *node, > > pci_device_add(dev, bus); > > + /* Disable MSI/MSI-X here to avoid bogus interrupts */ > + pci_msi_setup_pci_dev(dev); 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. 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? Bjorn [1] http://lkml.kernel.org/r/1427641227-7574-3-git-send-email-mst@redhat.com [2] http://lkml.kernel.org/r/1427641227-7574-4-git-send-email-mst@redhat.com > return dev; > } > EXPORT_SYMBOL(of_create_pci_dev); > -- > 2.1.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Let me know your opinion, and I'm sorry if I misunderstood something here. Cheers, Guilherme Piccoli [1] commit 22b6839b914b ("PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code") [2] commit 4d9aac397a5d ("powerpc/PCI: Disable MSI/MSI-X interrupts at PCI probe time in OF case") [3] commit 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel doesn't support MSI") -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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). > Let me know your opinion, and I'm sorry if I misunderstood something here. > > Cheers, > > > Guilherme Piccoli > > > > [1] commit 22b6839b914b ("PCI: Make pci_msi_setup_pci_dev() non-static for > use by arch code") > > [2] commit 4d9aac397a5d ("powerpc/PCI: Disable MSI/MSI-X interrupts at PCI > probe time in OF case") > > [3] commit 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel > doesn't support MSI") -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-09-03 at 12:56 -0500, Bjorn Helgaas wrote: > [+cc Fam, Yinghai, Yijing, Eric (reviewers of MST's original series), Dave] > > Hi Guilherme, > > On Wed, Aug 19, 2015 at 03:54:10PM -0300, Guilherme G. Piccoli wrote: > > diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c > > index 42e02a2..0e920f3 100644 > > --- a/arch/powerpc/kernel/pci_of_scan.c > > +++ b/arch/powerpc/kernel/pci_of_scan.c > > @@ -191,6 +191,9 @@ struct pci_dev *of_create_pci_dev(struct device_node *node, > > > > pci_device_add(dev, bus); > > > > + /* Disable MSI/MSI-X here to avoid bogus interrupts */ > > + pci_msi_setup_pci_dev(dev); > > 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. > > 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. Sounds reasonable to me. Guilherme can you please try this and let us know. cheers -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. But perhaps that's being too pedantic :) cheers -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Sun, 2015-09-06 at 17:44 +0300, Michael S. Tsirkin wrote: >>> 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? >> I think the reason why it's necessary is explained in >> commit log for commit 1851617cd2da9cc53cdc1738f4148f4f042c0e56 (that's >> [3] below). Thanks very much Michael. I re-read the text of your commit, and makes sense then to initialize the MSI capabilities even with CONFIG_PCI_MSI not set. > On 09/07/2015 12:17 AM, Michael Ellerman wrote: > 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. > > But perhaps that's being too pedantic :) I thought exactly this - that was the reason of my questioning. Thanks for your opinion Michael - I'd call the argument logical, not pedantic hehehe Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/07/2015 12:10 AM, Michael Ellerman wrote: >> 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. > > Sounds reasonable to me. > > Guilherme can you please try this and let us know. Sure Michael. I tested in pSeries and PowerNV and both worked. Couldn't test on SPARC. Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/15/2015 01:18 PM, Bjorn Helgaas wrote: > 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. Nice Bjorn, so let's follow your idea regarding moving the code of MSI capabilities initialization to allow some code sharing. It's good option specially since it avoids the same problem (MSI capabilities not found)to occur in SPARC arch too. Sorry for my delay in response, soon I'll send the patch to the list. Cheers, Guilherme -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c index 42e02a2..0e920f3 100644 --- a/arch/powerpc/kernel/pci_of_scan.c +++ b/arch/powerpc/kernel/pci_of_scan.c @@ -191,6 +191,9 @@ struct pci_dev *of_create_pci_dev(struct device_node *node, pci_device_add(dev, bus); + /* Disable MSI/MSI-X here to avoid bogus interrupts */ + pci_msi_setup_pci_dev(dev); + return dev; } EXPORT_SYMBOL(of_create_pci_dev);