[PATCH v2 1/3] PCI/MSI/PPC: Remove arch_msi_check

Discussion:

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

Alexander Gordeev

2014-09-07 18:57:53 UTC

Moving MSI checks from arch_msi_check_device() function to
arch_setup_msi_irqs() function makes code more compact and
allows removing unnecessary hook arch_msi_check_device()
from generic MSI code.

Cc: linuxppc-***@lists.ozlabs.org
Cc: linux-***@vger.kernel.org
Cc: Michael Ellerman <***@ellerman.id.au>
Signed-off-by: Alexander Gordeev <***@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 b125cea..3af7216 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 b854b57..b45c492 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) {
@@ -860,7 +852,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 18ff462..e68c196 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -336,26 +336,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;
@@ -397,15 +377,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;
@@ -416,12 +405,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
@@ -526,7 +517,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..22b5200 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(&dev->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.9.3

Alexander Gordeev

2014-09-07 18:57:54 UTC

Permalink

Moving MSI checks from arch_msi_check_device() function to
arch_setup_msi_irqs() function makes code more compact and
allows removing unnecessary hook arch_msi_check_device()
from generic MSI code.

Cc: Thomas Gleixner <***@linutronix.de>
Cc: Jason Cooper <***@lakedaemon.net>
Cc: linux-***@vger.kernel.org
Signed-off-by: Alexander Gordeev <***@redhat.com>
---
drivers/irqchip/irq-armada-370-xp.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 574aba0..df60eab 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -136,6 +136,10 @@ static int armada_370_xp_setup_msi_irq(struct msi_chip *chip,
struct msi_msg msg;
int virq, hwirq;

+ /* We support MSI, but not MSI-X */
+ if (desc->msi_attrib.is_msix)
+ return -EINVAL;
+
hwirq = armada_370_xp_alloc_msi();
if (hwirq < 0)
return hwirq;
@@ -166,15 +170,6 @@ static void armada_370_xp_teardown_msi_irq(struct msi_chip *chip,
armada_370_xp_free_msi(hwirq);
}

-static int armada_370_xp_check_msi_device(struct msi_chip *chip, struct pci_dev *dev,
- int nvec, int type)
-{
- /* We support MSI, but not MSI-X */
- if (type == PCI_CAP_ID_MSI)
- return 0;
- return -EINVAL;
-}
-
static struct irq_chip armada_370_xp_msi_irq_chip = {
.name = "armada_370_xp_msi_irq",
.irq_enable = unmask_msi_irq,
@@ -213,7 +208,6 @@ static int armada_370_xp_msi_init(struct device_node *node,

msi_chip->setup_irq = armada_370_xp_setup_msi_irq;
msi_chip->teardown_irq = armada_370_xp_teardown_msi_irq;
- msi_chip->check_device = armada_370_xp_check_msi_device;
msi_chip->of_node = node;

armada_370_xp_msi_domain =

--
1.9.3

Jason Cooper

2014-09-08 11:16:56 UTC

Permalink

+ free-electron's guys.

Post by Alexander Gordeev
Moving MSI checks from arch_msi_check_device() function to
arch_setup_msi_irqs() function makes code more compact and
allows removing unnecessary hook arch_msi_check_device()
from generic MSI code.
---
drivers/irqchip/irq-armada-370-xp.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 574aba0..df60eab 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -136,6 +136,10 @@ static int armada_370_xp_setup_msi_irq(struct msi_chip *chip,
struct msi_msg msg;
int virq, hwirq;
+ /* We support MSI, but not MSI-X */
+ if (desc->msi_attrib.is_msix)
+ return -EINVAL;
+
hwirq = armada_370_xp_alloc_msi();
if (hwirq < 0)
return hwirq;
@@ -166,15 +170,6 @@ static void armada_370_xp_teardown_msi_irq(struct msi_chip *chip,
armada_370_xp_free_msi(hwirq);
}
-static int armada_370_xp_check_msi_device(struct msi_chip *chip, struct pci_dev *dev,
- int nvec, int type)
-{
- /* We support MSI, but not MSI-X */
- if (type == PCI_CAP_ID_MSI)
- return 0;
- return -EINVAL;
-}
-
static struct irq_chip armada_370_xp_msi_irq_chip = {
.name = "armada_370_xp_msi_irq",
.irq_enable = unmask_msi_irq,
@@ -213,7 +208,6 @@ static int armada_370_xp_msi_init(struct device_node *node,
msi_chip->setup_irq = armada_370_xp_setup_msi_irq;
msi_chip->teardown_irq = armada_370_xp_teardown_msi_irq;
- msi_chip->check_device = armada_370_xp_check_msi_device;
msi_chip->of_node = node;
armada_370_xp_msi_domain =
--
1.9.3

Alexander Gordeev

2014-09-15 08:36:37 UTC

Permalink

Post by Jason Cooper
+ free-electron's guys.

Hi Gentlemen,

Any feedback on this patch?

Thanks!

Post by Jason Cooper

--
Regards,
Alexander Gordeev
***@redhat.com

Jason Cooper

2014-09-17 15:55:54 UTC

Permalink

Alexander,

Now that ThomasP has had a chance to test, please adjust the subject
line to 'irqchip: armada-370-xp: M...' and

Acked-by: Jason Cooper <***@lakedaemon.net>

thx,

Jason.

Alexander Gordeev

2014-09-17 16:17:55 UTC

Permalink

Post by Jason Cooper
Now that ThomasP has had a chance to test, please adjust the subject
line to 'irqchip: armada-370-xp: M...' and

Hi Bjorn,

Do you want me to resend the patch?

Post by Jason Cooper
thx,
Jason.

--
Regards,
Alexander Gordeev
***@redhat.com

Alexander Gordeev

2014-09-07 18:57:55 UTC

Permalink

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: Michael Ellerman <***@ellerman.id.au>
Cc: linuxppc-***@lists.ozlabs.org
Cc: linux-***@vger.kernel.org
Signed-off-by: Alexander Gordeev <***@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

Bjorn Helgaas

2014-09-23 20:58:20 UTC

Permalink

Post by 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.
---
drivers/pci/msi.c | 49 +++++++++++++------------------------------------
include/linux/msi.h | 3 ---
2 files changed, 13 insertions(+), 39 deletions(-)

I like this patch a lot, but it's doing several things at once, so I split
it into several patches as follows. I think it is equivalent, but I did
not add Thomas' tested-by.

I also reversed the sense of pci_msi_supported() so it returns true (1)
when MSI/MSI-X is supported and false (0) otherwise.

The whole thing is on my pci/msi branch at
https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/msi

Let me know if you see any issues.

Bjorn

commit 28a3ec3f672d42a877d8770a33305dfe82c2cc90
Author: Alexander Gordeev <***@redhat.com>
Date: Tue Sep 23 12:39:54 2014 -0600

PCI/MSI: Remove arch_msi_check_device()

No architectures implement arch_msi_check_device() or the struct msi_chip
.check_device() method, so remove them.

Remove the "type" parameter to pci_msi_check_device() because it was only
used to call arch_msi_check_device() and is no longer needed.

[bhelgaas: changelog, split to separate patch]
Signed-off-by: Alexander Gordeev <***@redhat.com>
Signed-off-by: Bjorn Helgaas <***@google.com>

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index aff384aea639..a0b623d97ad8 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;
@@ -788,10 +778,9 @@ out_free:
* 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_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)
@@ -816,10 +805,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;
}

@@ -925,7 +910,7 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
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_check_device(dev, nvec);
if (status)
return status;

@@ -1059,7 +1044,7 @@ 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);
+ rc = pci_msi_check_device(dev, nvec);
if (rc < 0) {
return rc;
} else if (rc > 0) {
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 6be66f48b026..36c63cfccf5b 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -55,7 +55,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);
@@ -72,8 +71,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 */

commit 9d686dd5bbf62a249d0e4f54bffd848d573d2380
Author: Alexander Gordeev <***@redhat.com>
Date: Tue Sep 23 14:25:11 2014 -0600

PCI/MSI: Move D0 check into pci_msi_check_device()

Both callers of pci_msi_check_device() check that the device is in D0
state, so move the check from the callers into pci_msi_check_device()
itself.

In pci_enable_msi_range(), note that pci_msi_check_device() never returns a
positive value any more, so the loop that called it until it returns zero
or negative is no longer necessary.

[bhelgaas: changelog, split to separate patch]
Signed-off-by: Alexander Gordeev <***@redhat.com>
Signed-off-by: Bjorn Helgaas <***@google.com>

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a0b623d97ad8..416a4917a64a 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -783,7 +783,10 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec)
struct pci_bus *bus;

/* 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;

/*
@@ -907,13 +910,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);
if (status)
return status;

+ if (!entries)
+ return -EINVAL;
+
nr_entries = pci_msix_vec_count(dev);
if (nr_entries < 0)
return nr_entries;
@@ -1020,8 +1023,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_check_device(dev, minvec);
+ if (rc)
+ return rc;

WARN_ON(!!dev->msi_enabled);

@@ -1044,17 +1048,6 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
nvec = maxvec;

do {
- rc = pci_msi_check_device(dev, nvec);
- 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;

commit 99b4b82ffbc665cfa363a89144c8cc29eb46ce3d
Author: Alexander Gordeev <***@redhat.com>
Date: Tue Sep 23 12:45:58 2014 -0600

PCI/MSI: Rename pci_msi_check_device() to pci_msi_supported()

Rename pci_msi_check_device() to pci_msi_supported() for clarity. Note
that pci_msi_supported() returns true if MSI/MSI-X is supported, so code
like:

if (pci_msi_supported(...))

reads naturally.

[bhelgaas: changelog, split to separate patch, reverse sense]
Signed-off-by: Alexander Gordeev <***@redhat.com>
Signed-off-by: Bjorn Helgaas <***@google.com>

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 416a4917a64a..d40323f0ed44 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -769,16 +769,15 @@ 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.
+ * supported return 1, else return 0.
**/
-static int pci_msi_check_device(struct pci_dev *dev, int nvec)
+static int pci_msi_supported(struct pci_dev *dev, int nvec)
{
struct pci_bus *bus;

@@ -795,7 +794,7 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec)
* b) the list manipulation code assumes nvec >= 1.
*/
if (nvec < 1)
- return -ERANGE;
+ return 0;

/*
* Any bridge which does NOT route MSI transactions from its
@@ -806,9 +805,9 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec)
*/
for (bus = dev->bus; bus; bus = bus->parent)
if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
- return -EINVAL;
+ return 0;

- return 0;
+ return 1;
}

/**
@@ -910,9 +909,8 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
int status, nr_entries;
int i, j;

- status = pci_msi_check_device(dev, nvec);
- if (status)
- return status;
+ if (!pci_msi_supported(dev, nvec))
+ return -EINVAL;

if (!entries)
return -EINVAL;
@@ -1023,9 +1021,8 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
int nvec;
int rc;

- rc = pci_msi_check_device(dev, minvec);
- if (rc)
- return rc;
+ if (!pci_msi_supported(dev, minvec))
+ return -EINVAL;

WARN_ON(!!dev->msi_enabled);

commit 83be7664f8492b8e4b72cd9375433909d40c018b
Author: Bjorn Helgaas <***@google.com>
Date: Tue Sep 23 14:38:28 2014 -0600

PCI/MSI: Remove unnecessary temporary variable

The only use of "status" is to hold a value which is immediately returned,
so just return and remove the variable directly.

Signed-off-by: Bjorn Helgaas <***@google.com>

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d40323f0ed44..aae2fc80692b 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -906,7 +906,7 @@ EXPORT_SYMBOL(pci_msix_vec_count);
**/
int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
{
- int status, nr_entries;
+ int nr_entries;
int i, j;

if (!pci_msi_supported(dev, nvec))
@@ -937,8 +937,7 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
dev_info(&dev->dev, "can't enable MSI-X (MSI IRQ already assigned)\n");
return -EINVAL;
}
- status = msix_capability_init(dev, entries, nvec);
- return status;
+ return msix_capability_init(dev, entries, nvec);
}
EXPORT_SYMBOL(pci_enable_msix);

Michael Ellerman

2014-09-15 02:42:50 UTC

Permalink

I already acked the previous version, but if you didn't want it that's fine :)

cheers

Alexander Gordeev

2014-09-15 08:34:20 UTC

Permalink

Post by Michael Ellerman

I already acked the previous version, but if you didn't want it that's fine :)

I do want it, but I thought you may not agree with the new changelog ;)

Post by Michael Ellerman
cheers

--
Regards,
Alexander Gordeev
***@redhat.com

Thomas Petazzoni

2014-09-17 15:25:58 UTC

Permalink

Dear Alexander Gordeev,

patch 1 - PCI/MSI/PPC: Remove arch_msi_check_device()
patch 2 - PCI/MSI/Armada-370-xp: Remove arch_msi_check_device()
patch 3 - PCI/MSI: Remove arch_msi_check_device()

For the entire series:

Tested-by: Thomas Petazzoni <***@free-electrons.com>

I tested on the Armada XP GP platform, which is particularly affected
by PATCH 2/3, as it is using the irq-armada-370-xp irqchip driver. I
tested with an igb PCIe NIC that supports MSI and MSI-X, and I verified
that MSI-X is still rejected (since we don't support it on Armada XP,
at least for now), and that MSI is accepted and actually works.

Thanks for doing this, and sorry for the delay in getting the patches
tested!

Best regards,

Thomas

--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Bjorn Helgaas

2014-09-23 21:01:54 UTC

Permalink

Hello,
This is a cleanup effort to get rid of arch_msi_check_device() function.
I am sending v2 series, since kbuild for v1 reports compile errors on
ppc4xx and Armada 370. Still, I have not checked the fixes on these
platforms.
- patch 1: 'pdev' undeclared compile error fixed on ppc4xx. I updated
changelog and removed ACK from this patch in case there are
any objections;
- patch 2: 'struct msi_chip' has no 'check_device' error fixed on
Armada 370 - armada_370_xp_check_msi_device() hook removed;
- patch 3: msi_check_device() renamed to pci_msi_supported(). This is
the very same patch I sent earlier;
Thanks!
patch 1 - PCI/MSI/PPC: Remove arch_msi_check_device()
patch 2 - PCI/MSI/Armada-370-xp: Remove arch_msi_check_device()
patch 3 - PCI/MSI: Remove arch_msi_check_device()

Applied (with some changes to 3/3) to pci/msi for v3.18, thanks! Let me
know if you see any issues.

Bjorn