[PATCH v5 0/3] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers

[PATCH v5 0/3] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers

[Bjorn Helgaas]

This is based on Dongdong's series:
  v3: http://lkml.kernel.org/r/1507553167-27833-1-git-send-email-liudongdong3@huawei.com
  v4: http://lkml.kernel.org/r/1507719178-112556-1-git-send-email-liudongdong3@huawei.com

The main idea is to fix IRQ issues when removing and adding back a Root
Port.  The problem is that when we allocate, free, and reallocate MSI/MSI-X
vectors for the device, we may not get the same vectors the second time,
but we saved IRQs based on the first set of vectors.

---

Bjorn Helgaas (2):
      PCI/portdrv: Remove excessive comments
      PCI/portdrv: Compute MSI/MSI-X IRQ vectors after final allocation

Dongdong Liu (1):
      PCI/portdrv: Add #defines for AER and DPC Interrupt Message Number masks


 drivers/pci/pcie/portdrv_core.c |  162 ++++++++++++++++-----------------------
 include/uapi/linux/pci_regs.h   |    2 
 2 files changed, 70 insertions(+), 94 deletions(-)

[PATCH v5 1/3] PCI/portdrv: Add #defines for AER and DPC Interrupt Message Number masks

[Bjorn Helgaas]

From: Dongdong Liu <liudongdong3@huawei.com>

In the AER case, the mask isn't strictly necessary because there are no
higher-order bits above the Interrupt Message Number, but using a #define
will make it possible to grep for it.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/portdrv_core.c |    4 ++--
 include/uapi/linux/pci_regs.h   |    2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 313a21df1692..72fcbe5567dd 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -114,7 +114,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 		 */
 		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
 		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &reg32);
-		entry = reg32 >> 27;
+		entry = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27;
 		if (entry >= nr_entries)
 			goto out_free_irqs;
 
@@ -141,7 +141,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 		 */
 		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC);
 		pci_read_config_word(dev, pos + PCI_EXP_DPC_CAP, &reg16);
-		entry = reg16 & 0x1f;
+		entry = reg16 & PCI_EXP_DPC_IRQ;
 		if (entry >= nr_entries)
 			goto out_free_irqs;
 
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 2763e4f9615d..0c4c362c3d9a 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -746,6 +746,7 @@
 #define PCI_ERR_ROOT_FIRST_FATAL	0x00000010 /* First UNC is Fatal */
 #define PCI_ERR_ROOT_NONFATAL_RCV	0x00000020 /* Non-Fatal Received */
 #define PCI_ERR_ROOT_FATAL_RCV		0x00000040 /* Fatal Received */
+#define PCI_ERR_ROOT_AER_IRQ		0xf8000000 /* Advanced Error Interrupt Message Number */
 #define PCI_ERR_ROOT_ERR_SRC	52	/* Error Source Identification */
 
 /* Virtual Channel */
@@ -960,6 +961,7 @@
 
 /* Downstream Port Containment */
 #define PCI_EXP_DPC_CAP			4	/* DPC Capability */
+#define PCI_EXP_DPC_IRQ			0x1f	/* DPC Interrupt Message Number */
 #define  PCI_EXP_DPC_CAP_RP_EXT		0x20	/* Root Port Extensions for DPC */
 #define  PCI_EXP_DPC_CAP_POISONED_TLP	0x40	/* Poisoned TLP Egress Blocking Supported */
 #define  PCI_EXP_DPC_CAP_SW_TRIGGER	0x80	/* Software Triggering Supported */

[PATCH v5 2/3] PCI/portdrv: Remove excessive comments

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Remove some wordy and repetitive comments so we can see the code better.
The register field #defines should be enough of a hook to find relevant
descriptions in the spec.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/portdrv_core.c |   56 +--------------------------------------
 1 file changed, 2 insertions(+), 54 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 72fcbe5567dd..7cd2eafda652 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -56,12 +56,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 {
 	int nr_entries, entry, nvec = 0;
 
-	/*
-	 * Allocate as many entries as the port wants, so that we can check
-	 * which of them will be useful.  Moreover, if nr_entries is correctly
-	 * equal to the number of entries this port actually uses, we'll happily
-	 * go through without any tricks.
-	 */
+	/* Allocate the maximum possible number of MSI/MSI-X vectors */
 	nr_entries = pci_alloc_irq_vectors(dev, 1, PCIE_PORT_MAX_MSI_ENTRIES,
 			PCI_IRQ_MSIX | PCI_IRQ_MSI);
 	if (nr_entries < 0)
@@ -70,21 +65,6 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 	if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP)) {
 		u16 reg16;
 
-		/*
-		 * Per PCIe r3.1, sec 6.1.6, "PME and Hot-Plug Event
-		 * interrupts (when both are implemented) always share the
-		 * same MSI or MSI-X vector, as indicated by the Interrupt
-		 * Message Number field in the PCI Express Capabilities
-		 * register".
-		 *
-		 * Per sec 7.8.2, "For MSI, the [Interrupt Message Number]
-		 * indicates the offset between the base Message Data and
-		 * the interrupt message that is generated."
-		 *
-		 * "For MSI-X, the [Interrupt Message Number] indicates
-		 * which MSI-X Table entry is used to generate the
-		 * interrupt message."
-		 */
 		pcie_capability_read_word(dev, PCI_EXP_FLAGS, &reg16);
 		entry = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9;
 		if (entry >= nr_entries)
@@ -99,19 +79,6 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 	if (mask & PCIE_PORT_SERVICE_AER) {
 		u32 reg32, pos;
 
-		/*
-		 * Per PCIe r3.1, sec 7.10.10, the Advanced Error Interrupt
-		 * Message Number in the Root Error Status register
-		 * indicates which MSI/MSI-X vector is used for AER.
-		 *
-		 * "For MSI, the [Advanced Error Interrupt Message Number]
-		 * indicates the offset between the base Message Data and
-		 * the interrupt message that is generated."
-		 *
-		 * "For MSI-X, the [Advanced Error Interrupt Message
-		 * Number] indicates which MSI-X Table entry is used to
-		 * generate the interrupt message."
-		 */
 		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
 		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &reg32);
 		entry = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27;
@@ -126,19 +93,6 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 	if (mask & PCIE_PORT_SERVICE_DPC) {
 		u16 reg16, pos;
 
-		/*
-		 * Per PCIe r4.0 (v0.9), sec 7.9.15.2, the DPC Interrupt
-		 * Message Number in the DPC Capability register indicates
-		 * which MSI/MSI-X vector is used for DPC.
-		 *
-		 * "For MSI, the [DPC Interrupt Message Number] indicates
-		 * the offset between the base Message Data and the
-		 * interrupt message that is generated."
-		 *
-		 * "For MSI-X, the [DPC Interrupt Message Number] indicates
-		 * which MSI-X Table entry is used to generate the
-		 * interrupt message."
-		 */
 		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC);
 		pci_read_config_word(dev, pos + PCI_EXP_DPC_CAP, &reg16);
 		entry = reg16 & PCI_EXP_DPC_IRQ;
@@ -150,16 +104,10 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 		nvec = max(nvec, entry + 1);
 	}
 
-	/*
-	 * If nvec is equal to the allocated number of entries, we can just use
-	 * what we have.  Otherwise, the port has some extra entries not for the
-	 * services we know and we need to work around that.
-	 */
+	/* If we allocated more than we need, free them and allocate fewer */
 	if (nvec != nr_entries) {
-		/* Drop the temporary MSI-X setup */
 		pci_free_irq_vectors(dev);
 
-		/* Now allocate the MSI-X vectors for real */
 		nr_entries = pci_alloc_irq_vectors(dev, nvec, nvec,
 				PCI_IRQ_MSIX | PCI_IRQ_MSI);
 		if (nr_entries < 0)

[PATCH v5 3/3] PCI/portdrv: Compute MSI/MSI-X IRQ vectors after final allocation

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

When setting up portdrv MSI/MSI-X interrupts, we previously allocated the
maximum possible number of vectors, read the Interrupt Message Numbers for
each service, saved the IRQ for each, freed the vectors, and finally used
the largest Message Number to reallocate only as many vectors as we need.

The problem is that freeing the vectors invalidates their IRQs, so the
saved IRQ numbers may now be invalid, which can result in errors like
this:

  pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
  pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
  aer: probe of 0000:00:00.0:pcie002 failed with error -22
  dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22

Change the setup so we save the Interrupt Message Numbers (not the IRQs)
before we free the original setup, then use the Message Numbers to compute
the IRQs (via pci_irq_vector()) *after* we reallocate the vectors.

This should always be safe for MSI-X because the Message Numbers are fixed.
For MSI, the hardware is allowed to change Message Numbers when we update
the MSI Multiple Message Enable field when reallocating the vectors, but
since we allocate enough vectors to accommodate the largest Message Number
we found, that's unlikely.  See PCIe r3.1, sec 7.8.2, 7.10.10, 7.31.2.

Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()")
Based-on-patch-by: Dongdong Liu <liudongdong3@huawei.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/portdrv_core.c |  112 ++++++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 43 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 7cd2eafda652..da03dc230f37 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -43,6 +43,46 @@ static void release_pcie_device(struct device *dev)
 	kfree(to_pcie_device(dev));
 }
 
+/*
+ * Fill in *pme, *aer, *dpc with the relevant Interrupt Message Numbers if
+ * services are enabled in "mask".  Return the number of MSI/MSI-X vectors
+ * required to accommodate the largest Message Number.
+ */
+static int pcie_message_numbers(struct pci_dev *dev, int mask,
+				u32 *pme, u32 *aer, u32 *dpc)
+{
+	u32 nvec = 0, pos, reg32;
+	u16 reg16;
+
+	if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP)) {
+		pcie_capability_read_word(dev, PCI_EXP_FLAGS, &reg16);
+		*pme = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9;
+		nvec = *pme + 1;
+	}
+
+	if (mask & PCIE_PORT_SERVICE_AER) {
+		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+		if (pos) {
+			pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS,
+					      &reg32);
+			*aer = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27;
+			nvec = max(nvec, *aer + 1);
+		}
+	}
+
+	if (mask & PCIE_PORT_SERVICE_DPC) {
+		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC);
+		if (pos) {
+			pci_read_config_word(dev, pos + PCI_EXP_DPC_CAP,
+					     &reg16);
+			*dpc = reg16 & PCI_EXP_DPC_IRQ;
+			nvec = max(nvec, *dpc + 1);
+		}
+	}
+
+	return nvec;
+}
+
 /**
  * pcie_port_enable_irq_vec - try to set up MSI-X or MSI as interrupt mode
  * for given port
@@ -54,7 +94,8 @@ static void release_pcie_device(struct device *dev)
  */
 static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 {
-	int nr_entries, entry, nvec = 0;
+	int nr_entries, nvec;
+	u32 pme = 0, aer = 0, dpc = 0;
 
 	/* Allocate the maximum possible number of MSI/MSI-X vectors */
 	nr_entries = pci_alloc_irq_vectors(dev, 1, PCIE_PORT_MAX_MSI_ENTRIES,
@@ -62,49 +103,22 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 	if (nr_entries < 0)
 		return nr_entries;
 
-	if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP)) {
-		u16 reg16;
-
-		pcie_capability_read_word(dev, PCI_EXP_FLAGS, &reg16);
-		entry = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9;
-		if (entry >= nr_entries)
-			goto out_free_irqs;
+	/* See how many and which Interrupt Message Numbers we actually use */
+	nvec = pcie_message_numbers(dev, mask, &pme, &aer, &dpc);
+	if (nvec > nr_entries)
+		goto out_free_irqs;
 
-		irqs[PCIE_PORT_SERVICE_PME_SHIFT] = pci_irq_vector(dev, entry);
-		irqs[PCIE_PORT_SERVICE_HP_SHIFT] = pci_irq_vector(dev, entry);
-
-		nvec = max(nvec, entry + 1);
-	}
-
-	if (mask & PCIE_PORT_SERVICE_AER) {
-		u32 reg32, pos;
-
-		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
-		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &reg32);
-		entry = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27;
-		if (entry >= nr_entries)
-			goto out_free_irqs;
-
-		irqs[PCIE_PORT_SERVICE_AER_SHIFT] = pci_irq_vector(dev, entry);
-
-		nvec = max(nvec, entry + 1);
-	}
-
-	if (mask & PCIE_PORT_SERVICE_DPC) {
-		u16 reg16, pos;
-
-		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC);
-		pci_read_config_word(dev, pos + PCI_EXP_DPC_CAP, &reg16);
-		entry = reg16 & PCI_EXP_DPC_IRQ;
-		if (entry >= nr_entries)
-			goto out_free_irqs;
-
-		irqs[PCIE_PORT_SERVICE_DPC_SHIFT] = pci_irq_vector(dev, entry);
-
-		nvec = max(nvec, entry + 1);
-	}
-
-	/* If we allocated more than we need, free them and allocate fewer */
+	/*
+	 * If we allocated more than we need, free them and reallocate fewer.
+	 *
+	 * Note that reallocating may change the specific vectors we get,
+	 * so pci_irq_vector() must be done *after* the reallocation.
+	 *
+	 * If we're using MSI, hardware is *allowed* to change the Interrupt
+	 * Message Numbers when we free and reallocate the vectors, but we
+	 * assume it won't because we allocate enough vectors for the
+	 * biggest Message Number we found.
+	 */
 	if (nvec != nr_entries) {
 		pci_free_irq_vectors(dev);
 
@@ -114,6 +128,18 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 			return nr_entries;
 	}
 
+	/* PME and hotplug share an MSI/MSI-X vector (PCIe r3.1, sec 6.1.6) */
+	if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP)) {
+		irqs[PCIE_PORT_SERVICE_PME_SHIFT] = pci_irq_vector(dev, pme);
+		irqs[PCIE_PORT_SERVICE_HP_SHIFT] = pci_irq_vector(dev, pme);
+	}
+
+	if (mask & PCIE_PORT_SERVICE_AER)
+		irqs[PCIE_PORT_SERVICE_AER_SHIFT] = pci_irq_vector(dev, aer);
+
+	if (mask & PCIE_PORT_SERVICE_DPC)
+		irqs[PCIE_PORT_SERVICE_DPC_SHIFT] = pci_irq_vector(dev, dpc);
+
 	return 0;
 
 out_free_irqs:

Re: [PATCH v5 1/3] PCI/portdrv: Add #defines for AER and DPC Interrupt Message Number masks

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v5 2/3] PCI/portdrv: Remove excessive comments

[Christoph Hellwig]

On Thu, Oct 19, 2017 at 06:01:15PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Remove some wordy and repetitive comments so we can see the code better.
> The register field #defines should be enough of a hook to find relevant
> descriptions in the spec.  No functional change.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Of course my comments were actually useful ;-)

Looks fine, and fit the style in the rest of the kernel:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v5 3/3] PCI/portdrv: Compute MSI/MSI-X IRQ vectors after final allocation

[Christoph Hellwig]

> +/*
> + * Fill in *pme, *aer, *dpc with the relevant Interrupt Message Numbers if
> + * services are enabled in "mask".  Return the number of MSI/MSI-X vectors
> + * required to accommodate the largest Message Number.
> + */
> +static int pcie_message_numbers(struct pci_dev *dev, int mask,
> +				u32 *pme, u32 *aer, u32 *dpc)
> +{

Can you split factoring out this helper into a separate clean up patch
that goes before the actual change?

Otherwise this looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Note that we should probably replace the irqs array with one storing
the relative vector number and use pci_request_irq().  In fact in that
case we could probably just pass said array to pcie_message_numbers()
to further simplify it.

Re: [PATCH v5 0/3] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers

[Dongdong Liu]

Hi Bjorn

Many thanks for your work on the patchset.
The pachset looks good to me.
It works ok on HiSilicon hip08 platform.
Tested-by: Dongdong Liu <liudongdong3@huawei.com>

Thanks,
Dongdong
在 2017/10/20 7:01, Bjorn Helgaas 写道:
> This is based on Dongdong's series:
>   v3: http://lkml.kernel.org/r/1507553167-27833-1-git-send-email-liudongdong3@huawei.com
>   v4: http://lkml.kernel.org/r/1507719178-112556-1-git-send-email-liudongdong3@huawei.com
>
> The main idea is to fix IRQ issues when removing and adding back a Root
> Port.  The problem is that when we allocate, free, and reallocate MSI/MSI-X
> vectors for the device, we may not get the same vectors the second time,
> but we saved IRQs based on the first set of vectors.
>
> ---
>
> Bjorn Helgaas (2):
>       PCI/portdrv: Remove excessive comments
>       PCI/portdrv: Compute MSI/MSI-X IRQ vectors after final allocation
>
> Dongdong Liu (1):
>       PCI/portdrv: Add #defines for AER and DPC Interrupt Message Number masks
>
>
>  drivers/pci/pcie/portdrv_core.c |  162 ++++++++++++++++-----------------------
>  include/uapi/linux/pci_regs.h   |    2
>  2 files changed, 70 insertions(+), 94 deletions(-)
>
> .
>

Re: [PATCH v5 2/3] PCI/portdrv: Remove excessive comments

[Bjorn Helgaas]

On Fri, Oct 20, 2017 at 09:07:31AM +0200, Christoph Hellwig wrote:
> On Thu, Oct 19, 2017 at 06:01:15PM -0500, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Remove some wordy and repetitive comments so we can see the code better.
> > The register field #defines should be enough of a hook to find relevant
> > descriptions in the spec.  No functional change.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> Of course my comments were actually useful ;-)

Oops, I thought I was mainly removing stuff I had added myself :)
I added back this single comment in place of the three essentially
similar ones:


+	/*
+	 * The Interrupt Message Number indicates which vector is used, i.e.,
+	 * the MSI-X table entry or the MSI offset between the base Message
+	 * Data and the generated interrupt message.  See PCIe r3.1, sec
+	 * 7.8.2, 7.10.10, 7.31.2.
+	 */
 	if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP)) {
 		u16 reg16;
 
-		/*
-		 * Per PCIe r3.1, sec 6.1.6, "PME and Hot-Plug Event
-		 * interrupts (when both are implemented) always share the
-		 * same MSI or MSI-X vector, as indicated by the Interrupt
-		 * Message Number field in the PCI Express Capabilities
-		 * register".
-		 *
-		 * Per sec 7.8.2, "For MSI, the [Interrupt Message Number]
-		 * indicates the offset between the base Message Data and
-		 * the interrupt message that is generated."
-		 *
-		 * "For MSI-X, the [Interrupt Message Number] indicates
-		 * which MSI-X Table entry is used to generate the
-		 * interrupt message."
-		 */
 		pcie_capability_read_word(dev, PCI_EXP_FLAGS, &reg16);
 		entry = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9;
 		if (entry >= nr_entries)

...

Re: [PATCH v5 3/3] PCI/portdrv: Compute MSI/MSI-X IRQ vectors after final allocation

[Bjorn Helgaas]

On Fri, Oct 20, 2017 at 09:15:50AM +0200, Christoph Hellwig wrote:
> > +/*
> > + * Fill in *pme, *aer, *dpc with the relevant Interrupt Message Numbers if
> > + * services are enabled in "mask".  Return the number of MSI/MSI-X vectors
> > + * required to accommodate the largest Message Number.
> > + */
> > +static int pcie_message_numbers(struct pci_dev *dev, int mask,
> > +				u32 *pme, u32 *aer, u32 *dpc)
> > +{
> 
> Can you split factoring out this helper into a separate clean up patch
> that goes before the actual change?

I did try that, but I'll try again.

> Otherwise this looks fine:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Note that we should probably replace the irqs array with one storing
> the relative vector number and use pci_request_irq().  In fact in that
> case we could probably just pass said array to pcie_message_numbers()
> to further simplify it.

I like that idea.  But I don't think I have time to do it personally
right now.

Bjorn

Re: [PATCH v5 2/3] PCI/portdrv: Remove excessive comments

[Christoph Hellwig]

On Fri, Oct 20, 2017 at 08:31:43AM -0500, Bjorn Helgaas wrote:
> On Fri, Oct 20, 2017 at 09:07:31AM +0200, Christoph Hellwig wrote:
> > On Thu, Oct 19, 2017 at 06:01:15PM -0500, Bjorn Helgaas wrote:
> > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > 
> > > Remove some wordy and repetitive comments so we can see the code better.
> > > The register field #defines should be enough of a hook to find relevant
> > > descriptions in the spec.  No functional change.
> > > 
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Of course my comments were actually useful ;-)
> 
> Oops, I thought I was mainly removing stuff I had added myself :)
> I added back this single comment in place of the three essentially
> similar ones:

I don't think we need these - it's pretty clear that you need to
spec for those.

It was just intended as a snarky comment that you basically kept mine,
just rewritten to be a little more clear.  Sorry for the confusion.