[PATCH 0/4] Add support to register platform service IRQ

[PATCH 0/4] Add support to register platform service IRQ

[Bharat Kumar Gogada]

Some platforms have dedicated IRQ lines for PCIe services like
AER/PME etc. The root complex on these platform will use these seperate
IRQ lines to report AER/PME etc., interrupts and will not generate
MSI/MSI-X/INTx interrupts for these services.
These patches will add new method for these kind of platforms
to register the platform IRQ number with respective PCIe services.

Bharat Kumar Gogada (4):
  PCI: Add setup_platform_service_irq hook to struct pci_host_bridge
  PCI: Add pci_check_platform_service_irqs
  PCI/portdrv: Check platform supported service IRQ's
  PCI: xilinx-nwl: Add method to setup_platform_service_irq hook

 drivers/pci/controller/pcie-xilinx-nwl.c |   16 ++++++++++++++++
 drivers/pci/pcie/portdrv_core.c          |   19 +++++++++++++++++--
 include/linux/pci.h                      |   25 +++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 2 deletions(-)

[PATCH 1/4] PCI: Add setup_platform_service_irq hook to struct pci_host_bridge

[Bharat Kumar Gogada]

Add setup_platform_service_irq hook to struct pci_host_bridge.
Some platforms have dedicated interrupt line from root complex to
interrupt controller for PCIe services like AER/PME etc.
This hook is to register platform IRQ's to PCIe port services.

Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
---
 include/linux/pci.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 340029b..c28f575 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -470,6 +470,7 @@ struct pci_host_bridge {
 	u8 (*swizzle_irq)(struct pci_dev *, u8 *); /* Platform IRQ swizzler */
 	int (*map_irq)(const struct pci_dev *, u8, u8);
 	void (*release_fn)(struct pci_host_bridge *);
+	int (*setup_platform_service_irq)(struct pci_host_bridge *, int *, int);
 	void		*release_data;
 	struct msi_controller *msi;
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
-- 
1.7.1

[PATCH 2/4] PCI: Add pci_check_platform_service_irqs

[Bharat Kumar Gogada]

Adding method pci_check_platform_service_irqs to check if platform
has registered method to proivde dedicated IRQ lines for PCIe services
like AER/PME etc.

Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
---
 include/linux/pci.h |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index c28f575..8eb6470 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2271,6 +2271,30 @@ static inline bool pci_ari_enabled(struct pci_bus *bus)
 }
 
 /**
+ * pci_check_platform_service_irqs - check platform service irq's
+ * @pdev: PCI Express device to check
+ * @irqs: Array of irqs to populate
+ * @mask: Bitmask of capabilities
+ *
+ * Return value: Bitmask after clearing platform supported service
+ * bits
+ */
+static inline int pci_check_platform_service_irqs(struct pci_dev *dev,
+						  int *irqs, int mask)
+{
+	struct pci_host_bridge *bridge;
+
+	if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
+		return -EINVAL;
+
+	bridge = pci_find_host_bridge(dev->bus);
+	if (bridge && bridge->setup_platform_service_irq)
+		return bridge->setup_platform_service_irq(bridge, irqs, mask);
+	else
+		return -EINVAL;
+}
+
+/**
  * pci_is_thunderbolt_attached - whether device is on a Thunderbolt daisy chain
  * @pdev: PCI device to check
  *
-- 
1.7.1

[PATCH 3/4] PCI/portdrv: Check platform supported service IRQ's

[Bharat Kumar Gogada]

Platforms may have dedicated IRQ lines for PCIe services like
AER/PME etc., check for such IRQ lines.
Check mask and fill legacy irq line for services other than
platform supported service IRQ number.

Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
---
 drivers/pci/pcie/portdrv_core.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index e0261ad..a7d024c 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -166,6 +166,19 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
 		irqs[i] = -1;
 
 	/*
+	 * Some platforms have dedicated interrupt line from root complex to
+	 * interrupt controller for PCIe services like AER/PME etc., check
+	 * if platform registered with any such IRQ.
+	 */
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
+		int plat_mask;
+
+		plat_mask = pci_check_platform_service_irqs(dev, irqs, mask);
+		if (plat_mask > 0)
+			mask = mask & plat_mask;
+	}
+
+	/*
 	 * If we support PME but can't use MSI/MSI-X for it, we have to
 	 * fall back to INTx or other interrupts, e.g., a system shared
 	 * interrupt.
@@ -183,8 +196,10 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
 	if (ret < 0)
 		return -ENODEV;
 
-	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
-		irqs[i] = pci_irq_vector(dev, 0);
+	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
+		if (mask & (1 << i))
+			irqs[i] = pci_irq_vector(dev, 0);
+	}
 
 	return 0;
 }
-- 
1.7.1

[PATCH 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook

[Bharat Kumar Gogada]

Add nwl_setup_service_irqs hook to setup_platform_service_irq IRQs to
register platform provided IRQ number to kernel AER service.

Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
---
 drivers/pci/controller/pcie-xilinx-nwl.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index fb32840..285647b 100644
--- a/drivers/pci/controller/pcie-xilinx-nwl.c
+++ b/drivers/pci/controller/pcie-xilinx-nwl.c
@@ -22,6 +22,7 @@
 #include <linux/irqchip/chained_irq.h>
 
 #include "../pci.h"
+#include "../pcie/portdrv.h"
 
 /* Bridge core config registers */
 #define BRCFG_PCIE_RX0			0x00000000
@@ -819,6 +820,20 @@ static int nwl_pcie_parse_dt(struct nwl_pcie *pcie,
 	return 0;
 }
 
+int nwl_setup_service_irqs(struct pci_host_bridge *bridge, int *irqs,
+			   int plat_mask)
+{
+	struct nwl_pcie *pcie;
+
+	pcie = pci_host_bridge_priv(bridge);
+	if (plat_mask & PCIE_PORT_SERVICE_AER) {
+		irqs[PCIE_PORT_SERVICE_AER_SHIFT] = pcie->irq_misc;
+		plat_mask &= ~(1 << PCIE_PORT_SERVICE_AER_SHIFT);
+	}
+
+	return plat_mask;
+}
+
 static const struct of_device_id nwl_pcie_of_match[] = {
 	{ .compatible = "xlnx,nwl-pcie-2.11", },
 	{}
@@ -880,6 +895,7 @@ static int nwl_pcie_probe(struct platform_device *pdev)
 	bridge->ops = &nwl_pcie_ops;
 	bridge->map_irq = of_irq_parse_and_map_pci;
 	bridge->swizzle_irq = pci_common_swizzle;
+	bridge->setup_platform_service_irq = nwl_setup_service_irqs;
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		err = nwl_pcie_enable_msi(pcie);
-- 
1.7.1

Re: [PATCH 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook

[kbuild test robot]

Hi Bharat,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pci/next]
[also build test WARNING on v4.18 next-20180810]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Bharat-Kumar-Gogada/Add-support-to-register-platform-service-IRQ/20180813-144216
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/pci/controller/pcie-xilinx-nwl.c:823:5: sparse: symbol 'nwl_setup_service_irqs' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] PCI: xilinx-nwl: nwl_setup_service_irqs() can be static

[kbuild test robot]

Fixes: 19cbd2e19134 ("PCI: xilinx-nwl: Add method to setup_platform_service_irq hook")
Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
---
 pcie-xilinx-nwl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index 285647b..af07db8 100644
--- a/drivers/pci/controller/pcie-xilinx-nwl.c
+++ b/drivers/pci/controller/pcie-xilinx-nwl.c
@@ -820,8 +820,8 @@ static int nwl_pcie_parse_dt(struct nwl_pcie *pcie,
 	return 0;
 }
 
-int nwl_setup_service_irqs(struct pci_host_bridge *bridge, int *irqs,
-			   int plat_mask)
+static int nwl_setup_service_irqs(struct pci_host_bridge *bridge, int *irqs,
+				  int plat_mask)
 {
 	struct nwl_pcie *pcie;
 

RE: [PATCH 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook

[Bharat Kumar Gogada]

Agreed, will Fix it in next version.

Regards,
Bharat
> -----Original Message-----
> From: kbuild test robot [mailto:lkp@intel.com]
> Sent: Monday, August 13, 2018 2:39 PM
> To: Bharat Kumar Gogada <bharatku@xilinx.com>
> Cc: kbuild-all@01.org; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org; bhelgaas@google.com; Ravikiran Gummaluri
> <rgummal@xilinx.com>; Bharat Kumar Gogada <bharatku@xilinx.com>
> Subject: Re: [PATCH 4/4] PCI: xilinx-nwl: Add method to
> setup_platform_service_irq hook
> 
> Hi Bharat,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on pci/next]
> [also build test WARNING on v4.18 next-20180810] [if your patch is applied to
> the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Bharat-Kumar-
> Gogada/Add-support-to-register-platform-service-IRQ/20180813-144216
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
> reproduce:
>         # apt-get install sparse
>         make ARCH=x86_64 allmodconfig
>         make C=1 CF=-D__CHECK_ENDIAN__
> 
> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> drivers/pci/controller/pcie-xilinx-nwl.c:823:5: sparse: symbol
> 'nwl_setup_service_irqs' was not declared. Should it be static?
> 
> Please review and possibly fold the followup patch.
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

RE: [PATCH 0/4] Add support to register platform service IRQ

[Bharat Kumar Gogada]

> Subject: [PATCH 0/4] Add support to register platform service IRQ
> 
> Some platforms have dedicated IRQ lines for PCIe services like AER/PME etc.
> The root complex on these platform will use these seperate IRQ lines to
> report AER/PME etc., interrupts and will not generate MSI/MSI-X/INTx
> interrupts for these services.
> These patches will add new method for these kind of platforms to register
> the platform IRQ number with respective PCIe services.
> 
> Bharat Kumar Gogada (4):
>   PCI: Add setup_platform_service_irq hook to struct pci_host_bridge
>   PCI: Add pci_check_platform_service_irqs
>   PCI/portdrv: Check platform supported service IRQ's
>   PCI: xilinx-nwl: Add method to setup_platform_service_irq hook
> 
>  drivers/pci/controller/pcie-xilinx-nwl.c |   16 ++++++++++++++++
>  drivers/pci/pcie/portdrv_core.c          |   19 +++++++++++++++++--
>  include/linux/pci.h                      |   25 +++++++++++++++++++++++++
>  3 files changed, 58 insertions(+), 2 deletions(-)

Hi Bjorn,
Can you comment on this series.

Bharat

Re: [PATCH 0/4] Add support to register platform service IRQ

[Bjorn Helgaas]

On Fri, Aug 24, 2018 at 12:16:20PM +0000, Bharat Kumar Gogada wrote:
> > Subject: [PATCH 0/4] Add support to register platform service IRQ
> > 
> > Some platforms have dedicated IRQ lines for PCIe services like AER/PME etc.
> > The root complex on these platform will use these seperate IRQ lines to
> > report AER/PME etc., interrupts and will not generate MSI/MSI-X/INTx
> > interrupts for these services.
> > These patches will add new method for these kind of platforms to register
> > the platform IRQ number with respective PCIe services.
> > 
> > Bharat Kumar Gogada (4):
> >   PCI: Add setup_platform_service_irq hook to struct pci_host_bridge
> >   PCI: Add pci_check_platform_service_irqs
> >   PCI/portdrv: Check platform supported service IRQ's
> >   PCI: xilinx-nwl: Add method to setup_platform_service_irq hook
> > 
> >  drivers/pci/controller/pcie-xilinx-nwl.c |   16 ++++++++++++++++
> >  drivers/pci/pcie/portdrv_core.c          |   19 +++++++++++++++++--
> >  include/linux/pci.h                      |   25 +++++++++++++++++++++++++
> >  3 files changed, 58 insertions(+), 2 deletions(-)
> 
> Hi Bjorn,
> Can you comment on this series.

I'm taking a breather during the merge window.  As soon as -rc1 comes
out (probably Sunday), I'll start processing the patches in the queue.

Bjorn

Re: [PATCH 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook

[Bjorn Helgaas]

On Fri, Aug 10, 2018 at 09:09:40PM +0530, Bharat Kumar Gogada wrote:
> Add nwl_setup_service_irqs hook to setup_platform_service_irq IRQs to
> register platform provided IRQ number to kernel AER service.
> 
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> ---
>  drivers/pci/controller/pcie-xilinx-nwl.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
> index fb32840..285647b 100644
> --- a/drivers/pci/controller/pcie-xilinx-nwl.c
> +++ b/drivers/pci/controller/pcie-xilinx-nwl.c
> @@ -22,6 +22,7 @@
>  #include <linux/irqchip/chained_irq.h>
>  
>  #include "../pci.h"
> +#include "../pcie/portdrv.h"
>  
>  /* Bridge core config registers */
>  #define BRCFG_PCIE_RX0			0x00000000
> @@ -819,6 +820,20 @@ static int nwl_pcie_parse_dt(struct nwl_pcie *pcie,
>  	return 0;
>  }
>  
> +int nwl_setup_service_irqs(struct pci_host_bridge *bridge, int *irqs,
> +			   int plat_mask)
> +{
> +	struct nwl_pcie *pcie;
> +
> +	pcie = pci_host_bridge_priv(bridge);
> +	if (plat_mask & PCIE_PORT_SERVICE_AER) {
> +		irqs[PCIE_PORT_SERVICE_AER_SHIFT] = pcie->irq_misc;
> +		plat_mask &= ~(1 << PCIE_PORT_SERVICE_AER_SHIFT);
> +	}

If I understand correctly, this ultimately results in pcie->irq_misc
being hooked up to aer_irq() via the aer_probe() path.  We already
have pcie->irq_misc being hooked up to nwl_pcie_misc_handler() via
nwl_pcie_bridge_init().

We can't rely on the ordering of the two handlers.  Is it safe if
nwl_pcie_misc_handler() runs first, followed by aer_irq()?  It looks
like nwl_pcie_misc_handler() might log messages and clear AER-related
errors.  If that's the case aer_irq() might not find anything to do.

> +
> +	return plat_mask;
> +}
> +
>  static const struct of_device_id nwl_pcie_of_match[] = {
>  	{ .compatible = "xlnx,nwl-pcie-2.11", },
>  	{}
> @@ -880,6 +895,7 @@ static int nwl_pcie_probe(struct platform_device *pdev)
>  	bridge->ops = &nwl_pcie_ops;
>  	bridge->map_irq = of_irq_parse_and_map_pci;
>  	bridge->swizzle_irq = pci_common_swizzle;
> +	bridge->setup_platform_service_irq = nwl_setup_service_irqs;
>  
>  	if (IS_ENABLED(CONFIG_PCI_MSI)) {
>  		err = nwl_pcie_enable_msi(pcie);
> -- 
> 1.7.1
> 

Re: [PATCH 3/4] PCI/portdrv: Check platform supported service IRQ's

[Bjorn Helgaas]

On Fri, Aug 10, 2018 at 09:09:39PM +0530, Bharat Kumar Gogada wrote:
> Platforms may have dedicated IRQ lines for PCIe services like
> AER/PME etc., check for such IRQ lines.
> Check mask and fill legacy irq line for services other than

Capitalize "IRQ" consistently in English text like this.

Insert blank lines between paragraphs.

Add a reference to the relevant spec sections.  PCIe r4.0, sec
6.2.4.1.2, 6.2.6, 7.5.3.12 seem pertinent.

> platform supported service IRQ number.
> 
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> ---
>  drivers/pci/pcie/portdrv_core.c |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index e0261ad..a7d024c 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -166,6 +166,19 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  		irqs[i] = -1;
>  
>  	/*
> +	 * Some platforms have dedicated interrupt line from root complex to
> +	 * interrupt controller for PCIe services like AER/PME etc., check
> +	 * if platform registered with any such IRQ.
> +	 */
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> +		int plat_mask;
> +
> +		plat_mask = pci_check_platform_service_irqs(dev, irqs, mask);
> +		if (plat_mask > 0)

Masks should be unsigned and tested for zero or "mask & bit".  The
rest of this file uses "int", which is sloppy because it does the
wrong thing if we happen to use the high-order bit in the mask.

> +			mask = mask & plat_mask;
> +	}

I think these platform IRQs are neither MSI-X/MSI nor PCI INTx wires.
But as written, I think this patch executes pcie_port_enable_irq_vec(),
which only does MSI-X/MSI stuff.  So this must rely on that failing?

And then we fall through to run pci_alloc_irq_vectors(), which is for
PCI INTx interrupts, which doesn't seem appropriate either.

It seems like this platform IRQ case should be completely separated
from the other MSI/INTx cases, i.e., set
irqs[PCIE_PORT_SERVICE_AER_SHIFT] directly (I think you already do
this inside pci_check_platform_service_irqs()) and immediately return.
Then I think you wouldn't need the other hunk below.

> +
> +	/*
>  	 * If we support PME but can't use MSI/MSI-X for it, we have to
>  	 * fall back to INTx or other interrupts, e.g., a system shared
>  	 * interrupt.
> @@ -183,8 +196,10 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  	if (ret < 0)
>  		return -ENODEV;
>  
> -	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
> -		irqs[i] = pci_irq_vector(dev, 0);
> +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
> +		if (mask & (1 << i))
> +			irqs[i] = pci_irq_vector(dev, 0);
> +	}
>  
>  	return 0;
>  }
> -- 
> 1.7.1
> 

RE: [PATCH 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook

[Bharat Kumar Gogada]

> Subject: Re: [PATCH 4/4] PCI: xilinx-nwl: Add method to
> setup_platform_service_irq hook
> 
> On Fri, Aug 10, 2018 at 09:09:40PM +0530, Bharat Kumar Gogada wrote:
> > Add nwl_setup_service_irqs hook to setup_platform_service_irq IRQs to
> > register platform provided IRQ number to kernel AER service.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> > ---
> >  drivers/pci/controller/pcie-xilinx-nwl.c |   16 ++++++++++++++++
> >  1 files changed, 16 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c
> > b/drivers/pci/controller/pcie-xilinx-nwl.c
> > index fb32840..285647b 100644
> > --- a/drivers/pci/controller/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/controller/pcie-xilinx-nwl.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/irqchip/chained_irq.h>
> >
> >  #include "../pci.h"
> > +#include "../pcie/portdrv.h"
> >
> >  /* Bridge core config registers */
> >  #define BRCFG_PCIE_RX0			0x00000000
> > @@ -819,6 +820,20 @@ static int nwl_pcie_parse_dt(struct nwl_pcie *pcie,
> >  	return 0;
> >  }
> >
> > +int nwl_setup_service_irqs(struct pci_host_bridge *bridge, int *irqs,
> > +			   int plat_mask)
> > +{
> > +	struct nwl_pcie *pcie;
> > +
> > +	pcie = pci_host_bridge_priv(bridge);
> > +	if (plat_mask & PCIE_PORT_SERVICE_AER) {
> > +		irqs[PCIE_PORT_SERVICE_AER_SHIFT] = pcie->irq_misc;
> > +		plat_mask &= ~(1 << PCIE_PORT_SERVICE_AER_SHIFT);
> > +	}
> 
> If I understand correctly, this ultimately results in pcie->irq_misc being
> hooked up to aer_irq() via the aer_probe() path.  We already have pcie-
> >irq_misc being hooked up to nwl_pcie_misc_handler() via
> nwl_pcie_bridge_init().
> 
> We can't rely on the ordering of the two handlers.  Is it safe if
> nwl_pcie_misc_handler() runs first, followed by aer_irq()?  It looks like
> nwl_pcie_misc_handler() might log messages and clear AER-related errors.  If
> that's the case aer_irq() might not find anything to do.
> 
Hi Bjorn, the nwl_pcie_misc_handler() prints all pcie errors along with AER and then clears 
controller register (MSGF_MISC_STATUS) in which these status bits are set. 
But clearing this controller register will not effect any bits in AER capabilities.
So in our case we need  to clear controller register and also handle AER as per spec.
This will not cause any ordering issue as both paths are accessing different set of registers.

Thanks,
Bharat

RE: [PATCH 3/4] PCI/portdrv: Check platform supported service IRQ's

[Bharat Kumar Gogada]

> Subject: Re: [PATCH 3/4] PCI/portdrv: Check platform supported service IRQ's
> 
> On Fri, Aug 10, 2018 at 09:09:39PM +0530, Bharat Kumar Gogada wrote:
> > Platforms may have dedicated IRQ lines for PCIe services like AER/PME
> > etc., check for such IRQ lines.
> > Check mask and fill legacy irq line for services other than
> 
> Capitalize "IRQ" consistently in English text like this.
> 
> Insert blank lines between paragraphs.
> 
> Add a reference to the relevant spec sections.  PCIe r4.0, sec 6.2.4.1.2, 6.2.6,
> 7.5.3.12 seem pertinent.
> 
Agreed, will do in next patch.
> > platform supported service IRQ number.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> > ---
> >  drivers/pci/pcie/portdrv_core.c |   19 +++++++++++++++++--
> >  1 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/portdrv_core.c
> > b/drivers/pci/pcie/portdrv_core.c index e0261ad..a7d024c 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -166,6 +166,19 @@ static int pcie_init_service_irqs(struct pci_dev
> *dev, int *irqs, int mask)
> >  		irqs[i] = -1;
> >
> >  	/*
> > +	 * Some platforms have dedicated interrupt line from root complex
> to
> > +	 * interrupt controller for PCIe services like AER/PME etc., check
> > +	 * if platform registered with any such IRQ.
> > +	 */
> > +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> > +		int plat_mask;
> > +
> > +		plat_mask = pci_check_platform_service_irqs(dev, irqs,
> mask);
> > +		if (plat_mask > 0)
> 
> Masks should be unsigned and tested for zero or "mask & bit".  The rest of
> this file uses "int", which is sloppy because it does the wrong thing if we
> happen to use the high-order bit in the mask.
> 
> > +			mask = mask & plat_mask;
> > +	}
> 
> I think these platform IRQs are neither MSI-X/MSI nor PCI INTx wires.
> But as written, I think this patch executes pcie_port_enable_irq_vec(), which
> only does MSI-X/MSI stuff.  So this must rely on that failing?
> 
> And then we fall through to run pci_alloc_irq_vectors(), which is for PCI INTx
> interrupts, which doesn't seem appropriate either.
> 
> It seems like this platform IRQ case should be completely separated from the
> other MSI/INTx cases, i.e., set irqs[PCIE_PORT_SERVICE_AER_SHIFT] directly
> (I think you already do this inside pci_check_platform_service_irqs()) and
> immediately return.
> Then I think you wouldn't need the other hunk below.
Agreed if we check platform service irqs here we need to deal with different combination
of service IRQ's like AER MSI, pme platform .. and change code accordingly to fill irqs[i].
yes it is better to call platform IRQ separately to avoid code changes in different scenarios and 
chunk below.

Can we do the following code flow: 
int pcie_port_device_register(struct pci_dev *dev)
{
 ... 
status = pcie_init_service_irqs(dev, irqs, capabilities);
        if (status) {
...
}
pci_check_platform_service_irqs(dev, irqs, capabilities);

Thanks,
Bharat