Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1420708 > unrolled thread

Re: [PATCH V6 2/9] genirq: Look-up trigger type if not specified by caller

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.

Contents

  Re: [PATCH V6 2/9] genirq: Look-up trigger type if not specified by  caller Marc Zyngier <marc.zyngier@arm.com> - 2016-06-13 12:50 +0200
    Re: [PATCH V6 2/9] genirq: Look-up trigger type if not specified by  caller Jon Hunter <jonathanh@nvidia.com> - 2016-06-13 13:10 +0200
      Re: [PATCH V6 2/9] genirq: Look-up trigger type if not specified by  caller Marc Zyngier <marc.zyngier@arm.com> - 2016-06-13 14:10 +0200
        Re: [PATCH V6 2/9] genirq: Look-up trigger type if not specified by  caller Jon Hunter <jonathanh@nvidia.com> - 2016-06-13 14:30 +0200
          Re: [PATCH V6 2/9] genirq: Look-up trigger type if not specified by  caller Marc Zyngier <marc.zyngier@arm.com> - 2016-06-13 14:40 +0200

#1420708 — Re: [PATCH V6 2/9] genirq: Look-up trigger type if not specified by caller

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);
+
 	if (type != IRQ_TYPE_NONE) {
 		int ret;
 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [next] | [standalone]

#1420721

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

-- 
nvpublic

[toc] | [prev] | [next] | [standalone]

#1420756

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.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1420765

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

-- 
nvpublic

[toc] | [prev] | [next] | [standalone]

#1420780

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.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web