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

Groups > linux.kernel > #1713971 > unrolled thread

Re: [V6,3/9] irqdomain: Don't set type when mapping an IRQ

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: [V6,3/9] irqdomain: Don't set type when mapping an IRQ jeffy <jeffy.chen@rock-chips.com> - 2017-08-17 14:50 +0200
    Re: [V6,3/9] irqdomain: Don't set type when mapping an IRQ Marc Zyngier <marc.zyngier@arm.com> - 2017-08-17 15:40 +0200
      Re: [V6,3/9] irqdomain: Don't set type when mapping an IRQ jeffy <jeffy.chen@rock-chips.com> - 2017-08-17 16:50 +0200

#1713971 — Re: [V6,3/9] irqdomain: Don't set type when mapping an IRQ

Hi guys,

I hit an problem when testing a level triggered irq with:

irq = platform_get_irq_byname(pdev, "wake");
...<-- irq trigger type is correct
irq_set_status_flags(irq, IRQ_NOAUTOEN);
...<-- irq trigger type become zero
request_threaded_irq(irq, ...)

the trigger type lost(irqd_get_trigger_type returns zero) after 
irq_set_status_flags.


it looks like irq_set_status_flags would try to use 
irq_settings_is_level to get level trigger type, which would get zero 
since we removed set type here...could you help to check this, thanks :)



more details in https://patchwork.kernel.org/patch/9903205/

On 06/07/2016 11:12 PM, 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> Reviewed-by: Marc
> Zyngier <marc.zyngier@arm.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 f1f36e04d885..317503763314 100644 ---
> a/include/linux/irqdomain.h +++ b/include/linux/irqdomain.h @@ -452,6
> +452,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
> f3ff1eb8dd09..caa6a63d26f0 100644 --- a/kernel/irq/irqdomain.c +++
> b/kernel/irq/irqdomain.c @@ -567,6 +567,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; @@ -614,7 +615,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; }
>
> @@ -634,10 +639,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);
>

[toc] | [next] | [standalone]

#1714036

On 17/08/17 13:46, jeffy wrote:
> Hi guys,
> 
> I hit an problem when testing a level triggered irq with:
> 
> irq = platform_get_irq_byname(pdev, "wake");
> ...<-- irq trigger type is correct
> irq_set_status_flags(irq, IRQ_NOAUTOEN);
> ...<-- irq trigger type become zero
> request_threaded_irq(irq, ...)
> 
> the trigger type lost(irqd_get_trigger_type returns zero) after 
> irq_set_status_flags.
> 
> 
> it looks like irq_set_status_flags would try to use 
> irq_settings_is_level to get level trigger type, which would get zero 
> since we removed set type here...could you help to check this, thanks :)

Hmm. That's quite unexpected. Can you please try this?

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index a3cc37c0c85e..3675c6004f2a 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1000,7 +1000,7 @@ EXPORT_SYMBOL_GPL(irq_set_chip_and_handler_name);
 
 void irq_modify_status(unsigned int irq, unsigned long clr, unsigned long set)
 {
-	unsigned long flags;
+	unsigned long flags, trigger, tmp;
 	struct irq_desc *desc = irq_get_desc_lock(irq, &flags, 0);
 
 	if (!desc)
@@ -1014,6 +1014,8 @@ void irq_modify_status(unsigned int irq, unsigned long clr, unsigned long set)
 
 	irq_settings_clr_and_set(desc, clr, set);
 
+	trigger = irqd_get_trigger_type(&desc->irq_data);
+
 	irqd_clear(&desc->irq_data, IRQD_NO_BALANCING | IRQD_PER_CPU |
 		   IRQD_TRIGGER_MASK | IRQD_LEVEL | IRQD_MOVE_PCNTXT);
 	if (irq_settings_has_no_balance_set(desc))
@@ -1025,7 +1027,11 @@ void irq_modify_status(unsigned int irq, unsigned long clr, unsigned long set)
 	if (irq_settings_is_level(desc))
 		irqd_set(&desc->irq_data, IRQD_LEVEL);
 
-	irqd_set(&desc->irq_data, irq_settings_get_trigger_mask(desc));
+	tmp = irq_settings_get_trigger_mask(desc);
+	if (tmp != IRQ_TYPE_NONE)
+		trigger = tmp;
+
+	irqd_set(&desc->irq_data, trigger);
 
 	irq_put_desc_unlock(desc, flags);
 }

Basically, save the trigger info before clearing it. If the interrupt
has already been configured, use that. Otherwise restore the previous
trigger settings. But I'm slightly confused as to which field is used
for what...

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

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

#1714101

Hi Marc,

Thanks for your reply.

On 08/17/2017 09:34 PM, Marc Zyngier wrote:
> On 17/08/17 13:46, jeffy wrote:
>> Hi guys,
>>
>> I hit an problem when testing a level triggered irq with:
>>
>> irq = platform_get_irq_byname(pdev, "wake");
>> ...<-- irq trigger type is correct
>> irq_set_status_flags(irq, IRQ_NOAUTOEN);
>> ...<-- irq trigger type become zero
>> request_threaded_irq(irq, ...)
>>
>> the trigger type lost(irqd_get_trigger_type returns zero) after
>> irq_set_status_flags.
>>
>>
>> it looks like irq_set_status_flags would try to use
>> irq_settings_is_level to get level trigger type, which would get zero
>> since we removed set type here...could you help to check this, thanks :)
>
> Hmm. That's quite unexpected. Can you please try this?
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index a3cc37c0c85e..3675c6004f2a 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -1000,7 +1000,7 @@ EXPORT_SYMBOL_GPL(irq_set_chip_and_handler_name);
>
>   void irq_modify_status(unsigned int irq, unsigned long clr, unsigned long set)
>   {
> -	unsigned long flags;
> +	unsigned long flags, trigger, tmp;
>   	struct irq_desc *desc = irq_get_desc_lock(irq, &flags, 0);
>
>   	if (!desc)
> @@ -1014,6 +1014,8 @@ void irq_modify_status(unsigned int irq, unsigned long clr, unsigned long set)
>
>   	irq_settings_clr_and_set(desc, clr, set);
>
> +	trigger = irqd_get_trigger_type(&desc->irq_data);
> +
>   	irqd_clear(&desc->irq_data, IRQD_NO_BALANCING | IRQD_PER_CPU |
>   		   IRQD_TRIGGER_MASK | IRQD_LEVEL | IRQD_MOVE_PCNTXT);
>   	if (irq_settings_has_no_balance_set(desc))
> @@ -1025,7 +1027,11 @@ void irq_modify_status(unsigned int irq, unsigned long clr, unsigned long set)
>   	if (irq_settings_is_level(desc))
>   		irqd_set(&desc->irq_data, IRQD_LEVEL);
>
> -	irqd_set(&desc->irq_data, irq_settings_get_trigger_mask(desc));
> +	tmp = irq_settings_get_trigger_mask(desc);
> +	if (tmp != IRQ_TYPE_NONE)
> +		trigger = tmp;
> +
> +	irqd_set(&desc->irq_data, trigger);
>
>   	irq_put_desc_unlock(desc, flags);
>   }
>
> Basically, save the trigger info before clearing it. If the interrupt
> has already been configured, use that. Otherwise restore the previous
> trigger settings. But I'm slightly confused as to which field is used
> for what...

Thanks:), this patch works well, so

Tested-by: Jeffy Chen <jeffy.chen@rock-chips.com>

>
> 	M.
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web