| Message ID | 5718F593.40605@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jon, On 21/04/16 16:45, Jon Hunter wrote: > > On 20/04/16 12:03, Jon Hunter wrote: >> Some IRQ chips, such as GPIO controllers or secondary level interrupt >> controllers, may require require additional runtime power management >> control to ensure they are accessible. For such IRQ chips, it makes sense >> to enable the IRQ chip when interrupts are requested and disabled them >> again once all interrupts have been freed. >> >> When mapping an IRQ, the IRQ type settings are read and then programmed. >> The mapping of the IRQ happens before the IRQ is requested and so the >> programming of the type settings occurs before the IRQ is requested. This >> is a problem for IRQ chips that require additional power management >> control because they may not be accessible yet. Therefore, when mapping >> the IRQ, don't program the type settings, just save them and then program >> these saved settings when the IRQ is requested (so long as if they are not >> overridden via the call to request the IRQ). >> >> Add a stub function for irq_domain_free_irqs() to avoid any compilation >> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> include/linux/irqdomain.h | 3 +++ >> kernel/irq/irqdomain.c | 17 +++++++++++++---- >> 2 files changed, 16 insertions(+), 4 deletions(-) > > [snip] > >> - /* Set type if specified and different than the current one */ >> - if (type != IRQ_TYPE_NONE && >> - type != irq_get_trigger_type(virq)) >> - irq_set_irq_type(virq, type); >> + irq_data = irq_get_irq_data(virq); >> + if (!irq_data) { >> + if (irq_domain_is_hierarchy(domain)) >> + irq_domain_free_irqs(virq, 1); >> + else >> + irq_dispose_mapping(virq); >> + return 0; >> + } >> + >> + /* Store trigger type */ >> + irqd_set_trigger_type(irq_data, type); >> + > > I appear to have missed another place for saving the irq type > which I had change in this version. Next time I will triple > check! Should have been ... > > > diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h > index 2aed04396210..fc66876d1965 100644 > --- a/include/linux/irqdomain.h > +++ b/include/linux/irqdomain.h > @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain, > return -1; > } > > +static inline void irq_domain_free_irqs(unsigned int virq, > + unsigned int nr_irqs) { } > + > static inline bool irq_domain_is_hierarchy(struct irq_domain *domain) > { > return false; > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > index 3d6ef5527b71..46ecf5d468b2 100644 > --- a/kernel/irq/irqdomain.c > +++ b/kernel/irq/irqdomain.c > @@ -569,6 +569,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data, > unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) > { > struct irq_domain *domain; > + struct irq_data *irq_data; > irq_hw_number_t hwirq; > unsigned int type = IRQ_TYPE_NONE; > int virq; > @@ -619,7 +620,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) > * it now and return the interrupt number. > */ > if (IRQ_TYPE_NONE == irq_get_trigger_type(virq)) { > - irq_set_irq_type(virq, type); > + irq_data = irq_get_irq_data(virq); > + if (!irq_data) > + return 0; Can you point out in which circumstances irq_data can be NULL? If we can lookup the interrupt in the domain, it'd better exist somewhere. Or am I missing something obvious? Thanks, M.
On 22/04/16 09:22, Marc Zyngier wrote: > Hi Jon, > > On 21/04/16 16:45, Jon Hunter wrote: >> >> On 20/04/16 12:03, Jon Hunter wrote: >>> Some IRQ chips, such as GPIO controllers or secondary level interrupt >>> controllers, may require require additional runtime power management >>> control to ensure they are accessible. For such IRQ chips, it makes sense >>> to enable the IRQ chip when interrupts are requested and disabled them >>> again once all interrupts have been freed. >>> >>> When mapping an IRQ, the IRQ type settings are read and then programmed. >>> The mapping of the IRQ happens before the IRQ is requested and so the >>> programming of the type settings occurs before the IRQ is requested. This >>> is a problem for IRQ chips that require additional power management >>> control because they may not be accessible yet. Therefore, when mapping >>> the IRQ, don't program the type settings, just save them and then program >>> these saved settings when the IRQ is requested (so long as if they are not >>> overridden via the call to request the IRQ). >>> >>> Add a stub function for irq_domain_free_irqs() to avoid any compilation >>> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected. >>> >>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>> --- >>> include/linux/irqdomain.h | 3 +++ >>> kernel/irq/irqdomain.c | 17 +++++++++++++---- >>> 2 files changed, 16 insertions(+), 4 deletions(-) >> >> [snip] >> >>> - /* Set type if specified and different than the current one */ >>> - if (type != IRQ_TYPE_NONE && >>> - type != irq_get_trigger_type(virq)) >>> - irq_set_irq_type(virq, type); >>> + irq_data = irq_get_irq_data(virq); >>> + if (!irq_data) { >>> + if (irq_domain_is_hierarchy(domain)) >>> + irq_domain_free_irqs(virq, 1); >>> + else >>> + irq_dispose_mapping(virq); >>> + return 0; >>> + } >>> + >>> + /* Store trigger type */ >>> + irqd_set_trigger_type(irq_data, type); >>> + >> >> I appear to have missed another place for saving the irq type >> which I had change in this version. Next time I will triple >> check! Should have been ... >> >> >> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h >> index 2aed04396210..fc66876d1965 100644 >> --- a/include/linux/irqdomain.h >> +++ b/include/linux/irqdomain.h >> @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain, >> return -1; >> } >> >> +static inline void irq_domain_free_irqs(unsigned int virq, >> + unsigned int nr_irqs) { } >> + >> static inline bool irq_domain_is_hierarchy(struct irq_domain *domain) >> { >> return false; >> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c >> index 3d6ef5527b71..46ecf5d468b2 100644 >> --- a/kernel/irq/irqdomain.c >> +++ b/kernel/irq/irqdomain.c >> @@ -569,6 +569,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data, >> unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) >> { >> struct irq_domain *domain; >> + struct irq_data *irq_data; >> irq_hw_number_t hwirq; >> unsigned int type = IRQ_TYPE_NONE; >> int virq; >> @@ -619,7 +620,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) >> * it now and return the interrupt number. >> */ >> if (IRQ_TYPE_NONE == irq_get_trigger_type(virq)) { >> - irq_set_irq_type(virq, type); >> + irq_data = irq_get_irq_data(virq); >> + if (!irq_data) >> + return 0; > > Can you point out in which circumstances irq_data can be NULL? If we can > lookup the interrupt in the domain, it'd better exist somewhere. Or am I > missing something obvious? To be honest, I was not 100% sure if this could be possible if we find an existing mapping or if there were any paths that could race. So if we can guarantee that it is not NULL here, I can drop this and simplify the change. By the way, is there any sort of reference count for mapping an IRQ so that if irq_create_fwspec_mapping() is called more than once for an IRQ, it is not freed on the first call to irq_dispose_mapping()? I assume there is but could not see where this is handled. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/04/16 09:48, Jon Hunter wrote: > > On 22/04/16 09:22, Marc Zyngier wrote: >> Hi Jon, >> >> On 21/04/16 16:45, Jon Hunter wrote: >>> >>> On 20/04/16 12:03, Jon Hunter wrote: >>>> Some IRQ chips, such as GPIO controllers or secondary level interrupt >>>> controllers, may require require additional runtime power management >>>> control to ensure they are accessible. For such IRQ chips, it makes sense >>>> to enable the IRQ chip when interrupts are requested and disabled them >>>> again once all interrupts have been freed. >>>> >>>> When mapping an IRQ, the IRQ type settings are read and then programmed. >>>> The mapping of the IRQ happens before the IRQ is requested and so the >>>> programming of the type settings occurs before the IRQ is requested. This >>>> is a problem for IRQ chips that require additional power management >>>> control because they may not be accessible yet. Therefore, when mapping >>>> the IRQ, don't program the type settings, just save them and then program >>>> these saved settings when the IRQ is requested (so long as if they are not >>>> overridden via the call to request the IRQ). >>>> >>>> Add a stub function for irq_domain_free_irqs() to avoid any compilation >>>> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected. >>>> >>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>>> --- >>>> include/linux/irqdomain.h | 3 +++ >>>> kernel/irq/irqdomain.c | 17 +++++++++++++---- >>>> 2 files changed, 16 insertions(+), 4 deletions(-) >>> >>> [snip] >>> >>>> - /* Set type if specified and different than the current one */ >>>> - if (type != IRQ_TYPE_NONE && >>>> - type != irq_get_trigger_type(virq)) >>>> - irq_set_irq_type(virq, type); >>>> + irq_data = irq_get_irq_data(virq); >>>> + if (!irq_data) { >>>> + if (irq_domain_is_hierarchy(domain)) >>>> + irq_domain_free_irqs(virq, 1); >>>> + else >>>> + irq_dispose_mapping(virq); >>>> + return 0; >>>> + } >>>> + >>>> + /* Store trigger type */ >>>> + irqd_set_trigger_type(irq_data, type); >>>> + >>> >>> I appear to have missed another place for saving the irq type >>> which I had change in this version. Next time I will triple >>> check! Should have been ... >>> >>> >>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h >>> index 2aed04396210..fc66876d1965 100644 >>> --- a/include/linux/irqdomain.h >>> +++ b/include/linux/irqdomain.h >>> @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain, >>> return -1; >>> } >>> >>> +static inline void irq_domain_free_irqs(unsigned int virq, >>> + unsigned int nr_irqs) { } >>> + >>> static inline bool irq_domain_is_hierarchy(struct irq_domain *domain) >>> { >>> return false; >>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c >>> index 3d6ef5527b71..46ecf5d468b2 100644 >>> --- a/kernel/irq/irqdomain.c >>> +++ b/kernel/irq/irqdomain.c >>> @@ -569,6 +569,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data, >>> unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) >>> { >>> struct irq_domain *domain; >>> + struct irq_data *irq_data; >>> irq_hw_number_t hwirq; >>> unsigned int type = IRQ_TYPE_NONE; >>> int virq; >>> @@ -619,7 +620,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) >>> * it now and return the interrupt number. >>> */ >>> if (IRQ_TYPE_NONE == irq_get_trigger_type(virq)) { >>> - irq_set_irq_type(virq, type); >>> + irq_data = irq_get_irq_data(virq); >>> + if (!irq_data) >>> + return 0; >> >> Can you point out in which circumstances irq_data can be NULL? If we can >> lookup the interrupt in the domain, it'd better exist somewhere. Or am I >> missing something obvious? > > To be honest, I was not 100% sure if this could be possible if we find > an existing mapping or if there were any paths that could race. So if we > can guarantee that it is not NULL here, I can drop this and simplify the > change. No, you're right. It could very well race against irq_dispose_mapping (and the disassociate method doesn't provide enough guarantee). Unfortunately, most of the irqdomain code still relies on this race not happening. How to fix this is still a bit unclear to me. > By the way, is there any sort of reference count for mapping an IRQ so > that if irq_create_fwspec_mapping() is called more than once for an IRQ, > it is not freed on the first call to irq_dispose_mapping()? I assume > there is but could not see where this is handled. There is none, unfortunately. Thanks, M.
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h index 2aed04396210..fc66876d1965 100644 --- a/include/linux/irqdomain.h +++ b/include/linux/irqdomain.h @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain, return -1; } +static inline void irq_domain_free_irqs(unsigned int virq, + unsigned int nr_irqs) { } + static inline bool irq_domain_is_hierarchy(struct irq_domain *domain) { return false; diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index 3d6ef5527b71..46ecf5d468b2 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -569,6 +569,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data, unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) { struct irq_domain *domain; + struct irq_data *irq_data; irq_hw_number_t hwirq; unsigned int type = IRQ_TYPE_NONE; int virq; @@ -619,7 +620,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) * it now and return the interrupt number. */ if (IRQ_TYPE_NONE == irq_get_trigger_type(virq)) { - irq_set_irq_type(virq, type); + irq_data = irq_get_irq_data(virq); + if (!irq_data) + return 0; + + irqd_set_trigger_type(irq_data, type); return virq; } @@ -639,10 +644,18 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) return virq; } - /* Set type if specified and different than the current one */ - if (type != IRQ_TYPE_NONE && - type != irq_get_trigger_type(virq)) - irq_set_irq_type(virq, type); + irq_data = irq_get_irq_data(virq); + if (!irq_data) { + if (irq_domain_is_hierarchy(domain)) + irq_domain_free_irqs(virq, 1); + else + irq_dispose_mapping(virq); + return 0; + } + + /* Store trigger type */ + irqd_set_trigger_type(irq_data, type); + return virq; } EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);