| Message ID | 1533915580-31805-5-git-send-email-bharat.kumar.gogada@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Add support to register platform service IRQ | expand |
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
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
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 >
> 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
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);
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(-)