[PATCH 0/2] PCI/MSI: Remove arch_msi_check_device()

[PATCH 0/2] PCI/MSI: Remove arch_msi_check_device()

[Alexander Gordeev]

Hello,

This is a cleanup effort to get rid of useless arch_msi_check_device().
I am not sure what were the reasons for its existence in the first place,
but at the moment it appears totally unnecessary.

Thanks!

Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-pci@vger.kernel.org

Alexander Gordeev (2):
  PCI/MSI/PPC: Remove arch_msi_check_device()
  PCI/MSI: Remove arch_msi_check_device()

 arch/powerpc/include/asm/machdep.h     |    2 -
 arch/powerpc/kernel/msi.c              |   12 +-------
 arch/powerpc/platforms/cell/axon_msi.c |    9 ------
 arch/powerpc/platforms/powernv/pci.c   |   19 +++---------
 arch/powerpc/platforms/pseries/msi.c   |   42 ++++++++++-----------------
 arch/powerpc/sysdev/fsl_msi.c          |   12 ++------
 arch/powerpc/sysdev/mpic_pasemi_msi.c  |   11 +------
 arch/powerpc/sysdev/mpic_u3msi.c       |   28 +++++++-----------
 arch/powerpc/sysdev/ppc4xx_hsta_msi.c  |   18 ++++--------
 arch/powerpc/sysdev/ppc4xx_msi.c       |   19 ++++--------
 drivers/pci/msi.c                      |   49 ++++++++-----------------------
 include/linux/msi.h                    |    3 --
 12 files changed, 63 insertions(+), 161 deletions(-)

-- 
1.7.7.6

[PATCH 1/2] PCI/MSI/PPC: Remove arch_msi_check_device()

[Alexander Gordeev]

PowerPC is the only architecture that makes use of hook
arch_msi_check_device() and does perform some checks to
figure out if MSI/MSI-X could be enabled for a device.
However, there are no reasons why those checks could not
be done within arch_setup_msi_irqs() hook.

Moving MSI checks into arch_setup_msi_irqs() makes code
more readable and allows getting rid of unnecessary hook
arch_msi_check_device().

Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-pci@vger.kernel.org
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 arch/powerpc/include/asm/machdep.h     |    2 -
 arch/powerpc/kernel/msi.c              |   12 +--------
 arch/powerpc/platforms/cell/axon_msi.c |    9 -------
 arch/powerpc/platforms/powernv/pci.c   |   19 ++++----------
 arch/powerpc/platforms/pseries/msi.c   |   42 ++++++++++++-------------------
 arch/powerpc/sysdev/fsl_msi.c          |   12 ++-------
 arch/powerpc/sysdev/mpic_pasemi_msi.c  |   11 +-------
 arch/powerpc/sysdev/mpic_u3msi.c       |   28 ++++++++-------------
 arch/powerpc/sysdev/ppc4xx_hsta_msi.c  |   18 ++++---------
 arch/powerpc/sysdev/ppc4xx_msi.c       |   19 ++++----------
 10 files changed, 50 insertions(+), 122 deletions(-)

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index f92b0b5..d05aa76 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -136,8 +136,6 @@ struct machdep_calls {
 	int		(*pci_setup_phb)(struct pci_controller *host);
 
 #ifdef CONFIG_PCI_MSI
-	int		(*msi_check_device)(struct pci_dev* dev,
-					    int nvec, int type);
 	int		(*setup_msi_irqs)(struct pci_dev *dev,
 					  int nvec, int type);
 	void		(*teardown_msi_irqs)(struct pci_dev *dev);
diff --git a/arch/powerpc/kernel/msi.c b/arch/powerpc/kernel/msi.c
index 8bbc12d..71bd161 100644
--- a/arch/powerpc/kernel/msi.c
+++ b/arch/powerpc/kernel/msi.c
@@ -13,7 +13,7 @@
 
 #include <asm/machdep.h>
 
-int arch_msi_check_device(struct pci_dev* dev, int nvec, int type)
+int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
 	if (!ppc_md.setup_msi_irqs || !ppc_md.teardown_msi_irqs) {
 		pr_debug("msi: Platform doesn't provide MSI callbacks.\n");
@@ -24,16 +24,6 @@ int arch_msi_check_device(struct pci_dev* dev, int nvec, int type)
 	if (type == PCI_CAP_ID_MSI && nvec > 1)
 		return 1;
 
-	if (ppc_md.msi_check_device) {
-		pr_debug("msi: Using platform check routine.\n");
-		return ppc_md.msi_check_device(dev, nvec, type);
-	}
-
-        return 0;
-}
-
-int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
-{
 	return ppc_md.setup_msi_irqs(dev, nvec, type);
 }
 
diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
index 85825b5..862b327 100644
--- a/arch/powerpc/platforms/cell/axon_msi.c
+++ b/arch/powerpc/platforms/cell/axon_msi.c
@@ -199,14 +199,6 @@ out_error:
 	return msic;
 }
 
-static int axon_msi_check_device(struct pci_dev *dev, int nvec, int type)
-{
-	if (!find_msi_translator(dev))
-		return -ENODEV;
-
-	return 0;
-}
-
 static int setup_msi_msg_address(struct pci_dev *dev, struct msi_msg *msg)
 {
 	struct device_node *dn;
@@ -416,7 +408,6 @@ static int axon_msi_probe(struct platform_device *device)
 
 	ppc_md.setup_msi_irqs = axon_msi_setup_msi_irqs;
 	ppc_md.teardown_msi_irqs = axon_msi_teardown_msi_irqs;
-	ppc_md.msi_check_device = axon_msi_check_device;
 
 	axon_msi_debug_setup(dn, msic);
 
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index f91a4e5..987b677 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -46,29 +46,21 @@
 //#define cfg_dbg(fmt...)	printk(fmt)
 
 #ifdef CONFIG_PCI_MSI
-static int pnv_msi_check_device(struct pci_dev* pdev, int nvec, int type)
-{
-	struct pci_controller *hose = pci_bus_to_host(pdev->bus);
-	struct pnv_phb *phb = hose->private_data;
-	struct pci_dn *pdn = pci_get_pdn(pdev);
-
-	if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
-		return -ENODEV;
-
-	return (phb && phb->msi_bmp.bitmap) ? 0 : -ENODEV;
-}
-
 static int pnv_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 {
 	struct pci_controller *hose = pci_bus_to_host(pdev->bus);
 	struct pnv_phb *phb = hose->private_data;
+	struct pci_dn *pdn = pci_get_pdn(pdev);
 	struct msi_desc *entry;
 	struct msi_msg msg;
 	int hwirq;
 	unsigned int virq;
 	int rc;
 
-	if (WARN_ON(!phb))
+	if (WARN_ON(!phb) || !phb->msi_bmp.bitmap)
+		return -ENODEV;
+
+	if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
 		return -ENODEV;
 
 	list_for_each_entry(entry, &pdev->msi_list, list) {
@@ -810,7 +802,6 @@ void __init pnv_pci_init(void)
 
 	/* Configure MSIs */
 #ifdef CONFIG_PCI_MSI
-	ppc_md.msi_check_device = pnv_msi_check_device;
 	ppc_md.setup_msi_irqs = pnv_setup_msi_irqs;
 	ppc_md.teardown_msi_irqs = pnv_teardown_msi_irqs;
 #endif
diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 0c882e8..24494fc 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -335,26 +335,6 @@ out:
 	return request;
 }
 
-static int rtas_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
-	int quota, rc;
-
-	if (type == PCI_CAP_ID_MSIX)
-		rc = check_req_msix(pdev, nvec);
-	else
-		rc = check_req_msi(pdev, nvec);
-
-	if (rc)
-		return rc;
-
-	quota = msi_quota_for_device(pdev, nvec);
-
-	if (quota && quota < nvec)
-		return quota;
-
-	return 0;
-}
-
 static int check_msix_entries(struct pci_dev *pdev)
 {
 	struct msi_desc *entry;
@@ -396,15 +376,24 @@ static void rtas_hack_32bit_msi_gen2(struct pci_dev *pdev)
 static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
 {
 	struct pci_dn *pdn;
-	int hwirq, virq, i, rc;
+	int hwirq, virq, i, quota, rc;
 	struct msi_desc *entry;
 	struct msi_msg msg;
 	int nvec = nvec_in;
 	int use_32bit_msi_hack = 0;
 
-	pdn = pci_get_pdn(pdev);
-	if (!pdn)
-		return -ENODEV;
+	if (type == PCI_CAP_ID_MSIX)
+		rc = check_req_msix(pdev, nvec);
+	else
+		rc = check_req_msi(pdev, nvec);
+
+	if (rc)
+		return rc;
+
+	quota = msi_quota_for_device(pdev, nvec);
+
+	if (quota && quota < nvec)
+		return quota;
 
 	if (type == PCI_CAP_ID_MSIX && check_msix_entries(pdev))
 		return -EINVAL;
@@ -415,12 +404,14 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
 	 */
 	if (type == PCI_CAP_ID_MSIX) {
 		int m = roundup_pow_of_two(nvec);
-		int quota = msi_quota_for_device(pdev, m);
+		quota = msi_quota_for_device(pdev, m);
 
 		if (quota >= m)
 			nvec = m;
 	}
 
+	pdn = pci_get_pdn(pdev);
+
 	/*
 	 * Try the new more explicit firmware interface, if that fails fall
 	 * back to the old interface. The old interface is known to never
@@ -525,7 +516,6 @@ static int rtas_msi_init(void)
 	WARN_ON(ppc_md.setup_msi_irqs);
 	ppc_md.setup_msi_irqs = rtas_setup_msi_irqs;
 	ppc_md.teardown_msi_irqs = rtas_teardown_msi_irqs;
-	ppc_md.msi_check_device = rtas_msi_check_device;
 
 	WARN_ON(ppc_md.pci_irq_fixup);
 	ppc_md.pci_irq_fixup = rtas_msi_pci_irq_fixup;
diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
index 77efbae..b32e79d 100644
--- a/arch/powerpc/sysdev/fsl_msi.c
+++ b/arch/powerpc/sysdev/fsl_msi.c
@@ -109,14 +109,6 @@ static int fsl_msi_init_allocator(struct fsl_msi *msi_data)
 	return 0;
 }
 
-static int fsl_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
-	if (type == PCI_CAP_ID_MSIX)
-		pr_debug("fslmsi: MSI-X untested, trying anyway.\n");
-
-	return 0;
-}
-
 static void fsl_teardown_msi_irqs(struct pci_dev *pdev)
 {
 	struct msi_desc *entry;
@@ -173,6 +165,9 @@ static int fsl_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 	struct msi_msg msg;
 	struct fsl_msi *msi_data;
 
+	if (type == PCI_CAP_ID_MSIX)
+		pr_debug("fslmsi: MSI-X untested, trying anyway.\n");
+
 	/*
 	 * If the PCI node has an fsl,msi property, then we need to use it
 	 * to find the specific MSI.
@@ -527,7 +522,6 @@ static int fsl_of_msi_probe(struct platform_device *dev)
 	if (!ppc_md.setup_msi_irqs) {
 		ppc_md.setup_msi_irqs = fsl_setup_msi_irqs;
 		ppc_md.teardown_msi_irqs = fsl_teardown_msi_irqs;
-		ppc_md.msi_check_device = fsl_msi_check_device;
 	} else if (ppc_md.setup_msi_irqs != fsl_setup_msi_irqs) {
 		dev_err(&dev->dev, "Different MSI driver already installed!\n");
 		err = -ENODEV;
diff --git a/arch/powerpc/sysdev/mpic_pasemi_msi.c b/arch/powerpc/sysdev/mpic_pasemi_msi.c
index 38e6238..15dccd3 100644
--- a/arch/powerpc/sysdev/mpic_pasemi_msi.c
+++ b/arch/powerpc/sysdev/mpic_pasemi_msi.c
@@ -63,14 +63,6 @@ static struct irq_chip mpic_pasemi_msi_chip = {
 	.name			= "PASEMI-MSI",
 };
 
-static int pasemi_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
-	if (type == PCI_CAP_ID_MSIX)
-		pr_debug("pasemi_msi: MSI-X untested, trying anyway\n");
-
-	return 0;
-}
-
 static void pasemi_msi_teardown_msi_irqs(struct pci_dev *pdev)
 {
 	struct msi_desc *entry;
@@ -97,6 +89,8 @@ static int pasemi_msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 	struct msi_msg msg;
 	int hwirq;
 
+	if (type == PCI_CAP_ID_MSIX)
+		pr_debug("pasemi_msi: MSI-X untested, trying anyway\n");
 	pr_debug("pasemi_msi_setup_msi_irqs, pdev %p nvec %d type %d\n",
 		 pdev, nvec, type);
 
@@ -169,7 +163,6 @@ int mpic_pasemi_msi_init(struct mpic *mpic)
 	WARN_ON(ppc_md.setup_msi_irqs);
 	ppc_md.setup_msi_irqs = pasemi_msi_setup_msi_irqs;
 	ppc_md.teardown_msi_irqs = pasemi_msi_teardown_msi_irqs;
-	ppc_md.msi_check_device = pasemi_msi_check_device;
 
 	return 0;
 }
diff --git a/arch/powerpc/sysdev/mpic_u3msi.c b/arch/powerpc/sysdev/mpic_u3msi.c
index 9a7aa0e..623d7fb 100644
--- a/arch/powerpc/sysdev/mpic_u3msi.c
+++ b/arch/powerpc/sysdev/mpic_u3msi.c
@@ -105,22 +105,6 @@ static u64 find_u4_magic_addr(struct pci_dev *pdev, unsigned int hwirq)
 	return 0;
 }
 
-static int u3msi_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
-	if (type == PCI_CAP_ID_MSIX)
-		pr_debug("u3msi: MSI-X untested, trying anyway.\n");
-
-	/* If we can't find a magic address then MSI ain't gonna work */
-	if (find_ht_magic_addr(pdev, 0) == 0 &&
-	    find_u4_magic_addr(pdev, 0) == 0) {
-		pr_debug("u3msi: no magic address found for %s\n",
-			 pci_name(pdev));
-		return -ENXIO;
-	}
-
-	return 0;
-}
-
 static void u3msi_teardown_msi_irqs(struct pci_dev *pdev)
 {
 	struct msi_desc *entry;
@@ -146,6 +130,17 @@ static int u3msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
 	u64 addr;
 	int hwirq;
 
+	if (type == PCI_CAP_ID_MSIX)
+		pr_debug("u3msi: MSI-X untested, trying anyway.\n");
+
+	/* If we can't find a magic address then MSI ain't gonna work */
+	if (find_ht_magic_addr(pdev, 0) == 0 &&
+	    find_u4_magic_addr(pdev, 0) == 0) {
+		pr_debug("u3msi: no magic address found for %s\n",
+			 pci_name(pdev));
+		return -ENXIO;
+	}
+
 	list_for_each_entry(entry, &pdev->msi_list, list) {
 		hwirq = msi_bitmap_alloc_hwirqs(&msi_mpic->msi_bitmap, 1);
 		if (hwirq < 0) {
@@ -202,7 +197,6 @@ int mpic_u3msi_init(struct mpic *mpic)
 	WARN_ON(ppc_md.setup_msi_irqs);
 	ppc_md.setup_msi_irqs = u3msi_setup_msi_irqs;
 	ppc_md.teardown_msi_irqs = u3msi_teardown_msi_irqs;
-	ppc_md.msi_check_device = u3msi_msi_check_device;
 
 	return 0;
 }
diff --git a/arch/powerpc/sysdev/ppc4xx_hsta_msi.c b/arch/powerpc/sysdev/ppc4xx_hsta_msi.c
index 11c8884..a6a4dbd 100644
--- a/arch/powerpc/sysdev/ppc4xx_hsta_msi.c
+++ b/arch/powerpc/sysdev/ppc4xx_hsta_msi.c
@@ -44,6 +44,12 @@ static int hsta_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 	int irq, hwirq;
 	u64 addr;
 
+	/* We don't support MSI-X */
+	if (type == PCI_CAP_ID_MSIX) {
+		pr_debug("%s: MSI-X not supported.\n", __func__);
+		return -EINVAL;
+	}
+
 	list_for_each_entry(entry, &dev->msi_list, list) {
 		irq = msi_bitmap_alloc_hwirqs(&ppc4xx_hsta_msi.bmp, 1);
 		if (irq < 0) {
@@ -117,17 +123,6 @@ static void hsta_teardown_msi_irqs(struct pci_dev *dev)
 	}
 }
 
-static int hsta_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
-	/* We don't support MSI-X */
-	if (type == PCI_CAP_ID_MSIX) {
-		pr_debug("%s: MSI-X not supported.\n", __func__);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int hsta_msi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -178,7 +173,6 @@ static int hsta_msi_probe(struct platform_device *pdev)
 
 	ppc_md.setup_msi_irqs = hsta_setup_msi_irqs;
 	ppc_md.teardown_msi_irqs = hsta_teardown_msi_irqs;
-	ppc_md.msi_check_device = hsta_msi_check_device;
 	return 0;
 
 out2:
diff --git a/arch/powerpc/sysdev/ppc4xx_msi.c b/arch/powerpc/sysdev/ppc4xx_msi.c
index 43948da..8577d82 100644
--- a/arch/powerpc/sysdev/ppc4xx_msi.c
+++ b/arch/powerpc/sysdev/ppc4xx_msi.c
@@ -85,8 +85,12 @@ static int ppc4xx_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 	struct msi_desc *entry;
 	struct ppc4xx_msi *msi_data = &ppc4xx_msi;
 
-	msi_data->msi_virqs = kmalloc((msi_irqs) * sizeof(int),
-					    GFP_KERNEL);
+	dev_dbg(&pdev->dev, "PCIE-MSI:%s called. vec %x type %d\n",
+		__func__, nvec, type);
+	if (type == PCI_CAP_ID_MSIX)
+		pr_debug("ppc4xx msi: MSI-X untested, trying anyway.\n");
+
+	msi_data->msi_virqs = kmalloc((msi_irqs) * sizeof(int), GFP_KERNEL);
 	if (!msi_data->msi_virqs)
 		return -ENOMEM;
 
@@ -134,16 +138,6 @@ void ppc4xx_teardown_msi_irqs(struct pci_dev *dev)
 	}
 }
 
-static int ppc4xx_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
-	dev_dbg(&pdev->dev, "PCIE-MSI:%s called. vec %x type %d\n",
-		__func__, nvec, type);
-	if (type == PCI_CAP_ID_MSIX)
-		pr_debug("ppc4xx msi: MSI-X untested, trying anyway.\n");
-
-	return 0;
-}
-
 static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
 				 struct resource res, struct ppc4xx_msi *msi)
 {
@@ -259,7 +253,6 @@ static int ppc4xx_msi_probe(struct platform_device *dev)
 
 	ppc_md.setup_msi_irqs = ppc4xx_setup_msi_irqs;
 	ppc_md.teardown_msi_irqs = ppc4xx_teardown_msi_irqs;
-	ppc_md.msi_check_device = ppc4xx_msi_check_device;
 	return err;
 
 error_out:
-- 
1.7.7.6

Re: [PATCH 1/2] PCI/MSI/PPC: Remove arch_msi_check_device()

[Alexander Gordeev]

On Sat, Jul 12, 2014 at 01:21:07PM +0200, Alexander Gordeev wrote:
> PowerPC is the only architecture that makes use of hook
> arch_msi_check_device() and does perform some checks to
> figure out if MSI/MSI-X could be enabled for a device.
> However, there are no reasons why those checks could not
> be done within arch_setup_msi_irqs() hook.
> 
> Moving MSI checks into arch_setup_msi_irqs() makes code
> more readable and allows getting rid of unnecessary hook
> arch_msi_check_device().

Michael, Ben,

Any feedback?

Thanks!

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/2] PCI/MSI/PPC: Remove arch_msi_check_device()

[Alexander Gordeev]

On Thu, Jul 31, 2014 at 03:53:33PM +0200, Alexander Gordeev wrote:
> Michael, Ben,
> 
> Any feedback?

Michael, Ben?

> Thanks!

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 1/2] PCI/MSI/PPC: Remove arch_msi_check_device()

[Michael Ellerman]

On Sat, 2014-07-12 at 13:21 +0200, Alexander Gordeev wrote:
> PowerPC is the only architecture that makes use of hook
> arch_msi_check_device() and does perform some checks to
> figure out if MSI/MSI-X could be enabled for a device.
> However, there are no reasons why those checks could not
> be done within arch_setup_msi_irqs() hook.
> 
> Moving MSI checks into arch_setup_msi_irqs() makes code
> more readable and allows getting rid of unnecessary hook
> arch_msi_check_device().

The intention was that this hook allowed a platform to reject the request
early, ie. before all the setup had been done. But if no one except us is using
it then fine, get rid of it.

Acked-by: Michael Ellerman <mpe@ellerman.id.au>

cheers

[PATCH 2/2] PCI/MSI: Remove arch_msi_check_device()

[Alexander Gordeev]

There are no archs that override arch_msi_check_device()
hook. Remove it as it is completely redundant.

If an arch would need to check MSI/MSI-X possibility for a
device it should make it within arch_setup_msi_irqs() hook.

Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-pci@vger.kernel.org
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/pci/msi.c   |   49 +++++++++++++------------------------------------
 include/linux/msi.h |    3 ---
 2 files changed, 13 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 13f3d30..19ac058 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -56,16 +56,6 @@ void __weak arch_teardown_msi_irq(unsigned int irq)
 	chip->teardown_irq(chip, irq);
 }
 
-int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
-{
-	struct msi_chip *chip = dev->bus->msi;
-
-	if (!chip || !chip->check_device)
-		return 0;
-
-	return chip->check_device(chip, dev, nvec, type);
-}
-
 int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
 	struct msi_desc *entry;
@@ -809,22 +799,23 @@ out_free:
 }
 
 /**
- * pci_msi_check_device - check whether MSI may be enabled on a device
+ * msi_check_device - check whether MSI may be enabled on a device
  * @dev: pointer to the pci_dev data structure of MSI device function
  * @nvec: how many MSIs have been requested ?
- * @type: are we checking for MSI or MSI-X ?
  *
  * Look at global flags, the device itself, and its parent buses
  * to determine if MSI/-X are supported for the device. If MSI/-X is
  * supported return 0, else return an error code.
  **/
-static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
+static int msi_check_device(struct pci_dev *dev, int nvec)
 {
 	struct pci_bus *bus;
-	int ret;
 
 	/* MSI must be globally enabled and supported by the device */
-	if (!pci_msi_enable || !dev || dev->no_msi)
+	if (!pci_msi_enable)
+		return -EINVAL;
+
+	if (!dev || dev->no_msi || dev->current_state != PCI_D0)
 		return -EINVAL;
 
 	/*
@@ -846,10 +837,6 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
 		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
 			return -EINVAL;
 
-	ret = arch_msi_check_device(dev, nvec, type);
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
@@ -954,13 +941,13 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
 	int status, nr_entries;
 	int i, j;
 
-	if (!entries || !dev->msix_cap || dev->current_state != PCI_D0)
-		return -EINVAL;
-
-	status = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSIX);
+	status = msi_check_device(dev, nvec);
 	if (status)
 		return status;
 
+	if (!entries)
+		return -EINVAL;
+
 	nr_entries = pci_msix_vec_count(dev);
 	if (nr_entries < 0)
 		return nr_entries;
@@ -1085,8 +1072,9 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
 	int nvec;
 	int rc;
 
-	if (dev->current_state != PCI_D0)
-		return -EINVAL;
+	rc = msi_check_device(dev, minvec);
+	if (rc)
+		return rc;
 
 	WARN_ON(!!dev->msi_enabled);
 
@@ -1109,17 +1097,6 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
 		nvec = maxvec;
 
 	do {
-		rc = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSI);
-		if (rc < 0) {
-			return rc;
-		} else if (rc > 0) {
-			if (rc < minvec)
-				return -ENOSPC;
-			nvec = rc;
-		}
-	} while (rc);
-
-	do {
 		rc = msi_capability_init(dev, nvec);
 		if (rc < 0) {
 			return rc;
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 92a2f99..3b873bc 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -59,7 +59,6 @@ int arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc);
 void arch_teardown_msi_irq(unsigned int irq);
 int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
 void arch_teardown_msi_irqs(struct pci_dev *dev);
-int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
 void arch_restore_msi_irqs(struct pci_dev *dev);
 
 void default_teardown_msi_irqs(struct pci_dev *dev);
@@ -76,8 +75,6 @@ struct msi_chip {
 	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
 			 struct msi_desc *desc);
 	void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
-	int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
-			    int nvec, int type);
 };
 
 #endif /* LINUX_MSI_H */
-- 
1.7.7.6

Re: [PATCH 2/2] PCI/MSI: Remove arch_msi_check_device()

[Yijing Wang]

>  /**
> - * pci_msi_check_device - check whether MSI may be enabled on a device
> + * msi_check_device - check whether MSI may be enabled on a device
>   * @dev: pointer to the pci_dev data structure of MSI device function
>   * @nvec: how many MSIs have been requested ?
> - * @type: are we checking for MSI or MSI-X ?
>   *
>   * Look at global flags, the device itself, and its parent buses
>   * to determine if MSI/-X are supported for the device. If MSI/-X is
>   * supported return 0, else return an error code.
>   **/
> -static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +static int msi_check_device(struct pci_dev *dev, int nvec)
>  {
>  	struct pci_bus *bus;
> -	int ret;
>  
>  	/* MSI must be globally enabled and supported by the device */
> -	if (!pci_msi_enable || !dev || dev->no_msi)
> +	if (!pci_msi_enable)
> +		return -EINVAL;
> +
> +	if (!dev || dev->no_msi || dev->current_state != PCI_D0)
>  		return -EINVAL;
>  
>  	/*
> @@ -846,10 +837,6 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
>  		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
>  			return -EINVAL;
>  
> -	ret = arch_msi_check_device(dev, nvec, type);
> -	if (ret)
> -		return ret;
> -

Move the arch_msi_check_device() into arch_msi_setup_irq(), make we can not detect whether the device in this platform
supports MSI or MSI-X aeap. If we delay this, maybe we will do a lot unnecessary working for MSI/MSI-X setup.

Thanks!
Yijing.

>  	return 0;
>  }
>  
> @@ -954,13 +941,13 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
>  	int status, nr_entries;
>  	int i, j;
>  
> -	if (!entries || !dev->msix_cap || dev->current_state != PCI_D0)
> -		return -EINVAL;
> -
> -	status = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSIX);
> +	status = msi_check_device(dev, nvec);
>  	if (status)
>  		return status;
>  
> +	if (!entries)
> +		return -EINVAL;
> +
>  	nr_entries = pci_msix_vec_count(dev);
>  	if (nr_entries < 0)
>  		return nr_entries;
> @@ -1085,8 +1072,9 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
>  	int nvec;
>  	int rc;
>  
> -	if (dev->current_state != PCI_D0)
> -		return -EINVAL;
> +	rc = msi_check_device(dev, minvec);
> +	if (rc)
> +		return rc;
>  
>  	WARN_ON(!!dev->msi_enabled);
>  
> @@ -1109,17 +1097,6 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
>  		nvec = maxvec;
>  
>  	do {
> -		rc = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSI);
> -		if (rc < 0) {
> -			return rc;
> -		} else if (rc > 0) {
> -			if (rc < minvec)
> -				return -ENOSPC;
> -			nvec = rc;
> -		}
> -	} while (rc);
> -
> -	do {
>  		rc = msi_capability_init(dev, nvec);
>  		if (rc < 0) {
>  			return rc;
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 92a2f99..3b873bc 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -59,7 +59,6 @@ int arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc);
>  void arch_teardown_msi_irq(unsigned int irq);
>  int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
>  void arch_teardown_msi_irqs(struct pci_dev *dev);
> -int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
>  void arch_restore_msi_irqs(struct pci_dev *dev);
>  
>  void default_teardown_msi_irqs(struct pci_dev *dev);
> @@ -76,8 +75,6 @@ struct msi_chip {
>  	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
>  			 struct msi_desc *desc);
>  	void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
> -	int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
> -			    int nvec, int type);
>  };
>  
>  #endif /* LINUX_MSI_H */
> 


-- 
Thanks!
Yijing

Re: [PATCH 2/2] PCI/MSI: Remove arch_msi_check_device()

[Alexander Gordeev]

On Mon, Jul 14, 2014 at 10:11:57AM +0800, Yijing Wang wrote:
> >  /**
> > - * pci_msi_check_device - check whether MSI may be enabled on a device
> > + * msi_check_device - check whether MSI may be enabled on a device
> >   * @dev: pointer to the pci_dev data structure of MSI device function
> >   * @nvec: how many MSIs have been requested ?
> > - * @type: are we checking for MSI or MSI-X ?
> >   *
> >   * Look at global flags, the device itself, and its parent buses
> >   * to determine if MSI/-X are supported for the device. If MSI/-X is
> >   * supported return 0, else return an error code.
> >   **/
> > -static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
> > +static int msi_check_device(struct pci_dev *dev, int nvec)
> >  {
> >  	struct pci_bus *bus;
> > -	int ret;
> >  
> >  	/* MSI must be globally enabled and supported by the device */
> > -	if (!pci_msi_enable || !dev || dev->no_msi)
> > +	if (!pci_msi_enable)
> > +		return -EINVAL;
> > +
> > +	if (!dev || dev->no_msi || dev->current_state != PCI_D0)
> >  		return -EINVAL;
> >  
> >  	/*
> > @@ -846,10 +837,6 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
> >  		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> >  			return -EINVAL;
> >  
> > -	ret = arch_msi_check_device(dev, nvec, type);
> > -	if (ret)
> > -		return ret;
> > -
> 
> Move the arch_msi_check_device() into arch_msi_setup_irq(), make we can not detect whether the device in this platform
> supports MSI or MSI-X aeap. If we delay this, maybe we will do a lot unnecessary working for MSI/MSI-X setup.

A traditional approach for a function is first to make sanity check and
then allocate resources. I do not see a reason to keep these two steps
in separate functions: arch_msi_check_device() and arch_setup_msi_irq().

Just make checks within arch_setup_msi_irq() and bail out early would be as
cheap as it is now, but more natural and would deflate the interface.

Moreover, some platforms duplicate checks in arch_msi_check_device() and
arch_setup_msi_irq(), which does not add to readability.

> Thanks!
> Yijing.
> 
> >  	return 0;
> >  }
> >  
> > @@ -954,13 +941,13 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
> >  	int status, nr_entries;
> >  	int i, j;
> >  
> > -	if (!entries || !dev->msix_cap || dev->current_state != PCI_D0)
> > -		return -EINVAL;
> > -
> > -	status = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSIX);
> > +	status = msi_check_device(dev, nvec);
> >  	if (status)
> >  		return status;
> >  
> > +	if (!entries)
> > +		return -EINVAL;
> > +
> >  	nr_entries = pci_msix_vec_count(dev);
> >  	if (nr_entries < 0)
> >  		return nr_entries;
> > @@ -1085,8 +1072,9 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
> >  	int nvec;
> >  	int rc;
> >  
> > -	if (dev->current_state != PCI_D0)
> > -		return -EINVAL;
> > +	rc = msi_check_device(dev, minvec);
> > +	if (rc)
> > +		return rc;
> >  
> >  	WARN_ON(!!dev->msi_enabled);
> >  
> > @@ -1109,17 +1097,6 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
> >  		nvec = maxvec;
> >  
> >  	do {
> > -		rc = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSI);
> > -		if (rc < 0) {
> > -			return rc;
> > -		} else if (rc > 0) {
> > -			if (rc < minvec)
> > -				return -ENOSPC;
> > -			nvec = rc;
> > -		}
> > -	} while (rc);
> > -
> > -	do {
> >  		rc = msi_capability_init(dev, nvec);
> >  		if (rc < 0) {
> >  			return rc;
> > diff --git a/include/linux/msi.h b/include/linux/msi.h
> > index 92a2f99..3b873bc 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -59,7 +59,6 @@ int arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc);
> >  void arch_teardown_msi_irq(unsigned int irq);
> >  int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
> >  void arch_teardown_msi_irqs(struct pci_dev *dev);
> > -int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
> >  void arch_restore_msi_irqs(struct pci_dev *dev);
> >  
> >  void default_teardown_msi_irqs(struct pci_dev *dev);
> > @@ -76,8 +75,6 @@ struct msi_chip {
> >  	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
> >  			 struct msi_desc *desc);
> >  	void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
> > -	int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
> > -			    int nvec, int type);
> >  };
> >  
> >  #endif /* LINUX_MSI_H */
> > 
> 
> 
> -- 
> Thanks!
> Yijing
> 

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 2/2] PCI/MSI: Remove arch_msi_check_device()

[Bjorn Helgaas]

On Sat, Jul 12, 2014 at 01:21:08PM +0200, Alexander Gordeev wrote:
> There are no archs that override arch_msi_check_device()
> hook. Remove it as it is completely redundant.
> 
> If an arch would need to check MSI/MSI-X possibility for a
> device it should make it within arch_setup_msi_irqs() hook.
> 
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-pci@vger.kernel.org
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  drivers/pci/msi.c   |   49 +++++++++++++------------------------------------
>  include/linux/msi.h |    3 ---
>  2 files changed, 13 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 13f3d30..19ac058 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -56,16 +56,6 @@ void __weak arch_teardown_msi_irq(unsigned int irq)
>  	chip->teardown_irq(chip, irq);
>  }
>  
> -int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> -{
> -	struct msi_chip *chip = dev->bus->msi;
> -
> -	if (!chip || !chip->check_device)
> -		return 0;
> -
> -	return chip->check_device(chip, dev, nvec, type);
> -}
> -
>  int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  {
>  	struct msi_desc *entry;
> @@ -809,22 +799,23 @@ out_free:
>  }
>  
>  /**
> - * pci_msi_check_device - check whether MSI may be enabled on a device
> + * msi_check_device - check whether MSI may be enabled on a device
>   * @dev: pointer to the pci_dev data structure of MSI device function
>   * @nvec: how many MSIs have been requested ?
> - * @type: are we checking for MSI or MSI-X ?
>   *
>   * Look at global flags, the device itself, and its parent buses
>   * to determine if MSI/-X are supported for the device. If MSI/-X is
>   * supported return 0, else return an error code.
>   **/
> -static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +static int msi_check_device(struct pci_dev *dev, int nvec)

I think "check_device" is a terrible name because it really doesn't
give a clue about what it's doing or what the return value means.
Since you're removing the external usage (arch_msi_check_device) and
this one is static, this would be a good time to fix it.  Maybe
"pci_msi_supported()" or something?

I *love* the idea of getting rid of this much code!

Bjorn

Re: [PATCH 2/2] PCI/MSI: Remove arch_msi_check_device()

[Alexander Gordeev]

On Wed, Jul 16, 2014 at 04:20:24PM -0600, Bjorn Helgaas wrote:
> > @@ -809,22 +799,23 @@ out_free:
> >  }
> >  
> >  /**
> > - * pci_msi_check_device - check whether MSI may be enabled on a device
> > + * msi_check_device - check whether MSI may be enabled on a device
> >   * @dev: pointer to the pci_dev data structure of MSI device function
> >   * @nvec: how many MSIs have been requested ?
> > - * @type: are we checking for MSI or MSI-X ?
> >   *
> >   * Look at global flags, the device itself, and its parent buses
> >   * to determine if MSI/-X are supported for the device. If MSI/-X is
> >   * supported return 0, else return an error code.
> >   **/
> > -static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
> > +static int msi_check_device(struct pci_dev *dev, int nvec)
> 
> I think "check_device" is a terrible name because it really doesn't
> give a clue about what it's doing or what the return value means.
> Since you're removing the external usage (arch_msi_check_device) and
> this one is static, this would be a good time to fix it.  Maybe
> "pci_msi_supported()" or something?

What about pci_can_enable_msi() or pci_msi_can_enable() or msi_can_enable()?

> I *love* the idea of getting rid of this much code!
> 
> Bjorn

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH v2 2/2] PCI/MSI: Remove arch_msi_check_device()

[Alexander Gordeev]

There are no archs that override arch_msi_check_device()
hook. Remove it as it is completely redundant.

If an arch would need to check MSI/MSI-X possibility for a
device it should make it within arch_setup_msi_irqs() hook.

Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-pci@vger.kernel.org
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 drivers/pci/msi.c   | 49 +++++++++++++------------------------------------
 include/linux/msi.h |  3 ---
 2 files changed, 13 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 5a40516..6c2cc41 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -56,16 +56,6 @@ void __weak arch_teardown_msi_irq(unsigned int irq)
 	chip->teardown_irq(chip, irq);
 }
 
-int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
-{
-	struct msi_chip *chip = dev->bus->msi;
-
-	if (!chip || !chip->check_device)
-		return 0;
-
-	return chip->check_device(chip, dev, nvec, type);
-}
-
 int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
 	struct msi_desc *entry;
@@ -806,22 +796,23 @@ out_free:
 }
 
 /**
- * pci_msi_check_device - check whether MSI may be enabled on a device
+ * pci_msi_supported - check whether MSI may be enabled on a device
  * @dev: pointer to the pci_dev data structure of MSI device function
  * @nvec: how many MSIs have been requested ?
- * @type: are we checking for MSI or MSI-X ?
  *
  * Look at global flags, the device itself, and its parent buses
  * to determine if MSI/-X are supported for the device. If MSI/-X is
  * supported return 0, else return an error code.
  **/
-static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
+static int pci_msi_supported(struct pci_dev *dev, int nvec)
 {
 	struct pci_bus *bus;
-	int ret;
 
 	/* MSI must be globally enabled and supported by the device */
-	if (!pci_msi_enable || !dev || dev->no_msi)
+	if (!pci_msi_enable)
+		return -EINVAL;
+
+	if (!dev || dev->no_msi || dev->current_state != PCI_D0)
 		return -EINVAL;
 
 	/*
@@ -843,10 +834,6 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
 		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
 			return -EINVAL;
 
-	ret = arch_msi_check_device(dev, nvec, type);
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
@@ -949,13 +936,13 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
 	int status, nr_entries;
 	int i, j;
 
-	if (!entries || !dev->msix_cap || dev->current_state != PCI_D0)
-		return -EINVAL;
-
-	status = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSIX);
+	status = pci_msi_supported(dev, nvec);
 	if (status)
 		return status;
 
+	if (!entries)
+		return -EINVAL;
+
 	nr_entries = pci_msix_vec_count(dev);
 	if (nr_entries < 0)
 		return nr_entries;
@@ -1062,8 +1049,9 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
 	int nvec;
 	int rc;
 
-	if (dev->current_state != PCI_D0)
-		return -EINVAL;
+	rc = pci_msi_supported(dev, minvec);
+	if (rc)
+		return rc;
 
 	WARN_ON(!!dev->msi_enabled);
 
@@ -1086,17 +1074,6 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
 		nvec = maxvec;
 
 	do {
-		rc = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSI);
-		if (rc < 0) {
-			return rc;
-		} else if (rc > 0) {
-			if (rc < minvec)
-				return -ENOSPC;
-			nvec = rc;
-		}
-	} while (rc);
-
-	do {
 		rc = msi_capability_init(dev, nvec);
 		if (rc < 0) {
 			return rc;
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 8103f32..dbf7cc9 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -60,7 +60,6 @@ int arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc);
 void arch_teardown_msi_irq(unsigned int irq);
 int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
 void arch_teardown_msi_irqs(struct pci_dev *dev);
-int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
 void arch_restore_msi_irqs(struct pci_dev *dev);
 
 void default_teardown_msi_irqs(struct pci_dev *dev);
@@ -77,8 +76,6 @@ struct msi_chip {
 	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
 			 struct msi_desc *desc);
 	void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
-	int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
-			    int nvec, int type);
 };
 
 #endif /* LINUX_MSI_H */
-- 
1.9.3

RE: [PATCH v2 2/2] PCI/MSI: Remove arch_msi_check_device()

[Bharat.Bhushan]

> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-owner@vger.kernel=
.org]
> On Behalf Of Alexander Gordeev
> Sent: Monday, August 11, 2014 5:16 PM
> To: Bjorn Helgaas
> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> pci@vger.kernel.org
> Subject: [PATCH v2 2/2] PCI/MSI: Remove arch_msi_check_device()
>=20
> There are no archs that override arch_msi_check_device() hook. Remove it =
as it
> is completely redundant.

Is not we are still overriding this in powerpc (not sure I missed some patc=
h , if so please point to that).

$ grep -r  arch_msi_check_device arch/powerpc/
Binary file arch/powerpc/kernel/msi.o matches
arch/powerpc/kernel/msi.c:int arch_msi_check_device(struct pci_dev* dev, in=
t nvec, int type)
Binary file arch/powerpc/kernel/built-in.o matches

Thanks
-Bharat

>=20
> If an arch would need to check MSI/MSI-X possibility for a device it shou=
ld make
> it within arch_setup_msi_irqs() hook.
>=20
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-pci@vger.kernel.org
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  drivers/pci/msi.c   | 49 +++++++++++++----------------------------------=
--
>  include/linux/msi.h |  3 ---
>  2 files changed, 13 insertions(+), 39 deletions(-)
>=20
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 5a40516..6c2cc41=
 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -56,16 +56,6 @@ void __weak arch_teardown_msi_irq(unsigned int irq)
>  	chip->teardown_irq(chip, irq);
>  }
>=20
> -int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type=
) -{
> -	struct msi_chip *chip =3D dev->bus->msi;
> -
> -	if (!chip || !chip->check_device)
> -		return 0;
> -
> -	return chip->check_device(chip, dev, nvec, type);
> -}
> -
>  int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) =
 {
>  	struct msi_desc *entry;
> @@ -806,22 +796,23 @@ out_free:
>  }
>=20
>  /**
> - * pci_msi_check_device - check whether MSI may be enabled on a device
> + * pci_msi_supported - check whether MSI may be enabled on a device
>   * @dev: pointer to the pci_dev data structure of MSI device function
>   * @nvec: how many MSIs have been requested ?
> - * @type: are we checking for MSI or MSI-X ?
>   *
>   * Look at global flags, the device itself, and its parent buses
>   * to determine if MSI/-X are supported for the device. If MSI/-X is
>   * supported return 0, else return an error code.
>   **/
> -static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +static int pci_msi_supported(struct pci_dev *dev, int nvec)
>  {
>  	struct pci_bus *bus;
> -	int ret;
>=20
>  	/* MSI must be globally enabled and supported by the device */
> -	if (!pci_msi_enable || !dev || dev->no_msi)
> +	if (!pci_msi_enable)
> +		return -EINVAL;
> +
> +	if (!dev || dev->no_msi || dev->current_state !=3D PCI_D0)
>  		return -EINVAL;
>=20
>  	/*
> @@ -843,10 +834,6 @@ static int pci_msi_check_device(struct pci_dev *dev,=
 int
> nvec, int type)
>  		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
>  			return -EINVAL;
>=20
> -	ret =3D arch_msi_check_device(dev, nvec, type);
> -	if (ret)
> -		return ret;
> -
>  	return 0;
>  }
>=20
> @@ -949,13 +936,13 @@ int pci_enable_msix(struct pci_dev *dev, struct msi=
x_entry
> *entries, int nvec)
>  	int status, nr_entries;
>  	int i, j;
>=20
> -	if (!entries || !dev->msix_cap || dev->current_state !=3D PCI_D0)
> -		return -EINVAL;
> -
> -	status =3D pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSIX);
> +	status =3D pci_msi_supported(dev, nvec);
>  	if (status)
>  		return status;
>=20
> +	if (!entries)
> +		return -EINVAL;
> +
>  	nr_entries =3D pci_msix_vec_count(dev);
>  	if (nr_entries < 0)
>  		return nr_entries;
> @@ -1062,8 +1049,9 @@ int pci_enable_msi_range(struct pci_dev *dev, int m=
invec,
> int maxvec)
>  	int nvec;
>  	int rc;
>=20
> -	if (dev->current_state !=3D PCI_D0)
> -		return -EINVAL;
> +	rc =3D pci_msi_supported(dev, minvec);
> +	if (rc)
> +		return rc;
>=20
>  	WARN_ON(!!dev->msi_enabled);
>=20
> @@ -1086,17 +1074,6 @@ int pci_enable_msi_range(struct pci_dev *dev, int =
minvec,
> int maxvec)
>  		nvec =3D maxvec;
>=20
>  	do {
> -		rc =3D pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSI);
> -		if (rc < 0) {
> -			return rc;
> -		} else if (rc > 0) {
> -			if (rc < minvec)
> -				return -ENOSPC;
> -			nvec =3D rc;
> -		}
> -	} while (rc);
> -
> -	do {
>  		rc =3D msi_capability_init(dev, nvec);
>  		if (rc < 0) {
>  			return rc;
> diff --git a/include/linux/msi.h b/include/linux/msi.h index 8103f32..dbf=
7cc9
> 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -60,7 +60,6 @@ int arch_setup_msi_irq(struct pci_dev *dev, struct msi_=
desc
> *desc);  void arch_teardown_msi_irq(unsigned int irq);  int
> arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);  void
> arch_teardown_msi_irqs(struct pci_dev *dev); -int arch_msi_check_device(s=
truct
> pci_dev* dev, int nvec, int type);  void arch_restore_msi_irqs(struct pci=
_dev
> *dev);
>=20
>  void default_teardown_msi_irqs(struct pci_dev *dev); @@ -77,8 +76,6 @@ s=
truct
> msi_chip {
>  	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
>  			 struct msi_desc *desc);
>  	void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
> -	int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
> -			    int nvec, int type);
>  };
>=20
>  #endif /* LINUX_MSI_H */
> --
> 1.9.3
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in t=
he body
> of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] PCI/MSI: Remove arch_msi_check_device()

[Alexander Gordeev]

On Mon, Aug 11, 2014 at 02:33:25PM +0000, Bharat.Bhushan@freescale.com wrote:
> 
> 
> > -----Original Message-----
> > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-owner@vger.kernel.org]
> > On Behalf Of Alexander Gordeev
> > Sent: Monday, August 11, 2014 5:16 PM
> > To: Bjorn Helgaas
> > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> > pci@vger.kernel.org
> > Subject: [PATCH v2 2/2] PCI/MSI: Remove arch_msi_check_device()
> > 
> > There are no archs that override arch_msi_check_device() hook. Remove it as it
> > is completely redundant.
> 
> Is not we are still overriding this in powerpc (not sure I missed some patch , if so please point to that).

Patch 1/2 ;)

> $ grep -r  arch_msi_check_device arch/powerpc/
> Binary file arch/powerpc/kernel/msi.o matches
> arch/powerpc/kernel/msi.c:int arch_msi_check_device(struct pci_dev* dev, int nvec, int type)
> Binary file arch/powerpc/kernel/built-in.o matches
> 
> Thanks
> -Bharat
> 
> > 
> > If an arch would need to check MSI/MSI-X possibility for a device it should make
> > it within arch_setup_msi_irqs() hook.
> > 
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: linux-pci@vger.kernel.org
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  drivers/pci/msi.c   | 49 +++++++++++++------------------------------------
> >  include/linux/msi.h |  3 ---
> >  2 files changed, 13 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 5a40516..6c2cc41 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -56,16 +56,6 @@ void __weak arch_teardown_msi_irq(unsigned int irq)
> >  	chip->teardown_irq(chip, irq);
> >  }
> > 
> > -int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type) -{
> > -	struct msi_chip *chip = dev->bus->msi;
> > -
> > -	if (!chip || !chip->check_device)
> > -		return 0;
> > -
> > -	return chip->check_device(chip, dev, nvec, type);
> > -}
> > -
> >  int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)  {
> >  	struct msi_desc *entry;
> > @@ -806,22 +796,23 @@ out_free:
> >  }
> > 
> >  /**
> > - * pci_msi_check_device - check whether MSI may be enabled on a device
> > + * pci_msi_supported - check whether MSI may be enabled on a device
> >   * @dev: pointer to the pci_dev data structure of MSI device function
> >   * @nvec: how many MSIs have been requested ?
> > - * @type: are we checking for MSI or MSI-X ?
> >   *
> >   * Look at global flags, the device itself, and its parent buses
> >   * to determine if MSI/-X are supported for the device. If MSI/-X is
> >   * supported return 0, else return an error code.
> >   **/
> > -static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
> > +static int pci_msi_supported(struct pci_dev *dev, int nvec)
> >  {
> >  	struct pci_bus *bus;
> > -	int ret;
> > 
> >  	/* MSI must be globally enabled and supported by the device */
> > -	if (!pci_msi_enable || !dev || dev->no_msi)
> > +	if (!pci_msi_enable)
> > +		return -EINVAL;
> > +
> > +	if (!dev || dev->no_msi || dev->current_state != PCI_D0)
> >  		return -EINVAL;
> > 
> >  	/*
> > @@ -843,10 +834,6 @@ static int pci_msi_check_device(struct pci_dev *dev, int
> > nvec, int type)
> >  		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> >  			return -EINVAL;
> > 
> > -	ret = arch_msi_check_device(dev, nvec, type);
> > -	if (ret)
> > -		return ret;
> > -
> >  	return 0;
> >  }
> > 
> > @@ -949,13 +936,13 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry
> > *entries, int nvec)
> >  	int status, nr_entries;
> >  	int i, j;
> > 
> > -	if (!entries || !dev->msix_cap || dev->current_state != PCI_D0)
> > -		return -EINVAL;
> > -
> > -	status = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSIX);
> > +	status = pci_msi_supported(dev, nvec);
> >  	if (status)
> >  		return status;
> > 
> > +	if (!entries)
> > +		return -EINVAL;
> > +
> >  	nr_entries = pci_msix_vec_count(dev);
> >  	if (nr_entries < 0)
> >  		return nr_entries;
> > @@ -1062,8 +1049,9 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec,
> > int maxvec)
> >  	int nvec;
> >  	int rc;
> > 
> > -	if (dev->current_state != PCI_D0)
> > -		return -EINVAL;
> > +	rc = pci_msi_supported(dev, minvec);
> > +	if (rc)
> > +		return rc;
> > 
> >  	WARN_ON(!!dev->msi_enabled);
> > 
> > @@ -1086,17 +1074,6 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec,
> > int maxvec)
> >  		nvec = maxvec;
> > 
> >  	do {
> > -		rc = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSI);
> > -		if (rc < 0) {
> > -			return rc;
> > -		} else if (rc > 0) {
> > -			if (rc < minvec)
> > -				return -ENOSPC;
> > -			nvec = rc;
> > -		}
> > -	} while (rc);
> > -
> > -	do {
> >  		rc = msi_capability_init(dev, nvec);
> >  		if (rc < 0) {
> >  			return rc;
> > diff --git a/include/linux/msi.h b/include/linux/msi.h index 8103f32..dbf7cc9
> > 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -60,7 +60,6 @@ int arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc
> > *desc);  void arch_teardown_msi_irq(unsigned int irq);  int
> > arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);  void
> > arch_teardown_msi_irqs(struct pci_dev *dev); -int arch_msi_check_device(struct
> > pci_dev* dev, int nvec, int type);  void arch_restore_msi_irqs(struct pci_dev
> > *dev);
> > 
> >  void default_teardown_msi_irqs(struct pci_dev *dev); @@ -77,8 +76,6 @@ struct
> > msi_chip {
> >  	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
> >  			 struct msi_desc *desc);
> >  	void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
> > -	int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
> > -			    int nvec, int type);
> >  };
> > 
> >  #endif /* LINUX_MSI_H */
> > --
> > 1.9.3
> > 
> > --
> > 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

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 0/2] PCI/MSI: Remove arch_msi_check_device()

[Bjorn Helgaas]

On Sat, Jul 12, 2014 at 01:21:06PM +0200, Alexander Gordeev wrote:
> Hello,
> 
> This is a cleanup effort to get rid of useless arch_msi_check_device().
> I am not sure what were the reasons for its existence in the first place,
> but at the moment it appears totally unnecessary.
> 
> Thanks!
> 
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-pci@vger.kernel.org
> 
> Alexander Gordeev (2):
>   PCI/MSI/PPC: Remove arch_msi_check_device()
>   PCI/MSI: Remove arch_msi_check_device()

I applied these (with Michael's ack on the first, and v2 of the second) to
pci/msi for v3.18, thanks!
> 
>  arch/powerpc/include/asm/machdep.h     |    2 -
>  arch/powerpc/kernel/msi.c              |   12 +-------
>  arch/powerpc/platforms/cell/axon_msi.c |    9 ------
>  arch/powerpc/platforms/powernv/pci.c   |   19 +++---------
>  arch/powerpc/platforms/pseries/msi.c   |   42
>  ++++++++++----------------- arch/powerpc/sysdev/fsl_msi.c          |
>  12 ++------ arch/powerpc/sysdev/mpic_pasemi_msi.c  |   11 +------
>  arch/powerpc/sysdev/mpic_u3msi.c       |   28 +++++++-----------
>  arch/powerpc/sysdev/ppc4xx_hsta_msi.c  |   18 ++++--------
>  arch/powerpc/sysdev/ppc4xx_msi.c       |   19 ++++--------
>  drivers/pci/msi.c                      |   49
>  ++++++++----------------------- include/linux/msi.h                    |
>  3 -- 12 files changed, 63 insertions(+), 161 deletions(-)
> 
> -- 1.7.7.6
> 
> -- 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

Re: [PATCH 0/2] PCI/MSI: Remove arch_msi_check_device()

[Bjorn Helgaas]

On Fri, Sep 5, 2014 at 3:25 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sat, Jul 12, 2014 at 01:21:06PM +0200, Alexander Gordeev wrote:
>> Hello,
>>
>> This is a cleanup effort to get rid of useless arch_msi_check_device().
>> I am not sure what were the reasons for its existence in the first place,
>> but at the moment it appears totally unnecessary.
>>
>> Thanks!
>>
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: linux-pci@vger.kernel.org
>>
>> Alexander Gordeev (2):
>>   PCI/MSI/PPC: Remove arch_msi_check_device()
>>   PCI/MSI: Remove arch_msi_check_device()
>
> I applied these (with Michael's ack on the first, and v2 of the second) to
> pci/msi for v3.18, thanks!
>>
>>  arch/powerpc/include/asm/machdep.h     |    2 -
>>  arch/powerpc/kernel/msi.c              |   12 +-------
>>  arch/powerpc/platforms/cell/axon_msi.c |    9 ------
>>  arch/powerpc/platforms/powernv/pci.c   |   19 +++---------
>>  arch/powerpc/platforms/pseries/msi.c   |   42
>>  ++++++++++----------------- arch/powerpc/sysdev/fsl_msi.c          |
>>  12 ++------ arch/powerpc/sysdev/mpic_pasemi_msi.c  |   11 +------
>>  arch/powerpc/sysdev/mpic_u3msi.c       |   28 +++++++-----------
>>  arch/powerpc/sysdev/ppc4xx_hsta_msi.c  |   18 ++++--------
>>  arch/powerpc/sysdev/ppc4xx_msi.c       |   19 ++++--------
>>  drivers/pci/msi.c                      |   49
>>  ++++++++----------------------- include/linux/msi.h                    |
>>  3 -- 12 files changed, 63 insertions(+), 161 deletions(-)

Oh, I forgot -- if you'd rather take the first one through the PPC
tree, you can do that and I can merge the second one later.  Let me
know if you want to do that.

Bjorn

Re: [PATCH 0/2] PCI/MSI: Remove arch_msi_check_device()

[Alexander Gordeev]

On Fri, Sep 05, 2014 at 03:27:49PM -0600, Bjorn Helgaas wrote:
> > I applied these (with Michael's ack on the first, and v2 of the second) to
> > pci/msi for v3.18, thanks!

Hi Bjorn,

I resent a series with updates that fix kbuild robot errors.
Hopefully, the rebase for pci/msi would not cause trouble for anyone.

> Oh, I forgot -- if you'd rather take the first one through the PPC
> tree, you can do that and I can merge the second one later.  Let me
> know if you want to do that.

Nah, your treee is just fine.

Thanks!

> Bjorn

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH 0/2] PCI/MSI: Remove arch_msi_check_device()

[Michael Ellerman]

On Fri, 2014-09-05 at 15:27 -0600, Bjorn Helgaas wrote:
> On Fri, Sep 5, 2014 at 3:25 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Sat, Jul 12, 2014 at 01:21:06PM +0200, Alexander Gordeev wrote:
> >> Hello,
> >>
> >> This is a cleanup effort to get rid of useless arch_msi_check_device().
> >> I am not sure what were the reasons for its existence in the first place,
> >> but at the moment it appears totally unnecessary.
> >>
> >> Thanks!
> >>
> >> Cc: linuxppc-dev@lists.ozlabs.org
> >> Cc: linux-pci@vger.kernel.org
> >>
> >> Alexander Gordeev (2):
> >>   PCI/MSI/PPC: Remove arch_msi_check_device()
> >>   PCI/MSI: Remove arch_msi_check_device()
> >
> > I applied these (with Michael's ack on the first, and v2 of the second) to
> > pci/msi for v3.18, thanks!
> 
> Oh, I forgot -- if you'd rather take the first one through the PPC
> tree, you can do that and I can merge the second one later.  Let me
> know if you want to do that.

You take them all, that code shouldn't be changing much in the powerpc tree.

If the merge gets messy closer to the merge window we can always take the first
patch into the powerpc tree then.

cheers