Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1293771 > unrolled thread
| Started by | Jon Hunter <jonathanh@nvidia.com> |
|---|---|
| First post | 2015-12-17 12:00 +0100 |
| Last post | 2015-12-22 12:40 +0100 |
| Articles | 7 — 3 participants |
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.
[RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping Jon Hunter <jonathanh@nvidia.com> - 2015-12-17 12:00 +0100
Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping Linus Walleij <linus.walleij@linaro.org> - 2015-12-17 14:20 +0100
Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping Jon Hunter <jonathanh@nvidia.com> - 2015-12-18 11:20 +0100
Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping Linus Walleij <linus.walleij@linaro.org> - 2015-12-22 11:00 +0100
Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping Linus Walleij <linus.walleij@linaro.org> - 2015-12-22 11:10 +0100
Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping Jon Hunter <jonathanh@nvidia.com> - 2015-12-22 12:30 +0100
Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping Grygorii Strashko <grygorii.strashko@ti.com> - 2015-12-22 12:40 +0100
| From | Jon Hunter <jonathanh@nvidia.com> |
|---|---|
| Date | 2015-12-17 12:00 +0100 |
| Subject | [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping |
| Message-ID | <qGErW-6H4-51@gated-at.bofh.it> |
When mapping an IRQ, if a mapping already exists, then we simply return
the virual IRQ number. However, we do not check that the type settings for
the existing mapping match those for the mapping that is about to be
created. It may be unlikely that the type settings would not match, but
check for this and don't return a valid IRQ mapping if the type settings
do not match.
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
kernel/irq/irqdomain.c | 58 +++++++++++++++++++++++++++++++++++++-------------
1 file changed, 43 insertions(+), 15 deletions(-)
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 22aa9612ef7c..eae31e978ab2 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -568,8 +568,10 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
{
+ struct device_node *of_node;
struct irq_domain *domain;
irq_hw_number_t hwirq;
+ unsigned int cur_type = IRQ_TYPE_NONE;
unsigned int type = IRQ_TYPE_NONE;
int virq;
@@ -587,23 +589,49 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
if (irq_domain_translate(domain, fwspec, &hwirq, &type))
return 0;
- if (irq_domain_is_hierarchy(domain)) {
- /*
- * If we've already configured this interrupt,
- * don't do it again, or hell will break loose.
- */
- virq = irq_find_mapping(domain, hwirq);
- if (virq)
- return virq;
+ of_node = irq_domain_get_of_node(domain);
- virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
- if (virq <= 0)
- return 0;
+ /*
+ * If we've already configured this interrupt,
+ * don't do it again, or hell will break loose.
+ */
+ virq = irq_find_mapping(domain, hwirq);
+ if (!virq) {
+ if (irq_domain_is_hierarchy(domain)) {
+ virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE,
+ fwspec);
+ if (virq <= 0)
+ return 0;
+ } else {
+ virq = irq_domain_alloc_descs(-1, 1, hwirq,
+ of_node_to_nid(of_node));
+ if (virq <= 0)
+ return 0;
+
+ if (irq_domain_associate(domain, virq, hwirq)) {
+ irq_free_desc(virq);
+ return 0;
+ }
+ }
} else {
- /* Create mapping */
- virq = irq_create_mapping(domain, hwirq);
- if (!virq)
- return virq;
+ cur_type = irq_get_trigger_type(virq);
+ }
+
+ /*
+ * If the trigger type is not specified or matches the current
+ * trigger type then we are done so return the interrupt number.
+ */
+ if (type == IRQ_TYPE_NONE || type == cur_type)
+ return virq;
+
+ /*
+ * If the trigger type is already set and does
+ * not match this interrupt, then return 0.
+ */
+ if (cur_type != IRQ_TYPE_NONE) {
+ pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
+ hwirq, of_node_full_name(of_node));
+ return 0;
}
/* Set type if specified and different than the current one */
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Linus Walleij <linus.walleij@linaro.org> |
|---|---|
| Date | 2015-12-17 14:20 +0100 |
| Subject | Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping |
| Message-ID | <qGGDo-8kx-11@gated-at.bofh.it> |
| In reply to | #1293771 |
On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> When mapping an IRQ, if a mapping already exists, then we simply return
> the virual IRQ number. However, we do not check that the type settings for
^virtual
Just that it isn't virtual, it's a Linux IRQ number, we actually use
hwirq for the non-virtual IRQ number/offse in this function.
But I know I may be fighting weathermills here.
> unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> {
> + struct device_node *of_node;
> struct irq_domain *domain;
> irq_hw_number_t hwirq;
> + unsigned int cur_type = IRQ_TYPE_NONE;
> unsigned int type = IRQ_TYPE_NONE;
> int virq;
>
> @@ -587,23 +589,49 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
> if (irq_domain_translate(domain, fwspec, &hwirq, &type))
> return 0;
>
> - if (irq_domain_is_hierarchy(domain)) {
> - /*
> - * If we've already configured this interrupt,
> - * don't do it again, or hell will break loose.
> - */
> - virq = irq_find_mapping(domain, hwirq);
> - if (virq)
> - return virq;
> + of_node = irq_domain_get_of_node(domain);
Marc's patches went to great lengths to do this fwspec-neutral,
i.e. it doesn't matter if it's done by DT or ACPI (or whatever).
This just drives a truck through all of that by making
the whole function OF-specific again.
>
> - virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> - if (virq <= 0)
> - return 0;
> + /*
> + * If we've already configured this interrupt,
> + * don't do it again, or hell will break loose.
> + */
> + virq = irq_find_mapping(domain, hwirq);
> + if (!virq) {
> + if (irq_domain_is_hierarchy(domain)) {
> + virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE,
> + fwspec);
> + if (virq <= 0)
> + return 0;
> + } else {
> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
> + of_node_to_nid(of_node));
What is this all of a sudden? Not even mentioned in the
commit. Plus I bet ACPI need something else than OF nid
passed here.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Jon Hunter <jonathanh@nvidia.com> |
|---|---|
| Date | 2015-12-18 11:20 +0100 |
| Subject | Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping |
| Message-ID | <qH0iK-4hR-15@gated-at.bofh.it> |
| In reply to | #1293877 |
On 17/12/15 13:16, Linus Walleij wrote:
> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>
>> When mapping an IRQ, if a mapping already exists, then we simply return
>> the virual IRQ number. However, we do not check that the type settings for
>
> ^virtual
>
> Just that it isn't virtual, it's a Linux IRQ number, we actually use
> hwirq for the non-virtual IRQ number/offse in this function.
>
> But I know I may be fighting weathermills here.
Ok, will re-word this.
>> unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>> {
>> + struct device_node *of_node;
>> struct irq_domain *domain;
>> irq_hw_number_t hwirq;
>> + unsigned int cur_type = IRQ_TYPE_NONE;
>> unsigned int type = IRQ_TYPE_NONE;
>> int virq;
>>
>> @@ -587,23 +589,49 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>> if (irq_domain_translate(domain, fwspec, &hwirq, &type))
>> return 0;
>>
>> - if (irq_domain_is_hierarchy(domain)) {
>> - /*
>> - * If we've already configured this interrupt,
>> - * don't do it again, or hell will break loose.
>> - */
>> - virq = irq_find_mapping(domain, hwirq);
>> - if (virq)
>> - return virq;
>> + of_node = irq_domain_get_of_node(domain);
>
> Marc's patches went to great lengths to do this fwspec-neutral,
> i.e. it doesn't matter if it's done by DT or ACPI (or whatever).
>
> This just drives a truck through all of that by making
> the whole function OF-specific again.
Yes, was not sure if this would be popular. I was on the fence, but I
saw the following ...
if (!domain) {
pr_warn("no irq domain found for %s !\n",
of_node_full_name(to_of_node(fwspec->fwnode)));
return 0;
}
... and thought we are not completely agnostic. However, if you prefer I
park my mini else where, I can definitely drop this, no big deal ;-)
>>
>> - virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
>> - if (virq <= 0)
>> - return 0;
>> + /*
>> + * If we've already configured this interrupt,
>> + * don't do it again, or hell will break loose.
>> + */
>> + virq = irq_find_mapping(domain, hwirq);
>> + if (!virq) {
>> + if (irq_domain_is_hierarchy(domain)) {
>> + virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE,
>> + fwspec);
>> + if (virq <= 0)
>> + return 0;
>> + } else {
>> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
>> + of_node_to_nid(of_node));
>
> What is this all of a sudden? Not even mentioned in the
> commit. Plus I bet ACPI need something else than OF nid
> passed here.
Do you mean the else part of all of the above?
So in the current code, the else part calls irq_create_mapping() and
this function internally, calls irq_find_mapping(). Given that I am now
calling irq_find_mapping() before the if, I don't really need to call
irq_create_mapping() again, I just need to call the functions in
irq_create_mapping() that allocate and setup the IRQ number. Sorry, I
did not really explain this. However, if it is simpler, I can call
irq_create_mapping() instead and may be this makes the change easier to
read.
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Walleij <linus.walleij@linaro.org> |
|---|---|
| Date | 2015-12-22 11:00 +0100 |
| Subject | Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping |
| Message-ID | <qIrTB-2h1-19@gated-at.bofh.it> |
| In reply to | #1294635 |
On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> On 17/12/15 13:16, Linus Walleij wrote:
>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> + } else {
>>> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>> + of_node_to_nid(of_node));
>>
>> What is this all of a sudden? Not even mentioned in the
>> commit. Plus I bet ACPI need something else than OF nid
>> passed here.
>
> Do you mean the else part of all of the above?
Yes
> So in the current code, the else part calls irq_create_mapping() (...)
No, not that... The fact that you all of a sudden have started
calling irq_domain_alloc_descs() which the function didn't do
before, totally changing the calling semantics for everyone in
the kernel, leading to the problem I then describe with this
potentially being called before the irqdomain for the irqchip
is initialized and descs getting "random" numbers.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Walleij <linus.walleij@linaro.org> |
|---|---|
| Date | 2015-12-22 11:10 +0100 |
| Subject | Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping |
| Message-ID | <qIs3g-2zy-1@gated-at.bofh.it> |
| In reply to | #1296666 |
On Tue, Dec 22, 2015 at 10:58 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>> On 17/12/15 13:16, Linus Walleij wrote:
>>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>> + } else {
>>>> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>>> + of_node_to_nid(of_node));
>>>
>>> What is this all of a sudden? Not even mentioned in the
>>> commit. Plus I bet ACPI need something else than OF nid
>>> passed here.
>>
>> Do you mean the else part of all of the above?
>
> Yes
>
>> So in the current code, the else part calls irq_create_mapping() (...)
>
> No, not that... The fact that you all of a sudden have started
> calling irq_domain_alloc_descs() which the function didn't do
> before, totally changing the calling semantics for everyone in
> the kernel, leading to the problem I then describe with this
> potentially being called before the irqdomain for the irqchip
> is initialized and descs getting "random" numbers.
I see I didn't go into those details in my first answer. Hm, I
guess I got uncertain and deleted it because I remember
writing it...
I am simply worries that starting to call irq_domain_alloc_descs()
has unintended side effects, especially on platforms using
legacy or simple irqdomains.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Jon Hunter <jonathanh@nvidia.com> |
|---|---|
| Date | 2015-12-22 12:30 +0100 |
| Subject | Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping |
| Message-ID | <qItiF-3h5-7@gated-at.bofh.it> |
| In reply to | #1296670 |
On 22/12/15 10:00, Linus Walleij wrote:
> On Tue, Dec 22, 2015 at 10:58 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>> On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> On 17/12/15 13:16, Linus Walleij wrote:
>>>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>> + } else {
>>>>> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>>>> + of_node_to_nid(of_node));
>>>>
>>>> What is this all of a sudden? Not even mentioned in the
>>>> commit. Plus I bet ACPI need something else than OF nid
>>>> passed here.
>>>
>>> Do you mean the else part of all of the above?
>>
>> Yes
>>
>>> So in the current code, the else part calls irq_create_mapping() (...)
>>
>> No, not that... The fact that you all of a sudden have started
>> calling irq_domain_alloc_descs() which the function didn't do
>> before, totally changing the calling semantics for everyone in
>> the kernel, leading to the problem I then describe with this
>> potentially being called before the irqdomain for the irqchip
>> is initialized and descs getting "random" numbers.
>
> I see I didn't go into those details in my first answer. Hm, I
> guess I got uncertain and deleted it because I remember
> writing it...
>
> I am simply worries that starting to call irq_domain_alloc_descs()
> has unintended side effects, especially on platforms using
> legacy or simple irqdomains.
Ok, no problem I will keep the existing irq_create_mapping() instead then.
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Grygorii Strashko <grygorii.strashko@ti.com> |
|---|---|
| Date | 2015-12-22 12:40 +0100 |
| Subject | Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping |
| Message-ID | <qItsm-3ml-19@gated-at.bofh.it> |
| In reply to | #1296670 |
On 12/22/2015 12:00 PM, Linus Walleij wrote:
> On Tue, Dec 22, 2015 at 10:58 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>> On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> On 17/12/15 13:16, Linus Walleij wrote:
>>>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>> + } else {
>>>>> + virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>>>> + of_node_to_nid(of_node));
>>>>
>>>> What is this all of a sudden? Not even mentioned in the
>>>> commit. Plus I bet ACPI need something else than OF nid
>>>> passed here.
>>>
>>> Do you mean the else part of all of the above?
>>
>> Yes
>>
>>> So in the current code, the else part calls irq_create_mapping() (...)
>>
>> No, not that... The fact that you all of a sudden have started
>> calling irq_domain_alloc_descs() which the function didn't do
>> before, totally changing the calling semantics for everyone in
>> the kernel, leading to the problem I then describe with this
>> potentially being called before the irqdomain for the irqchip
>> is initialized and descs getting "random" numbers.
>
> I see I didn't go into those details in my first answer. Hm, I
> guess I got uncertain and deleted it because I remember
> writing it...
>
> I am simply worries that starting to call irq_domain_alloc_descs()
> has unintended side effects, especially on platforms using
> legacy or simple irqdomains.
>
I think, some misunderstanding here introduced by replacing
irq_create_mapping() call on subset of direct calls to
irq_domain_alloc_descs() and irq_domain_associate().
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web