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: [email protected]
Cc: [email protected]
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
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: [email protected]
Cc: [email protected]
Signed-off-by: Alexander Gordeev <[email protected]>
---
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
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: [email protected]
Cc: [email protected]
Signed-off-by: Alexander Gordeev <[email protected]>
---
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
> /**
> - * 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
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
[email protected]
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: [email protected]
> Cc: [email protected]
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> 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
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
[email protected]
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
[email protected]