| Message ID | 575E8E29.3090808@arm.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 13/06/16 11:42, Marc Zyngier wrote: > On 07/06/16 16:12, Jon Hunter wrote: >> For some devices the IRQ trigger type for a device is read from >> firmware, such as device-tree. The IRQ trigger type is typically read >> when the mapping for IRQ is created, which is before the IRQ is >> requested. Hence, the IRQ trigger type is programmed when mapping the >> IRQ and not when requesting the IRQ. >> >> Although this works for most cases, in order to support IRQ chips which >> require runtime power management, which may not be accessible prior >> to requesting the IRQ, it is desirable to look-up the IRQ trigger type >> when it is requested. Therefore, if the IRQ trigger type is not >> specified when __setup_irq() is called, look-up the saved IRQ trigger >> type. This will allow us to defer the programming of the trigger type >> from when the IRQ is mapped to when it is actually requested. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> kernel/irq/manage.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >> index ef0bc02c3a70..eaedeb74b49d 100644 >> --- a/kernel/irq/manage.c >> +++ b/kernel/irq/manage.c >> @@ -1117,6 +1117,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) >> new->irq = irq; >> >> /* >> + * If the trigger type is not specified by the caller, >> + * then use the default for this interrupt. >> + */ >> + if (!(new->flags & IRQF_TRIGGER_MASK)) >> + new->flags |= irqd_get_trigger_type(&desc->irq_data); >> + >> + /* >> * Check whether the interrupt nests into another interrupt >> * thread. >> */ >> > > I've added the following patch to the queue, in order to deal with > percpu interrupts that were not handled by this patch: > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index f8fd1fb..00cfc85 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -1751,7 +1751,14 @@ void enable_percpu_irq(unsigned int irq, unsigned int type) > if (!desc) > return; > > + /* > + * If the trigger type is not specified by the caller, then > + * use the default for this interrupt. > + */ > type &= IRQ_TYPE_SENSE_MASK; > + if (type == IRQ_TYPE_NONE) > + type = irqd_get_trigger_type(&desc->irq_data); > + I am wondering if you need this here because now __setup_irq(), called by request_percpu_irq(), will actually look-up the saved type even for a percpu-irq. So shouldn't this already be handled? Or am I missing something? Cheers Jon
On 13/06/16 12:09, Jon Hunter wrote: > > On 13/06/16 11:42, Marc Zyngier wrote: >> On 07/06/16 16:12, Jon Hunter wrote: >>> For some devices the IRQ trigger type for a device is read from >>> firmware, such as device-tree. The IRQ trigger type is typically read >>> when the mapping for IRQ is created, which is before the IRQ is >>> requested. Hence, the IRQ trigger type is programmed when mapping the >>> IRQ and not when requesting the IRQ. >>> >>> Although this works for most cases, in order to support IRQ chips which >>> require runtime power management, which may not be accessible prior >>> to requesting the IRQ, it is desirable to look-up the IRQ trigger type >>> when it is requested. Therefore, if the IRQ trigger type is not >>> specified when __setup_irq() is called, look-up the saved IRQ trigger >>> type. This will allow us to defer the programming of the trigger type >>> from when the IRQ is mapped to when it is actually requested. >>> >>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> >>> --- >>> kernel/irq/manage.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >>> index ef0bc02c3a70..eaedeb74b49d 100644 >>> --- a/kernel/irq/manage.c >>> +++ b/kernel/irq/manage.c >>> @@ -1117,6 +1117,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) >>> new->irq = irq; >>> >>> /* >>> + * If the trigger type is not specified by the caller, >>> + * then use the default for this interrupt. >>> + */ >>> + if (!(new->flags & IRQF_TRIGGER_MASK)) >>> + new->flags |= irqd_get_trigger_type(&desc->irq_data); >>> + >>> + /* >>> * Check whether the interrupt nests into another interrupt >>> * thread. >>> */ >>> >> >> I've added the following patch to the queue, in order to deal with >> percpu interrupts that were not handled by this patch: >> >> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >> index f8fd1fb..00cfc85 100644 >> --- a/kernel/irq/manage.c >> +++ b/kernel/irq/manage.c >> @@ -1751,7 +1751,14 @@ void enable_percpu_irq(unsigned int irq, unsigned int type) >> if (!desc) >> return; >> >> + /* >> + * If the trigger type is not specified by the caller, then >> + * use the default for this interrupt. >> + */ >> type &= IRQ_TYPE_SENSE_MASK; >> + if (type == IRQ_TYPE_NONE) >> + type = irqd_get_trigger_type(&desc->irq_data); >> + > > I am wondering if you need this here because now __setup_irq(), called > by request_percpu_irq(), will actually look-up the saved type even for a > percpu-irq. So shouldn't this already be handled? Or am I missing something? You are overlooking the fact that the configuration registers are themselves per-CPU, which means that __setup_irq() will only configure the interrupt on the CPU it runs on. And you can't even tell which one, since you are in a preemptible context. Does it make more sense? M.
On 13/06/16 12:59, Marc Zyngier wrote: > On 13/06/16 12:09, Jon Hunter wrote: >> >> On 13/06/16 11:42, Marc Zyngier wrote: >>> On 07/06/16 16:12, Jon Hunter wrote: >>>> For some devices the IRQ trigger type for a device is read from >>>> firmware, such as device-tree. The IRQ trigger type is typically read >>>> when the mapping for IRQ is created, which is before the IRQ is >>>> requested. Hence, the IRQ trigger type is programmed when mapping the >>>> IRQ and not when requesting the IRQ. >>>> >>>> Although this works for most cases, in order to support IRQ chips which >>>> require runtime power management, which may not be accessible prior >>>> to requesting the IRQ, it is desirable to look-up the IRQ trigger type >>>> when it is requested. Therefore, if the IRQ trigger type is not >>>> specified when __setup_irq() is called, look-up the saved IRQ trigger >>>> type. This will allow us to defer the programming of the trigger type >>>> from when the IRQ is mapped to when it is actually requested. >>>> >>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>>> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> >>>> --- >>>> kernel/irq/manage.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >>>> index ef0bc02c3a70..eaedeb74b49d 100644 >>>> --- a/kernel/irq/manage.c >>>> +++ b/kernel/irq/manage.c >>>> @@ -1117,6 +1117,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) >>>> new->irq = irq; >>>> >>>> /* >>>> + * If the trigger type is not specified by the caller, >>>> + * then use the default for this interrupt. >>>> + */ >>>> + if (!(new->flags & IRQF_TRIGGER_MASK)) >>>> + new->flags |= irqd_get_trigger_type(&desc->irq_data); >>>> + >>>> + /* >>>> * Check whether the interrupt nests into another interrupt >>>> * thread. >>>> */ >>>> >>> >>> I've added the following patch to the queue, in order to deal with >>> percpu interrupts that were not handled by this patch: >>> >>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >>> index f8fd1fb..00cfc85 100644 >>> --- a/kernel/irq/manage.c >>> +++ b/kernel/irq/manage.c >>> @@ -1751,7 +1751,14 @@ void enable_percpu_irq(unsigned int irq, unsigned int type) >>> if (!desc) >>> return; >>> >>> + /* >>> + * If the trigger type is not specified by the caller, then >>> + * use the default for this interrupt. >>> + */ >>> type &= IRQ_TYPE_SENSE_MASK; >>> + if (type == IRQ_TYPE_NONE) >>> + type = irqd_get_trigger_type(&desc->irq_data); >>> + >> >> I am wondering if you need this here because now __setup_irq(), called >> by request_percpu_irq(), will actually look-up the saved type even for a >> percpu-irq. So shouldn't this already be handled? Or am I missing something? > > You are overlooking the fact that the configuration registers are > themselves per-CPU, which means that __setup_irq() will only configure > the interrupt on the CPU it runs on. And you can't even tell which one, > since you are in a preemptible context. > > Does it make more sense? Yes it does thanks. I am guessing we don't need to worry about there being different type configurations for the different CPUs for a given interrupt? Cheers Jon
On 13/06/16 13:24, Jon Hunter wrote: > > On 13/06/16 12:59, Marc Zyngier wrote: >> On 13/06/16 12:09, Jon Hunter wrote: >>> >>> On 13/06/16 11:42, Marc Zyngier wrote: >>>> On 07/06/16 16:12, Jon Hunter wrote: >>>>> For some devices the IRQ trigger type for a device is read from >>>>> firmware, such as device-tree. The IRQ trigger type is typically read >>>>> when the mapping for IRQ is created, which is before the IRQ is >>>>> requested. Hence, the IRQ trigger type is programmed when mapping the >>>>> IRQ and not when requesting the IRQ. >>>>> >>>>> Although this works for most cases, in order to support IRQ chips which >>>>> require runtime power management, which may not be accessible prior >>>>> to requesting the IRQ, it is desirable to look-up the IRQ trigger type >>>>> when it is requested. Therefore, if the IRQ trigger type is not >>>>> specified when __setup_irq() is called, look-up the saved IRQ trigger >>>>> type. This will allow us to defer the programming of the trigger type >>>>> from when the IRQ is mapped to when it is actually requested. >>>>> >>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>>>> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> >>>>> --- >>>>> kernel/irq/manage.c | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >>>>> index ef0bc02c3a70..eaedeb74b49d 100644 >>>>> --- a/kernel/irq/manage.c >>>>> +++ b/kernel/irq/manage.c >>>>> @@ -1117,6 +1117,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) >>>>> new->irq = irq; >>>>> >>>>> /* >>>>> + * If the trigger type is not specified by the caller, >>>>> + * then use the default for this interrupt. >>>>> + */ >>>>> + if (!(new->flags & IRQF_TRIGGER_MASK)) >>>>> + new->flags |= irqd_get_trigger_type(&desc->irq_data); >>>>> + >>>>> + /* >>>>> * Check whether the interrupt nests into another interrupt >>>>> * thread. >>>>> */ >>>>> >>>> >>>> I've added the following patch to the queue, in order to deal with >>>> percpu interrupts that were not handled by this patch: >>>> >>>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >>>> index f8fd1fb..00cfc85 100644 >>>> --- a/kernel/irq/manage.c >>>> +++ b/kernel/irq/manage.c >>>> @@ -1751,7 +1751,14 @@ void enable_percpu_irq(unsigned int irq, unsigned int type) >>>> if (!desc) >>>> return; >>>> >>>> + /* >>>> + * If the trigger type is not specified by the caller, then >>>> + * use the default for this interrupt. >>>> + */ >>>> type &= IRQ_TYPE_SENSE_MASK; >>>> + if (type == IRQ_TYPE_NONE) >>>> + type = irqd_get_trigger_type(&desc->irq_data); >>>> + >>> >>> I am wondering if you need this here because now __setup_irq(), called >>> by request_percpu_irq(), will actually look-up the saved type even for a >>> percpu-irq. So shouldn't this already be handled? Or am I missing something? >> >> You are overlooking the fact that the configuration registers are >> themselves per-CPU, which means that __setup_irq() will only configure >> the interrupt on the CPU it runs on. And you can't even tell which one, >> since you are in a preemptible context. >> >> Does it make more sense? > > Yes it does thanks. I am guessing we don't need to worry about there > being different type configurations for the different CPUs for a given > interrupt? I don't know of any firmware that would allow this configuration to be described (neither DT nor ACPI support this braindeadness). So if that happens, the only way to support it is to actively pass the trigger type to enable_percpu_irq(). Should we want to support it from the PoV of a firmware description, we'd need to expand the trigger to be per-cpu as well, which feels a bit mad. I'm not in any hurry here! Thanks, M.
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index f8fd1fb..00cfc85 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1751,7 +1751,14 @@ void enable_percpu_irq(unsigned int irq, unsigned int type) if (!desc) return; + /* + * If the trigger type is not specified by the caller, then + * use the default for this interrupt. + */ type &= IRQ_TYPE_SENSE_MASK; + if (type == IRQ_TYPE_NONE) + type = irqd_get_trigger_type(&desc->irq_data); + if (type != IRQ_TYPE_NONE) { int ret;