diff mbox

Enable MSI/MSI-X caps and disable MSI interrupts at PCI probe time - code move

Message ID 20151201013540.GB9306@localhost (mailing list archive)
State Not Applicable
Headers show

Commit Message

Bjorn Helgaas Dec. 1, 2015, 1:35 a.m. UTC 
On Wed, Oct 21, 2015 at 12:17:35PM -0200, Guilherme G. Piccoli wrote:
> Commit 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel
> doesn't support MSI") changed the location of the code that initializes
> dev->msi_cap/msix_cap and 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().
> 
> In Open Firmware code path (PowerPC pSeries/SPARC archs) the function
> pci_setup_device() is not called, so MSI capabilities are never enabled,
> leading to error messages as:
> 
> 	bnx2x 0000:01:00.0: no msix capability found
> 
> Commit 4d9aac397a5d ("powerpc/PCI: Disable MSI/MSI-X interrupts at PCI
> probe time in OF case") solved the issue on PowerPC pSeries arch calling
> manually pci_msi_setup_pci_dev() on appropriate place. However, this
> modification does not solve the general case (SPARC arch should be
> modified too) and duplicates a lot of code, as pointed by Bjorn Helgaas.
> As suggested by him, worth to reorganize the code to generally solve the
> MSI caps issue and avoid too much code duplication.
> 
> This patch does exactly this: we remove both the pci_msi_setup_pci_dev()
> call from pci_setup_device() and the same call in OF code path of PowerPC
> pSeries arch. Then, we call pci_msi_setup_pci_dev() directly from
> pci_init_capabilities(). So, we can initialize MSI caps and disable MSI
> interruptions during PCI probe in general fashion, avoiding code
> duplication.
> 
> Notice that this patch has the same practical effect of reverting
> commit 1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel
> doesn't support MSI") and commit 4d9aac397a5d ("powerpc/PCI: Disable
> MSI/MSI-X interrupts at PCI probe time in OF case"). Regarding the
> former, the author called pci_msi_setup_pci_dev() from pci_setup_device()
> because there was an early quirk used in pci_msi_off(), which depended on
> pci_msi_setup_pci_dev(). Since pci_msi_off() was completely removed by
> commit c6201cd8513d ("PCI/MSI: Remove unused pci_msi_off()"), we can call
> pci_msi_setup_pci_dev() directly from pci_init_capabilities().
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>

Thanks for your patience, Guilherme.

I applied this to pci/msi for v4.5.  I reworked the changelog and made
pci_msi_setup_pci_dev() static, since I think it's now only called
from drivers/pci/probe.c.

I didn't quite follow the last paragraph about reverting 1851617cd2da.
1851617cd2da moved the MSI init (dev->msi_cap init and MSI disable)
from pci/msi.c (where it was only done when CONFIG_PCI_MSI=y) to
pci/probe.c (where we did it for all non-OF arches, regardless of
CONFIG_PCI_MSI).  So I would characterize this patch as doing the MSI
init for *all* arches, regardless of CONFIG_PCI_MSI.

While looking at this, I noticed that pci_msi_init_pci_dev() is now
always empty, so I'll remove that in a separate patch.

Please let me know if this doesn't make sense:


commit e80e7edc55ba711f3fe23975061b3f1c336ceb95
Author: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
Date:   Wed Oct 21 12:17:35 2015 -0200

    PCI/MSI: Initialize MSI capability for all architectures
    
    1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel doesn't
    support MSI") moved dev->msi_cap and dev->msix_cap initialization from the
    pci_init_capabilities() path (used on all architectures) to the
    pci_setup_device() path (not used on Open Firmware architectures).
    
    This broke MSI or MSI-X on Open Firmware machines.  4d9aac397a5d
    ("powerpc/PCI: Disable MSI/MSI-X interrupts at PCI probe time in OF case")
    fixed it for PowerPC but not for SPARC.
    
    Set up MSI and MSI-X (initialize msi_cap and msix_cap and disable MSI and
    MSI-X) in pci_init_capabilities() so all architectures do it the same way.
    
    This reverts 4d9aac397a5d since this patch fixes the problem generically
    for both PowerPC and SPARC.
    
    [bhelgaas: changelog, make pci_msi_setup_pci_dev() 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>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Comments

Guilherme G. Piccoli Dec. 1, 2015, 12:41 p.m. UTC | #1 
On 11/30/2015 11:35 PM, Bjorn Helgaas wrote:
> Thanks for your patience, Guilherme.

Hello Bjorn, thank you for the very deep review of my patch. Much 
appreciated!


> I applied this to pci/msi for v4.5.  I reworked the changelog and made
> pci_msi_setup_pci_dev() static, since I think it's now only called
> from drivers/pci/probe.c.

Thanks. Regarding pci_msi_setup_pci_dev(), it really should be static. I 
changed it because PowerPC arch was calling this since my last patch, 
but this patch remove the call from PowerPC arch, so it's great to see 
this function static again.


> I didn't quite follow the last paragraph about reverting 1851617cd2da.
> 1851617cd2da moved the MSI init (dev->msi_cap init and MSI disable)
> from pci/msi.c (where it was only done when CONFIG_PCI_MSI=y) to
> pci/probe.c (where we did it for all non-OF arches, regardless of
> CONFIG_PCI_MSI).  So I would characterize this patch as doing the MSI
> init for *all* arches, regardless of CONFIG_PCI_MSI.

Well, my English skills aren't so good, so I possibly I wrote confusing 
text. What I meant is that, from practical viewpoint, what we did here 
has the same effect of reverting those patches (in the sense of 
initializing MSI caps). But you can ignore my statement, and I 
appreciate that you removed it from the commit message if it was 
confusing heh


> While looking at this, I noticed that pci_msi_init_pci_dev() is now
> always empty, so I'll remove that in a separate patch.
>
> Please let me know if this doesn't make sense:

The patch below is great, thanks for the modifications and for removing 
pci_msi_init_pci_dev() in a separated patch.


> commit e80e7edc55ba711f3fe23975061b3f1c336ceb95
> Author: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
> Date:   Wed Oct 21 12:17:35 2015 -0200
>
>      PCI/MSI: Initialize MSI capability for all architectures
>
>      1851617cd2da ("PCI/MSI: Disable MSI at enumeration even if kernel doesn't
>      support MSI") moved dev->msi_cap and dev->msix_cap initialization from the
>      pci_init_capabilities() path (used on all architectures) to the
>      pci_setup_device() path (not used on Open Firmware architectures).
>
>      This broke MSI or MSI-X on Open Firmware machines.  4d9aac397a5d
>      ("powerpc/PCI: Disable MSI/MSI-X interrupts at PCI probe time in OF case")
>      fixed it for PowerPC but not for SPARC.
>
>      Set up MSI and MSI-X (initialize msi_cap and msix_cap and disable MSI and
>      MSI-X) in pci_init_capabilities() so all architectures do it the same way.
>
>      This reverts 4d9aac397a5d since this patch fixes the problem generically
>      for both PowerPC and SPARC.
>
>      [bhelgaas: changelog, make pci_msi_setup_pci_dev() 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>
>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
> index 2e710c1..526ac67 100644
> --- a/arch/powerpc/kernel/pci_of_scan.c
> +++ b/arch/powerpc/kernel/pci_of_scan.c
> @@ -187,9 +187,6 @@ struct pci_dev *of_create_pci_dev(struct device_node *node,
>
>   	pci_device_add(dev, bus);
>
> -	/* Setup MSI caps & disable MSI/MSI-X interrupts */
> -	pci_msi_setup_pci_dev(dev);
> -
>   	return dev;
>   }
>   EXPORT_SYMBOL(of_create_pci_dev);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index edb1984..cd94737 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1145,7 +1145,7 @@ int pci_cfg_space_size(struct pci_dev *dev)
>
>   #define LEGACY_IO_RESOURCE	(IORESOURCE_IO | IORESOURCE_PCI_FIXED)
>
> -void pci_msi_setup_pci_dev(struct pci_dev *dev)
> +static void pci_msi_setup_pci_dev(struct pci_dev *dev)
>   {
>   	/*
>   	 * Disable the MSI hardware to avoid screaming interrupts
> @@ -1212,8 +1212,6 @@ int pci_setup_device(struct pci_dev *dev)
>   	/* "Unknown power state" */
>   	dev->current_state = PCI_UNKNOWN;
>
> -	pci_msi_setup_pci_dev(dev);
> -
>   	/* Early fixups, before probing the BARs */
>   	pci_fixup_device(pci_fixup_early, dev);
>   	/* device class may be changed after fixup */
> @@ -1606,6 +1604,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
>   	/* MSI/MSI-X list */
>   	pci_msi_init_pci_dev(dev);
>
> +	/* Setup MSI caps & disable MSI/MSI-X interrupts */
> +	pci_msi_setup_pci_dev(dev);
> +
>   	/* Buffers for saving PCIe and PCI-X capabilities */
>   	pci_allocate_cap_save_buffers(dev);
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e828e7b..f9f79ad 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1248,8 +1248,6 @@ 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);
>   void pci_msi_shutdown(struct pci_dev *dev);
>

Cheers,


Guilherme Piccoli
Bjorn Helgaas Dec. 1, 2015, 2:39 p.m. UTC | #2 
On Tue, Dec 01, 2015 at 10:41:42AM -0200, Guilherme G. Piccoli wrote:
> On 11/30/2015 11:35 PM, Bjorn Helgaas wrote:
> >Thanks for your patience, Guilherme.
> 
> Hello Bjorn, thank you for the very deep review of my patch. Much
> appreciated!
> 
> 
> >I applied this to pci/msi for v4.5.  I reworked the changelog and made
> >pci_msi_setup_pci_dev() static, since I think it's now only called
> >from drivers/pci/probe.c.
> 
> Thanks. Regarding pci_msi_setup_pci_dev(), it really should be
> static. I changed it because PowerPC arch was calling this since my
> last patch, but this patch remove the call from PowerPC arch, so
> it's great to see this function static again.
> 
> 
> >I didn't quite follow the last paragraph about reverting 1851617cd2da.
> >1851617cd2da moved the MSI init (dev->msi_cap init and MSI disable)
> >from pci/msi.c (where it was only done when CONFIG_PCI_MSI=y) to
> >pci/probe.c (where we did it for all non-OF arches, regardless of
> >CONFIG_PCI_MSI).  So I would characterize this patch as doing the MSI
> >init for *all* arches, regardless of CONFIG_PCI_MSI.
> 
> Well, my English skills aren't so good, so I possibly I wrote
> confusing text. What I meant is that, from practical viewpoint, what
> we did here has the same effect of reverting those patches (in the
> sense of initializing MSI caps). But you can ignore my statement,
> and I appreciate that you removed it from the commit message if it
> was confusing heh

No problem, just making sure I wasn't missing something.  It's not a
literal revert, but it certainly does touch the same code.

Bjorn
diff mbox

Patch

diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
index 2e710c1..526ac67 100644
--- a/arch/powerpc/kernel/pci_of_scan.c
+++ b/arch/powerpc/kernel/pci_of_scan.c
@@ -187,9 +187,6 @@  struct pci_dev *of_create_pci_dev(struct device_node *node,
 
 	pci_device_add(dev, bus);
 
-	/* Setup MSI caps & disable MSI/MSI-X interrupts */
-	pci_msi_setup_pci_dev(dev);
-
 	return dev;
 }
 EXPORT_SYMBOL(of_create_pci_dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index edb1984..cd94737 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1145,7 +1145,7 @@  int pci_cfg_space_size(struct pci_dev *dev)
 
 #define LEGACY_IO_RESOURCE	(IORESOURCE_IO | IORESOURCE_PCI_FIXED)
 
-void pci_msi_setup_pci_dev(struct pci_dev *dev)
+static void pci_msi_setup_pci_dev(struct pci_dev *dev)
 {
 	/*
 	 * Disable the MSI hardware to avoid screaming interrupts
@@ -1212,8 +1212,6 @@  int pci_setup_device(struct pci_dev *dev)
 	/* "Unknown power state" */
 	dev->current_state = PCI_UNKNOWN;
 
-	pci_msi_setup_pci_dev(dev);
-
 	/* Early fixups, before probing the BARs */
 	pci_fixup_device(pci_fixup_early, dev);
 	/* device class may be changed after fixup */
@@ -1606,6 +1604,9 @@  static void pci_init_capabilities(struct pci_dev *dev)
 	/* MSI/MSI-X list */
 	pci_msi_init_pci_dev(dev);
 
+	/* Setup MSI caps & disable MSI/MSI-X interrupts */
+	pci_msi_setup_pci_dev(dev);
+
 	/* Buffers for saving PCIe and PCI-X capabilities */
 	pci_allocate_cap_save_buffers(dev);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e828e7b..f9f79ad 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1248,8 +1248,6 @@  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);
 void pci_msi_shutdown(struct pci_dev *dev);