Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700470 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions Bjorn Helgaas <helgaas@kernel.org> - 2017-08-01 01:00 +0200
    Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions Paul Burton <paul.burton@imgtec.com> - 2017-08-01 01:20 +0200
      Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions Bjorn Helgaas <helgaas@kernel.org> - 2017-08-01 01:40 +0200
        Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions Paul Burton <paul.burton@imgtec.com> - 2017-08-01 02:00 +0200
          Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions Bjorn Helgaas <helgaas@kernel.org> - 2017-08-01 23:30 +0200
            Re: [PATCH] PCI: xilinx: Remove platform/architecture restrictions Paul Burton <paul.burton@imgtec.com> - 2017-08-02 00:10 +0200

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

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.
> 

[toc] | [next] | [standalone]

#1700498

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1700504

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.
> 

[toc] | [prev] | [next] | [standalone]

#1700521

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1701518

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

[toc] | [prev] | [next] | [standalone]

#1701536

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web