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

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

[Dongdong Liu]

This patchset is to optimize the interrupt vector allocation strategy
in pcie_port_enable_irq_vec() and add #defines for the AER and DPC
Interrupt Message Number masks.

v3->v4: 
- Optimize the interrupt vector allocation strategy in
pcie_port_enable_irq_vec().
- add #defines for the AER and DPC Interrupt Message Number masks.
v2->v3:
- Use a 12-character SHA1 commit id.
- Rebase on v4.14-rc4.
v1->v2:
- Fix comments on PATCH v1.
- Simplify implementation.

Dongdong Liu (2):
  PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers
  PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number
    masks

 drivers/pci/pcie/portdrv_core.c | 49 ++++++++++++++---------------------------
 include/uapi/linux/pci_regs.h   |  2 ++
 2 files changed, 18 insertions(+), 33 deletions(-)

-- 
1.9.1

[PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers

[Dongdong Liu]

After removing and adding back the PCI root port device,
we see the PCIe port service drivers request irq failed.
pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
aer: probe of 0000:00:00.0:pcie002 failed with error -22
pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd-
PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
dpc: probe of 0000:00:00.0:pcie010 failed with error -22

The current code basically does this:

  - allocate 32 vectors
  - figure out vector used by PME and hotplug
  - figure out vector used by AER
  - figure out vector used by DPC
  - free the 32 vectors we allocated
  - allocate only as many vectors as we need

but it is broken as calling pci_free_irq_vectors()
invalidates the IRQ numbers returned before by pci_irq_vectors();

The hardware works:
  - PME and hotplug use the Interrupt Message Number from the PCIe
    Capability register.

  - AER uses the AER Interrupt Message Number from the AER Root Error
    Status register.

  - DPC uses the DPC Interrupt Message Number from the DPC Capability
    register.

  - FRS (not supported by Linux yet) uses the FRS Interrupt Message
    Number from the FRS Queuing Capability register.

  - That's a total of 4 possible MSI/MSI-X vectors used for PME,
    hotplug, AER, DPC, and FRS, so there's no point in trying to
    allocate more than 4 vectors (we currently start with 32).

  - All these Interrupt Message Numbers are read-only to software but
    are updated by hardware when software writes the Multiple Message
    Enable field of the MSI Message Control register.  Thanks for
    pointing this out; I didn't realize this before.

  - Therefore, we must read the Interrupt Message Numbers *after* we
    write Multiple Message Enable.

Since we can use at most 4 vectors, this patch is to optimize the
interrupt vector allocation strategy in pcie_port_enable_irq_vec().
First figure out how many vectors we need,allocate them, and then fill
in the slots in the irqs[] table.

Cc: <stable@vger.kernel.org>
Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()")
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 drivers/pci/pcie/portdrv_core.c | 45 +++++++++++++----------------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 313a21d..9692379 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -56,14 +56,16 @@ 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.
-	 */
-	nr_entries = pci_alloc_irq_vectors(dev, 1, PCIE_PORT_MAX_MSI_ENTRIES,
-			PCI_IRQ_MSIX | PCI_IRQ_MSI);
+	if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP))
+		nvec++;
+	if (mask & PCIE_PORT_SERVICE_AER)
+		nvec++;
+	if (mask & PCIE_PORT_SERVICE_DPC)
+		nvec++;
+
+	/* Allocate as many vectors as the we need */
+	nr_entries = pci_alloc_irq_vectors(dev, 1, nvec,
+					   PCI_IRQ_MSIX | PCI_IRQ_MSI);
 	if (nr_entries < 0)
 		return nr_entries;
 
@@ -90,10 +92,10 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 		if (entry >= 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_PME)
+			irqs[PCIE_PORT_SERVICE_PME_SHIFT] = pci_irq_vector(dev, entry);
+		if (mask & PCIE_PORT_SERVICE_HP)
+			irqs[PCIE_PORT_SERVICE_HP_SHIFT] = pci_irq_vector(dev, entry);
 	}
 
 	if (mask & PCIE_PORT_SERVICE_AER) {
@@ -120,7 +122,6 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 
 		irqs[PCIE_PORT_SERVICE_AER_SHIFT] = pci_irq_vector(dev, entry);
 
-		nvec = max(nvec, entry + 1);
 	}
 
 	if (mask & PCIE_PORT_SERVICE_DPC) {
@@ -146,24 +147,6 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
 			goto out_free_irqs;
 
 		irqs[PCIE_PORT_SERVICE_DPC_SHIFT] = pci_irq_vector(dev, entry);
-
-		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 (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)
-			return nr_entries;
 	}
 
 	return 0;
-- 
1.9.1

[PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks

[Dongdong Liu]

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>
---
 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 9692379..2b48297 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -116,7 +116,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;
 
@@ -142,7 +142,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 f8d5804..756e704 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 */
-- 
1.9.1

Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers

[Bjorn Helgaas]

On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote:
> After removing and adding back the PCI root port device,
> we see the PCIe port service drivers request irq failed.
> pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
> aer: probe of 0000:00:00.0:pcie002 failed with error -22
> pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd-
> PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
> pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
> dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
> dpc: probe of 0000:00:00.0:pcie010 failed with error -22
> 
> The current code basically does this:
> 
>   - allocate 32 vectors
>   - figure out vector used by PME and hotplug
>   - figure out vector used by AER
>   - figure out vector used by DPC
>   - free the 32 vectors we allocated
>   - allocate only as many vectors as we need
> 
> but it is broken as calling pci_free_irq_vectors()
> invalidates the IRQ numbers returned before by pci_irq_vectors();
> 
> The hardware works:
>   - PME and hotplug use the Interrupt Message Number from the PCIe
>     Capability register.
> 
>   - AER uses the AER Interrupt Message Number from the AER Root Error
>     Status register.
> 
>   - DPC uses the DPC Interrupt Message Number from the DPC Capability
>     register.
> 
>   - FRS (not supported by Linux yet) uses the FRS Interrupt Message
>     Number from the FRS Queuing Capability register.
> 
>   - That's a total of 4 possible MSI/MSI-X vectors used for PME,
>     hotplug, AER, DPC, and FRS, so there's no point in trying to
>     allocate more than 4 vectors (we currently start with 32).

You pointed out that there actually may be some benefit to allocating
more than 4 vectors because the hardware may not distribute the
Interrupt Message Numbers across the allocated vectors, e.g., you have
a system with

  HP/PME - message number 0
  AER - message number 2
  DPC - message number 6

In that case, if we allocate 8 vectors, there's probably no sharing
(other than HP/PME), but if we allocate 4 vectors, the hardware might
put DPC on message number 0 or 2.

Out of curiosity, what does happen on this system when we only
allocate 4 vectors?  Does DPC end up sharing a vector with PME or AER?
Or is it smart enough to put DPC on message number 1 or 3?

Bjorn

RE: [PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks

[David Laight]

From: Dongdong Liu
> Sent: 11 October 2017 11:53
> 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>
> ---
>  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 9692379..2b48297 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -116,7 +116,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;

You've still got the hard coded 27 - which is almost more important than the mask.
Perhaps define things so this is:
		entry = PCI_ERR_ROOT_AER_IRQ(reg32);

Think I'd shift then mask with 0x1f.

	David

RE: [PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks

[David Laight]

From: Dongdong Liu
> Sent: 11 October 2017 11:53
> 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.
>=20
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> ---
>  drivers/pci/pcie/portdrv_core.c | 4 ++--
>  include/uapi/linux/pci_regs.h   | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
>=20
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_c=
ore.c
> index 9692379..2b48297 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -116,7 +116,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *d=
ev, int *irqs, int mask)
>  		 */
>  		pos =3D pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
>  		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &reg32);
> -		entry =3D reg32 >> 27;
> +		entry =3D (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27;

You've still got the hard coded 27 - which is almost more important than th=
e mask.
Perhaps define things so this is:
		entry =3D PCI_ERR_ROOT_AER_IRQ(reg32);

Think I'd shift then mask with 0x1f.

	David

Re: [PATCH V4 2/2] PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number masks

[Bjorn Helgaas]

On Wed, Oct 11, 2017 at 01:01:26PM +0000, David Laight wrote:
> From: Dongdong Liu
> > Sent: 11 October 2017 11:53
> > 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>
> > ---
> >  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 9692379..2b48297 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -116,7 +116,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;
> 
> You've still got the hard coded 27 - which is almost more important than the mask.
> Perhaps define things so this is:
> 		entry = PCI_ERR_ROOT_AER_IRQ(reg32);
> 
> Think I'd shift then mask with 0x1f.

Personally, I prefer to have the mask unshifted so it's easy to see
where the fields are in the register and to compare them with the
spec, e.g.,

  #define  PCI_EXP_DEVCAP_PAYLOAD 0x00000007 /* Max_Payload_Size */
  #define  PCI_EXP_DEVCAP_PHANTOM 0x00000018 /* Phantom functions */
  #define  PCI_EXP_DEVCAP_EXT_TAG 0x00000020 /* Extended tags */

Should there be a #define for the shift?  Maybe.  It doesn't bother me
unless it's used in many places.  Here I think there'd only be one
use, so having it hard-coded along with the symbolic mask is enough
for me.  Sometimes too many #defines clutter up the include file.

Just my personal preferences.

Bjorn

Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers

[Dongdong Liu]

在 2017/10/11 19:30, Bjorn Helgaas 写道:
> On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote:
>> After removing and adding back the PCI root port device,
>> we see the PCIe port service drivers request irq failed.
>> pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
>> aer: probe of 0000:00:00.0:pcie002 failed with error -22
>> pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd-
>> PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
>> pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
>> pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
>> dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
>> dpc: probe of 0000:00:00.0:pcie010 failed with error -22
>>
>> The current code basically does this:
>>
>>   - allocate 32 vectors
>>   - figure out vector used by PME and hotplug
>>   - figure out vector used by AER
>>   - figure out vector used by DPC
>>   - free the 32 vectors we allocated
>>   - allocate only as many vectors as we need
>>
>> but it is broken as calling pci_free_irq_vectors()
>> invalidates the IRQ numbers returned before by pci_irq_vectors();
>>
>> The hardware works:
>>   - PME and hotplug use the Interrupt Message Number from the PCIe
>>     Capability register.
>>
>>   - AER uses the AER Interrupt Message Number from the AER Root Error
>>     Status register.
>>
>>   - DPC uses the DPC Interrupt Message Number from the DPC Capability
>>     register.
>>
>>   - FRS (not supported by Linux yet) uses the FRS Interrupt Message
>>     Number from the FRS Queuing Capability register.
>>
>>   - That's a total of 4 possible MSI/MSI-X vectors used for PME,
>>     hotplug, AER, DPC, and FRS, so there's no point in trying to
>>     allocate more than 4 vectors (we currently start with 32).
>
> You pointed out that there actually may be some benefit to allocating
> more than 4 vectors because the hardware may not distribute the
> Interrupt Message Numbers across the allocated vectors, e.g., you have
> a system with
>
>   HP/PME - message number 0
>   AER - message number 2
>   DPC - message number 6
>
> In that case, if we allocate 8 vectors, there's probably no sharing
> (other than HP/PME), but if we allocate 4 vectors, the hardware might
> put DPC on message number 0 or 2.

Yes, test on HiSilicon hip08 platform, the HW puts DPC on message number 2.

>
> Out of curiosity, what does happen on this system when we only
> allocate 4 vectors?  Does DPC end up sharing a vector with PME or AER?
> Or is it smart enough to put DPC on message number 1 or 3?

Test on HiSilicon hip08 platform, DPC shares a vector withs AER.

[  139.106191] pcieport 0000:00:08.0: PME entry 0
[  139.112511] pcieport 0000:00:08.0: AER entry 2
[  139.117890] pcieport 0000:00:08.0: DPC entry 2

  19:          1   ITS-MSI 131074 Edge      aerdrv, pcie-dpc

Thanks,
Dongdong
>
> Bjorn
>
> .
>

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

[Dongdong Liu]

Hi Bjorn

Is this patchset ok ?  or do I still need to work v5 without optimizing
the interrupt vector allocation strategy(just use PATCH v3 and
add #defines for the AER and DPC Interrupt Message Number masks.)

Thanks,
Dongdong

锟斤拷 2017/10/11 18:52, Dongdong Liu 写锟斤拷:
> This patchset is to optimize the interrupt vector allocation strategy
> in pcie_port_enable_irq_vec() and add #defines for the AER and DPC
> Interrupt Message Number masks.
>
> v3->v4:
> - Optimize the interrupt vector allocation strategy in
> pcie_port_enable_irq_vec().
> - add #defines for the AER and DPC Interrupt Message Number masks.
> v2->v3:
> - Use a 12-character SHA1 commit id.
> - Rebase on v4.14-rc4.
> v1->v2:
> - Fix comments on PATCH v1.
> - Simplify implementation.
>
> Dongdong Liu (2):
>   PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers
>   PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number
>     masks
>
>  drivers/pci/pcie/portdrv_core.c | 49 ++++++++++++++---------------------------
>  include/uapi/linux/pci_regs.h   |  2 ++
>  2 files changed, 18 insertions(+), 33 deletions(-)
>

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

[Dongdong Liu]

Hi Bjorn

Is this patchset ok ?  or do I still need to work v5 without optimizing
the interrupt vector allocation strategy(just use PATCH v3 and
add #defines for the AER and DPC Interrupt Message Number masks.)

Thanks,
Dongdong

在 2017/10/11 18:52, Dongdong Liu 写道:
> This patchset is to optimize the interrupt vector allocation strategy
> in pcie_port_enable_irq_vec() and add #defines for the AER and DPC
> Interrupt Message Number masks.
>
> v3->v4:
> - Optimize the interrupt vector allocation strategy in
> pcie_port_enable_irq_vec().
> - add #defines for the AER and DPC Interrupt Message Number masks.
> v2->v3:
> - Use a 12-character SHA1 commit id.
> - Rebase on v4.14-rc4.
> v1->v2:
> - Fix comments on PATCH v1.
> - Simplify implementation.
>
> Dongdong Liu (2):
>   PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers
>   PCI/portdrv: Add #defines for the AER and DPC Interrupt Message Number
>     masks
>
>  drivers/pci/pcie/portdrv_core.c | 49 ++++++++++++++---------------------------
>  include/uapi/linux/pci_regs.h   |  2 ++
>  2 files changed, 18 insertions(+), 33 deletions(-)
>

Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers

[Bjorn Helgaas]

On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote:
> After removing and adding back the PCI root port device,
> we see the PCIe port service drivers request irq failed.
> pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
> aer: probe of 0000:00:00.0:pcie002 failed with error -22
> pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd-
> PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
> pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
> dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
> dpc: probe of 0000:00:00.0:pcie010 failed with error -22
> 
> The current code basically does this:
> 
>   - allocate 32 vectors
>   - figure out vector used by PME and hotplug
>   - figure out vector used by AER
>   - figure out vector used by DPC
>   - free the 32 vectors we allocated
>   - allocate only as many vectors as we need
> 
> but it is broken as calling pci_free_irq_vectors()
> invalidates the IRQ numbers returned before by pci_irq_vectors();
> 
> The hardware works:
>   - PME and hotplug use the Interrupt Message Number from the PCIe
>     Capability register.
> 
>   - AER uses the AER Interrupt Message Number from the AER Root Error
>     Status register.
> 
>   - DPC uses the DPC Interrupt Message Number from the DPC Capability
>     register.
> 
>   - FRS (not supported by Linux yet) uses the FRS Interrupt Message
>     Number from the FRS Queuing Capability register.
> 
>   - That's a total of 4 possible MSI/MSI-X vectors used for PME,
>     hotplug, AER, DPC, and FRS, so there's no point in trying to
>     allocate more than 4 vectors (we currently start with 32).
> 
>   - All these Interrupt Message Numbers are read-only to software but
>     are updated by hardware when software writes the Multiple Message
>     Enable field of the MSI Message Control register.  Thanks for
>     pointing this out; I didn't realize this before.
> 
>   - Therefore, we must read the Interrupt Message Numbers *after* we
>     write Multiple Message Enable.
> 
> Since we can use at most 4 vectors, this patch is to optimize the
> interrupt vector allocation strategy in pcie_port_enable_irq_vec().
> First figure out how many vectors we need,allocate them, and then fill
> in the slots in the irqs[] table.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()")
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>

Thanks a lot for all your work on this.  This was a confusing issue
with code to match.  I tried to simplify the changelog as below;
please correct anything I messed up.

A few hopefully last questions:

  - Your subject suggested that this was a bug for both MSI and MSI-X,
    but I think it only affected MSI, because the spec says MSI-X
    Interrupt Message Numbers must remain constant.  What do you
    think?

  - I think the problem was probably exposed by a1d5f18cafe6
    ("PCI/portdrv: Support multiple interrupts for MSI as well as
    MSI-X"), not 3674cc49da9a, because prior to a1d5f18cafe6, we only
    used a single MSI vector, and we didn't do the "allocate vectors,
    see which ones were used, reallocate only the required number of
    vectors" dance.
    
    It looks like we only allocated a single vector, so all the
    Interrupt Message Numbers should have been zero.  Or did you
    actually bisect it to 3674cc49da9a?

Bjorn


commit e402a536cf7122cda6b3bef420de36c22d2907e6
Author: Dongdong Liu <liudongdong3@huawei.com>
Date:   Wed Oct 11 18:52:57 2017 +0800

    PCI/portdrv: Fix MSI multiple interrupt setup
    
    When removing and adding back a PCIe Root Port device that uses MSI (not
    MSI-X), the port service driver IRQ request fails:
    
      pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
      aer: probe of 0000:00:00.0:pcie002 failed with error -22
      pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
      pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
      pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
      dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
      dpc: probe of 0000:00:00.0:pcie010 failed with error -22
    
    The previous code basically did this:
    
      - Write MSI Multiple Message Enable to enable 32 vectors
      - Read Interrupt Message Numbers for PME & hotplug, AER, and DPC
      - Rewrite MSI Multiple Message Enable to enable only as many vectors as
        we think we need
      - Assume the Interrupt Message Numbers are still valid
    
    The problem is that when we change the Multiple Message Enable field to
    what we think is the minimal number of vectors required, the hardware may
    change which vectors it uses.  See the "Interrupt Message Number"
    discussion in PCIe r3.1, sec 7.8.2 (PME & hotplug), 7.10.10 (AER), 7.31.2
    (DPC).
    
    Read the Interrupt Message Numbers *after* we write the Multiple Message
    Enable field so we use the correct IRQ vectors.
    
    Note that we can't predict how hardware will select Interrupt Message
    Numbers, so even if we allocate as many vectors as there are interrupt
    sources, the hardware may still select Message Numbers that result in
    sharing a vector.  E.g., on HiSilicon hip08, if we have 3 sources
    (PME/hotplug, AER, DPC) and 4 MSI vectors, hardware selects Message
    Numbers 0 for PME/hotplug and 2 for AER and DPC.  That leaves vectors 1
    and 3 unused, while AER and DPC share vector 2.
    
    Even though MSI-X doesn't have the issue with Interrupt Message Numbers
    changing, this patch may result in more IRQ sharing because we no longer
    read those Message Numbers and use them to determine how many MSI-X vectors
    to request.
    
    Fixes: a1d5f18cafe6 ("PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X")
    Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
    Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
    Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
    [bhelgaas: changelog]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Cc: stable@vger.kernel.org      # v4.13+

Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers

[Dongdong Liu]

Hi Bjorn
在 2017/10/18 4:25, Bjorn Helgaas 写道:
> On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote:
>> After removing and adding back the PCI root port device,
>> we see the PCIe port service drivers request irq failed.
>> pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
>> aer: probe of 0000:00:00.0:pcie002 failed with error -22
>> pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd-
>> PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
>> pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
>> pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
>> dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
>> dpc: probe of 0000:00:00.0:pcie010 failed with error -22
>>
>> The current code basically does this:
>>
>>   - allocate 32 vectors
>>   - figure out vector used by PME and hotplug
>>   - figure out vector used by AER
>>   - figure out vector used by DPC
>>   - free the 32 vectors we allocated
>>   - allocate only as many vectors as we need
>>
>> but it is broken as calling pci_free_irq_vectors()
>> invalidates the IRQ numbers returned before by pci_irq_vectors();
>>
>> The hardware works:
>>   - PME and hotplug use the Interrupt Message Number from the PCIe
>>     Capability register.
>>
>>   - AER uses the AER Interrupt Message Number from the AER Root Error
>>     Status register.
>>
>>   - DPC uses the DPC Interrupt Message Number from the DPC Capability
>>     register.
>>
>>   - FRS (not supported by Linux yet) uses the FRS Interrupt Message
>>     Number from the FRS Queuing Capability register.
>>
>>   - That's a total of 4 possible MSI/MSI-X vectors used for PME,
>>     hotplug, AER, DPC, and FRS, so there's no point in trying to
>>     allocate more than 4 vectors (we currently start with 32).
>>
>>   - All these Interrupt Message Numbers are read-only to software but
>>     are updated by hardware when software writes the Multiple Message
>>     Enable field of the MSI Message Control register.  Thanks for
>>     pointing this out; I didn't realize this before.
>>
>>   - Therefore, we must read the Interrupt Message Numbers *after* we
>>     write Multiple Message Enable.
>>
>> Since we can use at most 4 vectors, this patch is to optimize the
>> interrupt vector allocation strategy in pcie_port_enable_irq_vec().
>> First figure out how many vectors we need,allocate them, and then fill
>> in the slots in the irqs[] table.
>>
>> Cc: <stable@vger.kernel.org>
>> Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()")
>> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>
> Thanks a lot for all your work on this.  This was a confusing issue
> with code to match.  I tried to simplify the changelog as below;
> please correct anything I messed up.
>
> A few hopefully last questions:
>
>   - Your subject suggested that this was a bug for both MSI and MSI-X,
>     but I think it only affected MSI, because the spec says MSI-X
>     Interrupt Message Numbers must remain constant.  What do you
>     think?

Yes, for a given MSI-X implementation, the entry must remain constant.
But in fact, the bug PCIe port service drivers request irq failed does
not relate with this. As we said in PATCH V2,
The current code does:

   pci_alloc_irq_vectors(dev, ...)
   irqs[x] = pci_irq_vector(dev, entry)
   pci_free_irq_vectors(dev)
   pci_alloc_irq_vectors(dev, ...)

The problem is that the second pci_alloc_irq_vectors() call gets different
vectors than the first one. For MSI-X, It maybe also have such bug.

>
>   - I think the problem was probably exposed by a1d5f18cafe6
>     ("PCI/portdrv: Support multiple interrupts for MSI as well as
>     MSI-X"), not 3674cc49da9a, because prior to a1d5f18cafe6, we only
>     used a single MSI vector, and we didn't do the "allocate vectors,
>     see which ones were used, reallocate only the required number of
>     vectors" dance.

Yes, 3674cc49da9a, for MSI, it should be work ok as it just use used a
single MSI vector(not get the irq and free and alloc irq agian as MSI-X).
But for MSI-X, I think it maybe still have a bug.
>
>     It looks like we only allocated a single vector, so all the
>     Interrupt Message Numbers should have been zero.  Or did you
>     actually bisect it to 3674cc49da9a?

I think 3674cc49da9a is beeter as a1d5f18cafe6 maybe still have the bug for MSI-X.
a1d5f18cafe6 PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X
3674cc49da9a PCI/portdrv: Use pci_irq_alloc_vectors().

>
> Bjorn
>
>
> commit e402a536cf7122cda6b3bef420de36c22d2907e6
> Author: Dongdong Liu <liudongdong3@huawei.com>
> Date:   Wed Oct 11 18:52:57 2017 +0800
>
>     PCI/portdrv: Fix MSI multiple interrupt setup
>
>     When removing and adding back a PCIe Root Port device that uses MSI (not
>     MSI-X), the port service driver IRQ request fails:
>
>       pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
>       aer: probe of 0000:00:00.0:pcie002 failed with error -22
>       pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
>       pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
>       pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
>       dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
>       dpc: probe of 0000:00:00.0:pcie010 failed with error -22
>
>     The previous code basically did this:
>
>       - Write MSI Multiple Message Enable to enable 32 vectors
>       - Read Interrupt Message Numbers for PME & hotplug, AER, and DPC
>       - Rewrite MSI Multiple Message Enable to enable only as many vectors as
>         we think we need
>       - Assume the Interrupt Message Numbers are still valid
>
>     The problem is that when we change the Multiple Message Enable field to
>     what we think is the minimal number of vectors required, the hardware may
>     change which vectors it uses.  See the "Interrupt Message Number"
>     discussion in PCIe r3.1, sec 7.8.2 (PME & hotplug), 7.10.10 (AER), 7.31.2
>     (DPC).
>
>     Read the Interrupt Message Numbers *after* we write the Multiple Message
>     Enable field so we use the correct IRQ vectors.
>
>     Note that we can't predict how hardware will select Interrupt Message
>     Numbers, so even if we allocate as many vectors as there are interrupt
>     sources, the hardware may still select Message Numbers that result in
>     sharing a vector.  E.g., on HiSilicon hip08, if we have 3 sources
>     (PME/hotplug, AER, DPC) and 4 MSI vectors, hardware selects Message
>     Numbers 0 for PME/hotplug and 2 for AER and DPC.  That leaves vectors 1
>     and 3 unused, while AER and DPC share vector 2.
>
>     Even though MSI-X doesn't have the issue with Interrupt Message Numbers
>     changing, this patch may result in more IRQ sharing because we no longer
>     read those Message Numbers and use them to determine how many MSI-X vectors
>     to request.
>
>     Fixes: a1d5f18cafe6 ("PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X")
>     Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
>     Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>     Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>     [bhelgaas: changelog]
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Cc: stable@vger.kernel.org      # v4.13+
>

The commit message looks good to me.
I think Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") is better as
it still has a bug for MSI-X.

Thanks,
Dongdong
> .
>

Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers

[Bjorn Helgaas]

On Wed, Oct 18, 2017 at 06:50:05PM +0800, Dongdong Liu wrote:
> Hi Bjorn
> 在 2017/10/18 4:25, Bjorn Helgaas 写道:
> >On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote:
> >>After removing and adding back the PCI root port device,
> >>we see the PCIe port service drivers request irq failed.
> >>pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
> >>aer: probe of 0000:00:00.0:pcie002 failed with error -22
> >>pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd-
> >>PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> >>pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
> >>pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
> >>dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
> >>dpc: probe of 0000:00:00.0:pcie010 failed with error -22
> >>
> >>The current code basically does this:
> >>
> >>  - allocate 32 vectors
> >>  - figure out vector used by PME and hotplug
> >>  - figure out vector used by AER
> >>  - figure out vector used by DPC
> >>  - free the 32 vectors we allocated
> >>  - allocate only as many vectors as we need
> >>
> >>but it is broken as calling pci_free_irq_vectors()
> >>invalidates the IRQ numbers returned before by pci_irq_vectors();
> >>
> >>The hardware works:
> >>  - PME and hotplug use the Interrupt Message Number from the PCIe
> >>    Capability register.
> >>
> >>  - AER uses the AER Interrupt Message Number from the AER Root Error
> >>    Status register.
> >>
> >>  - DPC uses the DPC Interrupt Message Number from the DPC Capability
> >>    register.
> >>
> >>  - FRS (not supported by Linux yet) uses the FRS Interrupt Message
> >>    Number from the FRS Queuing Capability register.
> >>
> >>  - That's a total of 4 possible MSI/MSI-X vectors used for PME,
> >>    hotplug, AER, DPC, and FRS, so there's no point in trying to
> >>    allocate more than 4 vectors (we currently start with 32).
> >>
> >>  - All these Interrupt Message Numbers are read-only to software but
> >>    are updated by hardware when software writes the Multiple Message
> >>    Enable field of the MSI Message Control register.  Thanks for
> >>    pointing this out; I didn't realize this before.
> >>
> >>  - Therefore, we must read the Interrupt Message Numbers *after* we
> >>    write Multiple Message Enable.
> >>
> >>Since we can use at most 4 vectors, this patch is to optimize the
> >>interrupt vector allocation strategy in pcie_port_enable_irq_vec().
> >>First figure out how many vectors we need,allocate them, and then fill
> >>in the slots in the irqs[] table.
> >>
> >>Cc: <stable@vger.kernel.org>
> >>Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()")
> >>Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> >>Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> >>Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> >
> >Thanks a lot for all your work on this.  This was a confusing issue
> >with code to match.  I tried to simplify the changelog as below;
> >please correct anything I messed up.
> >
> >A few hopefully last questions:
> >
> >  - Your subject suggested that this was a bug for both MSI and MSI-X,
> >    but I think it only affected MSI, because the spec says MSI-X
> >    Interrupt Message Numbers must remain constant.  What do you
> >    think?
> 
> Yes, for a given MSI-X implementation, the entry must remain constant.
> But in fact, the bug PCIe port service drivers request irq failed does
> not relate with this. As we said in PATCH V2,
> The current code does:
> 
>   pci_alloc_irq_vectors(dev, ...)
>   irqs[x] = pci_irq_vector(dev, entry)
>   pci_free_irq_vectors(dev)
>   pci_alloc_irq_vectors(dev, ...)
> 
> The problem is that the second pci_alloc_irq_vectors() call gets different
> vectors than the first one. For MSI-X, It maybe also have such bug.

OK, I'm not sure I understand yet how this works with MSI-X, but I
suspect the current patch still breaks MSI-X.  Let me see if I can
make sense of what happens when the root port supports MSI-X:

  - The MSI-X Table Size (in Message Control) is fixed; this field is
    read-only in hardware.

  - When MSI-X is enabled, the Interrupt Message Numbers for PME/HP,
    AER, DPC, etc., are fixed, contain an MSI-X Table index, and must
    be less than 32.

  - pci_alloc_irq_vectors(3) will allocate 3 MSI-X vectors (0-2).
    There's no reason to assume the Interrupt Message Numbers (which
    can be 0-31) will be in that range.

Here's the flow through this path for MSI-X with the current patch
applied, with all services enabled.  You can see that in
msix_setup_entries(), we'll allocate 3 entries and they will use
indexes 0-2 in the MSI-X table.  The Interrupt Message Numbers index
that table, and they can be in the range 0-31.

  pcie_init_service_irqs
    pcie_port_enable_irq_vec
      nvec = 3            # PME/HP, AER, DPC
      pci_alloc_irq_vectors(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI)
        pci_alloc_irq_vectors_affinity(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI)
          __pci_enable_msix_range(entries==NULL, 1, 3)
            __pci_enable_msix(entries==NULL, nvec=3)
              msix_capability_init(entries==NULL, nvec=3)
                base = msix_map_region(dev, msix_table_size(control))
                msix_setup_entries(base, entries==NULL, nvec=3)
                  for (i = 0; i < nvec; i++)
                    entry = alloc_msi_entry()
                    entry->msi_attrib.entry_nr = i  # <-- MSI-X table index
                    entry->mask_base = base         # MSI-X table base
                msix_program_entries
                  for_each_pci_msi_entry()
                    writel(PCI_MSIX_ENTRY_CTRL_MASKBIT,
                           pci_msix_desc_addr(entry) + 
                           PCI_MSIX_ENTRY_VECTOR_CTRL)

        nr_entries = 3    # returned from pci_alloc_irq_vectors()
        # read message numbers:
        entry = <PME/HP, AER, or DPC message number>
        # assume entry == 4
        if (entry > nr_entries)
          goto out_free_irqs   # we should take this error path

I would expect pcie_port_enable_irq_vec() to fail on many root ports
with MSI-X, because one of PME/HP, AER, DPC may use an Interrupt
Message Number greater than 3.

The previous code (before this patch) first allocates 32 MSI-X
vectors, then reads all the Interrupt Message Numbers, then allocates
just enough MSI-X vectors so all the message numbers are valid table
indexes.

In the current patch, we don't know the message numbers until after
we've already enabled MSI-X with some number of vectors.

So for MSI, we have the constraint that we have to enable MSI before
reading the Message Numbers.  For MSI-X, we can't know how many
vectors to request until after we read the Message Numbers.  I wonder
if we need to split these two paths?

> >  - I think the problem was probably exposed by a1d5f18cafe6
> >    ("PCI/portdrv: Support multiple interrupts for MSI as well as
> >    MSI-X"), not 3674cc49da9a, because prior to a1d5f18cafe6, we only
> >    used a single MSI vector, and we didn't do the "allocate vectors,
> >    see which ones were used, reallocate only the required number of
> >    vectors" dance.
> 
> Yes, 3674cc49da9a, for MSI, it should be work ok as it just use used a
> single MSI vector(not get the irq and free and alloc irq agian as MSI-X).
> But for MSI-X, I think it maybe still have a bug.
> >
> >    It looks like we only allocated a single vector, so all the
> >    Interrupt Message Numbers should have been zero.  Or did you
> >    actually bisect it to 3674cc49da9a?
> 
> I think 3674cc49da9a is beeter as a1d5f18cafe6 maybe still have the bug for MSI-X.
> a1d5f18cafe6 PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X
> 3674cc49da9a PCI/portdrv: Use pci_irq_alloc_vectors().
> 
> >
> >Bjorn
> >
> >
> >commit e402a536cf7122cda6b3bef420de36c22d2907e6
> >Author: Dongdong Liu <liudongdong3@huawei.com>
> >Date:   Wed Oct 11 18:52:57 2017 +0800
> >
> >    PCI/portdrv: Fix MSI multiple interrupt setup
> >
> >    When removing and adding back a PCIe Root Port device that uses MSI (not
> >    MSI-X), the port service driver IRQ request fails:
> >
> >      pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
> >      aer: probe of 0000:00:00.0:pcie002 failed with error -22
> >      pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> >      pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
> >      pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
> >      dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
> >      dpc: probe of 0000:00:00.0:pcie010 failed with error -22
> >
> >    The previous code basically did this:
> >
> >      - Write MSI Multiple Message Enable to enable 32 vectors
> >      - Read Interrupt Message Numbers for PME & hotplug, AER, and DPC
> >      - Rewrite MSI Multiple Message Enable to enable only as many vectors as
> >        we think we need
> >      - Assume the Interrupt Message Numbers are still valid
> >
> >    The problem is that when we change the Multiple Message Enable field to
> >    what we think is the minimal number of vectors required, the hardware may
> >    change which vectors it uses.  See the "Interrupt Message Number"
> >    discussion in PCIe r3.1, sec 7.8.2 (PME & hotplug), 7.10.10 (AER), 7.31.2
> >    (DPC).
> >
> >    Read the Interrupt Message Numbers *after* we write the Multiple Message
> >    Enable field so we use the correct IRQ vectors.
> >
> >    Note that we can't predict how hardware will select Interrupt Message
> >    Numbers, so even if we allocate as many vectors as there are interrupt
> >    sources, the hardware may still select Message Numbers that result in
> >    sharing a vector.  E.g., on HiSilicon hip08, if we have 3 sources
> >    (PME/hotplug, AER, DPC) and 4 MSI vectors, hardware selects Message
> >    Numbers 0 for PME/hotplug and 2 for AER and DPC.  That leaves vectors 1
> >    and 3 unused, while AER and DPC share vector 2.
> >
> >    Even though MSI-X doesn't have the issue with Interrupt Message Numbers
> >    changing, this patch may result in more IRQ sharing because we no longer
> >    read those Message Numbers and use them to determine how many MSI-X vectors
> >    to request.
> >
> >    Fixes: a1d5f18cafe6 ("PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X")
> >    Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> >    Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> >    Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> >    [bhelgaas: changelog]
> >    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >    Cc: stable@vger.kernel.org      # v4.13+
> >
> 
> The commit message looks good to me.
> I think Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") is better as
> it still has a bug for MSI-X.
> 
> Thanks,
> Dongdong
> >.
> >
> 

Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers

[Dongdong Liu]

Hi Bjorn

在 2017/10/19 6:33, Bjorn Helgaas 写道:
> On Wed, Oct 18, 2017 at 06:50:05PM +0800, Dongdong Liu wrote:
>> Hi Bjorn
>> 在 2017/10/18 4:25, Bjorn Helgaas 写道:
>>> On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote:
>>>> After removing and adding back the PCI root port device,
>>>> we see the PCIe port service drivers request irq failed.
>>>> pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
>>>> aer: probe of 0000:00:00.0:pcie002 failed with error -22
>>>> pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd-
>>>> PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
>>>> pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
>>>> pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
>>>> dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
>>>> dpc: probe of 0000:00:00.0:pcie010 failed with error -22
>>>>
>>>> The current code basically does this:
>>>>
>>>>  - allocate 32 vectors
>>>>  - figure out vector used by PME and hotplug
>>>>  - figure out vector used by AER
>>>>  - figure out vector used by DPC
>>>>  - free the 32 vectors we allocated
>>>>  - allocate only as many vectors as we need
>>>>
>>>> but it is broken as calling pci_free_irq_vectors()
>>>> invalidates the IRQ numbers returned before by pci_irq_vectors();
>>>>
>>>> The hardware works:
>>>>  - PME and hotplug use the Interrupt Message Number from the PCIe
>>>>    Capability register.
>>>>
>>>>  - AER uses the AER Interrupt Message Number from the AER Root Error
>>>>    Status register.
>>>>
>>>>  - DPC uses the DPC Interrupt Message Number from the DPC Capability
>>>>    register.
>>>>
>>>>  - FRS (not supported by Linux yet) uses the FRS Interrupt Message
>>>>    Number from the FRS Queuing Capability register.
>>>>
>>>>  - That's a total of 4 possible MSI/MSI-X vectors used for PME,
>>>>    hotplug, AER, DPC, and FRS, so there's no point in trying to
>>>>    allocate more than 4 vectors (we currently start with 32).
>>>>
>>>>  - All these Interrupt Message Numbers are read-only to software but
>>>>    are updated by hardware when software writes the Multiple Message
>>>>    Enable field of the MSI Message Control register.  Thanks for
>>>>    pointing this out; I didn't realize this before.
>>>>
>>>>  - Therefore, we must read the Interrupt Message Numbers *after* we
>>>>    write Multiple Message Enable.
>>>>
>>>> Since we can use at most 4 vectors, this patch is to optimize the
>>>> interrupt vector allocation strategy in pcie_port_enable_irq_vec().
>>>> First figure out how many vectors we need,allocate them, and then fill
>>>> in the slots in the irqs[] table.
>>>>
>>>> Cc: <stable@vger.kernel.org>
>>>> Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()")
>>>> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
>>>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>>>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>>>
>>> Thanks a lot for all your work on this.  This was a confusing issue
>>> with code to match.  I tried to simplify the changelog as below;
>>> please correct anything I messed up.
>>>
>>> A few hopefully last questions:
>>>
>>>  - Your subject suggested that this was a bug for both MSI and MSI-X,
>>>    but I think it only affected MSI, because the spec says MSI-X
>>>    Interrupt Message Numbers must remain constant.  What do you
>>>    think?
>>
>> Yes, for a given MSI-X implementation, the entry must remain constant.
>> But in fact, the bug PCIe port service drivers request irq failed does
>> not relate with this. As we said in PATCH V2,
>> The current code does:
>>
>>   pci_alloc_irq_vectors(dev, ...)
>>   irqs[x] = pci_irq_vector(dev, entry)
>>   pci_free_irq_vectors(dev)
>>   pci_alloc_irq_vectors(dev, ...)
>>
>> The problem is that the second pci_alloc_irq_vectors() call gets different
>> vectors than the first one. For MSI-X, It maybe also have such bug.
>
> OK, I'm not sure I understand yet how this works with MSI-X, but I
> suspect the current patch still breaks MSI-X.  Let me see if I can
> make sense of what happens when the root port supports MSI-X:
>
>   - The MSI-X Table Size (in Message Control) is fixed; this field is
>     read-only in hardware.
>
>   - When MSI-X is enabled, the Interrupt Message Numbers for PME/HP,
>     AER, DPC, etc., are fixed, contain an MSI-X Table index, and must
>     be less than 32.
>
>   - pci_alloc_irq_vectors(3) will allocate 3 MSI-X vectors (0-2).
>     There's no reason to assume the Interrupt Message Numbers (which
>     can be 0-31) will be in that range.
>
> Here's the flow through this path for MSI-X with the current patch
> applied, with all services enabled.  You can see that in
> msix_setup_entries(), we'll allocate 3 entries and they will use
> indexes 0-2 in the MSI-X table.  The Interrupt Message Numbers index
> that table, and they can be in the range 0-31.
>
>   pcie_init_service_irqs
>     pcie_port_enable_irq_vec
>       nvec = 3            # PME/HP, AER, DPC
>       pci_alloc_irq_vectors(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI)
>         pci_alloc_irq_vectors_affinity(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI)
>           __pci_enable_msix_range(entries==NULL, 1, 3)
>             __pci_enable_msix(entries==NULL, nvec=3)
>               msix_capability_init(entries==NULL, nvec=3)
>                 base = msix_map_region(dev, msix_table_size(control))
>                 msix_setup_entries(base, entries==NULL, nvec=3)
>                   for (i = 0; i < nvec; i++)
>                     entry = alloc_msi_entry()
>                     entry->msi_attrib.entry_nr = i  # <-- MSI-X table index
>                     entry->mask_base = base         # MSI-X table base
>                 msix_program_entries
>                   for_each_pci_msi_entry()
>                     writel(PCI_MSIX_ENTRY_CTRL_MASKBIT,
>                            pci_msix_desc_addr(entry) +
>                            PCI_MSIX_ENTRY_VECTOR_CTRL)
>
>         nr_entries = 3    # returned from pci_alloc_irq_vectors()
>         # read message numbers:
>         entry = <PME/HP, AER, or DPC message number>
>         # assume entry == 4
>         if (entry > nr_entries)
>           goto out_free_irqs   # we should take this error path

Yes, the above analysis is correct.
>
> I would expect pcie_port_enable_irq_vec() to fail on many root ports
> with MSI-X, because one of PME/HP, AER, DPC may use an Interrupt
> Message Number greater than 3.
>
> The previous code (before this patch) first allocates 32 MSI-X
> vectors, then reads all the Interrupt Message Numbers, then allocates
> just enough MSI-X vectors so all the message numbers are valid table
> indexes.
>
> In the current patch, we don't know the message numbers until after
> we've already enabled MSI-X with some number of vectors.
>
> So for MSI, we have the constraint that we have to enable MSI before
> reading the Message Numbers.  For MSI-X, we can't know how many
> vectors to request until after we read the Message Numbers.  I wonder
> if we need to split these two paths?

If we worry about current patch will break MSI-X.
How about [PATCH V3] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers.
https://patchwork.kernel.org/patch/9992827/

I still think patch v3 will be ok for MSI/MSI-X.
----------------------------------------------
The patch v3 below contains this sequence:

   pci_alloc_irq_vectors(32)
     ...
      write Multiple Message Enable   <-- Interrupt Message Numbers change
   idx[x] = PME/hotplug Interrupt Message Number
   idx[y] = AER Interrupt Message Number
   idx[z] = DPC Interrupt Message Number
   pci_free_irq_vectors()
   pci_alloc_irq_vectors()
     ...
      write Multiple Message Enable   <-- Interrupt Message Numbers change
   for (i = 0; ...)
     irqs[i] = pci_irq_vector(idx[i])

This incorrectly assumes the Interrupt Message Numbers remain the same
after we write Multiple Message Enable.
--------------------------------------------
We just worry about Interrupt Message Numbers maybe are not the same.
pci_alloc_irq_vectors(32)
  write Multiple Message Enable=5(32 vectors)   <-- Interrupt Message Numbers change
  idx[x] = PME/hotplug Interrupt Message Number, assume 0
  idx[y] = AER Interrupt Message Number,         assume 2
  idx[z] = DPC Interrupt Message Number,         assume 6
  pci_free_irq_vectors()
  pci_alloc_irq_vectors(7)
     ...
      write Multiple Message Enable=3(8 vectors)
   for (i = 0; ...)
     irqs[i] = pci_irq_vector(idx[i])

As Multiple Message Enable is 8, Hardware must remain the same as before
(HiSilicon hip08 design as this).Test on HiSilicon hip08 platform, it works ok.

Hardware is required to update this field so that it is correct if the number of
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
MSI Messages assigned to the Function changes when software writes to the Multiple
Message Enable field in the MSI Message Control register

The PCIe spec is not very clear about this make us confused. We worry about the
hardwares are not consistent.

Split these two paths means msi/msi-x uses different algorithm.
Currently, it maybe a good choice.

Thanks,
Dongdong
>
>>>  - I think the problem was probably exposed by a1d5f18cafe6
>>>    ("PCI/portdrv: Support multiple interrupts for MSI as well as
>>>    MSI-X"), not 3674cc49da9a, because prior to a1d5f18cafe6, we only
>>>    used a single MSI vector, and we didn't do the "allocate vectors,
>>>    see which ones were used, reallocate only the required number of
>>>    vectors" dance.
>>
>> Yes, 3674cc49da9a, for MSI, it should be work ok as it just use used a
>> single MSI vector(not get the irq and free and alloc irq agian as MSI-X).
>> But for MSI-X, I think it maybe still have a bug.
>>>
>>>    It looks like we only allocated a single vector, so all the
>>>    Interrupt Message Numbers should have been zero.  Or did you
>>>    actually bisect it to 3674cc49da9a?
>>
>> I think 3674cc49da9a is beeter as a1d5f18cafe6 maybe still have the bug for MSI-X.
>> a1d5f18cafe6 PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X
>> 3674cc49da9a PCI/portdrv: Use pci_irq_alloc_vectors().
>>
>>>
>>> Bjorn
>>>
>>>
>>> commit e402a536cf7122cda6b3bef420de36c22d2907e6
>>> Author: Dongdong Liu <liudongdong3@huawei.com>
>>> Date:   Wed Oct 11 18:52:57 2017 +0800
>>>
>>>    PCI/portdrv: Fix MSI multiple interrupt setup
>>>
>>>    When removing and adding back a PCIe Root Port device that uses MSI (not
>>>    MSI-X), the port service driver IRQ request fails:
>>>
>>>      pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
>>>      aer: probe of 0000:00:00.0:pcie002 failed with error -22
>>>      pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
>>>      pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
>>>      pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
>>>      dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
>>>      dpc: probe of 0000:00:00.0:pcie010 failed with error -22
>>>
>>>    The previous code basically did this:
>>>
>>>      - Write MSI Multiple Message Enable to enable 32 vectors
>>>      - Read Interrupt Message Numbers for PME & hotplug, AER, and DPC
>>>      - Rewrite MSI Multiple Message Enable to enable only as many vectors as
>>>        we think we need
>>>      - Assume the Interrupt Message Numbers are still valid
>>>
>>>    The problem is that when we change the Multiple Message Enable field to
>>>    what we think is the minimal number of vectors required, the hardware may
>>>    change which vectors it uses.  See the "Interrupt Message Number"
>>>    discussion in PCIe r3.1, sec 7.8.2 (PME & hotplug), 7.10.10 (AER), 7.31.2
>>>    (DPC).
>>>
>>>    Read the Interrupt Message Numbers *after* we write the Multiple Message
>>>    Enable field so we use the correct IRQ vectors.
>>>
>>>    Note that we can't predict how hardware will select Interrupt Message
>>>    Numbers, so even if we allocate as many vectors as there are interrupt
>>>    sources, the hardware may still select Message Numbers that result in
>>>    sharing a vector.  E.g., on HiSilicon hip08, if we have 3 sources
>>>    (PME/hotplug, AER, DPC) and 4 MSI vectors, hardware selects Message
>>>    Numbers 0 for PME/hotplug and 2 for AER and DPC.  That leaves vectors 1
>>>    and 3 unused, while AER and DPC share vector 2.
>>>
>>>    Even though MSI-X doesn't have the issue with Interrupt Message Numbers
>>>    changing, this patch may result in more IRQ sharing because we no longer
>>>    read those Message Numbers and use them to determine how many MSI-X vectors
>>>    to request.
>>>
>>>    Fixes: a1d5f18cafe6 ("PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X")
>>>    Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
>>>    Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>>>    Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>>>    [bhelgaas: changelog]
>>>    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>    Cc: stable@vger.kernel.org      # v4.13+
>>>
>>
>> The commit message looks good to me.
>> I think Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") is better as
>> it still has a bug for MSI-X.
>>
>> Thanks,
>> Dongdong
>>> .
>>>
>>
>
> .
>

Re: [PATCH V4 1/2] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers

[Bjorn Helgaas]

On Thu, Oct 19, 2017 at 10:50:57AM +0800, Dongdong Liu wrote:
> Hi Bjorn
> 
> 在 2017/10/19 6:33, Bjorn Helgaas 写道:
> >On Wed, Oct 18, 2017 at 06:50:05PM +0800, Dongdong Liu wrote:
> >>Hi Bjorn
> >>在 2017/10/18 4:25, Bjorn Helgaas 写道:
> >>>On Wed, Oct 11, 2017 at 06:52:57PM +0800, Dongdong Liu wrote:
> >>>>After removing and adding back the PCI root port device,
> >>>>we see the PCIe port service drivers request irq failed.
> >>>>pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
> >>>>aer: probe of 0000:00:00.0:pcie002 failed with error -22
> >>>>pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd-
> >>>>PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> >>>>pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
> >>>>pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
> >>>>dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
> >>>>dpc: probe of 0000:00:00.0:pcie010 failed with error -22
> >>>>
> >>>>The current code basically does this:
> >>>>
> >>>> - allocate 32 vectors
> >>>> - figure out vector used by PME and hotplug
> >>>> - figure out vector used by AER
> >>>> - figure out vector used by DPC
> >>>> - free the 32 vectors we allocated
> >>>> - allocate only as many vectors as we need
> >>>>
> >>>>but it is broken as calling pci_free_irq_vectors()
> >>>>invalidates the IRQ numbers returned before by pci_irq_vectors();
> >>>>
> >>>>The hardware works:
> >>>> - PME and hotplug use the Interrupt Message Number from the PCIe
> >>>>   Capability register.
> >>>>
> >>>> - AER uses the AER Interrupt Message Number from the AER Root Error
> >>>>   Status register.
> >>>>
> >>>> - DPC uses the DPC Interrupt Message Number from the DPC Capability
> >>>>   register.
> >>>>
> >>>> - FRS (not supported by Linux yet) uses the FRS Interrupt Message
> >>>>   Number from the FRS Queuing Capability register.
> >>>>
> >>>> - That's a total of 4 possible MSI/MSI-X vectors used for PME,
> >>>>   hotplug, AER, DPC, and FRS, so there's no point in trying to
> >>>>   allocate more than 4 vectors (we currently start with 32).
> >>>>
> >>>> - All these Interrupt Message Numbers are read-only to software but
> >>>>   are updated by hardware when software writes the Multiple Message
> >>>>   Enable field of the MSI Message Control register.  Thanks for
> >>>>   pointing this out; I didn't realize this before.
> >>>>
> >>>> - Therefore, we must read the Interrupt Message Numbers *after* we
> >>>>   write Multiple Message Enable.
> >>>>
> >>>>Since we can use at most 4 vectors, this patch is to optimize the
> >>>>interrupt vector allocation strategy in pcie_port_enable_irq_vec().
> >>>>First figure out how many vectors we need,allocate them, and then fill
> >>>>in the slots in the irqs[] table.
> >>>>
> >>>>Cc: <stable@vger.kernel.org>
> >>>>Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()")
> >>>>Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> >>>>Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> >>>>Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> >>>
> >>>Thanks a lot for all your work on this.  This was a confusing issue
> >>>with code to match.  I tried to simplify the changelog as below;
> >>>please correct anything I messed up.
> >>>
> >>>A few hopefully last questions:
> >>>
> >>> - Your subject suggested that this was a bug for both MSI and MSI-X,
> >>>   but I think it only affected MSI, because the spec says MSI-X
> >>>   Interrupt Message Numbers must remain constant.  What do you
> >>>   think?
> >>
> >>Yes, for a given MSI-X implementation, the entry must remain constant.
> >>But in fact, the bug PCIe port service drivers request irq failed does
> >>not relate with this. As we said in PATCH V2,
> >>The current code does:
> >>
> >>  pci_alloc_irq_vectors(dev, ...)
> >>  irqs[x] = pci_irq_vector(dev, entry)
> >>  pci_free_irq_vectors(dev)
> >>  pci_alloc_irq_vectors(dev, ...)
> >>
> >>The problem is that the second pci_alloc_irq_vectors() call gets different
> >>vectors than the first one. For MSI-X, It maybe also have such bug.
> >
> >OK, I'm not sure I understand yet how this works with MSI-X, but I
> >suspect the current patch still breaks MSI-X.  Let me see if I can
> >make sense of what happens when the root port supports MSI-X:
> >
> >  - The MSI-X Table Size (in Message Control) is fixed; this field is
> >    read-only in hardware.
> >
> >  - When MSI-X is enabled, the Interrupt Message Numbers for PME/HP,
> >    AER, DPC, etc., are fixed, contain an MSI-X Table index, and must
> >    be less than 32.
> >
> >  - pci_alloc_irq_vectors(3) will allocate 3 MSI-X vectors (0-2).
> >    There's no reason to assume the Interrupt Message Numbers (which
> >    can be 0-31) will be in that range.
> >
> >Here's the flow through this path for MSI-X with the current patch
> >applied, with all services enabled.  You can see that in
> >msix_setup_entries(), we'll allocate 3 entries and they will use
> >indexes 0-2 in the MSI-X table.  The Interrupt Message Numbers index
> >that table, and they can be in the range 0-31.
> >
> >  pcie_init_service_irqs
> >    pcie_port_enable_irq_vec
> >      nvec = 3            # PME/HP, AER, DPC
> >      pci_alloc_irq_vectors(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI)
> >        pci_alloc_irq_vectors_affinity(..., 1, 3, PCI_IRQ_MSIX | PCI_IRQ_MSI)
> >          __pci_enable_msix_range(entries==NULL, 1, 3)
> >            __pci_enable_msix(entries==NULL, nvec=3)
> >              msix_capability_init(entries==NULL, nvec=3)
> >                base = msix_map_region(dev, msix_table_size(control))
> >                msix_setup_entries(base, entries==NULL, nvec=3)
> >                  for (i = 0; i < nvec; i++)
> >                    entry = alloc_msi_entry()
> >                    entry->msi_attrib.entry_nr = i  # <-- MSI-X table index
> >                    entry->mask_base = base         # MSI-X table base
> >                msix_program_entries
> >                  for_each_pci_msi_entry()
> >                    writel(PCI_MSIX_ENTRY_CTRL_MASKBIT,
> >                           pci_msix_desc_addr(entry) +
> >                           PCI_MSIX_ENTRY_VECTOR_CTRL)
> >
> >        nr_entries = 3    # returned from pci_alloc_irq_vectors()
> >        # read message numbers:
> >        entry = <PME/HP, AER, or DPC message number>
> >        # assume entry == 4
> >        if (entry > nr_entries)
> >          goto out_free_irqs   # we should take this error path
> 
> Yes, the above analysis is correct.
> >
> >I would expect pcie_port_enable_irq_vec() to fail on many root ports
> >with MSI-X, because one of PME/HP, AER, DPC may use an Interrupt
> >Message Number greater than 3.
> >
> >The previous code (before this patch) first allocates 32 MSI-X
> >vectors, then reads all the Interrupt Message Numbers, then allocates
> >just enough MSI-X vectors so all the message numbers are valid table
> >indexes.
> >
> >In the current patch, we don't know the message numbers until after
> >we've already enabled MSI-X with some number of vectors.
> >
> >So for MSI, we have the constraint that we have to enable MSI before
> >reading the Message Numbers.  For MSI-X, we can't know how many
> >vectors to request until after we read the Message Numbers.  I wonder
> >if we need to split these two paths?
> 
> If we worry about current patch will break MSI-X.
> How about [PATCH V3] PCI/portdrv: Fix MSI/MSI-X bug for PCIe port service drivers.
> https://patchwork.kernel.org/patch/9992827/
> 
> I still think patch v3 will be ok for MSI/MSI-X.
> ----------------------------------------------
> The patch v3 below contains this sequence:
> 
>   pci_alloc_irq_vectors(32)
>     ...
>      write Multiple Message Enable   <-- Interrupt Message Numbers change
>   idx[x] = PME/hotplug Interrupt Message Number
>   idx[y] = AER Interrupt Message Number
>   idx[z] = DPC Interrupt Message Number
>   pci_free_irq_vectors()
>   pci_alloc_irq_vectors()
>     ...
>      write Multiple Message Enable   <-- Interrupt Message Numbers change
>   for (i = 0; ...)
>     irqs[i] = pci_irq_vector(idx[i])
> 
> This incorrectly assumes the Interrupt Message Numbers remain the same
> after we write Multiple Message Enable.
> --------------------------------------------
> We just worry about Interrupt Message Numbers maybe are not the same.
> pci_alloc_irq_vectors(32)
>  write Multiple Message Enable=5(32 vectors)   <-- Interrupt Message Numbers change
>  idx[x] = PME/hotplug Interrupt Message Number, assume 0
>  idx[y] = AER Interrupt Message Number,         assume 2
>  idx[z] = DPC Interrupt Message Number,         assume 6
>  pci_free_irq_vectors()
>  pci_alloc_irq_vectors(7)
>     ...
>      write Multiple Message Enable=3(8 vectors)
>   for (i = 0; ...)
>     irqs[i] = pci_irq_vector(idx[i])
> 
> As Multiple Message Enable is 8, Hardware must remain the same as before
> (HiSilicon hip08 design as this).Test on HiSilicon hip08 platform, it works ok.

This isn't strictly required by the spec, since we do change Multiple
Message Enable, and hardware is allowed to change the message numbers when
we do that.  But I agree it is quite unlikely that the message numbers will
change since we're allocating enough vectors to accommodate all of them.

I'll post a v5 series that is basically a refinement of your v3.

> Hardware is required to update this field so that it is correct if the number of
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> MSI Messages assigned to the Function changes when software writes to the Multiple
> Message Enable field in the MSI Message Control register
> 
> The PCIe spec is not very clear about this make us confused. We worry about the
> hardwares are not consistent.
> 
> Split these two paths means msi/msi-x uses different algorithm.
> Currently, it maybe a good choice.
> 
> Thanks,
> Dongdong
> >
> >>> - I think the problem was probably exposed by a1d5f18cafe6
> >>>   ("PCI/portdrv: Support multiple interrupts for MSI as well as
> >>>   MSI-X"), not 3674cc49da9a, because prior to a1d5f18cafe6, we only
> >>>   used a single MSI vector, and we didn't do the "allocate vectors,
> >>>   see which ones were used, reallocate only the required number of
> >>>   vectors" dance.
> >>
> >>Yes, 3674cc49da9a, for MSI, it should be work ok as it just use used a
> >>single MSI vector(not get the irq and free and alloc irq agian as MSI-X).
> >>But for MSI-X, I think it maybe still have a bug.
> >>>
> >>>   It looks like we only allocated a single vector, so all the
> >>>   Interrupt Message Numbers should have been zero.  Or did you
> >>>   actually bisect it to 3674cc49da9a?
> >>
> >>I think 3674cc49da9a is beeter as a1d5f18cafe6 maybe still have the bug for MSI-X.
> >>a1d5f18cafe6 PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X
> >>3674cc49da9a PCI/portdrv: Use pci_irq_alloc_vectors().
> >>
> >>>
> >>>Bjorn
> >>>
> >>>
> >>>commit e402a536cf7122cda6b3bef420de36c22d2907e6
> >>>Author: Dongdong Liu <liudongdong3@huawei.com>
> >>>Date:   Wed Oct 11 18:52:57 2017 +0800
> >>>
> >>>   PCI/portdrv: Fix MSI multiple interrupt setup
> >>>
> >>>   When removing and adding back a PCIe Root Port device that uses MSI (not
> >>>   MSI-X), the port service driver IRQ request fails:
> >>>
> >>>     pcie_pme: probe of 0000:00:00.0:pcie001 failed with error -22
> >>>     aer: probe of 0000:00:00.0:pcie002 failed with error -22
> >>>     pciehp 0000:00:00.0:pcie004: Slot #0 AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+ Interlock- NoCompl- LLActRep+
> >>>     pciehp 0000:00:00.0:pcie004: Cannot get irq 20 for the hotplug controller
> >>>     pciehp 0000:00:00.0:pcie004: Notification initialization failed (-1)
> >>>     dpc 0000:00:00.0:pcie010: request IRQ22 failed: -22
> >>>     dpc: probe of 0000:00:00.0:pcie010 failed with error -22
> >>>
> >>>   The previous code basically did this:
> >>>
> >>>     - Write MSI Multiple Message Enable to enable 32 vectors
> >>>     - Read Interrupt Message Numbers for PME & hotplug, AER, and DPC
> >>>     - Rewrite MSI Multiple Message Enable to enable only as many vectors as
> >>>       we think we need
> >>>     - Assume the Interrupt Message Numbers are still valid
> >>>
> >>>   The problem is that when we change the Multiple Message Enable field to
> >>>   what we think is the minimal number of vectors required, the hardware may
> >>>   change which vectors it uses.  See the "Interrupt Message Number"
> >>>   discussion in PCIe r3.1, sec 7.8.2 (PME & hotplug), 7.10.10 (AER), 7.31.2
> >>>   (DPC).
> >>>
> >>>   Read the Interrupt Message Numbers *after* we write the Multiple Message
> >>>   Enable field so we use the correct IRQ vectors.
> >>>
> >>>   Note that we can't predict how hardware will select Interrupt Message
> >>>   Numbers, so even if we allocate as many vectors as there are interrupt
> >>>   sources, the hardware may still select Message Numbers that result in
> >>>   sharing a vector.  E.g., on HiSilicon hip08, if we have 3 sources
> >>>   (PME/hotplug, AER, DPC) and 4 MSI vectors, hardware selects Message
> >>>   Numbers 0 for PME/hotplug and 2 for AER and DPC.  That leaves vectors 1
> >>>   and 3 unused, while AER and DPC share vector 2.
> >>>
> >>>   Even though MSI-X doesn't have the issue with Interrupt Message Numbers
> >>>   changing, this patch may result in more IRQ sharing because we no longer
> >>>   read those Message Numbers and use them to determine how many MSI-X vectors
> >>>   to request.
> >>>
> >>>   Fixes: a1d5f18cafe6 ("PCI/portdrv: Support multiple interrupts for MSI as well as MSI-X")
> >>>   Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> >>>   Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> >>>   Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> >>>   [bhelgaas: changelog]
> >>>   Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >>>   Cc: stable@vger.kernel.org      # v4.13+
> >>>
> >>
> >>The commit message looks good to me.
> >>I think Fixes: 3674cc49da9a ("PCI/portdrv: Use pci_irq_alloc_vectors()") is better as
> >>it still has a bug for MSI-X.
> >>
> >>Thanks,
> >>Dongdong
> >>>.
> >>>
> >>
> >
> >.
> >
>