diff mbox

[PATCHv7,07/13] irqdomain: add function to find a MSI irq_domain

Message ID 1375867954-2320-8-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Thomas Petazzoni Aug. 7, 2013, 9:32 a.m. UTC
Now that an irq_domain can be associated to a msi_chip structure, a
given PCIe driver will want to find this irq_domain, based on the
Device Tree node of the interrupt controller, as pointed by the
'msi-controller' DT property.

However, since on those platforms a single piece of hardware,
represented by a single DT node can provide both a "normal" IRQ domain
and a MSI-type IRQ domain, we need separate lookup functions to
distinguish them.

This patch makes irq_find_host() find only non-MSI-type IRQ domains,
and introduces irq_find_msi_host() to find only MSI-type IRQ
domains. It does so by factorizing the irq_find_host() logic into
__irq_find_host().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Tested-by: Daniel Price <daniel.price@gmail.com>
---
 include/linux/irqdomain.h | 21 ++++++++++++++++++++-
 kernel/irq/irqdomain.c    | 13 ++++++++++---
 2 files changed, 30 insertions(+), 4 deletions(-)

Comments

Benjamin Herrenschmidt Aug. 7, 2013, 8:50 p.m. UTC | #1
On Wed, 2013-08-07 at 11:32 +0200, Thomas Petazzoni wrote:
> Now that an irq_domain can be associated to a msi_chip structure, a
> given PCIe driver will want to find this irq_domain, based on the
> Device Tree node of the interrupt controller, as pointed by the
> 'msi-controller' DT property.

I still don't quite understand why you have to do all that.

> However, since on those platforms a single piece of hardware,
> represented by a single DT node can provide both a "normal" IRQ domain
> and a MSI-type IRQ domain, we need separate lookup functions to
> distinguish them.

At least on power we have cases where an mpic does both MSIs and LSIs,
we have the XICS that happily mixes both in a single large domain,
etc... and never needed any of that.

I don't quite understand what problem that stuff is trying to solve
really. Are you trying to avoid having an added MSI bitmap allocator for
the MSI side of the PIC and use the irq domain stuff both as your virq
and your hwirq allocator ?

> This patch makes irq_find_host() find only non-MSI-type IRQ domains,
> and introduces irq_find_msi_host() to find only MSI-type IRQ
> domains. It does so by factorizing the irq_find_host() logic into
> __irq_find_host().


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni Aug. 7, 2013, 10:04 p.m. UTC | #2
Dear Benjamin Herrenschmidt,

On Thu, 08 Aug 2013 06:50:20 +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2013-08-07 at 11:32 +0200, Thomas Petazzoni wrote:
> > Now that an irq_domain can be associated to a msi_chip structure, a
> > given PCIe driver will want to find this irq_domain, based on the
> > Device Tree node of the interrupt controller, as pointed by the
> > 'msi-controller' DT property.
> 
> I still don't quite understand why you have to do all that.
> 
> > However, since on those platforms a single piece of hardware,
> > represented by a single DT node can provide both a "normal" IRQ domain
> > and a MSI-type IRQ domain, we need separate lookup functions to
> > distinguish them.
> 
> At least on power we have cases where an mpic does both MSIs and LSIs,
> we have the XICS that happily mixes both in a single large domain,
> etc... and never needed any of that.
> 
> I don't quite understand what problem that stuff is trying to solve
> really. Are you trying to avoid having an added MSI bitmap allocator for
> the MSI side of the PIC and use the irq domain stuff both as your virq
> and your hwirq allocator ?

Yes. Originally, the allocator was in the driver, but Grant Likely
suggested that it should be done by the irqdomain code itself. If
needed, I can dig the link, but I already gave you the pointers to each
iteration of this patch series, and the comment from Grant is
definitely part of one of those threads.

Basically, the PCIe driver in drivers/pci/host/pci-mvebu.c needs to
give the Linux PCI core a pointer to a msi_chip, which contains
operations to setup/teardown an MSI irq.

On Marvell platforms, it's directly the main interrupt controller that
provides the MSI mechanism. So the msi_chip is created and handled by
the drivers/irqchip/irq-armada-370-xp.c driver. It implements the
->setup_irq() and ->teardown_irq() operations of the msi_chip
structure, using irq_domain functions, as Grant suggested that the
irq_domain infrastructure should be used instead of having another
bitmap allocator in each MSI driver.

Then, we need to "connect" the PCI driver and the IRQ driver. Arnd
Bergmann suggested that the DT should look like this:

	mpic {
		interrupt-controller;
		msi-controller;
		...
	};

	pcie-controller {
		msi-parent = <&mpic>;
		...
	};

So, the pcie-controller needs to be able, from a DT node pointer to
"mpic" to get the corresponding msi_chip. The way to do this has gone
through different steps over the different iteration of the patch
series:

 (1) A small registry in drivers/pci that associates a device_node*
     with a msi_chip*. The IRQ driver registers the msi_chip* with the
     corresponding device_node*, and the PCI driver lookups the right
     msi_chip* using the device_node* pointer by the 'msi-parent'
     property.

     Bjorn Helgaas disliked that and suggested that the registry should
     be in drivers/of/

 (2) The same registry was moved to drivers/of/, with the same
     principle, except that the functions were renamed to match the
     conventions of drivers/of/.

     Rob Herring (drivers/of maintainer) ACKed this approach. However,
     later, Grant Likely came in the discussion and NAKed this
     approach, and suggested instead that the irq_domain should be
     associated with the msi_chip directly.

 (3) The current approach, where irq_domain contains a pointer to the
     msi_chip. So the PCI driver can look up an irq_domain using the
     device_node* pointer to by its 'msi-parent' property. However,
     there are *two* irq_domain that are related to the same 'mpic'
     node: the irq_domain for normal interrupts, and the irq_domain for
     MSI interrupts. And they should not be confused together. This is
     what this patch does: it makes sure that irq_find_host() matches
     only "normal irq_domain", while irq_find_msi_host() matches only
     "MSI irq_domain".

Again, this has been discussed at lengths in the previous iterations,
for which I already gave you all the links, as you requested in a
private e-mail. It'd be great if this discussion was read seriously,
because I really have the feeling we are restarting from zero on this
whole MSI thing...

Thanks,

Thomas Petazzoni
Benjamin Herrenschmidt Aug. 7, 2013, 10:31 p.m. UTC | #3
On Thu, 2013-08-08 at 00:04 +0200, Thomas Petazzoni wrote:
> Again, this has been discussed at lengths in the previous iterations,
> for which I already gave you all the links, as you requested in a
> private e-mail. It'd be great if this discussion was read seriously,
> because I really have the feeling we are restarting from zero on this
> whole MSI thing...

Well, two things here:

 - You don't need my ack since I am not the maintainer of the irqdomain
code anymore, Grant is :-)

 - I still don't like it. I find that it's looking more and more like
over engineering. I don't like having any kind of infrastructure
relationship between MSI stuff and irqdomain, ie, a PCI/PCIe specific
construct and a generic interrupt remapper.

Trying to use irqdomain for HW number allocation seems to be pushing it
where it wasn't designed to go. Are those interrupts really different
domains ? Do they have separate number spaces, separate DT encodings and
overall characteristics ?

What's wrong with the bitmap allocator in the PIC driver ? It's simple,
and does the job just fine. If anything, take it from powerpc and sparc
and move it to generic. It's already a "generic" (ie shared)
infrastructure in powerpc.

Let's ask somebody of well known taste ... Thomas ! :-) (Yes, you tglx,
I know you are lurking ...). What do you reckon ?

That series makes me feel nervous, it feels like a hack. I really don't
like creating that relationship between msi_chip and irqdomain. In fact,
I think it makes it harder to understand what's happening in the code
and following things.

It's a LOT clearer to me to have an irq domain for the PIC and an
explicit bitmap allocation for MSIs, I see where things come from, I can
follow the code path etc... much more easily.

I suspect we have a case of over-abstracting happening here. This is a
dangerous illness and can be contagious :-)

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt Aug. 7, 2013, 10:42 p.m. UTC | #4
On Thu, 2013-08-08 at 08:31 +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2013-08-08 at 00:04 +0200, Thomas Petazzoni wrote:
> > Again, this has been discussed at lengths in the previous iterations,
> > for which I already gave you all the links, as you requested in a
> > private e-mail. It'd be great if this discussion was read seriously,
> > because I really have the feeling we are restarting from zero on this
> > whole MSI thing...
> 
> Well, two things here:
> 
>  - You don't need my ack since I am not the maintainer of the irqdomain
> code anymore, Grant is :-)

Hrm, I'm being told Grant isn't anymore... I can step in and take it all
back but you might not like the result ....

Ben.

>  - I still don't like it. I find that it's looking more and more like
> over engineering. I don't like having any kind of infrastructure
> relationship between MSI stuff and irqdomain, ie, a PCI/PCIe specific
> construct and a generic interrupt remapper.
> 
> Trying to use irqdomain for HW number allocation seems to be pushing it
> where it wasn't designed to go. Are those interrupts really different
> domains ? Do they have separate number spaces, separate DT encodings and
> overall characteristics ?
> 
> What's wrong with the bitmap allocator in the PIC driver ? It's simple,
> and does the job just fine. If anything, take it from powerpc and sparc
> and move it to generic. It's already a "generic" (ie shared)
> infrastructure in powerpc.
> 
> Let's ask somebody of well known taste ... Thomas ! :-) (Yes, you tglx,
> I know you are lurking ...). What do you reckon ?
> 
> That series makes me feel nervous, it feels like a hack. I really don't
> like creating that relationship between msi_chip and irqdomain. In fact,
> I think it makes it harder to understand what's happening in the code
> and following things.
> 
> It's a LOT clearer to me to have an irq domain for the PIC and an
> explicit bitmap allocation for MSIs, I see where things come from, I can
> follow the code path etc... much more easily.
> 
> I suspect we have a case of over-abstracting happening here. This is a
> dangerous illness and can be contagious :-)
> 
> Cheers,
> Ben.
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt Aug. 7, 2013, 10:45 p.m. UTC | #5
On Thu, 2013-08-08 at 08:42 +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2013-08-08 at 08:31 +1000, Benjamin Herrenschmidt wrote:
> > On Thu, 2013-08-08 at 00:04 +0200, Thomas Petazzoni wrote:
> > > Again, this has been discussed at lengths in the previous iterations,
> > > for which I already gave you all the links, as you requested in a
> > > private e-mail. It'd be great if this discussion was read seriously,
> > > because I really have the feeling we are restarting from zero on this
> > > whole MSI thing...
> > 
> > Well, two things here:
> > 
> >  - You don't need my ack since I am not the maintainer of the irqdomain
> > code anymore, Grant is :-)
> 
> Hrm, I'm being told Grant isn't anymore... I can step in and take it all
> back but you might not like the result ....

Oh but it looks like MAINTAINERS says I am :-)

Ok so from that perspective, I don't like it at all. You can try to
convince me otherwise but I don't think we need to introduce a
dependency to something like msi into the core remapper. It's already to
complex.

Why don't you move the powerpc bitmap allocator over to a generic
place ? I feel like it would be actually simpler but feel free to prove
me wrong.

Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni Aug. 8, 2013, 8:16 a.m. UTC | #6
Dear Benjamin Herrenschmidt,

On Thu, 08 Aug 2013 08:31:05 +1000, Benjamin Herrenschmidt wrote:

>  - I still don't like it. I find that it's looking more and more like
> over engineering. I don't like having any kind of infrastructure
> relationship between MSI stuff and irqdomain, ie, a PCI/PCIe specific
> construct and a generic interrupt remapper.

This is *exactly* the opposite of what Grant Likely said in:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/187083.html
[PATCHv5 05/11] of: pci: add registry of MSI chips

Grant said:

"""
Actually, I'm going to disagree on this one and say NAK. I don't think
it is a good idea to create a completely separate registry of msi_chips
for binding to dt nodes. I think it would be better to include the
msi_chip pointer directly into the irq_domain which has to be there
anyway. It then becomes another feature for irq controllers if it can
support doing MSI.
"""

So Grant is completely in favor of a strong relationship between MSI
stuff and irqdomain.

> Trying to use irqdomain for HW number allocation seems to be pushing it
> where it wasn't designed to go. Are those interrupts really different
> domains ? Do they have separate number spaces, separate DT encodings and
> overall characteristics ?

This is also *exactly* the opoosite of what Grant Likely said in:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/175430.html
[PATCH v2 4/8] irqchip: armada-370-xp: implement MSI support

He was replying to the patch that did a bitmap based allocation scheme,
within the IRQ controller driver, for MSI interrupts. And Grant said:

"""
This looks like something that the irq_domain should handle for you.
It would be good to have a variant of irq_create_mapping() that keeps
track of available hwirqs, allocates a free one, and maps it all with
one function call. I can see that being useful by other MSI interrupt
controllers and would eliminate needing to open-code it above.
"""

> What's wrong with the bitmap allocator in the PIC driver ? It's simple,
> and does the job just fine. If anything, take it from powerpc and sparc
> and move it to generic. It's already a "generic" (ie shared)
> infrastructure in powerpc.

See above. Grant said to *NOT* implement a bitmap allocator. He even
said it would be useful for other MSI interrupt controllers, and that
ultimately they should be migrated to not use the bitmap allocator that
you're talking about, but instead rely on irqdomain based allocations.

> That series makes me feel nervous, it feels like a hack. I really don't
> like creating that relationship between msi_chip and irqdomain. In fact,
> I think it makes it harder to understand what's happening in the code
> and following things.

Please see above, you're going completely backwards to what Grant
Likely was saying.

Best regards,

Thomas
Thomas Petazzoni Aug. 8, 2013, 8:22 a.m. UTC | #7
Dear Benjamin Herrenschmidt,

On Thu, 08 Aug 2013 08:45:36 +1000, Benjamin Herrenschmidt wrote:

> > >  - You don't need my ack since I am not the maintainer of the irqdomain
> > > code anymore, Grant is :-)
> > 
> > Hrm, I'm being told Grant isn't anymore... I can step in and take it all
> > back but you might not like the result ....
> 
> Oh but it looks like MAINTAINERS says I am :-)
> 
> Ok so from that perspective, I don't like it at all. You can try to
> convince me otherwise but I don't think we need to introduce a
> dependency to something like msi into the core remapper. It's already to
> complex.
> 
> Why don't you move the powerpc bitmap allocator over to a generic
> place ? I feel like it would be actually simpler but feel free to prove
> me wrong.

I'm sorry, but I'm not buying this. There must be some continuity when
the maintenance of one subsystem transitions from one maintainer to
another. I'm perfectly ok with accepting some hick-ups, but not radical
changes in design decisions.

What you're asking me to do is to go completely backwards compared to
the comments and review Grant made. The irqdomain-based allocator was
suggested by Grant (see my previous e-mail, or Grant reply at
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/175430.html)
and was even Acked-by Grant in
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/187082.html.

Note that this patch set has been posted at the following dates:

 * PATCH version 7 sent on August, 7th 2013
 * PATCH version 6 sent on August, 1st 2013
 * PATCH version 5 sent on July, 15th 2013
 * PATCH version 4 sent on July, 1st 2013
 * PATCH version 3 sent on June, 19th 2013
 * PATCH version 2 sent on June, 6th 2013
 * RFC version 1 sent on March, 26th 2013

So it has been around since 4 months, I've taken into account all the
comments from the various maintainers who were involved, and especially
the comments from Grant. You cannot ask me now, as we are approaching
the next merge window for which this code is intended, to take
completely opposite design choices than what the previous irqdomain
maintainer was suggesting.

Could you contact Grant and align with him on those design decisions?
It would also be good if you could read the past discussions on this
patch set, because all what you're pointing at has already been
discussed at length, as I pointed out in my previous e-mail.

Thanks,

Thomas
Benjamin Herrenschmidt Aug. 8, 2013, 8:38 a.m. UTC | #8
On Thu, 2013-08-08 at 10:16 +0200, Thomas Petazzoni wrote:

> This is *exactly* the opposite of what Grant Likely said in:

Grant and I tend to disagree from time to time :-)

> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/187083.html
> [PATCHv5 05/11] of: pci: add registry of MSI chips
> 
> Grant said:
> 
> """
> Actually, I'm going to disagree on this one and say NAK. I don't think
> it is a good idea to create a completely separate registry of msi_chips
> for binding to dt nodes. I think it would be better to include the
> msi_chip pointer directly into the irq_domain which has to be there
> anyway. It then becomes another feature for irq controllers if it can
> support doing MSI.
> """
> 
> So Grant is completely in favor of a strong relationship between MSI
> stuff and irqdomain.

And I disagree. If you consider msi_chip purely as a mechanism for
binding dt-nodes, then he *might* have some kind of argument but I think
that's the wrong approach.

msi-chip looks like the right low level abstraction for providing
different hooks for different MSI HW implementations that may exist in
an architecture, and in fact as such it's welcome. We currently have
arch specific function pointers we could then easily replace.

However this has always been my cardinal rule with the DT stuff from the
ground up on powerpc: Such a mechanism must be independent from and
orthogonal to the device-tree.

For example the generic irq_chip is orthogonal to irq remapping via
irq_domain. It's possible to instanciate irq_chips without device nodes,
and with a completely different firmware representation (ACPI ?). It
should be the same with msi-chip.

> > Trying to use irqdomain for HW number allocation seems to be pushing it
> > where it wasn't designed to go. Are those interrupts really different
> > domains ? Do they have separate number spaces, separate DT encodings and
> > overall characteristics ?
> 
> This is also *exactly* the opoosite of what Grant Likely said in:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/175430.html
> [PATCH v2 4/8] irqchip: armada-370-xp: implement MSI support
> 
> He was replying to the patch that did a bitmap based allocation scheme,
> within the IRQ controller driver, for MSI interrupts. And Grant said:
>
> """
> This looks like something that the irq_domain should handle for you.
> It would be good to have a variant of irq_create_mapping() that keeps
> track of available hwirqs, allocates a free one, and maps it all with
> one function call. I can see that being useful by other MSI interrupt
> controllers and would eliminate needing to open-code it above.
> """

Now, having some kind of HW allocator within the irq domain is actually
not completely idiotic I suppose since it somewhat keeps track of the HW
interrupts, but it's pushing beyond its original design, and even if
that was to be something we would allow, I would *still* keep it
separate from msi-chip.

A given msi-chip implementation may use that irq-domain provided
facility if it exist, indeed, but I don't want a strong tie between the
two.

> > What's wrong with the bitmap allocator in the PIC driver ? It's simple,
> > and does the job just fine. If anything, take it from powerpc and sparc
> > and move it to generic. It's already a "generic" (ie shared)
> > infrastructure in powerpc.
> 
> See above. Grant said to *NOT* implement a bitmap allocator. He even
> said it would be useful for other MSI interrupt controllers, and that
> ultimately they should be migrated to not use the bitmap allocator that
> you're talking about, but instead rely on irqdomain based allocations.

And that ends up into a bloody cathedral instead of nicely separated and
individually useful small components. I actually disagree with Grant
pretty strongly on that.

> > That series makes me feel nervous, it feels like a hack. I really don't
> > like creating that relationship between msi_chip and irqdomain. In fact,
> > I think it makes it harder to understand what's happening in the code
> > and following things.
> 
> Please see above, you're going completely backwards to what Grant
> Likely was saying.

Or Grant is going completely backward from what I've always wanted that
stuff to be... I've been too busy to monitor closely and frankly, I'm
also too busy to actively do so now either.

I suggest we bring in the opinion of Thomas Gleixner. I trust his sense
of taste and will bow if he says that I'm full of shit.

Cheers,
Ben.

> Best regards,
> 
> Thomas


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt Aug. 8, 2013, 8:41 a.m. UTC | #9
On Thu, 2013-08-08 at 10:22 +0200, Thomas Petazzoni wrote:
> Dear Benjamin Herrenschmidt,

> I'm sorry, but I'm not buying this. There must be some continuity when
> the maintenance of one subsystem transitions from one maintainer to
> another. I'm perfectly ok with accepting some hick-ups, but not radical
> changes in design decisions.

Well, I wrote it in the first place :-)

> What you're asking me to do is to go completely backwards compared to
> the comments and review Grant made. The irqdomain-based allocator was
> suggested by Grant (see my previous e-mail, or Grant reply at
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/175430.html)
> and was even Acked-by Grant in
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/187082.html.
> 
> Note that this patch set has been posted at the following dates:
> 
>  * PATCH version 7 sent on August, 7th 2013
>  * PATCH version 6 sent on August, 1st 2013
>  * PATCH version 5 sent on July, 15th 2013
>  * PATCH version 4 sent on July, 1st 2013
>  * PATCH version 3 sent on June, 19th 2013
>  * PATCH version 2 sent on June, 6th 2013
>  * RFC version 1 sent on March, 26th 2013

I'm really sorry and I feel your pain. I have not actively been
monitoring any of that stuff, and you might have gotten away without
CC'ing me or asking for my point of view but you did (and I thank you
for that), and sadly this is my opinion.

> So it has been around since 4 months, I've taken into account all the
> comments from the various maintainers who were involved, and especially
> the comments from Grant. You cannot ask me now, as we are approaching
> the next merge window for which this code is intended, to take
> completely opposite design choices than what the previous irqdomain
> maintainer was suggesting.

I can and I do. However, I also leave the opportunity of bringing in a
third party into the debate with a well known track record to overrule
me if he thinks I'm being unnecessarily obstructive.

> Could you contact Grant and align with him on those design decisions?
> It would also be good if you could read the past discussions on this
> patch set, because all what you're pointing at has already been
> discussed at length, as I pointed out in my previous e-mail.

I can try... Grant, are you around ? (I've added you to the CC list), we
might be able to catch up on IRC and discuss it ...

Cheers,
Ben.


> Thanks,
> 
> Thomas


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt Aug. 8, 2013, 8:54 a.m. UTC | #10
On Thu, 2013-08-08 at 18:38 +1000, Benjamin Herrenschmidt wrote:
> For example the generic irq_chip is orthogonal to irq remapping via
> irq_domain. It's possible to instanciate irq_chips without device nodes,
> and with a completely different firmware representation (ACPI ?). It
> should be the same with msi-chip.

In fact, to a large extent, the original irq_domain was also orthogonal
to the device-tree ...

I did add the ability to match a device-node with an irq domain but that
has always been just an optional addition, it was possible (and should
still be though I haven't looked in a while) to create irq domains
completely independently of the device-tree.

Now there is one thing that might sway me ... if you can show me (sorry
don't have the bandwidth to look in details and scrutinize the patch)
that overall, having the msi_chip in the domain as an optional facility
does indeed overall make the code *much* smaller than keeping them
separate, and for more than just your use case.

One reason I don't like the allocator being in irq domain is that it
really only is useful for a subset of the different types of domains
around.

For example, on power server, I have a unique domain accross the fabric
(irqs are special powerbus messages that are encoded in a 24 bit number),
but each "source" (a PCI host bridge for example) gets a subset of that
domain, typically a fixed range.

So your allocator would only be useful to that case if:

  - It can be used to allocate within specific boundaries

  - It works with radix based domains

This is just an example... I don't like bolting a facility (allocation)
in a lawyer originally designed to do something else (mapping) unless
that facility is directly useful to the vast majority of the users of the
layer in question.

In fact, there is an argument to be made to provide a generic bitmap
allocator specialized for MSIs. MSIs have quirks ... alignment constraints
for multiple MSI-non-X for example, which might potentially benefit in
having an allocator with some smarts to limit fragmentation. That sort
of things....

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index b0504ff..fc669b4 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -129,7 +129,8 @@  struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
 					 irq_hw_number_t first_hwirq,
 					 const struct irq_domain_ops *ops,
 					 void *host_data);
-extern struct irq_domain *irq_find_host(struct device_node *node);
+struct irq_domain *__irq_find_host(struct device_node *node,
+				   bool findmsi);
 extern void irq_set_default_host(struct irq_domain *host);
 
 /**
@@ -196,6 +197,24 @@  static inline struct irq_domain *irq_domain_add_msi(struct device_node *of_node,
 }
 
 
+/**
+ * irq_find_host() - Locates a domain for a given device node
+ * @node: device-tree node of the interrupt controller
+ */
+static inline struct irq_domain *irq_find_host(struct device_node *node)
+{
+	return __irq_find_host(node, false);
+}
+
+/**
+ * irq_find_msi_host() - Locates a MSI domain for a given device node
+ * @node: device-tree node of the interrupt controller
+ */
+static inline struct irq_domain *irq_find_msi_host(struct device_node *node)
+{
+	return __irq_find_host(node, true);
+}
+
 extern void irq_domain_remove(struct irq_domain *host);
 
 extern int irq_domain_associate(struct irq_domain *domain, unsigned int irq,
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 8d02af7..6d066e2 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -197,10 +197,14 @@  struct irq_domain *irq_domain_add_legacy(struct device_node *of_node,
 EXPORT_SYMBOL_GPL(irq_domain_add_legacy);
 
 /**
- * irq_find_host() - Locates a domain for a given device node
+ * __irq_find_host() - Locates a domain for a given device node,
+ * taking into account whether the domain is of MSI-type or not.
  * @node: device-tree node of the interrupt controller
+ * @findmsi: true when the domain being search is of MSI-type, false
+ * otherwise.
  */
-struct irq_domain *irq_find_host(struct device_node *node)
+struct irq_domain *__irq_find_host(struct device_node *node,
+				   bool findmsi)
 {
 	struct irq_domain *h, *found = NULL;
 	int rc;
@@ -212,6 +216,9 @@  struct irq_domain *irq_find_host(struct device_node *node)
 	 */
 	mutex_lock(&irq_domain_mutex);
 	list_for_each_entry(h, &irq_domain_list, link) {
+		if ((findmsi && !h->msi_chip) ||
+		    (!findmsi && h->msi_chip))
+			continue;
 		if (h->ops->match)
 			rc = h->ops->match(h, node);
 		else
@@ -225,7 +232,7 @@  struct irq_domain *irq_find_host(struct device_node *node)
 	mutex_unlock(&irq_domain_mutex);
 	return found;
 }
-EXPORT_SYMBOL_GPL(irq_find_host);
+EXPORT_SYMBOL_GPL(__irq_find_host);
 
 /**
  * irq_set_default_host() - Set a "default" irq domain