| Message ID | 54662062.8030807@arm.com |
|---|---|
| State | New |
| Headers | show |
On 2014/11/14 23:31, Marc Zyngier wrote: > On 12/11/14 13:43, Thomas Gleixner wrote: >> From: Jiang Liu <jiang.liu@linux.intel.com> >> >> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> Cc: Grant Likely <grant.likely@linaro.org> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> Cc: Yingjoe Chen <yingjoe.chen@mediatek.com> >> Cc: Yijing Wang <wangyijing@huawei.com> >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> >> --- >> 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. > -- 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/
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 <jiang.liu@linux.intel.com> >>> >>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> >>> Cc: Bjorn Helgaas <bhelgaas@google.com> >>> Cc: Grant Likely <grant.likely@linaro.org> >>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>> Cc: Yingjoe Chen <yingjoe.chen@mediatek.com> >>> Cc: Yijing Wang <wangyijing@huawei.com> >>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> >>> --- >>> 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.
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 <jiang.liu@linux.intel.com> >>>> >>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> >>>> Cc: Bjorn Helgaas <bhelgaas@google.com> >>>> Cc: Grant Likely <grant.likely@linaro.org> >>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>> Cc: Yingjoe Chen <yingjoe.chen@mediatek.com> >>>> Cc: Yijing Wang <wangyijing@huawei.com> >>>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> >>>> --- >>>> 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. > -- 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/
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; }