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

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 <redacted>
---
 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);
+
 	return dev;
 }
 EXPORT_SYMBOL(of_create_pci_dev);

-- 
2.1.0

[+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:

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

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")

One other thing.

The way to present all the people who submitted, such that each one got some amount of exposure, was a dynamic thing that just occurred to me as we were setting up.

Sort of like you first starting to select the judging panels, by ‘guessing’ a number… which didn’t work very well, but then counting out groups did. (even if there were groups that had some extras because the group was an odd number…).

Next time, we will have this in mind and it will be very straightforward… 1,2,3… for both how to select the panels, and how to present the submissions.

Love,
John.


On Sep 4, 2015, at 3:46 PM, Guilherme G. Piccoli [via linuxppc] [off-list ref] wrote:

Hello Bjorn, 

quoted

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! 

quoted

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") 

_______________________________________________ 
Linuxppc-dev mailing list 
[hidden email] 
https://lists.ozlabs.org/listinfo/linuxppc-dev 

If you reply to this email, your message will be added to the discussion below:
http://linuxppc.10917.n7.nabble.com/PATCH-0-2-Disable-MSI-MSI-X-interrupts-manually-at-PCI-probe-time-in-PowerPC-architecture-tp97680p98194.html
To start a new topic under linuxppc-dev, email ml-node+s10917n3h38@n7.nabble.com 
To unsubscribe from linuxppc, click here.
NAML

--
View this message in context: http://linuxppc.10917.n7.nabble.com/PATCH-0-2-Disable-MSI-MSI-X-interrupts-manually-at-PCI-probe-time-in-PowerPC-architecture-tp97680p98195.html
Sent from the linuxppc-dev mailing list archive at Nabble.com.

On Fri, Sep 04, 2015 at 08:17:12PM -0300, Guilherme G. Piccoli wrote:

Hello Bjorn,

quoted

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!

quoted

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")

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:

quoted

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

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:

quoted

Hello Bjorn,

quoted

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!

quoted

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

On Sun, 2015-09-06 at 17:44 +0300, Michael S. Tsirkin wrote:

quoted

quoted

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?

quoted

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

On 09/07/2015 12:10 AM, Michael Ellerman wrote:

quoted

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

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:

quoted

On Fri, Sep 04, 2015 at 08:17:12PM -0300, Guilherme G. Piccoli wrote:

quoted

Hello Bjorn,

quoted

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!

quoted

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

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