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

Groups > linux.kernel > #1486272 > unrolled thread

[PATCH] genirq: Skip chained interrupt trigger configuration if type is IRQ_TYPE_NONE

Back to article view | Back to linux.kernel

Contents

  [PATCH] genirq: Skip chained interrupt trigger configuration if type is IRQ_TYPE_NONE Marc Zyngier <marc.zyngier@arm.com> - 2016-09-19 11:00 +0200
    Re: [PATCH] genirq: Skip chained interrupt trigger configuration if  type is IRQ_TYPE_NONE Thomas Gleixner <tglx@linutronix.de> - 2016-09-19 11:20 +0200
      Re: [PATCH] genirq: Skip chained interrupt trigger configuration if  type is IRQ_TYPE_NONE Marc Zyngier <marc.zyngier@arm.com> - 2016-09-19 11:30 +0200
        Re: [PATCH] genirq: Skip chained interrupt trigger configuration if  type is IRQ_TYPE_NONE Thomas Gleixner <tglx@linutronix.de> - 2016-09-19 11:40 +0200
          Re: [PATCH] genirq: Skip chained interrupt trigger configuration if  type is IRQ_TYPE_NONE Thomas Gleixner <tglx@linutronix.de> - 2016-09-19 11:40 +0200
          Re: [PATCH] genirq: Skip chained interrupt trigger configuration if  type is IRQ_TYPE_NONE Geert Uytterhoeven <geert@linux-m68k.org> - 2016-09-19 11:40 +0200
    Re: [PATCH] genirq: Skip chained interrupt trigger configuration if  type is IRQ_TYPE_NONE Geert Uytterhoeven <geert@linux-m68k.org> - 2016-09-19 11:30 +0200
    [tip:irq/urgent] genirq: Skip chained interrupt trigger setup if  type is IRQ_TYPE_NONE tip-bot for Marc Zyngier <tipbot@zytor.com> - 2016-09-19 11:50 +0200
    [tip:irq/urgent] genirq: Skip chained interrupt trigger setup if  type is IRQ_TYPE_NONE tip-bot for Marc Zyngier <tipbot@zytor.com> - 2016-09-20 01:20 +0200
      Re: [tip:irq/urgent] genirq: Skip chained interrupt trigger setup  if type is IRQ_TYPE_NONE Thomas Gleixner <tglx@linutronix.de> - 2016-09-20 01:50 +0200

#1486272 — [PATCH] genirq: Skip chained interrupt trigger configuration if type is IRQ_TYPE_NONE

There is no point in trying to configure the trigger of a chained
interrupt if no trigger information has been configured. At best
this is ignored, and at the worse this confuses the underlying
irqchip (which is likely not to handle such a thing), and
unnecessarily alarms the user.

Only apply the configuration if type is not IRQ_TYPE_NONE.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Fixes: 1e12c4a9393b ("genirq: Correctly configure the trigger on chained interrupts")
Link: https://lkml.kernel.org/r/CAMuHMdVW1eTn20=EtYcJ8hkVwohaSuH_yQXrY2MGBEvZ8fpFOg@mail.gmail.com
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 kernel/irq/chip.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6373890..26ba565 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -820,6 +820,8 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
 	desc->name = name;
 
 	if (handle != handle_bad_irq && is_chained) {
+		unsigned int type = irqd_get_trigger_type(&desc->irq_data);
+
 		/*
 		 * We're about to start this interrupt immediately,
 		 * hence the need to set the trigger configuration.
@@ -828,8 +830,10 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
 		 * chained interrupt. Reset it immediately because we
 		 * do know better.
 		 */
-		__irq_set_trigger(desc, irqd_get_trigger_type(&desc->irq_data));
-		desc->handle_irq = handle;
+		if (type != IRQ_TYPE_NONE) {
+			__irq_set_trigger(desc, type);
+			desc->handle_irq = handle;
+		}
 
 		irq_settings_set_noprobe(desc);
 		irq_settings_set_norequest(desc);
-- 
2.1.4

[toc] | [next] | [standalone]

#1486283 — Re: [PATCH] genirq: Skip chained interrupt trigger configuration if type is IRQ_TYPE_NONE

On Mon, 19 Sep 2016, Marc Zyngier wrote:
> There is no point in trying to configure the trigger of a chained
> interrupt if no trigger information has been configured. At best
> this is ignored, and at the worse this confuses the underlying
> irqchip (which is likely not to handle such a thing), and
> unnecessarily alarms the user.
> 
> Only apply the configuration if type is not IRQ_TYPE_NONE.
> 
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Fixes: 1e12c4a9393b ("genirq: Correctly configure the trigger on chained interrupts")
> Link: https://lkml.kernel.org/r/CAMuHMdVW1eTn20=EtYcJ8hkVwohaSuH_yQXrY2MGBEvZ8fpFOg@mail.gmail.com
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  kernel/irq/chip.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 6373890..26ba565 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -820,6 +820,8 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
>  	desc->name = name;
>  
>  	if (handle != handle_bad_irq && is_chained) {
> +		unsigned int type = irqd_get_trigger_type(&desc->irq_data);
> +
>  		/*
>  		 * We're about to start this interrupt immediately,
>  		 * hence the need to set the trigger configuration.
> @@ -828,8 +830,10 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
>  		 * chained interrupt. Reset it immediately because we
>  		 * do know better.
>  		 */
> -		__irq_set_trigger(desc, irqd_get_trigger_type(&desc->irq_data));
> -		desc->handle_irq = handle;
> +		if (type != IRQ_TYPE_NONE) {
> +			__irq_set_trigger(desc, type);
> +			desc->handle_irq = handle;

Are you really sure that the handler should only be set when the trigger
type is != NONE? I seriously doubt that this is correct.

Thanks,

	tglx

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

#1486291 — Re: [PATCH] genirq: Skip chained interrupt trigger configuration if type is IRQ_TYPE_NONE

On 19/09/16 10:12, Thomas Gleixner wrote:
> On Mon, 19 Sep 2016, Marc Zyngier wrote:
>> There is no point in trying to configure the trigger of a chained
>> interrupt if no trigger information has been configured. At best
>> this is ignored, and at the worse this confuses the underlying
>> irqchip (which is likely not to handle such a thing), and
>> unnecessarily alarms the user.
>>
>> Only apply the configuration if type is not IRQ_TYPE_NONE.
>>
>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> Fixes: 1e12c4a9393b ("genirq: Correctly configure the trigger on chained interrupts")
>> Link: https://lkml.kernel.org/r/CAMuHMdVW1eTn20=EtYcJ8hkVwohaSuH_yQXrY2MGBEvZ8fpFOg@mail.gmail.com
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  kernel/irq/chip.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index 6373890..26ba565 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -820,6 +820,8 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
>>  	desc->name = name;
>>  
>>  	if (handle != handle_bad_irq && is_chained) {
>> +		unsigned int type = irqd_get_trigger_type(&desc->irq_data);
>> +
>>  		/*
>>  		 * We're about to start this interrupt immediately,
>>  		 * hence the need to set the trigger configuration.
>> @@ -828,8 +830,10 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
>>  		 * chained interrupt. Reset it immediately because we
>>  		 * do know better.
>>  		 */
>> -		__irq_set_trigger(desc, irqd_get_trigger_type(&desc->irq_data));
>> -		desc->handle_irq = handle;
>> +		if (type != IRQ_TYPE_NONE) {
>> +			__irq_set_trigger(desc, type);
>> +			desc->handle_irq = handle;
> 
> Are you really sure that the handler should only be set when the trigger
> type is != NONE? I seriously doubt that this is correct.

The handler has already been set outside of if() statement (at line
819). Here, we only set it again if we've actually called
__irq_set_trigger() which could have changed it to something that takes
the type into account (handle_level_irq or handle_edge_irq, for example).

Thanks,

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

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

#1486296 — Re: [PATCH] genirq: Skip chained interrupt trigger configuration if type is IRQ_TYPE_NONE

On Mon, 19 Sep 2016, Marc Zyngier wrote:
> On 19/09/16 10:12, Thomas Gleixner wrote:
> > On Mon, 19 Sep 2016, Marc Zyngier wrote:
> >>  	if (handle != handle_bad_irq && is_chained) {
> >> +		unsigned int type = irqd_get_trigger_type(&desc->irq_data);
> >> +
> >>  		/*
> >>  		 * We're about to start this interrupt immediately,
> >>  		 * hence the need to set the trigger configuration.
> >> @@ -828,8 +830,10 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
> >>  		 * chained interrupt. Reset it immediately because we
> >>  		 * do know better.
> >>  		 */
> >> -		__irq_set_trigger(desc, irqd_get_trigger_type(&desc->irq_data));
> >> -		desc->handle_irq = handle;
> >> +		if (type != IRQ_TYPE_NONE) {
> >> +			__irq_set_trigger(desc, type);
> >> +			desc->handle_irq = handle;
> > 
> > Are you really sure that the handler should only be set when the trigger
> > type is != NONE? I seriously doubt that this is correct.
> 
> The handler has already been set outside of if() statement (at line
> 819). Here, we only set it again if we've actually called
> __irq_set_trigger() which could have changed it to something that takes
> the type into account (handle_level_irq or handle_edge_irq, for example).

Ah. I'll add a comment...

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

#1486298 — Re: [PATCH] genirq: Skip chained interrupt trigger configuration if type is IRQ_TYPE_NONE

On Mon, 19 Sep 2016, Thomas Gleixner wrote:
> On Mon, 19 Sep 2016, Marc Zyngier wrote:
> > On 19/09/16 10:12, Thomas Gleixner wrote:
> > > On Mon, 19 Sep 2016, Marc Zyngier wrote:
> > >>  	if (handle != handle_bad_irq && is_chained) {
> > >> +		unsigned int type = irqd_get_trigger_type(&desc->irq_data);
> > >> +
> > >>  		/*
> > >>  		 * We're about to start this interrupt immediately,
> > >>  		 * hence the need to set the trigger configuration.
> > >> @@ -828,8 +830,10 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
> > >>  		 * chained interrupt. Reset it immediately because we
> > >>  		 * do know better.
> > >>  		 */
> > >> -		__irq_set_trigger(desc, irqd_get_trigger_type(&desc->irq_data));
> > >> -		desc->handle_irq = handle;
> > >> +		if (type != IRQ_TYPE_NONE) {
> > >> +			__irq_set_trigger(desc, type);
> > >> +			desc->handle_irq = handle;
> > > 
> > > Are you really sure that the handler should only be set when the trigger
> > > type is != NONE? I seriously doubt that this is correct.
> > 
> > The handler has already been set outside of if() statement (at line
> > 819). Here, we only set it again if we've actually called
> > __irq_set_trigger() which could have changed it to something that takes
> > the type into account (handle_level_irq or handle_edge_irq, for example).
> 
> Ah. I'll add a comment...

Bah. There is one already. Reading patches before being awake seems not to
be a brilliant idea.

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

#1486300 — Re: [PATCH] genirq: Skip chained interrupt trigger configuration if type is IRQ_TYPE_NONE

Hi Thomas,

On Mon, Sep 19, 2016 at 11:28 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 19 Sep 2016, Marc Zyngier wrote:
>> On 19/09/16 10:12, Thomas Gleixner wrote:
>> > On Mon, 19 Sep 2016, Marc Zyngier wrote:
>> >>    if (handle != handle_bad_irq && is_chained) {
>> >> +          unsigned int type = irqd_get_trigger_type(&desc->irq_data);
>> >> +
>> >>            /*
>> >>             * We're about to start this interrupt immediately,
>> >>             * hence the need to set the trigger configuration.
>> >> @@ -828,8 +830,10 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
>> >>             * chained interrupt. Reset it immediately because we
>> >>             * do know better.
>> >>             */
>> >> -          __irq_set_trigger(desc, irqd_get_trigger_type(&desc->irq_data));
>> >> -          desc->handle_irq = handle;
>> >> +          if (type != IRQ_TYPE_NONE) {
>> >> +                  __irq_set_trigger(desc, type);
>> >> +                  desc->handle_irq = handle;
>> >
>> > Are you really sure that the handler should only be set when the trigger
>> > type is != NONE? I seriously doubt that this is correct.
>>
>> The handler has already been set outside of if() statement (at line
>> 819). Here, we only set it again if we've actually called
>> __irq_set_trigger() which could have changed it to something that takes
>> the type into account (handle_level_irq or handle_edge_irq, for example).
>
> Ah. I'll add a comment...

The comment is already there ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

#1486289 — Re: [PATCH] genirq: Skip chained interrupt trigger configuration if type is IRQ_TYPE_NONE

Hi Marc,

On Mon, Sep 19, 2016 at 10:49 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> There is no point in trying to configure the trigger of a chained
> interrupt if no trigger information has been configured. At best
> this is ignored, and at the worse this confuses the underlying
> irqchip (which is likely not to handle such a thing), and
> unnecessarily alarms the user.
>
> Only apply the configuration if type is not IRQ_TYPE_NONE.

Thanks!

> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Fixes: 1e12c4a9393b ("genirq: Correctly configure the trigger on chained interrupts")
> Link: https://lkml.kernel.org/r/CAMuHMdVW1eTn20=EtYcJ8hkVwohaSuH_yQXrY2MGBEvZ8fpFOg@mail.gmail.com
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

#1486307 — [tip:irq/urgent] genirq: Skip chained interrupt trigger setup if type is IRQ_TYPE_NONE

Commit-ID:  1984e075915cbae65336a99b1879865080d8e55e
Gitweb:     http://git.kernel.org/tip/1984e075915cbae65336a99b1879865080d8e55e
Author:     Marc Zyngier <marc.zyngier@arm.com>
AuthorDate: Mon, 19 Sep 2016 09:49:27 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 19 Sep 2016 11:31:36 +0200

genirq: Skip chained interrupt trigger setup if type is IRQ_TYPE_NONE

There is no point in trying to configure the trigger of a chained
interrupt if no trigger information has been configured. At best
this is ignored, and at the worse this confuses the underlying
irqchip (which is likely not to handle such a thing), and
unnecessarily alarms the user.

Only apply the configuration if type is not IRQ_TYPE_NONE.

Fixes: 1e12c4a9393b ("genirq: Correctly configure the trigger on chained interrupts")
Reported-and-tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Link: https://lkml.kernel.org/r/CAMuHMdVW1eTn20=EtYcJ8hkVwohaSuH_yQXrY2MGBEvZ8fpFOg@mail.gmail.com
Link: http://lkml.kernel.org/r/1474274967-15984-1-git-send-email-marc.zyngier@arm.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 kernel/irq/chip.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6373890..26ba565 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -820,6 +820,8 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
 	desc->name = name;
 
 	if (handle != handle_bad_irq && is_chained) {
+		unsigned int type = irqd_get_trigger_type(&desc->irq_data);
+
 		/*
 		 * We're about to start this interrupt immediately,
 		 * hence the need to set the trigger configuration.
@@ -828,8 +830,10 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
 		 * chained interrupt. Reset it immediately because we
 		 * do know better.
 		 */
-		__irq_set_trigger(desc, irqd_get_trigger_type(&desc->irq_data));
-		desc->handle_irq = handle;
+		if (type != IRQ_TYPE_NONE) {
+			__irq_set_trigger(desc, type);
+			desc->handle_irq = handle;
+		}
 
 		irq_settings_set_noprobe(desc);
 		irq_settings_set_norequest(desc);

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

#1486940 — [tip:irq/urgent] genirq: Skip chained interrupt trigger setup if type is IRQ_TYPE_NONE

Commit-ID:  e2a5069355e5e3c0644a831d3e690049c4d0624b
Gitweb:     http://git.kernel.org/tip/e2a5069355e5e3c0644a831d3e690049c4d0624b
Author:     Marc Zyngier <marc.zyngier@arm.com>
AuthorDate: Mon, 19 Sep 2016 09:49:27 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 20 Sep 2016 01:04:49 +0200

genirq: Skip chained interrupt trigger setup if type is IRQ_TYPE_NONE

There is no point in trying to configure the trigger of a chained
interrupt if no trigger information has been configured. At best
this is ignored, and at the worse this confuses the underlying
irqchip (which is likely not to handle such a thing), and
unnecessarily alarms the user.

Only apply the configuration if type is not IRQ_TYPE_NONE.

Fixes: 1e12c4a9393b ("genirq: Correctly configure the trigger on chained interrupts")
Reported-and-tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Link: https://lkml.kernel.org/r/CAMuHMdVW1eTn20=EtYcJ8hkVwohaSuH_yQXrY2MGBEvZ8fpFOg@mail.gmail.com
Link: http://lkml.kernel.org/r/1474274967-15984-1-git-send-email-marc.zyngier@arm.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/irq-gic-v3.c | 7 ++++---
 kernel/irq/chip.c            | 8 ++++++--
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index ede5672..da6c0ba 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -548,7 +548,7 @@ static int gic_starting_cpu(unsigned int cpu)
 static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
 				   unsigned long cluster_id)
 {
-	int cpu = *base_cpu;
+	int next_cpu, cpu = *base_cpu;
 	unsigned long mpidr = cpu_logical_map(cpu);
 	u16 tlist = 0;
 
@@ -562,9 +562,10 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
 
 		tlist |= 1 << (mpidr & 0xf);
 
-		cpu = cpumask_next(cpu, mask);
-		if (cpu >= nr_cpu_ids)
+		next_cpu = cpumask_next(cpu, mask);
+		if (next_cpu >= nr_cpu_ids)
 			goto out;
+		cpu = next_cpu;
 
 		mpidr = cpu_logical_map(cpu);
 
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6373890..26ba565 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -820,6 +820,8 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
 	desc->name = name;
 
 	if (handle != handle_bad_irq && is_chained) {
+		unsigned int type = irqd_get_trigger_type(&desc->irq_data);
+
 		/*
 		 * We're about to start this interrupt immediately,
 		 * hence the need to set the trigger configuration.
@@ -828,8 +830,10 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
 		 * chained interrupt. Reset it immediately because we
 		 * do know better.
 		 */
-		__irq_set_trigger(desc, irqd_get_trigger_type(&desc->irq_data));
-		desc->handle_irq = handle;
+		if (type != IRQ_TYPE_NONE) {
+			__irq_set_trigger(desc, type);
+			desc->handle_irq = handle;
+		}
 
 		irq_settings_set_noprobe(desc);
 		irq_settings_set_norequest(desc);

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

#1486959 — Re: [tip:irq/urgent] genirq: Skip chained interrupt trigger setup if type is IRQ_TYPE_NONE

On Mon, 19 Sep 2016, tip-bot for Marc Zyngier wrote:

> Commit-ID:  e2a5069355e5e3c0644a831d3e690049c4d0624b
> Gitweb:     http://git.kernel.org/tip/e2a5069355e5e3c0644a831d3e690049c4d0624b
> Author:     Marc Zyngier <marc.zyngier@arm.com>
> AuthorDate: Mon, 19 Sep 2016 09:49:27 +0100
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Tue, 20 Sep 2016 01:04:49 +0200
> 
> genirq: Skip chained interrupt trigger setup if type is IRQ_TYPE_NONE

Bah. I completely fatfingered that one ... Fixing.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web