diff mbox

[V3,06/17] irqdomain: Don't set type when mapping an IRQ

Message ID 1462379130-11742-7-git-send-email-jonathanh@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jon Hunter May 4, 2016, 4:25 p.m. UTC
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    | 23 ++++++++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

Comments

Marc Zyngier May 9, 2016, 12:23 p.m. UTC | #1
On 04/05/16 17:25, 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    | 23 ++++++++++++++++++-----
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> 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 d68371213fc9..bbf5b9b8ac3d 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -564,6 +564,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;
> @@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  		 * it now and return the interrupt number.
>  		 */
>  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> -			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;
>  		}
>  
> @@ -633,10 +638,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);
> 

This patch have the effect of making misconfigured PPIs absolutely
obvious. I still need to wrap my head around the root cause, but here's
the findings I have so far:

- kvmtool generates a DT with the wrong trigger information (edge
instead of level) for the timer.
- with this patch applied, "cyclictest -S" reliably locks up when run in
a guest (missing a timer interrupt, goodbye CPU).
- Either fixing kvmtool or reverting that patch makes it work reliably
again.

My gut feeling is that until that patch, the failing irq_set_irq_type()
wasn't affecting the kernel's view of the trigger (it was still treated
as level). With this patch, the kernel now trusts whatever is coming
from the firmware, and the misconfiguration becomes obvious. And just
grepping through the DT files for arm and arm64 sends makes me thing
"Holly effin' crap!".

I'm not saying that we shouldn't perform this change though. But it is
quite obvious that it is going to break an awful lot of existing code
and platforms. I'm also cooking a small patch for the arch timer (which
seems to be described in DT with a fairly high level of brokenness), so
that we can mop-up most of the brain damage.

Thanks,

	M.
Jon Hunter May 9, 2016, 1:13 p.m. UTC | #2
On 09/05/16 13:23, Marc Zyngier wrote:
> On 04/05/16 17:25, 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    | 23 ++++++++++++++++++-----
>>  2 files changed, 21 insertions(+), 5 deletions(-)
>>
>> 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 d68371213fc9..bbf5b9b8ac3d 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -564,6 +564,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;
>> @@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>  		 * it now and return the interrupt number.
>>  		 */
>>  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>> -			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;
>>  		}
>>  
>> @@ -633,10 +638,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);
>>
> 
> This patch have the effect of making misconfigured PPIs absolutely
> obvious. I still need to wrap my head around the root cause, but here's
> the findings I have so far:
>
> - kvmtool generates a DT with the wrong trigger information (edge
> instead of level) for the timer.
> - with this patch applied, "cyclictest -S" reliably locks up when run in
> a guest (missing a timer interrupt, goodbye CPU).
> - Either fixing kvmtool or reverting that patch makes it work reliably
> again.
> 
> My gut feeling is that until that patch, the failing irq_set_irq_type()
> wasn't affecting the kernel's view of the trigger (it was still treated
> as level). With this patch, the kernel now trusts whatever is coming
> from the firmware, and the misconfiguration becomes obvious. And just
> grepping through the DT files for arm and arm64 sends makes me thing
> "Holly effin' crap!".
> 
> I'm not saying that we shouldn't perform this change though. But it is
> quite obvious that it is going to break an awful lot of existing code
> and platforms. I'm also cooking a small patch for the arch timer (which
> seems to be described in DT with a fairly high level of brokenness), so
> that we can mop-up most of the brain damage.

Hmmm ... yes I see. I wonder if we should make the setting of the type
here dependent upon PM being enabled for an irqchip? We could check to
see if the .parent_device is populated and if so only then save the type
and otherwise just set it as we do today.

We could add a WARN to the existing irq_set_irq_type() or may be just a
pr_warn() if a WARN is too verbose so people can fix up any issues.

I am also wondering if patch 4/17 "iqdomain: Fix handling of type
settings for existing mappings" could generate a lot of reports
interrupts failing due to bad firmware? I wonder if I should tone this
patch down to a warning message as well as opposed to a complete failure.

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
Marc Zyngier May 9, 2016, 3:10 p.m. UTC | #3
On 09/05/16 14:13, Jon Hunter wrote:
> 
> On 09/05/16 13:23, Marc Zyngier wrote:
>> On 04/05/16 17:25, 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    | 23 ++++++++++++++++++-----
>>>  2 files changed, 21 insertions(+), 5 deletions(-)
>>>
>>> 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 d68371213fc9..bbf5b9b8ac3d 100644
>>> --- a/kernel/irq/irqdomain.c
>>> +++ b/kernel/irq/irqdomain.c
>>> @@ -564,6 +564,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;
>>> @@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>>  		 * it now and return the interrupt number.
>>>  		 */
>>>  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>> -			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;
>>>  		}
>>>  
>>> @@ -633,10 +638,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);
>>>
>>
>> This patch have the effect of making misconfigured PPIs absolutely
>> obvious. I still need to wrap my head around the root cause, but here's
>> the findings I have so far:
>>
>> - kvmtool generates a DT with the wrong trigger information (edge
>> instead of level) for the timer.
>> - with this patch applied, "cyclictest -S" reliably locks up when run in
>> a guest (missing a timer interrupt, goodbye CPU).
>> - Either fixing kvmtool or reverting that patch makes it work reliably
>> again.
>>
>> My gut feeling is that until that patch, the failing irq_set_irq_type()
>> wasn't affecting the kernel's view of the trigger (it was still treated
>> as level). With this patch, the kernel now trusts whatever is coming
>> from the firmware, and the misconfiguration becomes obvious. And just
>> grepping through the DT files for arm and arm64 sends makes me thing
>> "Holly effin' crap!".
>>
>> I'm not saying that we shouldn't perform this change though. But it is
>> quite obvious that it is going to break an awful lot of existing code
>> and platforms. I'm also cooking a small patch for the arch timer (which
>> seems to be described in DT with a fairly high level of brokenness), so
>> that we can mop-up most of the brain damage.
> 
> Hmmm ... yes I see. I wonder if we should make the setting of the type
> here dependent upon PM being enabled for an irqchip? We could check to
> see if the .parent_device is populated and if so only then save the type
> and otherwise just set it as we do today.

I don't really like the idea of having multiple code paths for the same thing.
This is very error prone, and likely to bitrot pretty quickly.

> We could add a WARN to the existing irq_set_irq_type() or may be just a
> pr_warn() if a WARN is too verbose so people can fix up any issues.
> 
> I am also wondering if patch 4/17 "iqdomain: Fix handling of type
> settings for existing mappings" could generate a lot of reports
> interrupts failing due to bad firmware? I wonder if I should tone this
> patch down to a warning message as well as opposed to a complete failure.

We'll see. We can always tone it down a notch, should it prove to be too noisy...
So far, I haven't seen it firing. On the other hand, I get the following stuff
on my APM board:

[    0.000000] GIC: PPI0 is either secure or misconfigured
[    0.000000] GIC: PPI13 is either secure or misconfigured
[    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ1, assuming level low
[    0.000000] arm_arch_timer: WARNING: Please fix your firmware
[    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2, assuming level low
[    0.000000] arm_arch_timer: WARNING: Please fix your firmware

Pretty awesome...

	M.
Jon Hunter May 9, 2016, 3:44 p.m. UTC | #4
On 09/05/16 16:10, Marc Zyngier wrote:
> On 09/05/16 14:13, Jon Hunter wrote:
>> On 09/05/16 13:23, Marc Zyngier wrote:

[snip]

>>> This patch have the effect of making misconfigured PPIs absolutely
>>> obvious. I still need to wrap my head around the root cause, but here's
>>> the findings I have so far:
>>>
>>> - kvmtool generates a DT with the wrong trigger information (edge
>>> instead of level) for the timer.
>>> - with this patch applied, "cyclictest -S" reliably locks up when run in
>>> a guest (missing a timer interrupt, goodbye CPU).
>>> - Either fixing kvmtool or reverting that patch makes it work reliably
>>> again.
>>>
>>> My gut feeling is that until that patch, the failing irq_set_irq_type()
>>> wasn't affecting the kernel's view of the trigger (it was still treated
>>> as level). With this patch, the kernel now trusts whatever is coming
>>> from the firmware, and the misconfiguration becomes obvious. And just
>>> grepping through the DT files for arm and arm64 sends makes me thing
>>> "Holly effin' crap!".
>>>
>>> I'm not saying that we shouldn't perform this change though. But it is
>>> quite obvious that it is going to break an awful lot of existing code
>>> and platforms. I'm also cooking a small patch for the arch timer (which
>>> seems to be described in DT with a fairly high level of brokenness), so
>>> that we can mop-up most of the brain damage.
>>
>> Hmmm ... yes I see. I wonder if we should make the setting of the type
>> here dependent upon PM being enabled for an irqchip? We could check to
>> see if the .parent_device is populated and if so only then save the type
>> and otherwise just set it as we do today.
> 
> I don't really like the idea of having multiple code paths for the same thing.
> This is very error prone, and likely to bitrot pretty quickly.

True. However, we really need this change for irqchips and runtime-pm.
So to confirm what are you suggesting we do? We could add a WARN around
irq_set_irq_type() in irq_create_fwspec_mapping() for v4.7 and see how
many complaints we get :-)

>> We could add a WARN to the existing irq_set_irq_type() or may be just a
>> pr_warn() if a WARN is too verbose so people can fix up any issues.
>>
>> I am also wondering if patch 4/17 "iqdomain: Fix handling of type
>> settings for existing mappings" could generate a lot of reports
>> interrupts failing due to bad firmware? I wonder if I should tone this
>> patch down to a warning message as well as opposed to a complete failure.
> 
> We'll see. We can always tone it down a notch, should it prove to be too noisy...
> So far, I haven't seen it firing. On the other hand, I get the following stuff
> on my APM board:
> 
> [    0.000000] GIC: PPI0 is either secure or misconfigured
> [    0.000000] GIC: PPI13 is either secure or misconfigured
> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ1, assuming level low
> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2, assuming level low
> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
> 
> Pretty awesome...

Indeed.

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
Marc Zyngier May 10, 2016, 12:20 p.m. UTC | #5
On 09/05/16 16:44, Jon Hunter wrote:
> 
> On 09/05/16 16:10, Marc Zyngier wrote:
>> On 09/05/16 14:13, Jon Hunter wrote:
>>> On 09/05/16 13:23, Marc Zyngier wrote:
> 
> [snip]
> 
>>>> This patch have the effect of making misconfigured PPIs absolutely
>>>> obvious. I still need to wrap my head around the root cause, but here's
>>>> the findings I have so far:
>>>>
>>>> - kvmtool generates a DT with the wrong trigger information (edge
>>>> instead of level) for the timer.
>>>> - with this patch applied, "cyclictest -S" reliably locks up when run in
>>>> a guest (missing a timer interrupt, goodbye CPU).
>>>> - Either fixing kvmtool or reverting that patch makes it work reliably
>>>> again.
>>>>
>>>> My gut feeling is that until that patch, the failing irq_set_irq_type()
>>>> wasn't affecting the kernel's view of the trigger (it was still treated
>>>> as level). With this patch, the kernel now trusts whatever is coming
>>>> from the firmware, and the misconfiguration becomes obvious. And just
>>>> grepping through the DT files for arm and arm64 sends makes me thing
>>>> "Holly effin' crap!".
>>>>
>>>> I'm not saying that we shouldn't perform this change though. But it is
>>>> quite obvious that it is going to break an awful lot of existing code
>>>> and platforms. I'm also cooking a small patch for the arch timer (which
>>>> seems to be described in DT with a fairly high level of brokenness), so
>>>> that we can mop-up most of the brain damage.
>>>
>>> Hmmm ... yes I see. I wonder if we should make the setting of the type
>>> here dependent upon PM being enabled for an irqchip? We could check to
>>> see if the .parent_device is populated and if so only then save the type
>>> and otherwise just set it as we do today.
>>
>> I don't really like the idea of having multiple code paths for the same thing.
>> This is very error prone, and likely to bitrot pretty quickly.
> 
> True. However, we really need this change for irqchips and runtime-pm.
> So to confirm what are you suggesting we do? We could add a WARN around
> irq_set_irq_type() in irq_create_fwspec_mapping() for v4.7 and see how
> many complaints we get :-)

Let's add a pr_warn(), and see how noisy this becomes. We may have to
remove it if it becomes too loud though.

Thanks,

	M.
diff mbox

Patch

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 d68371213fc9..bbf5b9b8ac3d 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -564,6 +564,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;
@@ -613,7 +614,11 @@  unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 		 * it now and return the interrupt number.
 		 */
 		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
-			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;
 		}
 
@@ -633,10 +638,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);