[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
>=20
> Hi Bharat,
>=20
> Thank you for the patch! Perhaps something to improve:
>=20
> [auto build test WARNING on pci/next]
> [also build test WARNING on v4.18 next-20180810] [if your patch is applie=
d to
> the wrong git tree, please drop us a note to help improve the system]
>=20
> 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 n=
ext
> reproduce:
>         # apt-get install sparse
>         make ARCH=3Dx86_64 allmodconfig
>         make C=3D1 CF=3D-D__CHECK_ENDIAN__
>=20
>=20
> sparse warnings: (new ones prefixed by >>)
>=20
> >> drivers/pci/controller/pcie-xilinx-nwl.c:823:5: sparse: symbol
> 'nwl_setup_service_irqs' was not declared. Should it be static?
>=20
> Please review and possibly fold the followup patch.
>=20
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Ce=
nter
> https://lists.01.org/pipermail/kbuild-all                   Intel Corpora=
tion

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
>=20
> Some platforms have dedicated IRQ lines for PCIe services like AER/PME et=
c.
> 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.
>=20
> 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
>=20
>  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
>=20
> 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 =3D pci_host_bridge_priv(bridge);
> > +	if (plat_mask & PCIE_PORT_SERVICE_AER) {
> > +		irqs[PCIE_PORT_SERVICE_AER_SHIFT] =3D pcie->irq_misc;
> > +		plat_mask &=3D ~(1 << PCIE_PORT_SERVICE_AER_SHIFT);
> > +	}
>=20
> If I understand correctly, this ultimately results in pcie->irq_misc bein=
g
> 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().
>=20
> 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.
>=20
Hi Bjorn, the nwl_pcie_misc_handler() prints all pcie errors along with AER=
 and then clears=20
controller register (MSGF_MISC_STATUS) in which these status bits are set.=
=20
But clearing this controller register will not effect any bits in AER capab=
ilities.
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 differen=
t 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 IR=
Q's
>=20
> 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
>=20
> Capitalize "IRQ" consistently in English text like this.
>=20
> Insert blank lines between paragraphs.
>=20
> 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.
>=20
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] =3D -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) =3D=3D PCI_EXP_TYPE_ROOT_PORT) {
> > +		int plat_mask;
> > +
> > +		plat_mask =3D pci_check_platform_service_irqs(dev, irqs,
> mask);
> > +		if (plat_mask > 0)
>=20
> Masks should be unsigned and tested for zero or "mask & bit".  The rest o=
f
> 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.
>=20
> > +			mask =3D mask & plat_mask;
> > +	}
>=20
> 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(), w=
hich
> only does MSI-X/MSI stuff.  So this must rely on that failing?
>=20
> And then we fall through to run pci_alloc_irq_vectors(), which is for PCI=
 INTx
> interrupts, which doesn't seem appropriate either.
>=20
> 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] directl=
y
> (I think you already do this inside pci_check_platform_service_irqs()) an=
d
> 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 differen=
t 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 d=
ifferent scenarios and=20
chunk below.

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

Thanks,
Bharat