From: Linus Walleij <[email protected]>
Currently we rely on all IRQ chip instances to dynamically
allocate their IRQ descriptors unless they use the linear
IRQ domain. So for irqdomain_add_legacy() and
irqdomain_add_simple() the caller need to make sure that
descriptors are allocated.
Let's slightly augment the yet unused irqdomain_add_simple()
to also allocate descriptors as a means to simplify usage
and avoid code duplication throughout the kernel.
We warn if descriptors cannot be allocated, e.g. if a
platform has the bad habit of hogging descriptors at boot
time.
Cc: Rob Herring <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Paul Mundt <[email protected]>
Cc: Russell King <[email protected]>
Cc: Lee Jones <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
---
ChangeLog v1->v2:
- Switch descriptor allocation on IS_ENABLED(CONFIG_SPARSE_IRQ)
so it won't attempt to allocate descriptors in the non-sparse
case.
- Use of_node_to_nid() to make sure we work on platforms with their
own node concept.
- Specify irq_alloc_descs(first_irq, first_irq ...) to emulate
irq_alloc_desc_at().
---
kernel/irq/irqdomain.c | 33 ++++++++++++++++++++++++++++-----
1 file changed, 28 insertions(+), 5 deletions(-)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 49a7772..4e69e24 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -148,7 +148,8 @@ static unsigned int irq_domain_legacy_revmap(struct irq_domain *domain,
* @host_data: Controller private data pointer
*
* Allocates a legacy irq_domain if irq_base is positive or a linear
- * domain otherwise.
+ * domain otherwise. For the legacy domain, IRQ descriptors will also
+ * be allocated.
*
* This is intended to implement the expected behaviour for most
* interrupt controllers which is that a linear mapping should
@@ -162,11 +163,33 @@ struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
const struct irq_domain_ops *ops,
void *host_data)
{
- if (first_irq > 0)
- return irq_domain_add_legacy(of_node, size, first_irq, 0,
+ if (first_irq > 0) {
+ int irq_base;
+
+ if (IS_ENABLED(CONFIG_SPARSE_IRQ)) {
+ /*
+ * Set the descriptor allocator to search for a
+ * 1-to-1 mapping, such as irq_alloc_desc_at().
+ * Use of_node_to_nid() which is defined to
+ * numa_node_id() on platforms that have no custom
+ * implementation.
+ */
+ irq_base = irq_alloc_descs(first_irq, first_irq, size,
+ of_node_to_nid(of_node));
+ if (irq_base < 0) {
+ WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
+ first_irq);
+ irq_base = first_irq;
+ }
+ } else
+ irq_base = first_irq;
+
+ return irq_domain_add_legacy(of_node, size, irq_base, 0,
ops, host_data);
- else
- return irq_domain_add_linear(of_node, size, ops, host_data);
+ }
+
+ /* A linear domain is the default */
+ return irq_domain_add_linear(of_node, size, ops, host_data);
}
/**
--
1.7.11.3
On 10/01/2012 02:35 AM, Linus Walleij wrote:
> From: Linus Walleij <[email protected]>
>
> Currently we rely on all IRQ chip instances to dynamically
> allocate their IRQ descriptors unless they use the linear
> IRQ domain. So for irqdomain_add_legacy() and
> irqdomain_add_simple() the caller need to make sure that
> descriptors are allocated.
>
> Let's slightly augment the yet unused irqdomain_add_simple()
> to also allocate descriptors as a means to simplify usage
> and avoid code duplication throughout the kernel.
>
> We warn if descriptors cannot be allocated, e.g. if a
> platform has the bad habit of hogging descriptors at boot
> time.
>
> Cc: Rob Herring <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Paul Mundt <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Lee Jones <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
Looks good.
Reviewed-by: Rob Herring <[email protected]>
> ---
> ChangeLog v1->v2:
> - Switch descriptor allocation on IS_ENABLED(CONFIG_SPARSE_IRQ)
> so it won't attempt to allocate descriptors in the non-sparse
> case.
> - Use of_node_to_nid() to make sure we work on platforms with their
> own node concept.
> - Specify irq_alloc_descs(first_irq, first_irq ...) to emulate
> irq_alloc_desc_at().
> ---
> kernel/irq/irqdomain.c | 33 ++++++++++++++++++++++++++++-----
> 1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 49a7772..4e69e24 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -148,7 +148,8 @@ static unsigned int irq_domain_legacy_revmap(struct irq_domain *domain,
> * @host_data: Controller private data pointer
> *
> * Allocates a legacy irq_domain if irq_base is positive or a linear
> - * domain otherwise.
> + * domain otherwise. For the legacy domain, IRQ descriptors will also
> + * be allocated.
> *
> * This is intended to implement the expected behaviour for most
> * interrupt controllers which is that a linear mapping should
> @@ -162,11 +163,33 @@ struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
> const struct irq_domain_ops *ops,
> void *host_data)
> {
> - if (first_irq > 0)
> - return irq_domain_add_legacy(of_node, size, first_irq, 0,
> + if (first_irq > 0) {
> + int irq_base;
> +
> + if (IS_ENABLED(CONFIG_SPARSE_IRQ)) {
> + /*
> + * Set the descriptor allocator to search for a
> + * 1-to-1 mapping, such as irq_alloc_desc_at().
> + * Use of_node_to_nid() which is defined to
> + * numa_node_id() on platforms that have no custom
> + * implementation.
> + */
> + irq_base = irq_alloc_descs(first_irq, first_irq, size,
> + of_node_to_nid(of_node));
> + if (irq_base < 0) {
> + WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
> + first_irq);
> + irq_base = first_irq;
> + }
> + } else
> + irq_base = first_irq;
> +
> + return irq_domain_add_legacy(of_node, size, irq_base, 0,
> ops, host_data);
> - else
> - return irq_domain_add_linear(of_node, size, ops, host_data);
> + }
> +
> + /* A linear domain is the default */
> + return irq_domain_add_linear(of_node, size, ops, host_data);
> }
>
> /**
>
On Mon, Oct 1, 2012 at 2:11 PM, Rob Herring <[email protected]> wrote:
> On 10/01/2012 02:35 AM, Linus Walleij wrote:
>> From: Linus Walleij <[email protected]>
>>
>> Currently we rely on all IRQ chip instances to dynamically
>> allocate their IRQ descriptors unless they use the linear
>> IRQ domain. So for irqdomain_add_legacy() and
>> irqdomain_add_simple() the caller need to make sure that
>> descriptors are allocated.
>>
>> Let's slightly augment the yet unused irqdomain_add_simple()
>> to also allocate descriptors as a means to simplify usage
>> and avoid code duplication throughout the kernel.
>>
>> We warn if descriptors cannot be allocated, e.g. if a
>> platform has the bad habit of hogging descriptors at boot
>> time.
>>
>> Cc: Rob Herring <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Grant Likely <[email protected]>
>> Cc: Paul Mundt <[email protected]>
>> Cc: Russell King <[email protected]>
>> Cc: Lee Jones <[email protected]>
>> Signed-off-by: Linus Walleij <[email protected]>
>
> Looks good.
>
> Reviewed-by: Rob Herring <[email protected]>
So what do we do with this patch? Grant?
I think the change is good to get in ASAP and since I
now have one patch in pinctrl depending on it I have
tentatively applied it there.
Nobody sent any irqdomain fixes for this merge
window, maybe this is the only relevant patch...
Yours,
Linus Walleij
On 10/10/2012 01:54 AM, Linus Walleij wrote:
> On Mon, Oct 1, 2012 at 2:11 PM, Rob Herring <[email protected]> wrote:
>> On 10/01/2012 02:35 AM, Linus Walleij wrote:
>>> From: Linus Walleij <[email protected]>
>>>
>>> Currently we rely on all IRQ chip instances to dynamically
>>> allocate their IRQ descriptors unless they use the linear
>>> IRQ domain. So for irqdomain_add_legacy() and
>>> irqdomain_add_simple() the caller need to make sure that
>>> descriptors are allocated.
>>>
>>> Let's slightly augment the yet unused irqdomain_add_simple()
>>> to also allocate descriptors as a means to simplify usage
>>> and avoid code duplication throughout the kernel.
>>>
>>> We warn if descriptors cannot be allocated, e.g. if a
>>> platform has the bad habit of hogging descriptors at boot
>>> time.
>>>
>>> Cc: Rob Herring <[email protected]>
>>> Cc: Thomas Gleixner <[email protected]>
>>> Cc: Grant Likely <[email protected]>
>>> Cc: Paul Mundt <[email protected]>
>>> Cc: Russell King <[email protected]>
>>> Cc: Lee Jones <[email protected]>
>>> Signed-off-by: Linus Walleij <[email protected]>
>>
>> Looks good.
>>
>> Reviewed-by: Rob Herring <[email protected]>
>
> So what do we do with this patch? Grant?
>
> I think the change is good to get in ASAP and since I
> now have one patch in pinctrl depending on it I have
> tentatively applied it there.
>
> Nobody sent any irqdomain fixes for this merge
> window, maybe this is the only relevant patch...
I say merge it with what depends on it. There's been plenty of time for
review.
Rob
On Mon, 1 Oct 2012 09:35:22 +0200, Linus Walleij <[email protected]> wrote:
> From: Linus Walleij <[email protected]>
>
> Currently we rely on all IRQ chip instances to dynamically
> allocate their IRQ descriptors unless they use the linear
> IRQ domain. So for irqdomain_add_legacy() and
> irqdomain_add_simple() the caller need to make sure that
> descriptors are allocated.
>
> Let's slightly augment the yet unused irqdomain_add_simple()
> to also allocate descriptors as a means to simplify usage
> and avoid code duplication throughout the kernel.
>
> We warn if descriptors cannot be allocated, e.g. if a
> platform has the bad habit of hogging descriptors at boot
> time.
>
> Cc: Rob Herring <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Paul Mundt <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Lee Jones <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> ChangeLog v1->v2:
> - Switch descriptor allocation on IS_ENABLED(CONFIG_SPARSE_IRQ)
> so it won't attempt to allocate descriptors in the non-sparse
> case.
> - Use of_node_to_nid() to make sure we work on platforms with their
> own node concept.
> - Specify irq_alloc_descs(first_irq, first_irq ...) to emulate
> irq_alloc_desc_at().
> ---
> kernel/irq/irqdomain.c | 33 ++++++++++++++++++++++++++++-----
> 1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 49a7772..4e69e24 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -148,7 +148,8 @@ static unsigned int irq_domain_legacy_revmap(struct irq_domain *domain,
> * @host_data: Controller private data pointer
> *
> * Allocates a legacy irq_domain if irq_base is positive or a linear
> - * domain otherwise.
> + * domain otherwise. For the legacy domain, IRQ descriptors will also
> + * be allocated.
> *
> * This is intended to implement the expected behaviour for most
> * interrupt controllers which is that a linear mapping should
> @@ -162,11 +163,33 @@ struct irq_domain *irq_domain_add_simple(struct device_node *of_node,
> const struct irq_domain_ops *ops,
> void *host_data)
> {
> - if (first_irq > 0)
> - return irq_domain_add_legacy(of_node, size, first_irq, 0,
> + if (first_irq > 0) {
> + int irq_base;
> +
> + if (IS_ENABLED(CONFIG_SPARSE_IRQ)) {
> + /*
> + * Set the descriptor allocator to search for a
> + * 1-to-1 mapping, such as irq_alloc_desc_at().
> + * Use of_node_to_nid() which is defined to
> + * numa_node_id() on platforms that have no custom
> + * implementation.
> + */
> + irq_base = irq_alloc_descs(first_irq, first_irq, size,
> + of_node_to_nid(of_node));
> + if (irq_base < 0) {
> + WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
> + first_irq);
> + irq_base = first_irq;
As I just commented on the previous version, WARN() is probably too
verbose (and scary). Make it an informational.
However, I see another problem. What is the requested range straddles
the boundary between reserved and non-reserved IRQs? It would be good to
give some information about which irq range was requested and maybe
report which ones were available.... or check to see if the request is
inside or partially inside the reserved region?
g.
On Mon, Nov 26, 2012 at 9:26 PM, Grant Likely <[email protected]> wrote:
>> + if (irq_base < 0) {
>> + WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
>> + first_irq);
>> + irq_base = first_irq;
>
> As I just commented on the previous version, WARN() is probably too
> verbose (and scary). Make it an informational.
So the discussion began with me removing exactly that kind of WARN()
from arch/arm/common/gic.c:
http://marc.info/?l=linux-arm-kernel&m=134860088710574&w=2
Which was NACKed by Rob:
http://marc.info/?l=linux-arm-kernel&m=134860136515611&w=2
Who prefered to leave it in to encourage platforms to get fixed.
This code just follows exactly that pattern.
I'm happy to patch out *both* (or rather patch gic.c to use
irq_domain_add_simple()) because I never quite liked
it in the first place.
> However, I see another problem. What is the requested range straddles
> the boundary between reserved and non-reserved IRQs? It would be good to
> give some information about which irq range was requested and maybe
> report which ones were available.... or check to see if the request is
> inside or partially inside the reserved region?
Right now the usual symptom of that is that the system hangs.
Do you mean we should probe around a bit with
irq_get_next_irq() to figure out more precisely what the
problem is, or did you have something more elegant
in mind?
Yours,
Linus Walleij
On 11/26/2012 06:13 PM, Linus Walleij wrote:
> On Mon, Nov 26, 2012 at 9:26 PM, Grant Likely <[email protected]> wrote:
>
>>> + if (irq_base < 0) {
>>> + WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
>>> + first_irq);
>>> + irq_base = first_irq;
>>
>> As I just commented on the previous version, WARN() is probably too
>> verbose (and scary). Make it an informational.
>
> So the discussion began with me removing exactly that kind of WARN()
> from arch/arm/common/gic.c:
> http://marc.info/?l=linux-arm-kernel&m=134860088710574&w=2
>
> Which was NACKed by Rob:
> http://marc.info/?l=linux-arm-kernel&m=134860136515611&w=2
> Who prefered to leave it in to encourage platforms to get fixed.
>
> This code just follows exactly that pattern.
>
> I'm happy to patch out *both* (or rather patch gic.c to use
> irq_domain_add_simple()) because I never quite liked
> it in the first place.
>
>> However, I see another problem. What is the requested range straddles
>> the boundary between reserved and non-reserved IRQs? It would be good to
>> give some information about which irq range was requested and maybe
>> report which ones were available.... or check to see if the request is
>> inside or partially inside the reserved region?
>
> Right now the usual symptom of that is that the system hangs.
>
> Do you mean we should probe around a bit with
> irq_get_next_irq() to figure out more precisely what the
> problem is, or did you have something more elegant
> in mind?
My objection was removing completely (which a pr_debug effectively
does). I think Grant is saying just make the warning more informative
about why it failed which is fine with me. nr_irqs is already printed
out, so that provides some info already (although it is pretty terse).
Rob
On Tue, Nov 27, 2012 at 1:24 AM, Rob Herring <[email protected]> wrote:
> My objection was removing completely (which a pr_debug effectively
> does). I think Grant is saying just make the warning more informative
> about why it failed which is fine with me. nr_irqs is already printed
> out, so that provides some info already (although it is pretty terse).
OK sent a patch for this...
Yours,
Linus Walleij