[PATCH 0/2] PCI/portdrv: add MSI support for PCIe port services and DPC IRQ support

[PATCH 0/2] PCI/portdrv: add MSI support for PCIe port services and DPC IRQ support

[Gabriele Paoloni]

From: gabriele paoloni <gabriele.paoloni@huawei.com>

This patchset:
1) adds support for MSI interrupt vectors to be used for Roor Port services
2) adds support for DPC Root Port service interrupt 

The patchset has been tested on Hisilicon Hip08 Chipset

Gabriele Paoloni (1):
  PCI/portdrv: add support for different MSI interrupts for PCIe port
    services

gabriele paoloni (1):
  PCI/portdrv: allocate MSI/MSIx vector for DPC RP service

 drivers/pci/pcie/portdrv.h      |  5 ++++
 drivers/pci/pcie/portdrv_core.c | 62 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 61 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH 1/2] PCI/portdrv: add support for different MSI interrupts for PCIe port services

[Gabriele Paoloni]

Currently PCIe port services are assigned with different interrutps
only if MSI-x are supported by calling pcie_port_enable_msix().
If a root port supports MSI instead of MSI-x currently we fall back
to use a single shared interrupt for all the services.
This patch renames and extends pcie_port_enable_msix() to use MSI in
case MSI-x allocation fails.

Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 drivers/pci/pcie/portdrv.h      |  5 +++++
 drivers/pci/pcie/portdrv_core.c | 33 +++++++++++++++++++++++++++------
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index 587aef3..729ee90 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -18,6 +18,11 @@
  */
 #define PCIE_PORT_MAX_MSIX_ENTRIES	32
 
+/* According to the PCI Local Bus Specification REV. 3.0 the max number
+ * of MSI vectors per function is 32
+ */
+#define PCIE_PORT_MAX_MSI_ENTRIES	32
+
 #define get_descriptor_id(type, service) (((type - 4) << 8) | service)
 
 extern struct bus_type pcie_port_bus_type;
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index cea504f..e2c7bfd 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -44,14 +44,16 @@ static void release_pcie_device(struct device *dev)
 }
 
 /**
- * pcie_port_enable_msix - try to set up MSI-X as interrupt mode for given port
+ * pcie_port_enable_msix_or_msi - try to set up MSI-X or MSI as interrupt mode
+ * for given port
  * @dev: PCI Express port to handle
  * @irqs: Array of interrupt vectors to populate
  * @mask: Bitmask of port capabilities returned by get_port_device_capability()
  *
  * Return value: 0 on success, error code on failure
  */
-static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask)
+static
+int pcie_port_enable_msix_or_msi(struct pci_dev *dev, int *irqs, int mask)
 {
 	int nr_entries, entry, nvec = 0;
 
@@ -63,6 +65,10 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask)
 	 */
 	nr_entries = pci_alloc_irq_vectors(dev, 1, PCIE_PORT_MAX_MSIX_ENTRIES,
 			PCI_IRQ_MSIX);
+	if (nr_entries < 0) /* MSI-x failed let's try with MSI */
+		nr_entries = pci_alloc_irq_vectors(dev, 1,
+				PCIE_PORT_MAX_MSI_ENTRIES,
+				PCI_IRQ_MSI);
 	if (nr_entries < 0)
 		return nr_entries;
 
@@ -77,7 +83,13 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask)
 		 * Number field in the PCI Express Capabilities register", where
 		 * according to Section 7.8.2 of the specification "For MSI-X,
 		 * the value in this field indicates which MSI-X Table entry is
-		 * used to generate the interrupt message."
+		 * used to generate the interrupt message." and "For MSI, the
+		 * value in this field indicates the offset between the base
+		 * Message Data and the interrupt message that is generated."
+		 *
+		 * pci_irq_vector() below is able to handle entry differently
+		 * depending on MSI vs MSI-x case
+		 *
 		 */
 		pcie_capability_read_word(dev, PCI_EXP_FLAGS, &reg16);
 		entry = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9;
@@ -100,7 +112,13 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask)
 		 * MSI/MSI-X vectors assigned to the port is going to be used
 		 * for AER, where "For MSI-X, the value in this register
 		 * indicates which MSI-X Table entry is used to generate the
-		 * interrupt message."
+		 * interrupt message."  and "For MSI, the value
+		 * in this field indicates the offset between the base Message
+		 * Data and the interrupt message that is generated."
+		 *
+		 * pci_irq_vector() below is able to handle entry differently
+		 * depending on MSI vs MSI-x case
+		 *
 		 */
 		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
 		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &reg32);
@@ -125,6 +143,9 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask)
 		/* Now allocate the MSI-X vectors for real */
 		nr_entries = pci_alloc_irq_vectors(dev, nvec, nvec,
 				PCI_IRQ_MSIX);
+		if (nr_entries < 0) /* MSI-x failed, let's try MSI*/
+			nr_entries = pci_alloc_irq_vectors(dev, nvec, nvec,
+					PCI_IRQ_MSI);
 		if (nr_entries < 0)
 			return nr_entries;
 	}
@@ -160,8 +181,8 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
 	    ((mask & PCIE_PORT_SERVICE_HP) && pciehp_no_msi())) {
 		flags &= ~PCI_IRQ_MSI;
 	} else {
-		/* Try to use MSI-X if supported */
-		if (!pcie_port_enable_msix(dev, irqs, mask))
+		/* Try to use MSI-X or MSI if supported */
+		if (!pcie_port_enable_msix_or_msi(dev, irqs, mask))
 			return 0;
 	}
 
-- 
2.7.4

[PATCH 2/2] PCI/portdrv: allocate MSI/MSIx vector for DPC RP service

[Gabriele Paoloni]

From: gabriele paoloni <gabriele.paoloni@huawei.com>

Currently the MSI/MSIx vectors for the root port services are
allocated calling pcie_init_service_irqs(). At the moment these
vectors are only allocated for AER, PME, HP.
This patch allocate an MSI/MSIx vector also for DPC.

Signed-off-by: Liudongdong <liudongdong3@huawei.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 drivers/pci/pcie/portdrv_core.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index e2c7bfd..477bf14 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -131,6 +131,35 @@ int pcie_port_enable_msix_or_msi(struct pci_dev *dev, int *irqs, int mask)
 		nvec = max(nvec, entry + 1);
 	}
 
+	if (mask & PCIE_PORT_SERVICE_DPC) {
+		u16 reg16, pos;
+
+		/*
+		 * The code below follows Section 6.2.10.1 of the PCI Express
+		 * Base Specification 4.0 stating that bits 4-0 of DPC
+		 * Capability Register contain a value indicating which of the
+		 * MSI/MSI-X vectors assigned to the port is going to be used
+		 * for DPC, where "For MSI-X, the value in this register
+		 * indicates which MSI-X Table entry is used to generate the
+		 * interrupt message." and "For MSI, the value in this field
+		 * indicates the offset between the base Message Data and the
+		 * interrupt message that is generated."
+		 *
+		 * pci_irq_vector() below is able to handle entry differently
+		 * depending on MSI vs MSI-x case
+		 *
+		 */
+		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;
+		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 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
-- 
2.7.4

Re: [PATCH 1/2] PCI/portdrv: add support for different MSI interrupts for PCIe port services

[Christoph Hellwig]

> --- a/drivers/pci/pcie/portdrv.h
> +++ b/drivers/pci/pcie/portdrv.h
> @@ -18,6 +18,11 @@
>   */
>  #define PCIE_PORT_MAX_MSIX_ENTRIES	32
>  
> +/* According to the PCI Local Bus Specification REV. 3.0 the max number
> + * of MSI vectors per function is 32
> + */
> +#define PCIE_PORT_MAX_MSI_ENTRIES	32

Just rename the above define to PCIE_PORT_MAX_MSI_ENTRIES and update
the comment.

>  /**
> - * pcie_port_enable_msix - try to set up MSI-X as interrupt mode for given port
> + * pcie_port_enable_msix_or_msi - try to set up MSI-X or MSI as interrupt mode

.. and just call this pcie_port_enable_multi_vec or maybe even just
pcie_port_enable_msi.

> +static
> +int pcie_port_enable_msix_or_msi(struct pci_dev *dev, int *irqs, int mask)

This style of indentation is entirely wrong.

>  {
>  	int nr_entries, entry, nvec = 0;
>  
> @@ -63,6 +65,10 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask)
>  	 */
>  	nr_entries = pci_alloc_irq_vectors(dev, 1, PCIE_PORT_MAX_MSIX_ENTRIES,
>  			PCI_IRQ_MSIX);
> +	if (nr_entries < 0) /* MSI-x failed let's try with MSI */
> +		nr_entries = pci_alloc_irq_vectors(dev, 1,
> +				PCIE_PORT_MAX_MSI_ENTRIES,
> +				PCI_IRQ_MSI);

Just pass PCI_IRQ_MSI | PCI_IRQ_MSIX in the above call.


>  		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
>  		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &reg32);
> @@ -125,6 +143,9 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask)
>  		/* Now allocate the MSI-X vectors for real */
>  		nr_entries = pci_alloc_irq_vectors(dev, nvec, nvec,
>  				PCI_IRQ_MSIX);
> +		if (nr_entries < 0) /* MSI-x failed, let's try MSI*/
> +			nr_entries = pci_alloc_irq_vectors(dev, nvec, nvec,
> +					PCI_IRQ_MSI);

Same here.

>  		if (nr_entries < 0)
>  			return nr_entries;
>  	}
> @@ -160,8 +181,8 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  	    ((mask & PCIE_PORT_SERVICE_HP) && pciehp_no_msi())) {
>  		flags &= ~PCI_IRQ_MSI;
>  	} else {
> -		/* Try to use MSI-X if supported */
> -		if (!pcie_port_enable_msix(dev, irqs, mask))
> +		/* Try to use MSI-X or MSI if supported */
> +		if (!pcie_port_enable_msix_or_msi(dev, irqs, mask))
>  			return 0;
>  	}

We have another pci_alloc_irq_vectors call just below this which
also passes PCI_IRQ_MSI, but we won't reach it anymore with your
changes I think, so pcie_init_service_irqs will need some updates.
(after checking that your code still works fine for single-MSI setups,
of course)

Re: [PATCH 2/2] PCI/portdrv: allocate MSI/MSIx vector for DPC RP service

[Christoph Hellwig]

> +		 * pci_irq_vector() below is able to handle entry differently
> +		 * depending on MSI vs MSI-x case
> +		 *

No need to mention that for every users.

Except for that this looks fine:

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

RE: [PATCH 1/2] PCI/portdrv: add support for different MSI interrupts for PCIe port services

[Gabriele Paoloni]

Hi Christoph

Many thanks for your comments

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: 16 May 2017 13:07
> To: Gabriele Paoloni
> Cc: bhelgaas@google.com; helgaas@kernel.org; Linuxarm; linux-
> pci@vger.kernel.org; lukas@wunner.de; linux-kernel@vger.kernel.org;
> mika.westerberg@linux.intel.com
> Subject: Re: [PATCH 1/2] PCI/portdrv: add support for different MSI
> interrupts for PCIe port services
> 
> > --- a/drivers/pci/pcie/portdrv.h
> > +++ b/drivers/pci/pcie/portdrv.h
> > @@ -18,6 +18,11 @@
> >   */
> >  #define PCIE_PORT_MAX_MSIX_ENTRIES	32
> >
> > +/* According to the PCI Local Bus Specification REV. 3.0 the max
> number
> > + * of MSI vectors per function is 32
> > + */
> > +#define PCIE_PORT_MAX_MSI_ENTRIES	32
> 
> Just rename the above define to PCIE_PORT_MAX_MSI_ENTRIES and update
> the comment.

Ok agreed

> 
> >  /**
> > - * pcie_port_enable_msix - try to set up MSI-X as interrupt mode for
> given port
> > + * pcie_port_enable_msix_or_msi - try to set up MSI-X or MSI as
> interrupt mode
> 
> .. and just call this pcie_port_enable_multi_vec or maybe even just
> pcie_port_enable_msi.

Maybe pcie_port_enable_irq_vec ?
 
> 
> > +static
> > +int pcie_port_enable_msix_or_msi(struct pci_dev *dev, int *irqs, int
> mask)
> 
> This style of indentation is entirely wrong.

Sorry about this; I'll fix it in V2

> 
> >  {
> >  	int nr_entries, entry, nvec = 0;
> >
> > @@ -63,6 +65,10 @@ static int pcie_port_enable_msix(struct pci_dev
> *dev, int *irqs, int mask)
> >  	 */
> >  	nr_entries = pci_alloc_irq_vectors(dev, 1,
> PCIE_PORT_MAX_MSIX_ENTRIES,
> >  			PCI_IRQ_MSIX);
> > +	if (nr_entries < 0) /* MSI-x failed let's try with MSI */
> > +		nr_entries = pci_alloc_irq_vectors(dev, 1,
> > +				PCIE_PORT_MAX_MSI_ENTRIES,
> > +				PCI_IRQ_MSI);
> 
> Just pass PCI_IRQ_MSI | PCI_IRQ_MSIX in the above call.

Yes you're right, thanks for spotting this

> 
> 
> >  		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> >  		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS,
> &reg32);
> > @@ -125,6 +143,9 @@ static int pcie_port_enable_msix(struct pci_dev
> *dev, int *irqs, int mask)
> >  		/* Now allocate the MSI-X vectors for real */
> >  		nr_entries = pci_alloc_irq_vectors(dev, nvec, nvec,
> >  				PCI_IRQ_MSIX);
> > +		if (nr_entries < 0) /* MSI-x failed, let's try MSI*/
> > +			nr_entries = pci_alloc_irq_vectors(dev, nvec, nvec,
> > +					PCI_IRQ_MSI);
> 
> Same here.

Yep, thanks

> 
> >  		if (nr_entries < 0)
> >  			return nr_entries;
> >  	}
> > @@ -160,8 +181,8 @@ static int pcie_init_service_irqs(struct pci_dev
> *dev, int *irqs, int mask)
> >  	    ((mask & PCIE_PORT_SERVICE_HP) && pciehp_no_msi())) {
> >  		flags &= ~PCI_IRQ_MSI;
> >  	} else {
> > -		/* Try to use MSI-X if supported */
> > -		if (!pcie_port_enable_msix(dev, irqs, mask))
> > +		/* Try to use MSI-X or MSI if supported */
> > +		if (!pcie_port_enable_msix_or_msi(dev, irqs, mask))
> >  			return 0;
> >  	}
> 
> We have another pci_alloc_irq_vectors call just below this which
> also passes PCI_IRQ_MSI, but we won't reach it anymore with your
> changes I think, so pcie_init_service_irqs will need some updates.
> (after checking that your code still works fine for single-MSI setups,
> of course)

I think we can reach that call if both MSI and MSIx fails and then it
will fall back to legacy IRQ. However you are right in saying that the
code should be reworked. Now it would be:

static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
{
	int ret, i;

	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
		irqs[i] = -1;

	/*
	 * Make sure MSI can be used for PCIe PME or hotplug. otherwise we have to
	 * use INTx or other interrupts, e.g. system shared interrupt.
	 */
	if (!((mask & PCIE_PORT_SERVICE_PME) && pcie_pme_no_msi()) &&
	    !((mask & PCIE_PORT_SERVICE_HP) && pciehp_no_msi()))
		/* Try to use MSI-X or MSI if supported */
		if (!pcie_port_enable_msix_or_msi(dev, irqs, mask))
			return 0;

	/* fall back to legacy IRQ */
	ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_LEGACY);
	if (ret < 0)
		return -ENODEV;

	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
		if (i != PCIE_PORT_SERVICE_VC_SHIFT)
			irqs[i] = pci_irq_vector(dev, 0);
	}

	return 0;
}

What do you think?

Again many thanks
Gab    

RE: [PATCH 2/2] PCI/portdrv: allocate MSI/MSIx vector for DPC RP service

[Gabriele Paoloni]

Hi Christoph

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: 16 May 2017 13:11
> To: Gabriele Paoloni
> Cc: bhelgaas@google.com; helgaas@kernel.org; Linuxarm; linux-
> pci@vger.kernel.org; lukas@wunner.de; linux-kernel@vger.kernel.org;
> mika.westerberg@linux.intel.com
> Subject: Re: [PATCH 2/2] PCI/portdrv: allocate MSI/MSIx vector for DPC
> RP service
> 
> > +		 * pci_irq_vector() below is able to handle entry
> differently
> > +		 * depending on MSI vs MSI-x case
> > +		 *

Ok I'll remove this in V2

> 
> No need to mention that for every users.
> 
> Except for that this looks fine:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Many thanks
Gab