[PATCH] PCI: xilinx: Remove platform/architecture restrictions

[PATCH] PCI: xilinx: Remove platform/architecture restrictions

[Guenter Roeck]

The MIPS Boston board configuration tries to enable CONFIG_PCIE_XILINX.
That doesn't work since PCIE_XILINX depends on ARCH_ZYNQ || MICROBLAZE.
Remove that restriction.

Cc: Paul Burton <paul.burton@imgtec.com>
Cc: James Hogan <james.hogan@imgtec.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/pci/host/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 89d61c2cbfaa..ed905a5401c3 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -71,7 +71,6 @@ config PCI_HOST_GENERIC
 
 config PCIE_XILINX
 	bool "Xilinx AXI PCIe host bridge support"
-	depends on ARCH_ZYNQ || MICROBLAZE
 	help
 	  Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
 	  Host Bridge driver.
-- 
2.7.4

Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions

[Paul Burton]

[-- Attachment #1: Type: text/plain, Size: 1840 bytes --]

Hi Guenter & all,

On Monday, 24 July 2017 01:39:37 BST Guenter Roeck wrote:
> The MIPS Boston board configuration tries to enable CONFIG_PCIE_XILINX.
> That doesn't work since PCIE_XILINX depends on ARCH_ZYNQ || MICROBLAZE.
> Remove that restriction.

I'd prefer that this patch does not go in standalone. The intent for the MIPS 
Boston board is that this driver is enabled for MIPS by this patch:

https://patchwork.kernel.org/patch/9794361/

But not until after earlier patches in that series fix issues with the driver:

https://patchwork.kernel.org/patch/9794355/
https://patchwork.kernel.org/patch/9794357/
https://patchwork.kernel.org/patch/9794359/

That has been held up by disagreement about whether the driver should be using 
0-3 or 1-4 for hardware IRQ numbers, sadly, despite the driver already being 
in tree & clearly broken, and my series not changing which the driver uses...

In any case, I don't really mind if people would rather remove the 
architecture restrictions than just add MIPS, but I'd prefer this doesn't go 
in until the rest of my series since without at least patch 1 of my seres this 
will lead to various WARN()s on Boston boards.

Thanks,
    Paul

> 
> Cc: Paul Burton <paul.burton@imgtec.com>
> Cc: James Hogan <james.hogan@imgtec.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/pci/host/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 89d61c2cbfaa..ed905a5401c3 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -71,7 +71,6 @@ config PCI_HOST_GENERIC
> 
>  config PCIE_XILINX
>  	bool "Xilinx AXI PCIe host bridge support"
> -	depends on ARCH_ZYNQ || MICROBLAZE
>  	help
>  	  Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
>  	  Host Bridge driver.


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions

[Guenter Roeck]

On 07/24/2017 03:49 AM, Paul Burton wrote:
> Hi Guenter & all,
> 
> On Monday, 24 July 2017 01:39:37 BST Guenter Roeck wrote:
>> The MIPS Boston board configuration tries to enable CONFIG_PCIE_XILINX.
>> That doesn't work since PCIE_XILINX depends on ARCH_ZYNQ || MICROBLAZE.
>> Remove that restriction.
> 
> I'd prefer that this patch does not go in standalone. The intent for the MIPS
> Boston board is that this driver is enabled for MIPS by this patch:
> 
> https://patchwork.kernel.org/patch/9794361/
> 
> But not until after earlier patches in that series fix issues with the driver:
> 
> https://patchwork.kernel.org/patch/9794355/
> https://patchwork.kernel.org/patch/9794357/
> https://patchwork.kernel.org/patch/9794359/
> 
> That has been held up by disagreement about whether the driver should be using
> 0-3 or 1-4 for hardware IRQ numbers, sadly, despite the driver already being
> in tree & clearly broken, and my series not changing which the driver uses...
> 
> In any case, I don't really mind if people would rather remove the
> architecture restrictions than just add MIPS, but I'd prefer this doesn't go
> in until the rest of my series since without at least patch 1 of my seres this
> will lead to various WARN()s on Boston boards.
> 

Not with qemu, at least not yet.

from the exchange, it doesn't look like that is going to be resolved anytime
soon.

Ok, I'll hold back with adding mips/boston to my qemu tests. Too bad.
Hope this is going to be resolved before qemu and/or platform support
for boston falls apart.

Makes me wonder - is there a way to add initrd support for the platform ?

Guenter

> Thanks,
>      Paul
> 
>>
>> Cc: Paul Burton <paul.burton@imgtec.com>
>> Cc: James Hogan <james.hogan@imgtec.com>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/pci/host/Kconfig | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index 89d61c2cbfaa..ed905a5401c3 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -71,7 +71,6 @@ config PCI_HOST_GENERIC
>>
>>   config PCIE_XILINX
>>   	bool "Xilinx AXI PCIe host bridge support"
>> -	depends on ARCH_ZYNQ || MICROBLAZE
>>   	help
>>   	  Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
>>   	  Host Bridge driver.
> 

Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions

[Bjorn Helgaas]

On Mon, Jul 24, 2017 at 11:49:22AM +0100, Paul Burton wrote:
> Hi Guenter & all,
> 
> On Monday, 24 July 2017 01:39:37 BST Guenter Roeck wrote:
> > The MIPS Boston board configuration tries to enable CONFIG_PCIE_XILINX.
> > That doesn't work since PCIE_XILINX depends on ARCH_ZYNQ || MICROBLAZE.
> > Remove that restriction.
> 
> I'd prefer that this patch does not go in standalone. The intent for the MIPS 
> Boston board is that this driver is enabled for MIPS by this patch:
> 
> https://patchwork.kernel.org/patch/9794361/
> 
> But not until after earlier patches in that series fix issues with the driver:
> 
> https://patchwork.kernel.org/patch/9794355/
> https://patchwork.kernel.org/patch/9794357/
> https://patchwork.kernel.org/patch/9794359/
> 
> That has been held up by disagreement about whether the driver should be using 
> 0-3 or 1-4 for hardware IRQ numbers, sadly, despite the driver already being 
> in tree & clearly broken, and my series not changing which the driver uses...

It's true that your v5 series only changes xilinx from using hwirq 0-3
to 0-4 (with 0 being unused in both cases, and the addition of 4
fixing the "INTD doesn't work" bug).

However, I *would* like to see this issue cleaned up consistently
across all our drivers.  I mooted a couple ideas in [1], but nobody
seemed interested.  If I merged your series as-is, there would be even
less interest.

[1] http://lkml.kernel.org/r/20170712221455.GJ14614@bhelgaas-glaptop.roam.corp.google.com

> In any case, I don't really mind if people would rather remove the 
> architecture restrictions than just add MIPS, but I'd prefer this doesn't go 
> in until the rest of my series since without at least patch 1 of my seres this 
> will lead to various WARN()s on Boston boards.
> 
> Thanks,
>     Paul
> 
> > 
> > Cc: Paul Burton <paul.burton@imgtec.com>
> > Cc: James Hogan <james.hogan@imgtec.com>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/pci/host/Kconfig | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> > index 89d61c2cbfaa..ed905a5401c3 100644
> > --- a/drivers/pci/host/Kconfig
> > +++ b/drivers/pci/host/Kconfig
> > @@ -71,7 +71,6 @@ config PCI_HOST_GENERIC
> > 
> >  config PCIE_XILINX
> >  	bool "Xilinx AXI PCIe host bridge support"
> > -	depends on ARCH_ZYNQ || MICROBLAZE
> >  	help
> >  	  Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
> >  	  Host Bridge driver.
> 

Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions

[Paul Burton]

[-- Attachment #1: Type: text/plain, Size: 3108 bytes --]

Hi Bjorn,

On Monday, 31 July 2017 15:58:22 PDT Bjorn Helgaas wrote:
> On Mon, Jul 24, 2017 at 11:49:22AM +0100, Paul Burton wrote:
> > Hi Guenter & all,
> > 
> > On Monday, 24 July 2017 01:39:37 BST Guenter Roeck wrote:
> > > The MIPS Boston board configuration tries to enable CONFIG_PCIE_XILINX.
> > > That doesn't work since PCIE_XILINX depends on ARCH_ZYNQ || MICROBLAZE.
> > > Remove that restriction.
> > 
> > I'd prefer that this patch does not go in standalone. The intent for the
> > MIPS Boston board is that this driver is enabled for MIPS by this patch:
> > 
> > https://patchwork.kernel.org/patch/9794361/
> > 
> > But not until after earlier patches in that series fix issues with the
> > driver:
> > 
> > https://patchwork.kernel.org/patch/9794355/
> > https://patchwork.kernel.org/patch/9794357/
> > https://patchwork.kernel.org/patch/9794359/
> > 
> > That has been held up by disagreement about whether the driver should be
> > using 0-3 or 1-4 for hardware IRQ numbers, sadly, despite the driver
> > already being in tree & clearly broken, and my series not changing which
> > the driver uses...
>
> It's true that your v5 series only changes xilinx from using hwirq 0-3
> to 0-4 (with 0 being unused in both cases, and the addition of 4
> fixing the "INTD doesn't work" bug).

That isn't true - the xilinx-pcie driver already uses 1-4, and my change 
simply prevents it from hitting a WARN() in the IRQ code when doing so.

> However, I *would* like to see this issue cleaned up consistently
> across all our drivers.  I mooted a couple ideas in [1], but nobody
> seemed interested.  If I merged your series as-is, there would be even
> less interest.

I've been travelling & haven't had time to look at any reworks as of yet, but 
I do think the driver as-is is clearly broken & my fix is a pretty obvious 
one, even if you would like the driver(s) to improve further in future.

Thanks,
    Paul

> [1]
> http://lkml.kernel.org/r/20170712221455.GJ14614@bhelgaas-glaptop.roam.corp.
> google.com
> > In any case, I don't really mind if people would rather remove the
> > architecture restrictions than just add MIPS, but I'd prefer this doesn't
> > go in until the rest of my series since without at least patch 1 of my
> > seres this will lead to various WARN()s on Boston boards.
> > 
> > Thanks,
> > 
> >     Paul
> > > 
> > > Cc: Paul Burton <paul.burton@imgtec.com>
> > > Cc: James Hogan <james.hogan@imgtec.com>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > > 
> > >  drivers/pci/host/Kconfig | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> > > index 89d61c2cbfaa..ed905a5401c3 100644
> > > --- a/drivers/pci/host/Kconfig
> > > +++ b/drivers/pci/host/Kconfig
> > > @@ -71,7 +71,6 @@ config PCI_HOST_GENERIC
> > > 
> > >  config PCIE_XILINX
> > >  
> > >  	bool "Xilinx AXI PCIe host bridge support"
> > > 
> > > -	depends on ARCH_ZYNQ || MICROBLAZE
> > > 
> > >  	help
> > >  	
> > >  	  Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
> > >  	  Host Bridge driver.


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions

[Bjorn Helgaas]

On Mon, Jul 31, 2017 at 04:19:13PM -0700, Paul Burton wrote:
> Hi Bjorn,
> 
> On Monday, 31 July 2017 15:58:22 PDT Bjorn Helgaas wrote:
> > On Mon, Jul 24, 2017 at 11:49:22AM +0100, Paul Burton wrote:
> > > Hi Guenter & all,
> > > 
> > > On Monday, 24 July 2017 01:39:37 BST Guenter Roeck wrote:
> > > > The MIPS Boston board configuration tries to enable CONFIG_PCIE_XILINX.
> > > > That doesn't work since PCIE_XILINX depends on ARCH_ZYNQ || MICROBLAZE.
> > > > Remove that restriction.
> > > 
> > > I'd prefer that this patch does not go in standalone. The intent for the
> > > MIPS Boston board is that this driver is enabled for MIPS by this patch:
> > > 
> > > https://patchwork.kernel.org/patch/9794361/
> > > 
> > > But not until after earlier patches in that series fix issues with the
> > > driver:
> > > 
> > > https://patchwork.kernel.org/patch/9794355/
> > > https://patchwork.kernel.org/patch/9794357/
> > > https://patchwork.kernel.org/patch/9794359/
> > > 
> > > That has been held up by disagreement about whether the driver should be
> > > using 0-3 or 1-4 for hardware IRQ numbers, sadly, despite the driver
> > > already being in tree & clearly broken, and my series not changing which
> > > the driver uses...
> >
> > It's true that your v5 series only changes xilinx from using hwirq 0-3
> > to 0-4 (with 0 being unused in both cases, and the addition of 4
> > fixing the "INTD doesn't work" bug).
> 
> That isn't true - the xilinx-pcie driver already uses 1-4, and my change 
> simply prevents it from hitting a WARN() in the IRQ code when doing so.

My apologies.  I was relying on the changelog, which says the current
code "creates an IRQ domain of size 4 (ie.  IRQ numbers 0 through 3)"
and the patch:

  -       port->leg_domain = irq_domain_add_linear(pcie_intc_node, 4,
  +       port->leg_domain = irq_domain_add_linear(pcie_intc_node, 1 + 4,

I'm not enough of an IRQ expert to understand why what I said was
incorrect (other than maybe INTD actually works, but emits a warning?)

> > However, I *would* like to see this issue cleaned up consistently
> > across all our drivers.  I mooted a couple ideas in [1], but nobody
> > seemed interested.  If I merged your series as-is, there would be even
> > less interest.
> 
> I've been travelling & haven't had time to look at any reworks as of yet, but 
> I do think the driver as-is is clearly broken & my fix is a pretty obvious 
> one, even if you would like the driver(s) to improve further in future.

My problem is that if all the drivers work because they use 5 numbers
(0-4), the issue will completely drop off everybody's radar.

> > [1]
> > http://lkml.kernel.org/r/20170712221455.GJ14614@bhelgaas-glaptop.roam.corp.
> > google.com
> > > In any case, I don't really mind if people would rather remove the
> > > architecture restrictions than just add MIPS, but I'd prefer this doesn't
> > > go in until the rest of my series since without at least patch 1 of my
> > > seres this will lead to various WARN()s on Boston boards.
> > > 
> > > Thanks,
> > > 
> > >     Paul
> > > > 
> > > > Cc: Paul Burton <paul.burton@imgtec.com>
> > > > Cc: James Hogan <james.hogan@imgtec.com>
> > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > ---
> > > > 
> > > >  drivers/pci/host/Kconfig | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> > > > index 89d61c2cbfaa..ed905a5401c3 100644
> > > > --- a/drivers/pci/host/Kconfig
> > > > +++ b/drivers/pci/host/Kconfig
> > > > @@ -71,7 +71,6 @@ config PCI_HOST_GENERIC
> > > > 
> > > >  config PCIE_XILINX
> > > >  
> > > >  	bool "Xilinx AXI PCIe host bridge support"
> > > > 
> > > > -	depends on ARCH_ZYNQ || MICROBLAZE
> > > > 
> > > >  	help
> > > >  	
> > > >  	  Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
> > > >  	  Host Bridge driver.
> 

Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions

[Paul Burton]

[-- Attachment #1: Type: text/plain, Size: 3759 bytes --]

On Monday, 31 July 2017 16:36:08 PDT Bjorn Helgaas wrote:
> On Mon, Jul 31, 2017 at 04:19:13PM -0700, Paul Burton wrote:
> > Hi Bjorn,
> > 
> > On Monday, 31 July 2017 15:58:22 PDT Bjorn Helgaas wrote:
> > > On Mon, Jul 24, 2017 at 11:49:22AM +0100, Paul Burton wrote:
> > > > Hi Guenter & all,
> > > > 
> > > > On Monday, 24 July 2017 01:39:37 BST Guenter Roeck wrote:
> > > > > The MIPS Boston board configuration tries to enable
> > > > > CONFIG_PCIE_XILINX.
> > > > > That doesn't work since PCIE_XILINX depends on ARCH_ZYNQ ||
> > > > > MICROBLAZE.
> > > > > Remove that restriction.
> > > > 
> > > > I'd prefer that this patch does not go in standalone. The intent for
> > > > the
> > > > MIPS Boston board is that this driver is enabled for MIPS by this
> > > > patch:
> > > > 
> > > > https://patchwork.kernel.org/patch/9794361/
> > > > 
> > > > But not until after earlier patches in that series fix issues with the
> > > > driver:
> > > > 
> > > > https://patchwork.kernel.org/patch/9794355/
> > > > https://patchwork.kernel.org/patch/9794357/
> > > > https://patchwork.kernel.org/patch/9794359/
> > > > 
> > > > That has been held up by disagreement about whether the driver should
> > > > be
> > > > using 0-3 or 1-4 for hardware IRQ numbers, sadly, despite the driver
> > > > already being in tree & clearly broken, and my series not changing
> > > > which
> > > > the driver uses...
> > > 
> > > It's true that your v5 series only changes xilinx from using hwirq 0-3
> > > to 0-4 (with 0 being unused in both cases, and the addition of 4
> > > fixing the "INTD doesn't work" bug).
> > 
> > That isn't true - the xilinx-pcie driver already uses 1-4, and my change
> > simply prevents it from hitting a WARN() in the IRQ code when doing so.
> 
> My apologies.  I was relying on the changelog, which says the current
> code "creates an IRQ domain of size 4 (ie.  IRQ numbers 0 through 3)"
> and the patch:
> 
>   -       port->leg_domain = irq_domain_add_linear(pcie_intc_node, 4,
>   +       port->leg_domain = irq_domain_add_linear(pcie_intc_node, 1 + 4,
> 
> I'm not enough of an IRQ expert to understand why what I said was
> incorrect (other than maybe INTD actually works, but emits a warning?)

The driver does create an IRQ domain of size 4, as though it is going to use 
numbering 0-3 with it. However the driver then goes on to use numbers 1-4, 
which leads to a warning from the IRQ code because the domain isn't big enough 
to cover the case where hwirq=4 (ie. INTD).

It still works because irq_domain_associate() ends up inserting a mapping for 
the IRQ into a radix tree rather than the linear_revmap array, but it's 
clearly wrong that the driver creates a domain of size 4 & then uses hwirq=4, 
hence the warnings.

> > > However, I *would* like to see this issue cleaned up consistently
> > > across all our drivers.  I mooted a couple ideas in [1], but nobody
> > > seemed interested.  If I merged your series as-is, there would be even
> > > less interest.
> > 
> > I've been travelling & haven't had time to look at any reworks as of yet,
> > but I do think the driver as-is is clearly broken & my fix is a pretty
> > obvious one, even if you would like the driver(s) to improve further in
> > future.
> 
> My problem is that if all the drivers work because they use 5 numbers
> (0-4), the issue will completely drop off everybody's radar.

I understand, and it's your call, but I'd argue that the driver as-is isn't 
just suboptimal but plain broken - and I think that fixing it so that it's 
"just" suboptimal is a worthwhile improvement that shouldn't be held up. But 
you're the maintainer, and if you'd like to use this to bribe me or someone 
else into improving things at some later date then so be it.

Thanks,
    Paul

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions

[Bjorn Helgaas]

On Mon, Jul 31, 2017 at 04:49:59PM -0700, Paul Burton wrote:
> On Monday, 31 July 2017 16:36:08 PDT Bjorn Helgaas wrote:
> > On Mon, Jul 31, 2017 at 04:19:13PM -0700, Paul Burton wrote:
> > > Hi Bjorn,
> > > 
> > > On Monday, 31 July 2017 15:58:22 PDT Bjorn Helgaas wrote:
> > > > On Mon, Jul 24, 2017 at 11:49:22AM +0100, Paul Burton wrote:
> > > > > Hi Guenter & all,
> > > > > 
> > > > > On Monday, 24 July 2017 01:39:37 BST Guenter Roeck wrote:
> > > > > > The MIPS Boston board configuration tries to enable
> > > > > > CONFIG_PCIE_XILINX.
> > > > > > That doesn't work since PCIE_XILINX depends on ARCH_ZYNQ ||
> > > > > > MICROBLAZE.
> > > > > > Remove that restriction.
> > > > > 
> > > > > I'd prefer that this patch does not go in standalone. The intent for
> > > > > the
> > > > > MIPS Boston board is that this driver is enabled for MIPS by this
> > > > > patch:
> > > > > 
> > > > > https://patchwork.kernel.org/patch/9794361/
> > > > > 
> > > > > But not until after earlier patches in that series fix issues with the
> > > > > driver:
> > > > > 
> > > > > https://patchwork.kernel.org/patch/9794355/
> > > > > https://patchwork.kernel.org/patch/9794357/
> > > > > https://patchwork.kernel.org/patch/9794359/
> > > > > 
> > > > > That has been held up by disagreement about whether the driver should
> > > > > be
> > > > > using 0-3 or 1-4 for hardware IRQ numbers, sadly, despite the driver
> > > > > already being in tree & clearly broken, and my series not changing
> > > > > which
> > > > > the driver uses...
> > > > 
> > > > It's true that your v5 series only changes xilinx from using hwirq 0-3
> > > > to 0-4 (with 0 being unused in both cases, and the addition of 4
> > > > fixing the "INTD doesn't work" bug).
> > > 
> > > That isn't true - the xilinx-pcie driver already uses 1-4, and my change
> > > simply prevents it from hitting a WARN() in the IRQ code when doing so.
> > 
> > My apologies.  I was relying on the changelog, which says the current
> > code "creates an IRQ domain of size 4 (ie.  IRQ numbers 0 through 3)"
> > and the patch:
> > 
> >   -       port->leg_domain = irq_domain_add_linear(pcie_intc_node, 4,
> >   +       port->leg_domain = irq_domain_add_linear(pcie_intc_node, 1 + 4,
> > 
> > I'm not enough of an IRQ expert to understand why what I said was
> > incorrect (other than maybe INTD actually works, but emits a warning?)
> 
> The driver does create an IRQ domain of size 4, as though it is going to use 
> numbering 0-3 with it. However the driver then goes on to use numbers 1-4, 
> which leads to a warning from the IRQ code because the domain isn't big enough 
> to cover the case where hwirq=4 (ie. INTD).
> 
> It still works because irq_domain_associate() ends up inserting a mapping for 
> the IRQ into a radix tree rather than the linear_revmap array, but it's 
> clearly wrong that the driver creates a domain of size 4 & then uses hwirq=4, 
> hence the warnings.

Does it really work?  It looks like irq_domain_associate() returns
-EINVAL after emitting the "error: hwirq 0x%x is too large for %s"
warning, so it doesn't look like it would call radix_tree_insert().

> > > > However, I *would* like to see this issue cleaned up consistently
> > > > across all our drivers.  I mooted a couple ideas in [1], but nobody
> > > > seemed interested.  If I merged your series as-is, there would be even
> > > > less interest.
> > > 
> > > I've been travelling & haven't had time to look at any reworks as of yet,
> > > but I do think the driver as-is is clearly broken & my fix is a pretty
> > > obvious one, even if you would like the driver(s) to improve further in
> > > future.
> > 
> > My problem is that if all the drivers work because they use 5 numbers
> > (0-4), the issue will completely drop off everybody's radar.
> 
> I understand, and it's your call, but I'd argue that the driver as-is isn't 
> just suboptimal but plain broken - and I think that fixing it so that it's 
> "just" suboptimal is a worthwhile improvement that shouldn't be held up. But 
> you're the maintainer, and if you'd like to use this to bribe me or someone 
> else into improving things at some later date then so be it.

This issue has been raised before.  Each time it comes up it takes me
a long time to re-figure out what's going on, and I'm sort of tired of
doing that.  Given that I have no budget or staff, my tools for
getting things fixed are pretty limited, so I'm going to hold out for
a more comprehensive fix here.

Bjorn

Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions

[Paul Burton]

[-- Attachment #1: Type: text/plain, Size: 5298 bytes --]

Hi Bjorn,

On Tuesday, 1 August 2017 14:29:31 PDT Bjorn Helgaas wrote:
> On Mon, Jul 31, 2017 at 04:49:59PM -0700, Paul Burton wrote:
> > On Monday, 31 July 2017 16:36:08 PDT Bjorn Helgaas wrote:
> > > On Mon, Jul 31, 2017 at 04:19:13PM -0700, Paul Burton wrote:
> > > > Hi Bjorn,
> > > > 
> > > > On Monday, 31 July 2017 15:58:22 PDT Bjorn Helgaas wrote:
> > > > > On Mon, Jul 24, 2017 at 11:49:22AM +0100, Paul Burton wrote:
> > > > > > Hi Guenter & all,
> > > > > > 
> > > > > > On Monday, 24 July 2017 01:39:37 BST Guenter Roeck wrote:
> > > > > > > The MIPS Boston board configuration tries to enable
> > > > > > > CONFIG_PCIE_XILINX.
> > > > > > > That doesn't work since PCIE_XILINX depends on ARCH_ZYNQ ||
> > > > > > > MICROBLAZE.
> > > > > > > Remove that restriction.
> > > > > > 
> > > > > > I'd prefer that this patch does not go in standalone. The intent
> > > > > > for
> > > > > > the
> > > > > > MIPS Boston board is that this driver is enabled for MIPS by this
> > > > > > patch:
> > > > > > 
> > > > > > https://patchwork.kernel.org/patch/9794361/
> > > > > > 
> > > > > > But not until after earlier patches in that series fix issues with
> > > > > > the
> > > > > > driver:
> > > > > > 
> > > > > > https://patchwork.kernel.org/patch/9794355/
> > > > > > https://patchwork.kernel.org/patch/9794357/
> > > > > > https://patchwork.kernel.org/patch/9794359/
> > > > > > 
> > > > > > That has been held up by disagreement about whether the driver
> > > > > > should
> > > > > > be
> > > > > > using 0-3 or 1-4 for hardware IRQ numbers, sadly, despite the
> > > > > > driver
> > > > > > already being in tree & clearly broken, and my series not changing
> > > > > > which
> > > > > > the driver uses...
> > > > > 
> > > > > It's true that your v5 series only changes xilinx from using hwirq
> > > > > 0-3
> > > > > to 0-4 (with 0 being unused in both cases, and the addition of 4
> > > > > fixing the "INTD doesn't work" bug).
> > > > 
> > > > That isn't true - the xilinx-pcie driver already uses 1-4, and my
> > > > change
> > > > simply prevents it from hitting a WARN() in the IRQ code when doing
> > > > so.
> > > 
> > > My apologies.  I was relying on the changelog, which says the current
> > > code "creates an IRQ domain of size 4 (ie.  IRQ numbers 0 through 3)"
> > > 
> > > and the patch:
> > >   -       port->leg_domain = irq_domain_add_linear(pcie_intc_node, 4,
> > >   +       port->leg_domain = irq_domain_add_linear(pcie_intc_node, 1 +
> > >   4,
> > > 
> > > I'm not enough of an IRQ expert to understand why what I said was
> > > incorrect (other than maybe INTD actually works, but emits a warning?)
> > 
> > The driver does create an IRQ domain of size 4, as though it is going to
> > use numbering 0-3 with it. However the driver then goes on to use numbers
> > 1-4, which leads to a warning from the IRQ code because the domain isn't
> > big enough to cover the case where hwirq=4 (ie. INTD).
> > 
> > It still works because irq_domain_associate() ends up inserting a mapping
> > for the IRQ into a radix tree rather than the linear_revmap array, but
> > it's clearly wrong that the driver creates a domain of size 4 & then uses
> > hwirq=4, hence the warnings.
> 
> Does it really work?  It looks like irq_domain_associate() returns
> -EINVAL after emitting the "error: hwirq 0x%x is too large for %s"
> warning, so it doesn't look like it would call radix_tree_insert().

True - the driver is more broken than I thought it was.

> > > > > However, I *would* like to see this issue cleaned up consistently
> > > > > across all our drivers.  I mooted a couple ideas in [1], but nobody
> > > > > seemed interested.  If I merged your series as-is, there would be
> > > > > even
> > > > > less interest.
> > > > 
> > > > I've been travelling & haven't had time to look at any reworks as of
> > > > yet,
> > > > but I do think the driver as-is is clearly broken & my fix is a pretty
> > > > obvious one, even if you would like the driver(s) to improve further
> > > > in
> > > > future.
> > > 
> > > My problem is that if all the drivers work because they use 5 numbers
> > > (0-4), the issue will completely drop off everybody's radar.
> > 
> > I understand, and it's your call, but I'd argue that the driver as-is
> > isn't
> > just suboptimal but plain broken - and I think that fixing it so that it's
> > "just" suboptimal is a worthwhile improvement that shouldn't be held up.
> > But you're the maintainer, and if you'd like to use this to bribe me or
> > someone else into improving things at some later date then so be it.
> 
> This issue has been raised before.  Each time it comes up it takes me
> a long time to re-figure out what's going on, and I'm sort of tired of
> doing that.  Given that I have no budget or staff, my tools for
> getting things fixed are pretty limited, so I'm going to hold out for
> a more comprehensive fix here.
> 
> Bjorn

You're not unique in any of that I'm afraid, and I find it sad that holding 
out for a subsystem-wide improvement which will save one single entry in an 
IRQ domain results in a driver remaining broken for the time being despite a 
fix existing...

I'll have a look at the suggestions you made when I get time, but I have other 
things on my plate so can't yet say when that will be.

Thanks,
    Paul

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]