From: Jiang Liu <[email protected]>
Signed-off-by: Jiang Liu <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Yingjoe Chen <[email protected]>
Cc: Yijing Wang <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
include/linux/irqdomain.h | 5 +++++
kernel/irq/irqdomain.c | 10 ++++++++++
2 files changed, 15 insertions(+)
Index: tip/include/linux/irqdomain.h
===================================================================
--- tip.orig/include/linux/irqdomain.h
+++ tip/include/linux/irqdomain.h
@@ -33,6 +33,7 @@
#define _LINUX_IRQDOMAIN_H
#include <linux/types.h>
+#include <linux/irqhandler.h>
#include <linux/radix-tree.h>
struct device_node;
@@ -263,6 +264,10 @@ extern int irq_domain_set_hwirq_and_chip
irq_hw_number_t hwirq,
struct irq_chip *chip,
void *chip_data);
+extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+ irq_hw_number_t hwirq, struct irq_chip *chip,
+ void *chip_data, irq_flow_handler_t handler,
+ void *handler_data, const char *handler_name);
extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
extern void irq_domain_free_irqs_common(struct irq_domain *domain,
int virq, int nr_irqs);
Index: tip/kernel/irq/irqdomain.c
===================================================================
--- tip.orig/kernel/irq/irqdomain.c
+++ tip/kernel/irq/irqdomain.c
@@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
return 0;
}
+void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
+ irq_hw_number_t hwirq, struct irq_chip *chip,
+ void *chip_data, irq_flow_handler_t handler,
+ void *handler_data, const char *handler_name)
+{
+ irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
+ __irq_set_handler(virq, handler, 0, handler_name);
+ irq_set_handler_data(virq, handler_data);
+}
+
void irq_domain_reset_irq_data(struct irq_data *irq_data)
{
irq_data->hwirq = 0;
On Wed, 2014-11-12 at 13:43 +0000, Thomas Gleixner wrote:
> plain text document attachment
> (rfc-part4-v1-02-17-genirq-introduce-helper-irq_domain_set_info-to-reduce-duplicated-code.patch)
> From: Jiang Liu <[email protected]>
>
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Yingjoe Chen <[email protected]>
> Cc: Yijing Wang <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> include/linux/irqdomain.h | 5 +++++
> kernel/irq/irqdomain.c | 10 ++++++++++
> 2 files changed, 15 insertions(+)
>
> Index: tip/include/linux/irqdomain.h
> ===================================================================
> --- tip.orig/include/linux/irqdomain.h
> +++ tip/include/linux/irqdomain.h
> @@ -33,6 +33,7 @@
> #define _LINUX_IRQDOMAIN_H
>
> #include <linux/types.h>
> +#include <linux/irqhandler.h>
> #include <linux/radix-tree.h>
>
> struct device_node;
> @@ -263,6 +264,10 @@ extern int irq_domain_set_hwirq_and_chip
> irq_hw_number_t hwirq,
> struct irq_chip *chip,
> void *chip_data);
> +extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
> + irq_hw_number_t hwirq, struct irq_chip *chip,
> + void *chip_data, irq_flow_handler_t handler,
> + void *handler_data, const char *handler_name);
> extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
> extern void irq_domain_free_irqs_common(struct irq_domain *domain,
> int virq, int nr_irqs);
> Index: tip/kernel/irq/irqdomain.c
> ===================================================================
> --- tip.orig/kernel/irq/irqdomain.c
> +++ tip/kernel/irq/irqdomain.c
> @@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
> return 0;
> }
>
> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
> + irq_hw_number_t hwirq, struct irq_chip *chip,
> + void *chip_data, irq_flow_handler_t handler,
> + void *handler_data, const char *handler_name)
> +{
> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
> + __irq_set_handler(virq, handler, 0, handler_name);
> + irq_set_handler_data(virq, handler_data);
> +}
> +
Hi,
While trying to use this function, I'm not sure about its interface.
This function have 8 parameters but only save 3 function calls. After
checking uses in Jiang's original patch, I think this make code harder
to understand.
Joe.C
On 2014/11/13 17:57, Yingjoe Chen wrote:
> On Wed, 2014-11-12 at 13:43 +0000, Thomas Gleixner wrote:
>> Index: tip/kernel/irq/irqdomain.c
>> ===================================================================
>> --- tip.orig/kernel/irq/irqdomain.c
>> +++ tip/kernel/irq/irqdomain.c
>> @@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
>> return 0;
>> }
>>
>> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>> + void *chip_data, irq_flow_handler_t handler,
>> + void *handler_data, const char *handler_name)
>> +{
>> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
>> + __irq_set_handler(virq, handler, 0, handler_name);
>> + irq_set_handler_data(virq, handler_data);
>> +}
>> +
>
> Hi,
>
> While trying to use this function, I'm not sure about its interface.
> This function have 8 parameters but only save 3 function calls. After
> checking uses in Jiang's original patch, I think this make code harder
> to understand.
Hi Joe,
It's true, but we also want to reduce duplicated code:(
Regards!
Gerry
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On 13/11/14 10:00, Jiang Liu wrote:
> On 2014/11/13 17:57, Yingjoe Chen wrote:
>> On Wed, 2014-11-12 at 13:43 +0000, Thomas Gleixner wrote:
>>> Index: tip/kernel/irq/irqdomain.c
>>> ===================================================================
>>> --- tip.orig/kernel/irq/irqdomain.c
>>> +++ tip/kernel/irq/irqdomain.c
>>> @@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
>>> return 0;
>>> }
>>>
>>> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>>> + void *chip_data, irq_flow_handler_t handler,
>>> + void *handler_data, const char *handler_name)
>>> +{
>>> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
>>> + __irq_set_handler(virq, handler, 0, handler_name);
>>> + irq_set_handler_data(virq, handler_data);
>>> +}
>>> +
>>
>> Hi,
>>
>> While trying to use this function, I'm not sure about its interface.
>> This function have 8 parameters but only save 3 function calls. After
>> checking uses in Jiang's original patch, I think this make code harder
>> to understand.
> Hi Joe,
> It's true, but we also want to reduce duplicated code:(
I'm in two minds about this one.
Yes, it looks horrible (to be fair, I hate anything that has more than 3
or 4 parameters, because I can never remember what they're for).
On the other hand, it gives you a nice check-point in your code, with
all the bits you need to set, or at least consider. Because it depends
on so many things that you may have to construct/obtain, you quickly
realize that there is not that many locations in your stack that fits
these requirements. Yes, this a twisted reasoning.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 12/11/14 13:43, Thomas Gleixner wrote:
> From: Jiang Liu <[email protected]>
>
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Yingjoe Chen <[email protected]>
> Cc: Yijing Wang <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> include/linux/irqdomain.h | 5 +++++
> kernel/irq/irqdomain.c | 10 ++++++++++
> 2 files changed, 15 insertions(+)
>
> Index: tip/include/linux/irqdomain.h
> ===================================================================
> --- tip.orig/include/linux/irqdomain.h
> +++ tip/include/linux/irqdomain.h
> @@ -33,6 +33,7 @@
> #define _LINUX_IRQDOMAIN_H
>
> #include <linux/types.h>
> +#include <linux/irqhandler.h>
> #include <linux/radix-tree.h>
>
> struct device_node;
> @@ -263,6 +264,10 @@ extern int irq_domain_set_hwirq_and_chip
> irq_hw_number_t hwirq,
> struct irq_chip *chip,
> void *chip_data);
> +extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
> + irq_hw_number_t hwirq, struct irq_chip *chip,
> + void *chip_data, irq_flow_handler_t handler,
> + void *handler_data, const char *handler_name);
> extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
> extern void irq_domain_free_irqs_common(struct irq_domain *domain,
> int virq, int nr_irqs);
> Index: tip/kernel/irq/irqdomain.c
> ===================================================================
> --- tip.orig/kernel/irq/irqdomain.c
> +++ tip/kernel/irq/irqdomain.c
> @@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
> return 0;
> }
>
> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
> + irq_hw_number_t hwirq, struct irq_chip *chip,
> + void *chip_data, irq_flow_handler_t handler,
> + void *handler_data, const char *handler_name)
> +{
> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
> + __irq_set_handler(virq, handler, 0, handler_name);
> + irq_set_handler_data(virq, handler_data);
> +}
> +
We still have the issue that, depending on where in the stack this is
called, this will succeed or fail: If this is called from the inner
irqchip, __irq_set_handler() will fail, as it will look at the outer
domain as the (desc->irq_data.chip == &no_irq_chip) test fails (we
haven't set the top level yet).
I have this very imperfect workaround in my tree:
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index d028b34..91e6515 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -731,7 +731,16 @@ __irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
if (!handle) {
handle = handle_bad_irq;
} else {
- if (WARN_ON(desc->irq_data.chip == &no_irq_chip))
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+ struct irq_data *irq_data = &desc->irq_data;
+ while (irq_data) {
+ if (irq_data->chip != &no_irq_chip)
+ break;
+ irq_data = irq_data->parent_data;
+ }
+#endif
+
+ if (WARN_ON(!irq_data || irq_data->chip == &no_irq_chip))
goto out;
}
Which translate into: If there is at least one irqchip in the domain,
it will probably sort itself out. Not ideal. Any real solution to
this problem?
GICv2 faces this exact problem, as some of its interrupts are used
directly, and some others are used through the MSI domain. In the
GIC driver, it is almost impossible to find out...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2014/11/14 23:31, Marc Zyngier wrote:
> On 12/11/14 13:43, Thomas Gleixner wrote:
>> From: Jiang Liu <[email protected]>
>>
>> Signed-off-by: Jiang Liu <[email protected]>
>> Cc: Bjorn Helgaas <[email protected]>
>> Cc: Grant Likely <[email protected]>
>> Cc: Marc Zyngier <[email protected]>
>> Cc: Yingjoe Chen <[email protected]>
>> Cc: Yijing Wang <[email protected]>
>> Signed-off-by: Thomas Gleixner <[email protected]>
>> ---
>> include/linux/irqdomain.h | 5 +++++
>> kernel/irq/irqdomain.c | 10 ++++++++++
>> 2 files changed, 15 insertions(+)
>>
>> Index: tip/include/linux/irqdomain.h
>> ===================================================================
>> --- tip.orig/include/linux/irqdomain.h
>> +++ tip/include/linux/irqdomain.h
>> @@ -33,6 +33,7 @@
>> #define _LINUX_IRQDOMAIN_H
>>
>> #include <linux/types.h>
>> +#include <linux/irqhandler.h>
>> #include <linux/radix-tree.h>
>>
>> struct device_node;
>> @@ -263,6 +264,10 @@ extern int irq_domain_set_hwirq_and_chip
>> irq_hw_number_t hwirq,
>> struct irq_chip *chip,
>> void *chip_data);
>> +extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>> + void *chip_data, irq_flow_handler_t handler,
>> + void *handler_data, const char *handler_name);
>> extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
>> extern void irq_domain_free_irqs_common(struct irq_domain *domain,
>> int virq, int nr_irqs);
>> Index: tip/kernel/irq/irqdomain.c
>> ===================================================================
>> --- tip.orig/kernel/irq/irqdomain.c
>> +++ tip/kernel/irq/irqdomain.c
>> @@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
>> return 0;
>> }
>>
>> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>> + void *chip_data, irq_flow_handler_t handler,
>> + void *handler_data, const char *handler_name)
>> +{
>> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
>> + __irq_set_handler(virq, handler, 0, handler_name);
>> + irq_set_handler_data(virq, handler_data);
>> +}
>> +
>
> We still have the issue that, depending on where in the stack this is
> called, this will succeed or fail: If this is called from the inner
> irqchip, __irq_set_handler() will fail, as it will look at the outer
> domain as the (desc->irq_data.chip == &no_irq_chip) test fails (we
> haven't set the top level yet).
>
> I have this very imperfect workaround in my tree:
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index d028b34..91e6515 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -731,7 +731,16 @@ __irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
> if (!handle) {
> handle = handle_bad_irq;
> } else {
> - if (WARN_ON(desc->irq_data.chip == &no_irq_chip))
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> + struct irq_data *irq_data = &desc->irq_data;
> + while (irq_data) {
> + if (irq_data->chip != &no_irq_chip)
> + break;
> + irq_data = irq_data->parent_data;
> + }
> +#endif
> +
> + if (WARN_ON(!irq_data || irq_data->chip == &no_irq_chip))
> goto out;
> }
>
> Which translate into: If there is at least one irqchip in the domain,
> it will probably sort itself out. Not ideal. Any real solution to
> this problem?
>
> GICv2 faces this exact problem, as some of its interrupts are used
> directly, and some others are used through the MSI domain. In the
> GIC driver, it is almost impossible to find out...
Hi Marc,
I prefer the above solution to relax the warning conditions.
Changing the calling order in irq_domain_ops->alloc() looks a little
strange, and other interrupt drivers may still run into the same issue.
Regards!
Gerry
>
> Thanks,
>
> M.
>
On 14/11/14 15:41, Jiang Liu wrote:
> On 2014/11/14 23:31, Marc Zyngier wrote:
>> On 12/11/14 13:43, Thomas Gleixner wrote:
>>> From: Jiang Liu <[email protected]>
>>>
>>> Signed-off-by: Jiang Liu <[email protected]>
>>> Cc: Bjorn Helgaas <[email protected]>
>>> Cc: Grant Likely <[email protected]>
>>> Cc: Marc Zyngier <[email protected]>
>>> Cc: Yingjoe Chen <[email protected]>
>>> Cc: Yijing Wang <[email protected]>
>>> Signed-off-by: Thomas Gleixner <[email protected]>
>>> ---
>>> include/linux/irqdomain.h | 5 +++++
>>> kernel/irq/irqdomain.c | 10 ++++++++++
>>> 2 files changed, 15 insertions(+)
>>>
>>> Index: tip/include/linux/irqdomain.h
>>> ===================================================================
>>> --- tip.orig/include/linux/irqdomain.h
>>> +++ tip/include/linux/irqdomain.h
>>> @@ -33,6 +33,7 @@
>>> #define _LINUX_IRQDOMAIN_H
>>>
>>> #include <linux/types.h>
>>> +#include <linux/irqhandler.h>
>>> #include <linux/radix-tree.h>
>>>
>>> struct device_node;
>>> @@ -263,6 +264,10 @@ extern int irq_domain_set_hwirq_and_chip
>>> irq_hw_number_t hwirq,
>>> struct irq_chip *chip,
>>> void *chip_data);
>>> +extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>>> + void *chip_data, irq_flow_handler_t handler,
>>> + void *handler_data, const char *handler_name);
>>> extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
>>> extern void irq_domain_free_irqs_common(struct irq_domain *domain,
>>> int virq, int nr_irqs);
>>> Index: tip/kernel/irq/irqdomain.c
>>> ===================================================================
>>> --- tip.orig/kernel/irq/irqdomain.c
>>> +++ tip/kernel/irq/irqdomain.c
>>> @@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
>>> return 0;
>>> }
>>>
>>> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>>> + void *chip_data, irq_flow_handler_t handler,
>>> + void *handler_data, const char *handler_name)
>>> +{
>>> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
>>> + __irq_set_handler(virq, handler, 0, handler_name);
>>> + irq_set_handler_data(virq, handler_data);
>>> +}
>>> +
>>
>> We still have the issue that, depending on where in the stack this is
>> called, this will succeed or fail: If this is called from the inner
>> irqchip, __irq_set_handler() will fail, as it will look at the outer
>> domain as the (desc->irq_data.chip == &no_irq_chip) test fails (we
>> haven't set the top level yet).
>>
>> I have this very imperfect workaround in my tree:
>>
>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index d028b34..91e6515 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -731,7 +731,16 @@ __irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
>> if (!handle) {
>> handle = handle_bad_irq;
>> } else {
>> - if (WARN_ON(desc->irq_data.chip == &no_irq_chip))
>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> + struct irq_data *irq_data = &desc->irq_data;
>> + while (irq_data) {
>> + if (irq_data->chip != &no_irq_chip)
>> + break;
>> + irq_data = irq_data->parent_data;
>> + }
>> +#endif
>> +
>> + if (WARN_ON(!irq_data || irq_data->chip == &no_irq_chip))
>> goto out;
>> }
>>
>> Which translate into: If there is at least one irqchip in the domain,
>> it will probably sort itself out. Not ideal. Any real solution to
>> this problem?
>>
>> GICv2 faces this exact problem, as some of its interrupts are used
>> directly, and some others are used through the MSI domain. In the
>> GIC driver, it is almost impossible to find out...
> Hi Marc,
> I prefer the above solution to relax the warning conditions.
> Changing the calling order in irq_domain_ops->alloc() looks a little
> strange, and other interrupt drivers may still run into the same issue.
OK. Where do we from from there? Do you want a proper patch, or will you
fold this into the existing code?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2014/11/15 1:35, Marc Zyngier wrote:
> On 14/11/14 15:41, Jiang Liu wrote:
>> On 2014/11/14 23:31, Marc Zyngier wrote:
>>> On 12/11/14 13:43, Thomas Gleixner wrote:
>>>> From: Jiang Liu <[email protected]>
>>>>
>>>> Signed-off-by: Jiang Liu <[email protected]>
>>>> Cc: Bjorn Helgaas <[email protected]>
>>>> Cc: Grant Likely <[email protected]>
>>>> Cc: Marc Zyngier <[email protected]>
>>>> Cc: Yingjoe Chen <[email protected]>
>>>> Cc: Yijing Wang <[email protected]>
>>>> Signed-off-by: Thomas Gleixner <[email protected]>
>>>> ---
>>>> include/linux/irqdomain.h | 5 +++++
>>>> kernel/irq/irqdomain.c | 10 ++++++++++
>>>> 2 files changed, 15 insertions(+)
>>>>
>>>> Index: tip/include/linux/irqdomain.h
>>>> ===================================================================
>>>> --- tip.orig/include/linux/irqdomain.h
>>>> +++ tip/include/linux/irqdomain.h
>>>> @@ -33,6 +33,7 @@
>>>> #define _LINUX_IRQDOMAIN_H
>>>>
>>>> #include <linux/types.h>
>>>> +#include <linux/irqhandler.h>
>>>> #include <linux/radix-tree.h>
>>>>
>>>> struct device_node;
>>>> @@ -263,6 +264,10 @@ extern int irq_domain_set_hwirq_and_chip
>>>> irq_hw_number_t hwirq,
>>>> struct irq_chip *chip,
>>>> void *chip_data);
>>>> +extern void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>>>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>>>> + void *chip_data, irq_flow_handler_t handler,
>>>> + void *handler_data, const char *handler_name);
>>>> extern void irq_domain_reset_irq_data(struct irq_data *irq_data);
>>>> extern void irq_domain_free_irqs_common(struct irq_domain *domain,
>>>> int virq, int nr_irqs);
>>>> Index: tip/kernel/irq/irqdomain.c
>>>> ===================================================================
>>>> --- tip.orig/kernel/irq/irqdomain.c
>>>> +++ tip/kernel/irq/irqdomain.c
>>>> @@ -882,6 +882,16 @@ int irq_domain_set_hwirq_and_chip(struct
>>>> return 0;
>>>> }
>>>>
>>>> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>>>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>>>> + void *chip_data, irq_flow_handler_t handler,
>>>> + void *handler_data, const char *handler_name)
>>>> +{
>>>> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
>>>> + __irq_set_handler(virq, handler, 0, handler_name);
>>>> + irq_set_handler_data(virq, handler_data);
>>>> +}
>>>> +
>>>
>>> We still have the issue that, depending on where in the stack this is
>>> called, this will succeed or fail: If this is called from the inner
>>> irqchip, __irq_set_handler() will fail, as it will look at the outer
>>> domain as the (desc->irq_data.chip == &no_irq_chip) test fails (we
>>> haven't set the top level yet).
>>>
>>> I have this very imperfect workaround in my tree:
>>>
>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>>> index d028b34..91e6515 100644
>>> --- a/kernel/irq/chip.c
>>> +++ b/kernel/irq/chip.c
>>> @@ -731,7 +731,16 @@ __irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
>>> if (!handle) {
>>> handle = handle_bad_irq;
>>> } else {
>>> - if (WARN_ON(desc->irq_data.chip == &no_irq_chip))
>>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>>> + struct irq_data *irq_data = &desc->irq_data;
>>> + while (irq_data) {
>>> + if (irq_data->chip != &no_irq_chip)
>>> + break;
>>> + irq_data = irq_data->parent_data;
>>> + }
>>> +#endif
>>> +
>>> + if (WARN_ON(!irq_data || irq_data->chip == &no_irq_chip))
>>> goto out;
>>> }
>>>
>>> Which translate into: If there is at least one irqchip in the domain,
>>> it will probably sort itself out. Not ideal. Any real solution to
>>> this problem?
>>>
>>> GICv2 faces this exact problem, as some of its interrupts are used
>>> directly, and some others are used through the MSI domain. In the
>>> GIC driver, it is almost impossible to find out...
>> Hi Marc,
>> I prefer the above solution to relax the warning conditions.
>> Changing the calling order in irq_domain_ops->alloc() looks a little
>> strange, and other interrupt drivers may still run into the same issue.
>
> OK. Where do we from from there? Do you want a proper patch, or will you
> fold this into the existing code?
A patch will be great:)
>
> Thanks,
>
> M.
>
Hi Thomas, Jiang,
On 2014/11/12 21:43, Thomas Gleixner wrote:
> From: Jiang Liu <[email protected]>
[...]
> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
> + irq_hw_number_t hwirq, struct irq_chip *chip,
> + void *chip_data, irq_flow_handler_t handler,
> + void *handler_data, const char *handler_name)
> +{
> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
> + __irq_set_handler(virq, handler, 0, handler_name);
> + irq_set_handler_data(virq, handler_data);
> +}
When stacked domain enabled, there will be a semantic shift to the linux interrupt
identifiers. The @virq now delivers much more than before.
More specifically, now we need both @virq and @domain, rather than only @irq, to
determine which irq_data we want to configure. And once we configure @irq without
providing the exact domain, it means we are configuring all the domains related to
that @irq. So I think this routine just messed all things up.
Regards,
Abel
On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
> Hi Thomas, Jiang,
> On 2014/11/12 21:43, Thomas Gleixner wrote:
>
> > From: Jiang Liu <[email protected]>
> [...]
> > +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
> > + irq_hw_number_t hwirq, struct irq_chip *chip,
> > + void *chip_data, irq_flow_handler_t handler,
> > + void *handler_data, const char *handler_name)
> > +{
> > + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
> > + __irq_set_handler(virq, handler, 0, handler_name);
> > + irq_set_handler_data(virq, handler_data);
> > +}
>
> When stacked domain enabled, there will be a semantic shift to the linux interrupt
> identifiers. The @virq now delivers much more than before.
> More specifically, now we need both @virq and @domain, rather than only @irq, to
> determine which irq_data we want to configure. And once we configure @irq without
> providing the exact domain, it means we are configuring all the domains related to
> that @irq. So I think this routine just messed all things up.
You can mess up anything by using an interface in the wrong way. Open
coding will not make that harder.
Thanks,
tglx
On 2014/11/18 18:03, Thomas Gleixner wrote:
> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>
>> Hi Thomas, Jiang,
>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>
>>> From: Jiang Liu <[email protected]>
>> [...]
>>> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>>> + void *chip_data, irq_flow_handler_t handler,
>>> + void *handler_data, const char *handler_name)
>>> +{
>>> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
>>> + __irq_set_handler(virq, handler, 0, handler_name);
>>> + irq_set_handler_data(virq, handler_data);
>>> +}
>>
>> When stacked domain enabled, there will be a semantic shift to the linux interrupt
>> identifiers. The @virq now delivers much more than before.
>> More specifically, now we need both @virq and @domain, rather than only @irq, to
>> determine which irq_data we want to configure. And once we configure @irq without
>> providing the exact domain, it means we are configuring all the domains related to
>> that @irq. So I think this routine just messed all things up.
>
> You can mess up anything by using an interface in the wrong way. Open
> coding will not make that harder.
>
But what's the correct way to use this interface?
Thanks,
Abel
On 2014/11/18 19:47, Yun Wu (Abel) wrote:
> On 2014/11/18 18:03, Thomas Gleixner wrote:
>
>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>
>>> Hi Thomas, Jiang,
>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>
>>>> From: Jiang Liu <[email protected]>
>>> [...]
>>>> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>>>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>>>> + void *chip_data, irq_flow_handler_t handler,
>>>> + void *handler_data, const char *handler_name)
>>>> +{
>>>> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
>>>> + __irq_set_handler(virq, handler, 0, handler_name);
>>>> + irq_set_handler_data(virq, handler_data);
>>>> +}
>>>
>>> When stacked domain enabled, there will be a semantic shift to the linux interrupt
>>> identifiers. The @virq now delivers much more than before.
>>> More specifically, now we need both @virq and @domain, rather than only @irq, to
>>> determine which irq_data we want to configure. And once we configure @irq without
>>> providing the exact domain, it means we are configuring all the domains related to
>>> that @irq. So I think this routine just messed all things up.
>>
>> You can mess up anything by using an interface in the wrong way. Open
>> coding will not make that harder.
>>
>
> But what's the correct way to use this interface?
Hi Yun,
It's to be used by interrupt controller drivers to implement
hierarchy irqdomains.
Regards!
Gerry
>
> Thanks,
> Abel
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On 2014/11/18 20:38, Jiang Liu wrote:
> On 2014/11/18 19:47, Yun Wu (Abel) wrote:
>> On 2014/11/18 18:03, Thomas Gleixner wrote:
>>
>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>
>>>> Hi Thomas, Jiang,
>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>
>>>>> From: Jiang Liu <[email protected]>
>>>> [...]
>>>>> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>>>>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>>>>> + void *chip_data, irq_flow_handler_t handler,
>>>>> + void *handler_data, const char *handler_name)
>>>>> +{
>>>>> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
>>>>> + __irq_set_handler(virq, handler, 0, handler_name);
>>>>> + irq_set_handler_data(virq, handler_data);
>>>>> +}
>>>>
>>>> When stacked domain enabled, there will be a semantic shift to the linux interrupt
>>>> identifiers. The @virq now delivers much more than before.
>>>> More specifically, now we need both @virq and @domain, rather than only @irq, to
>>>> determine which irq_data we want to configure. And once we configure @irq without
>>>> providing the exact domain, it means we are configuring all the domains related to
>>>> that @irq. So I think this routine just messed all things up.
>>>
>>> You can mess up anything by using an interface in the wrong way. Open
>>> coding will not make that harder.
>>>
>>
>> But what's the correct way to use this interface?
>
> It's to be used by interrupt controller drivers to implement
> hierarchy irqdomains.
>
Each time an interrupt domain calls this, the previous (@handler, @handler_name,
@handler_data) will be overrode. It's because the routines __irq_set_handler()
and irq_set_handler_data() only configure top level (without Marc's fix which is
not ideal). Is this what we really want to see?
Thanks,
Abel
On 2014/11/18 21:28, Yun Wu (Abel) wrote:
> On 2014/11/18 20:38, Jiang Liu wrote:
>
>> On 2014/11/18 19:47, Yun Wu (Abel) wrote:
>>> On 2014/11/18 18:03, Thomas Gleixner wrote:
>>>
>>>> On Tue, 18 Nov 2014, Yun Wu (Abel) wrote:
>>>>
>>>>> Hi Thomas, Jiang,
>>>>> On 2014/11/12 21:43, Thomas Gleixner wrote:
>>>>>
>>>>>> From: Jiang Liu <[email protected]>
>>>>> [...]
>>>>>> +void irq_domain_set_info(struct irq_domain *domain, unsigned int virq,
>>>>>> + irq_hw_number_t hwirq, struct irq_chip *chip,
>>>>>> + void *chip_data, irq_flow_handler_t handler,
>>>>>> + void *handler_data, const char *handler_name)
>>>>>> +{
>>>>>> + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, chip, chip_data);
>>>>>> + __irq_set_handler(virq, handler, 0, handler_name);
>>>>>> + irq_set_handler_data(virq, handler_data);
>>>>>> +}
>>>>>
>>>>> When stacked domain enabled, there will be a semantic shift to the linux interrupt
>>>>> identifiers. The @virq now delivers much more than before.
>>>>> More specifically, now we need both @virq and @domain, rather than only @irq, to
>>>>> determine which irq_data we want to configure. And once we configure @irq without
>>>>> providing the exact domain, it means we are configuring all the domains related to
>>>>> that @irq. So I think this routine just messed all things up.
>>>>
>>>> You can mess up anything by using an interface in the wrong way. Open
>>>> coding will not make that harder.
>>>>
>>>
>>> But what's the correct way to use this interface?
>>
>> It's to be used by interrupt controller drivers to implement
>> hierarchy irqdomains.
>>
>
> Each time an interrupt domain calls this, the previous (@handler, @handler_name,
> @handler_data) will be overrode. It's because the routines __irq_set_handler()
> and irq_set_handler_data() only configure top level (without Marc's fix which is
> not ideal). Is this what we really want to see?
Hi Yun,
There are different interfaces for different purposes.
irq_domain_set_hwirq_and_chip() sets hwirq, irq_chip and irq_chip_data.
irq_domain_set_info() sets handler and handler_data in addition to
irq_domain_set_hwirq_and_chip().
Which irqdomain to call irq_domain_set_info() to set irq_handler
is determined by the interrupt hierarchy. And at least on irqdomain in
the hierarchy must set flow handler for the irq.
Thanks!
Gerry
>
> Thanks,
> Abel
>