[PATCH v2 0/5] Retrieve MSI/MSIX cap struct for once on setup

[PATCH v2 0/5] Retrieve MSI/MSIX cap struct for once on setup

[Gavin Shan]

While we setup MSI or MSI-X for specific PCI device, the address of
MSI or MSI-X capability structure is figured out from the config
space for multiple times. That's unnecessary and the patchset addresses
that. With the patchset applied, the latency for MSI or MSI-X setup
would be decreased hopefully.

v1 -> v2:
	* Cache the MSI/MSI-X capability offset to pci_dev directly according
	  to Bjorn's suggestion.
	* Rebase to 3.9.RC5

drivers/pci/msi.c   |  102 +++++++++++++++++++++++++--------------------------
include/linux/pci.h |    2 +
2 files changed, 52 insertions(+), 52 deletions(-)

Thanks,
Gavin

[PATCH 1/5] PCI: Cache MSI/MSI-X cap in PCI device

[Gavin Shan]

The patch caches the MSI and MSI-X capability offset in PCI device
(struct pci_dev) so that we needn't poll it from the config space
upon enabling or disabling MSI or MSI-X interrupts.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/msi.c   |   36 +++++++++++++++++++-----------------
 include/linux/pci.h |    2 ++
 2 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 00cc78c..3459bdf 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -111,31 +111,31 @@ void default_restore_msi_irqs(struct pci_dev *dev, int irq)
 }
 #endif
 
-static void msi_set_enable(struct pci_dev *dev, int pos, int enable)
+static void msi_set_enable(struct pci_dev *dev, int enable)
 {
 	u16 control;
 
-	BUG_ON(!pos);
+	BUG_ON(!dev->msi_cap);
 
-	pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &control);
+	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
 	control &= ~PCI_MSI_FLAGS_ENABLE;
 	if (enable)
 		control |= PCI_MSI_FLAGS_ENABLE;
-	pci_write_config_word(dev, pos + PCI_MSI_FLAGS, control);
+	pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, control);
 }
 
 static void msix_set_enable(struct pci_dev *dev, int enable)
 {
-	int pos;
 	u16 control;
 
-	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
-	if (pos) {
-		pci_read_config_word(dev, pos + PCI_MSIX_FLAGS, &control);
+	if (dev->msix_cap) {
+		pci_read_config_word(dev,
+			dev->msix_cap + PCI_MSIX_FLAGS, &control);
 		control &= ~PCI_MSIX_FLAGS_ENABLE;
 		if (enable)
 			control |= PCI_MSIX_FLAGS_ENABLE;
-		pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
+		pci_write_config_word(dev,
+			dev->msix_cap + PCI_MSIX_FLAGS, control);
 	}
 }
 
@@ -402,7 +402,7 @@ static void __pci_restore_msi_state(struct pci_dev *dev)
 	pos = entry->msi_attrib.pos;
 
 	pci_intx_for_msi(dev, 0);
-	msi_set_enable(dev, pos, 0);
+	msi_set_enable(dev, 0);
 	arch_restore_msi_irqs(dev, dev->irq);
 
 	pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &control);
@@ -557,7 +557,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
 	unsigned mask;
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
-	msi_set_enable(dev, pos, 0);	/* Disable MSI during set up */
+	msi_set_enable(dev, 0);	/* Disable MSI during set up */
 
 	pci_read_config_word(dev, msi_control_reg(pos), &control);
 	/* MSI Entry Initialization */
@@ -598,7 +598,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
 
 	/* Set MSI enabled bits	 */
 	pci_intx_for_msi(dev, 0);
-	msi_set_enable(dev, pos, 1);
+	msi_set_enable(dev, 1);
 	dev->msi_enabled = 1;
 
 	dev->irq = entry->irq;
@@ -885,7 +885,7 @@ void pci_msi_shutdown(struct pci_dev *dev)
 	desc = list_first_entry(&dev->msi_list, struct msi_desc, list);
 	pos = desc->msi_attrib.pos;
 
-	msi_set_enable(dev, pos, 0);
+	msi_set_enable(dev, 0);
 	pci_intx_for_msi(dev, 1);
 	dev->msi_enabled = 0;
 
@@ -1048,15 +1048,17 @@ EXPORT_SYMBOL(pci_msi_enabled);
 
 void pci_msi_init_pci_dev(struct pci_dev *dev)
 {
-	int pos;
 	INIT_LIST_HEAD(&dev->msi_list);
 
 	/* Disable the msi hardware to avoid screaming interrupts
 	 * during boot.  This is the power on reset default so
 	 * usually this should be a noop.
 	 */
-	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
-	if (pos)
-		msi_set_enable(dev, pos, 0);
+	dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
+	if (dev->msi_cap)
+		msi_set_enable(dev, 0);
+
+	/* We needn't check if we have valid MSI-X capability here */
+	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
 	msix_set_enable(dev, 0);
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2461033a..f8314c7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -249,6 +249,8 @@ struct pci_dev {
 	pci_power_t     current_state;  /* Current operating state. In ACPI-speak,
 					   this is D0-D3, D0 being fully functional,
 					   and D3 being off. */
+	int		msi_cap;	/* MSI capability offset */
+	int		msix_cap;	/* MSI-X capability offset */
 	int		pm_cap;		/* PM capability offset in the
 					   configuration space */
 	unsigned int	pme_support:5;	/* Bitmask of states from which PME#
-- 
1.7.5.4

[PATCH 2/5] PCI: Use MSI/MSI-X cap cache in pci_msi_check_device()

[Gavin Shan]

The function pci_msi_check_device() is called while enabling MSI
or MSI-X interrupts to make sure the PCI device can support MSI
or MSI-X capability. The patch changes the function for a bit to
use the cached MSI or MSI-X capability in pci_dev instead of polling
them from config space.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/msi.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 3459bdf..5f51e10 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -772,6 +772,11 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
 	if (!pci_msi_enable || !dev || dev->no_msi)
 		return -EINVAL;
 
+	/* Check if the PCI device has MSI or MSI-X capability */
+	if ((type == PCI_CAP_ID_MSI && !dev->msi_cap) ||
+	    (type == PCI_CAP_ID_MSIX && !dev->msix_cap))
+		return -EINVAL;
+
 	/*
 	 * You can't ask to have 0 or less MSIs configured.
 	 *  a) it's stupid ..
@@ -795,9 +800,6 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
 	if (ret)
 		return ret;
 
-	if (!pci_find_capability(dev, type))
-		return -EINVAL;
-
 	return 0;
 }
 
-- 
1.7.5.4

[PATCH 3/5] PCI: Use cached MSI cap while enabling MSI interrupts

[Gavin Shan]

The patch intends to use the cached MSI capability offset in
pci_dev instead of polling that from config space when enabling
MSI interrupts.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/msi.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 5f51e10..182474d 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -552,14 +552,14 @@ out_unroll:
 static int msi_capability_init(struct pci_dev *dev, int nvec)
 {
 	struct msi_desc *entry;
-	int pos, ret;
+	int ret;
 	u16 control;
 	unsigned mask;
 
-	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
-	msi_set_enable(dev, 0);	/* Disable MSI during set up */
+	/* Disable MSI during set up */
+	msi_set_enable(dev, 0);
 
-	pci_read_config_word(dev, msi_control_reg(pos), &control);
+	pci_read_config_word(dev, msi_control_reg(dev->msi_cap), &control);
 	/* MSI Entry Initialization */
 	entry = alloc_msi_entry(dev);
 	if (!entry)
@@ -570,9 +570,9 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
 	entry->msi_attrib.entry_nr	= 0;
 	entry->msi_attrib.maskbit	= is_mask_bit_support(control);
 	entry->msi_attrib.default_irq	= dev->irq;	/* Save IOAPIC IRQ */
-	entry->msi_attrib.pos		= pos;
+	entry->msi_attrib.pos		= dev->msi_cap;
 
-	entry->mask_pos = msi_mask_reg(pos, entry->msi_attrib.is_64);
+	entry->mask_pos = msi_mask_reg(dev->msi_cap, entry->msi_attrib.is_64);
 	/* All MSIs are unmasked by default, Mask them all */
 	if (entry->msi_attrib.maskbit)
 		pci_read_config_dword(dev, entry->mask_pos, &entry->masked);
@@ -818,13 +818,12 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
  */
 int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
 {
-	int status, pos, maxvec;
+	int status, maxvec;
 	u16 msgctl;
 
-	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
-	if (!pos)
+	if (!dev->msi_cap)
 		return -EINVAL;
-	pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &msgctl);
+	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &msgctl);
 	maxvec = 1 << ((msgctl & PCI_MSI_FLAGS_QMASK) >> 1);
 	if (nvec > maxvec)
 		return maxvec;
-- 
1.7.5.4

[PATCH 4/5] PCI: Use cached MSI cap in pci_enable_msi_block_auto()

[Gavin Shan]

The patch intends to use cached MSI capability offset instead of
polling that from config space in pci_enable_msi_block_auto().

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/msi.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 182474d..0b6b254 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -848,14 +848,13 @@ EXPORT_SYMBOL(pci_enable_msi_block);
 
 int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *maxvec)
 {
-	int ret, pos, nvec;
+	int ret, nvec;
 	u16 msgctl;
 
-	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
-	if (!pos)
+	if (!dev->msi_cap)
 		return -EINVAL;
 
-	pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &msgctl);
+	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &msgctl);
 	ret = 1 << ((msgctl & PCI_MSI_FLAGS_QMASK) >> 1);
 
 	if (maxvec)
-- 
1.7.5.4

[PATCH 5/5] PCI: Use cached MSI-X cap while enabling MSI-X interrupts

[Gavin Shan]

The patch intends to use the cached MSI-X capability offset in
pci_dev instead of polling that from config space when enabling
MSI-X interrupts.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 drivers/pci/msi.c |   34 +++++++++++++++-------------------
 1 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 0b6b254..13c002a 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -605,14 +605,14 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
 	return 0;
 }
 
-static void __iomem *msix_map_region(struct pci_dev *dev, unsigned pos,
-							unsigned nr_entries)
+static void __iomem *msix_map_region(struct pci_dev *dev, unsigned nr_entries)
 {
 	resource_size_t phys_addr;
 	u32 table_offset;
 	u8 bir;
 
-	pci_read_config_dword(dev, msix_table_offset_reg(pos), &table_offset);
+	pci_read_config_dword(dev,
+		msix_table_offset_reg(dev->msix_cap), &table_offset);
 	bir = (u8)(table_offset & PCI_MSIX_FLAGS_BIRMASK);
 	table_offset &= ~PCI_MSIX_FLAGS_BIRMASK;
 	phys_addr = pci_resource_start(dev, bir) + table_offset;
@@ -620,9 +620,8 @@ static void __iomem *msix_map_region(struct pci_dev *dev, unsigned pos,
 	return ioremap_nocache(phys_addr, nr_entries * PCI_MSIX_ENTRY_SIZE);
 }
 
-static int msix_setup_entries(struct pci_dev *dev, unsigned pos,
-				void __iomem *base, struct msix_entry *entries,
-				int nvec)
+static int msix_setup_entries(struct pci_dev *dev, void __iomem *base,
+			      struct msix_entry *entries, int nvec)
 {
 	struct msi_desc *entry;
 	int i;
@@ -642,7 +641,7 @@ static int msix_setup_entries(struct pci_dev *dev, unsigned pos,
 		entry->msi_attrib.is_64		= 1;
 		entry->msi_attrib.entry_nr	= entries[i].entry;
 		entry->msi_attrib.default_irq	= dev->irq;
-		entry->msi_attrib.pos		= pos;
+		entry->msi_attrib.pos		= dev->msix_cap;
 		entry->mask_base		= base;
 
 		list_add_tail(&entry->list, &dev->msi_list);
@@ -682,23 +681,22 @@ static void msix_program_entries(struct pci_dev *dev,
 static int msix_capability_init(struct pci_dev *dev,
 				struct msix_entry *entries, int nvec)
 {
-	int pos, ret;
+	int ret;
 	u16 control;
 	void __iomem *base;
 
-	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
-	pci_read_config_word(dev, pos + PCI_MSIX_FLAGS, &control);
+	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &control);
 
 	/* Ensure MSI-X is disabled while it is set up */
 	control &= ~PCI_MSIX_FLAGS_ENABLE;
-	pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
+	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, control);
 
 	/* Request & Map MSI-X table region */
-	base = msix_map_region(dev, pos, multi_msix_capable(control));
+	base = msix_map_region(dev, multi_msix_capable(control));
 	if (!base)
 		return -ENOMEM;
 
-	ret = msix_setup_entries(dev, pos, base, entries, nvec);
+	ret = msix_setup_entries(dev, base, entries, nvec);
 	if (ret)
 		return ret;
 
@@ -712,7 +710,7 @@ static int msix_capability_init(struct pci_dev *dev,
 	 * interrupts coming in before they're fully set up.
 	 */
 	control |= PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE;
-	pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
+	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, control);
 
 	msix_program_entries(dev, entries);
 
@@ -727,7 +725,7 @@ static int msix_capability_init(struct pci_dev *dev,
 	dev->msix_enabled = 1;
 
 	control &= ~PCI_MSIX_FLAGS_MASKALL;
-	pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
+	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, control);
 
 	return 0;
 
@@ -917,14 +915,12 @@ EXPORT_SYMBOL(pci_disable_msi);
  */
 int pci_msix_table_size(struct pci_dev *dev)
 {
-	int pos;
 	u16 control;
 
-	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
-	if (!pos)
+	if (!dev->msix_cap)
 		return 0;
 
-	pci_read_config_word(dev, msi_control_reg(pos), &control);
+	pci_read_config_word(dev, msi_control_reg(dev->msix_cap), &control);
 	return multi_msix_capable(control);
 }
 
-- 
1.7.5.4

Re: [PATCH 1/5] PCI: Cache MSI/MSI-X cap in PCI device

[Bjorn Helgaas]

On Thu, Apr 4, 2013 at 5:39 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
> The patch caches the MSI and MSI-X capability offset in PCI device
> (struct pci_dev) so that we needn't poll it from the config space
> upon enabling or disabling MSI or MSI-X interrupts.
>
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  drivers/pci/msi.c   |   36 +++++++++++++++++++-----------------
>  include/linux/pci.h |    2 ++
>  2 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 00cc78c..3459bdf 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -111,31 +111,31 @@ void default_restore_msi_irqs(struct pci_dev *dev, int irq)
>  }
>  #endif
>
> -static void msi_set_enable(struct pci_dev *dev, int pos, int enable)
> +static void msi_set_enable(struct pci_dev *dev, int enable)
>  {
>         u16 control;
>
> -       BUG_ON(!pos);
> +       BUG_ON(!dev->msi_cap);
>
> -       pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &control);
> +       pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
>         control &= ~PCI_MSI_FLAGS_ENABLE;
>         if (enable)
>                 control |= PCI_MSI_FLAGS_ENABLE;
> -       pci_write_config_word(dev, pos + PCI_MSI_FLAGS, control);
> +       pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, control);
>  }
>
>  static void msix_set_enable(struct pci_dev *dev, int enable)
>  {
> -       int pos;
>         u16 control;
>
> -       pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> -       if (pos) {
> -               pci_read_config_word(dev, pos + PCI_MSIX_FLAGS, &control);
> +       if (dev->msix_cap) {
> +               pci_read_config_word(dev,
> +                       dev->msix_cap + PCI_MSIX_FLAGS, &control);
>                 control &= ~PCI_MSIX_FLAGS_ENABLE;
>                 if (enable)
>                         control |= PCI_MSIX_FLAGS_ENABLE;
> -               pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
> +               pci_write_config_word(dev,
> +                       dev->msix_cap + PCI_MSIX_FLAGS, control);
>         }
>  }
>
> @@ -402,7 +402,7 @@ static void __pci_restore_msi_state(struct pci_dev *dev)
>         pos = entry->msi_attrib.pos;
>
>         pci_intx_for_msi(dev, 0);
> -       msi_set_enable(dev, pos, 0);
> +       msi_set_enable(dev, 0);
>         arch_restore_msi_irqs(dev, dev->irq);
>
>         pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &control);
> @@ -557,7 +557,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>         unsigned mask;
>
>         pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> -       msi_set_enable(dev, pos, 0);    /* Disable MSI during set up */
> +       msi_set_enable(dev, 0); /* Disable MSI during set up */
>
>         pci_read_config_word(dev, msi_control_reg(pos), &control);
>         /* MSI Entry Initialization */
> @@ -598,7 +598,7 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>
>         /* Set MSI enabled bits  */
>         pci_intx_for_msi(dev, 0);
> -       msi_set_enable(dev, pos, 1);
> +       msi_set_enable(dev, 1);
>         dev->msi_enabled = 1;
>
>         dev->irq = entry->irq;
> @@ -885,7 +885,7 @@ void pci_msi_shutdown(struct pci_dev *dev)
>         desc = list_first_entry(&dev->msi_list, struct msi_desc, list);
>         pos = desc->msi_attrib.pos;
>
> -       msi_set_enable(dev, pos, 0);
> +       msi_set_enable(dev, 0);
>         pci_intx_for_msi(dev, 1);
>         dev->msi_enabled = 0;
>
> @@ -1048,15 +1048,17 @@ EXPORT_SYMBOL(pci_msi_enabled);
>
>  void pci_msi_init_pci_dev(struct pci_dev *dev)
>  {
> -       int pos;
>         INIT_LIST_HEAD(&dev->msi_list);
>
>         /* Disable the msi hardware to avoid screaming interrupts
>          * during boot.  This is the power on reset default so
>          * usually this should be a noop.
>          */
> -       pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> -       if (pos)
> -               msi_set_enable(dev, pos, 0);
> +       dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
> +       if (dev->msi_cap)
> +               msi_set_enable(dev, 0);
> +
> +       /* We needn't check if we have valid MSI-X capability here */
> +       dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
>         msix_set_enable(dev, 0);

Sheesh, this is a gratuitous difference between msi_set_enable() and
msix_set_enable().

Personally, I would remove the BUG_ON from msi_set_enable() and remove
the "if (pos)" from msix_set_enable() and make sure we never call them
when dev->msi_cap or dev->msix_cap are NULL, respectively.  This is
probably the only place where we'd need to add a guard to keep from
calling msix_set_enable().

>  }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2461033a..f8314c7 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -249,6 +249,8 @@ struct pci_dev {
>         pci_power_t     current_state;  /* Current operating state. In ACPI-speak,
>                                            this is D0-D3, D0 being fully functional,
>                                            and D3 being off. */
> +       int             msi_cap;        /* MSI capability offset */
> +       int             msix_cap;       /* MSI-X capability offset */

MSI and MSI-X are defined by PCI 3.0, so these capabilities must be in
PCI Configuration Space (not in the PCIe Extended Configuration
Space).  So u8 is big enough for these.  The same is true for pm_cap
below.

>         int             pm_cap;         /* PM capability offset in the
>                                            configuration space */
>         unsigned int    pme_support:5;  /* Bitmask of states from which PME#
> --
> 1.7.5.4
>

Re: [PATCH 2/5] PCI: Use MSI/MSI-X cap cache in pci_msi_check_device()

[Bjorn Helgaas]

On Thu, Apr 4, 2013 at 5:39 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
> The function pci_msi_check_device() is called while enabling MSI
> or MSI-X interrupts to make sure the PCI device can support MSI
> or MSI-X capability. The patch changes the function for a bit to
> use the cached MSI or MSI-X capability in pci_dev instead of polling
> them from config space.
>
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  drivers/pci/msi.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 3459bdf..5f51e10 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -772,6 +772,11 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
>         if (!pci_msi_enable || !dev || dev->no_msi)
>                 return -EINVAL;
>
> +       /* Check if the PCI device has MSI or MSI-X capability */
> +       if ((type == PCI_CAP_ID_MSI && !dev->msi_cap) ||
> +           (type == PCI_CAP_ID_MSIX && !dev->msix_cap))
> +               return -EINVAL;

The whole point of this is simplification, so just add "if
(!dev->msi_cap) return -EINVAL" (or "if (!dev->msix_cap)") to the
callers and drop the checking here in pci_msi_check_device().

> +
>         /*
>          * You can't ask to have 0 or less MSIs configured.
>          *  a) it's stupid ..
> @@ -795,9 +800,6 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
>         if (ret)
>                 return ret;
>
> -       if (!pci_find_capability(dev, type))
> -               return -EINVAL;
> -
>         return 0;
>  }
>
> --
> 1.7.5.4
>

Re: [PATCH 3/5] PCI: Use cached MSI cap while enabling MSI interrupts

[Bjorn Helgaas]

On Thu, Apr 4, 2013 at 5:39 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
> The patch intends to use the cached MSI capability offset in
> pci_dev instead of polling that from config space when enabling
> MSI interrupts.
>
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  drivers/pci/msi.c |   19 +++++++++----------
>  1 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 5f51e10..182474d 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -552,14 +552,14 @@ out_unroll:
>  static int msi_capability_init(struct pci_dev *dev, int nvec)
>  {
>         struct msi_desc *entry;
> -       int pos, ret;
> +       int ret;
>         u16 control;
>         unsigned mask;
>
> -       pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> -       msi_set_enable(dev, 0); /* Disable MSI during set up */
> +       /* Disable MSI during set up */
> +       msi_set_enable(dev, 0);
>
> -       pci_read_config_word(dev, msi_control_reg(pos), &control);
> +       pci_read_config_word(dev, msi_control_reg(dev->msi_cap), &control);

While you're at at, can you get rid of msi_control_reg() and friends?
They are needlessly different from the common style in this file,
e.g., "pos + PCI_MSI_FLAGS".

>         /* MSI Entry Initialization */
>         entry = alloc_msi_entry(dev);
>         if (!entry)
> @@ -570,9 +570,9 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
>         entry->msi_attrib.entry_nr      = 0;
>         entry->msi_attrib.maskbit       = is_mask_bit_support(control);
>         entry->msi_attrib.default_irq   = dev->irq;     /* Save IOAPIC IRQ */
> -       entry->msi_attrib.pos           = pos;
> +       entry->msi_attrib.pos           = dev->msi_cap;
>
> -       entry->mask_pos = msi_mask_reg(pos, entry->msi_attrib.is_64);
> +       entry->mask_pos = msi_mask_reg(dev->msi_cap, entry->msi_attrib.is_64);
>         /* All MSIs are unmasked by default, Mask them all */
>         if (entry->msi_attrib.maskbit)
>                 pci_read_config_dword(dev, entry->mask_pos, &entry->masked);
> @@ -818,13 +818,12 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
>   */
>  int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
>  {
> -       int status, pos, maxvec;
> +       int status, maxvec;
>         u16 msgctl;
>
> -       pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> -       if (!pos)
> +       if (!dev->msi_cap)
>                 return -EINVAL;
> -       pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &msgctl);
> +       pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &msgctl);
>         maxvec = 1 << ((msgctl & PCI_MSI_FLAGS_QMASK) >> 1);
>         if (nvec > maxvec)
>                 return maxvec;
> --
> 1.7.5.4
>

Re: [PATCH 4/5] PCI: Use cached MSI cap in pci_enable_msi_block_auto()

[Bjorn Helgaas]

On Thu, Apr 4, 2013 at 5:39 AM, Gavin Shan <shangw@linux.vnet.ibm.com> wrote:
> The patch intends to use cached MSI capability offset instead of
> polling that from config space in pci_enable_msi_block_auto().
>
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  drivers/pci/msi.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 182474d..0b6b254 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -848,14 +848,13 @@ EXPORT_SYMBOL(pci_enable_msi_block);
>
>  int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *maxvec)
>  {
> -       int ret, pos, nvec;
> +       int ret, nvec;
>         u16 msgctl;
>
> -       pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> -       if (!pos)
> +       if (!dev->msi_cap)
>                 return -EINVAL;
>
> -       pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &msgctl);
> +       pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &msgctl);

I don't understand why this is in a separate patch.  Just add and
populate the cache, then replace all the pci_find_capability() calls
with a dev->msi_cap reference at once.

>         ret = 1 << ((msgctl & PCI_MSI_FLAGS_QMASK) >> 1);
>
>         if (maxvec)
> --
> 1.7.5.4
>