[PATCH 0/2] Disable MSI/MSI-X interrupts manually at PCI probe time in PowerPC architecture

[PATCH 0/2] Disable MSI/MSI-X interrupts manually at PCI probe time in PowerPC architecture

[Guilherme G. Piccoli]

These 2 patches correct a bogus behaviour introduced by commit 1851617cd2
("PCI/MSI: Disable MSI at enumeration even if kernel doesn't support MSI").
The commit moved the logic responsible to disable MSI/MSI-X interrupts
at PCI probe time to a new function, named pci_msi_setup_pci_dev(), that
is not reachable in the code path of PowerPC pSeries platform.

Since then, devices aren't able to activate MSI/MSI-X capability, even
after boot. The first patch makes the function pci_msi_setup_pci_dev()
non-static. The second patch inserts a call to the function in powerpc
code, so it explicitly disables MSI/MSI-X interrupts at PCI probe time.

Guilherme G. Piccoli (2):
  PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code
  powerpc/PCI: Disable MSI/MSI-X interrupts at PCI probe time in OF case

 arch/powerpc/kernel/pci_of_scan.c | 3 +++
 drivers/pci/probe.c               | 2 +-
 include/linux/pci.h               | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.1.0

[PATCH 1/2] PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code

[Guilherme G. Piccoli]

Commit 1851617cd2 ("PCI/MSI: Disable MSI at enumeration even if kernel
doesn't support MSI") changed the location of the code that disables
MSI/MSI-X interrupts at PCI probe time in devices that have this flag set.
It moved the code from pci_msi_init_pci_dev() to a new function named
pci_msi_setup_pci_dev(), called by pci_setup_device().

Since then, the pSeries platform of the powerpc architecture needs to
disable MSI at PCI probe time manually, as the code flow doesn't
reach pci_setup_device(). For doing so, it wants to call
pci_msi_setup_pci_dev(). This patch makes the required function
non-static, so that it will be called on PCI probe path on powerpc pSeries
platform in next patch.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
---
 drivers/pci/probe.c | 2 +-
 include/linux/pci.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636..520c5b6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1103,7 +1103,7 @@ int pci_cfg_space_size(struct pci_dev *dev)
 
 #define LEGACY_IO_RESOURCE	(IORESOURCE_IO | IORESOURCE_PCI_FIXED)
 
-static void pci_msi_setup_pci_dev(struct pci_dev *dev)
+void pci_msi_setup_pci_dev(struct pci_dev *dev)
 {
 	/*
 	 * Disable the MSI hardware to avoid screaming interrupts
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8a0321a..860c751 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1202,6 +1202,7 @@ struct msix_entry {
 	u16	entry;	/* driver uses to specify entry, OS writes */
 };
 
+void pci_msi_setup_pci_dev(struct pci_dev *dev);
 
 #ifdef CONFIG_PCI_MSI
 int pci_msi_vec_count(struct pci_dev *dev);
-- 
2.1.0

[PATCH v2 1/2] PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code

[Guilherme G. Piccoli]

Thanks very much for your suggestions Michael. I agree with them all,
so I'm sending the patch v2 (see below).

About the relevant mailing lists, I already sent to the linux-pci and
already cc'ed Bjorn Helgaas - the problem is that I made a mistake and
sent 2 different emails using git send-email. I'm really sorry about
this.

Now I'm trying to correct my mistake sending this message
simultaneously to both lists (and to a bunch of cc's) using two
Message-IDs in my reply. Hope it works...

Cheers


Changes since v2:
 * Made commit message more clear
 * Added "Fixes" line
 * Improved commit reference by using 12 first chars of SHA

>8----------8<

Commit 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel
doesn't support MSI") changed the location of the code that disables
MSI/MSI-X interrupts at PCI probe time in devices that have this flag
set. It moved the code from pci_msi_init_pci_dev() to a new function
named pci_msi_setup_pci_dev(), called by pci_setup_device().

The pseries PCI probing code does not call pci_setup_device(), so since
the aforementioned commit the function pci_msi_setup_pci_dev() is not
called and MSI/MSI-X interrupts are left enabled, which is a bug. To fix
this, the pseries PCI probe should manually call pci_msi_setup_pci_dev(),
so this patch makes it non-static.

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>
---
 drivers/pci/probe.c | 2 +-
 include/linux/pci.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636..520c5b6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1103,7 +1103,7 @@ int pci_cfg_space_size(struct pci_dev *dev)
 
 #define LEGACY_IO_RESOURCE	(IORESOURCE_IO | IORESOURCE_PCI_FIXED)
 
-static void pci_msi_setup_pci_dev(struct pci_dev *dev)
+void pci_msi_setup_pci_dev(struct pci_dev *dev)
 {
 	/*
 	 * Disable the MSI hardware to avoid screaming interrupts
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8a0321a..860c751 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1202,6 +1202,7 @@ struct msix_entry {
 	u16	entry;	/* driver uses to specify entry, OS writes */
 };
 
+void pci_msi_setup_pci_dev(struct pci_dev *dev);
 
 #ifdef CONFIG_PCI_MSI
 int pci_msi_vec_count(struct pci_dev *dev);
-- 
2.1.0

Re: [PATCH v2 1/2] PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code

[Michael Ellerman]

On Wed, 2015-08-19 at 15:45 -0300, Guilherme G. Piccoli wrote:
> Thanks very much for your suggestions Michael. I agree with them all,
> so I'm sending the patch v2 (see below).
> 
> About the relevant mailing lists, I already sent to the linux-pci and
> already cc'ed Bjorn Helgaas - the problem is that I made a mistake and
> sent 2 different emails using git send-email. I'm really sorry about
> this.
> 
> Now I'm trying to correct my mistake sending this message
> simultaneously to both lists (and to a bunch of cc's) using two
> Message-IDs in my reply. Hope it works...

OK.

In future you should send a reply like the above to my mail, and then
separately send the new patch series. My preference is that the new series is
not a reply to anything, though some other maintainers may disagree on that
point.

The other question, which I neglected to ask yesterday, is what is the symptom
of the bug? ie. does the system fail to boot or otherwise crash etc.?


> Changes since v2:

This is changes *in* v2, or since v1.

And it doesn't go here, it goes below ...

>  * Made commit message more clear
>  * Added "Fixes" line
>  * Improved commit reference by using 12 first chars of SHA
> 
> >8----------8<
> 
> Commit 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel
> doesn't support MSI") changed the location of the code that disables
> MSI/MSI-X interrupts at PCI probe time in devices that have this flag
> set. It moved the code from pci_msi_init_pci_dev() to a new function
> named pci_msi_setup_pci_dev(), called by pci_setup_device().
> 
> The pseries PCI probing code does not call pci_setup_device(), so since
> the aforementioned commit the function pci_msi_setup_pci_dev() is not
> called and MSI/MSI-X interrupts are left enabled, which is a bug. To fix
> this, the pseries PCI probe should manually call pci_msi_setup_pci_dev(),
> so this patch makes it non-static.
> 
> 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>
> ---

Here.

Or anywhere after the first '---', which means the version commentary is
discarded in the final commit.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index cefd636..520c5b6 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1103,7 +1103,7 @@ int pci_cfg_space_size(struct pci_dev *dev)
>  
>  #define LEGACY_IO_RESOURCE	(IORESOURCE_IO | IORESOURCE_PCI_FIXED)
>  
> -static void pci_msi_setup_pci_dev(struct pci_dev *dev)
> +void pci_msi_setup_pci_dev(struct pci_dev *dev)
>  {
>  	/*
>  	 * Disable the MSI hardware to avoid screaming interrupts
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8a0321a..860c751 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1202,6 +1202,7 @@ struct msix_entry {
>  	u16	entry;	/* driver uses to specify entry, OS writes */
>  };
>  
> +void pci_msi_setup_pci_dev(struct pci_dev *dev);
>  
>  #ifdef CONFIG_PCI_MSI
>  int pci_msi_vec_count(struct pci_dev *dev);


cheers

Re: [PATCH v2 1/2] PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code

[Guilherme G. Piccoli]

On 08/19/2015 10:02 PM, Michael Ellerman wrote:
> In future you should send a reply like the above to my mail, and then
> separately send the new patch series. My preference is that the new series is
> not a reply to anything, though some other maintainers may disagree on that
> point.

OK, sure. I can send new patch series as new messages instead of replies 
to the same thread.


> The other question, which I neglected to ask yesterday, is what is the symptom
> of the bug? ie. does the system fail to boot or otherwise crash etc.?

This is briefly explained on cover-letter, but I can elaborate a bit 
more: I was testing driver issues on kernel 2.6.32 (RHEL 6.6), and when 
I tried the mainline kernel, the driver wasn't able to enable MSI-X 
capabilities. Interestingly, on kernel 4.1 this behavior doesn't happen 
and the driver can use MSI-X interrupts.

So, I figured that something was wrong and found the problem described 
on the patches. I tried the proposed solution (calling manually the 
function that is not reachable anymore) and it works.

Regarding the bnx2x driver, below are two dmesg outputs:

1) With kernel 4.2-rc7
bnx2x 0000:01:00.0: no msix capability found

2) With kernel 4.1
bnx2x 0000:01:00.0: msix capability found
bnx2x 0000:01:00.0 eth2: using MSI-X  IRQs: sp 24  fp[0] 26 ... fp[7] 33

> This is changes *in* v2, or since v1.

My bad, sorry.


> Or anywhere after the first '---', which means the version commentary is
> discarded in the final commit.

I used scissors, but there's no problem in stop using it in this list. 
Thanks for the suggestion.

Cheers

Re: [PATCH v2 1/2] PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code

[Michael Ellerman]

On Thu, 2015-08-20 at 16:10 -0300, Guilherme G. Piccoli wrote:
> On 08/19/2015 10:02 PM, Michael Ellerman wrote:
> > In future you should send a reply like the above to my mail, and then
> > separately send the new patch series. My preference is that the new series is
> > not a reply to anything, though some other maintainers may disagree on that
> > point.
> 
> OK, sure. I can send new patch series as new messages instead of replies 
> to the same thread.

Thanks.

> > The other question, which I neglected to ask yesterday, is what is the symptom
> > of the bug? ie. does the system fail to boot or otherwise crash etc.?
> 
> This is briefly explained on cover-letter, but I can elaborate a bit 

Sure, but the cover-letter is not committed, so the commit change logs need to
be self describing.

> more: I was testing driver issues on kernel 2.6.32 (RHEL 6.6), and when 
> I tried the mainline kernel, the driver wasn't able to enable MSI-X 
> capabilities. Interestingly, on kernel 4.1 this behavior doesn't happen 
> and the driver can use MSI-X interrupts.
> 
> So, I figured that something was wrong and found the problem described 
> on the patches. I tried the proposed solution (calling manually the 
> function that is not reachable anymore) and it works.
> 
> Regarding the bnx2x driver, below are two dmesg outputs:
> 
> 1) With kernel 4.2-rc7
> bnx2x 0000:01:00.0: no msix capability found

OK. This is because the initialisation of dev->msix_cap was lost due to commit
1851617cd2da.

> 2) With kernel 4.1
> bnx2x 0000:01:00.0: msix capability found
> bnx2x 0000:01:00.0 eth2: using MSI-X  IRQs: sp 24  fp[0] 26 ... fp[7] 33

OK. And I assume with these patches you see the above output again.

> > This is changes *in* v2, or since v1.
> 
> My bad, sorry.
> 
> > Or anywhere after the first '---', which means the version commentary is
> > discarded in the final commit.
> 
> I used scissors, but there's no problem in stop using it in this list.

Thanks, but my scripts don't grok scissors. So I prefer the commentary after
the '---'.

cheers

Re: [PATCH v2 1/2] PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code

[Guilherme G. Piccoli]

On 08/24/2015 04:37 AM, Michael Ellerman wrote:
>> more: I was testing driver issues on kernel 2.6.32 (RHEL 6.6), and when
>> I tried the mainline kernel, the driver wasn't able to enable MSI-X
>> capabilities. Interestingly, on kernel 4.1 this behavior doesn't happen
>> and the driver can use MSI-X interrupts.
>>
>> So, I figured that something was wrong and found the problem described
>> on the patches. I tried the proposed solution (calling manually the
>> function that is not reachable anymore) and it works.
>>
>> Regarding the bnx2x driver, below are two dmesg outputs:
>>
>> 1) With kernel 4.2-rc7
>> bnx2x 0000:01:00.0: no msix capability found
>
> OK. This is because the initialisation of dev->msix_cap was lost due to commit
> 1851617cd2da.
>
>> 2) With kernel 4.1
>> bnx2x 0000:01:00.0: msix capability found
>> bnx2x 0000:01:00.0 eth2: using MSI-X  IRQs: sp 24  fp[0] 26 ... fp[7] 33
>
> OK. And I assume with these patches you see the above output again.

Exactly. With the proposed patches applied, dev->msix_cap is initialized 
normally, so the driver is able to do its job as usual.


>>> Or anywhere after the first '---', which means the version commentary is
>>> discarded in the final commit.
>>
>> I used scissors, but there's no problem in stop using it in this list.
>
> Thanks, but my scripts don't grok scissors. So I prefer the commentary after
> the '---'.

Thanks for the info.

Cheers

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

[Guilherme G. Piccoli]

Since the commit 1851617cd2 ("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.

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

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

[Guilherme G. Piccoli]

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

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

[Bjorn Helgaas]

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

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

[Guilherme G. Piccoli]

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

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

[jeclark2006]

[-- Attachment #1: Type: text/plain, Size: 3702 bytes --]

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] <ml-node+s10917n98194h59@n7.nabble.com> 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. 
> 
> 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.

[-- Attachment #2: Type: text/html, Size: 5946 bytes --]

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

[Michael S. Tsirkin]

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

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

[Michael Ellerman]

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

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

[Guilherme G. Piccoli]

> 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

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

[Bjorn Helgaas]

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

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

[Guilherme G. Piccoli]

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

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

[Michael Ellerman]

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

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

[Guilherme G. Piccoli]

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

[PATCH 0/2] Disable MSI/MSI-X interrupts manually at PCI probe time in PowerPC architecture

[Guilherme G. Piccoli]

These 2 patches correct a bogus behaviour introduced by commit 1851617cd2
("PCI/MSI: Disable MSI at enumeration even if kernel doesn't support MSI").
The commit moved the logic responsible to disable MSI/MSI-X interrupts
at PCI probe time to a new function, named pci_msi_setup_pci_dev(), that
is not reachable in the code path of PowerPC pSeries platform.

Since then, devices aren't able to activate MSI/MSI-X capability, even
after boot. The first patch makes the function pci_msi_setup_pci_dev()
non-static. The second patch inserts a call to the function in powerpc
code, so it explicitly disables MSI/MSI-X interrupts at PCI probe time.

Guilherme G. Piccoli (2):
  PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code
  powerpc/PCI: Disable MSI/MSI-X interrupts at PCI probe time in OF case

 arch/powerpc/kernel/pci_of_scan.c | 3 +++
 drivers/pci/probe.c               | 2 +-
 include/linux/pci.h               | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.1.0

[PATCH 1/2] PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code

[Guilherme G. Piccoli]

Commit 1851617cd2 ("PCI/MSI: Disable MSI at enumeration even if kernel
doesn't support MSI") changed the location of the code that disables
MSI/MSI-X interrupts at PCI probe time in devices that have this flag set.
It moved the code from pci_msi_init_pci_dev() to a new function named
pci_msi_setup_pci_dev(), called by pci_setup_device().

Since then, the pSeries platform of the powerpc architecture needs to
disable MSI at PCI probe time manually, as the code flow doesn't
reach pci_setup_device(). For doing so, it wants to call
pci_msi_setup_pci_dev(). This patch makes the required function
non-static, so that it will be called on PCI probe path on powerpc pSeries
platform in next patch.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
---
 drivers/pci/probe.c | 2 +-
 include/linux/pci.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636..520c5b6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1103,7 +1103,7 @@ int pci_cfg_space_size(struct pci_dev *dev)
 
 #define LEGACY_IO_RESOURCE	(IORESOURCE_IO | IORESOURCE_PCI_FIXED)
 
-static void pci_msi_setup_pci_dev(struct pci_dev *dev)
+void pci_msi_setup_pci_dev(struct pci_dev *dev)
 {
 	/*
 	 * Disable the MSI hardware to avoid screaming interrupts
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8a0321a..860c751 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1202,6 +1202,7 @@ struct msix_entry {
 	u16	entry;	/* driver uses to specify entry, OS writes */
 };
 
+void pci_msi_setup_pci_dev(struct pci_dev *dev);
 
 #ifdef CONFIG_PCI_MSI
 int pci_msi_vec_count(struct pci_dev *dev);
-- 
2.1.0

Re: [PATCH 1/2] PCI: Make pci_msi_setup_pci_dev() non-static for use by arch code

[Michael Ellerman]

Hi Guilherme,

Thanks for the patches.

On Tue, 2015-08-18 at 18:13 -0300, Guilherme G. Piccoli wrote:
> Commit 1851617cd2 ("PCI/MSI: Disable MSI at enumeration even if kernel
> doesn't support MSI") changed the location of the code that disables
> MSI/MSI-X interrupts at PCI probe time in devices that have this flag set.
> It moved the code from pci_msi_init_pci_dev() to a new function named
> pci_msi_setup_pci_dev(), called by pci_setup_device().

OK.

> Since then, the pSeries platform of the powerpc architecture needs to
> disable MSI at PCI probe time manually, as the code flow doesn't
> reach pci_setup_device(). 
>
> For doing so, it wants to call
> pci_msi_setup_pci_dev(). This patch makes the required function
> non-static, so that it will be called on PCI probe path on powerpc pSeries
> platform in next patch.

I didn't follow that entirely, I think you mean something like:

  The pseries PCI probing code does not call pci_setup_device(), so since
  commit 1851617cd2 pci_msi_setup_pci_dev() is not called and MSIs are left
  enabled, which is a bug.

  To fix this the pseries PCI probe should manually call
  pci_msi_setup_pci_dev(), so make it non-static.


Does that look OK?

Also you haven't CC'ed the original author of the commit, or the PCI
maintainer, or the relevant lists.

That would be:

    Michael S. Tsirkin <mst@redhat.com>
    Bjorn Helgaas <bhelgaas@google.com>
    linux-pci@vger.kernel.org
    linux-kernel@vger.kernel.org


And finally both patches should have a fixes line, such as:

Fixes: 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel doesn't support MSI")

cheers

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

[Guilherme G. Piccoli]

Since the commit 1851617cd2 ("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.

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