[Xenomai] [PATCH] x86/ipipe: reserve irq num we use for IRQ_MOVE_CLEANUP_VECTOR

[Xenomai] [PATCH] x86/ipipe: reserve irq num we use for IRQ_MOVE_CLEANUP_VECTOR

[Henning Schild]

vector_irq must not contain the same irq number for two different
vectors because ipipe dispatches based on irq numbers.
For the special case of IRQ_MOVE_CLEANUP_VECTOR, the ipipe patch
inserts a new entry in vector_irq. To ensure the required uniqueness
we have to make sure Linux will never make a vector point to irq number
IRQ_MOVE_CLEANUP_VECTOR.

Before that patch ipipe did dispatch device interrupts that happened to
receive number 32 (IRQ_MOVE_CLEANUP_VECTOR) to the cleanup function, the
device effectively lost its interrupts. Up to 3.18 that was no problem
since the allocator did not hand out the number ipipe failed to reserve.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 arch/x86/kernel/irqinit.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
index e850981..5ca0ec7 100644
--- a/arch/x86/kernel/irqinit.c
+++ b/arch/x86/kernel/irqinit.c
@@ -143,9 +143,12 @@ static void __init smp_intr_init(void)
 	/* Low priority IPI to cleanup after moving an irq */
 	set_intr_gate(IRQ_MOVE_CLEANUP_VECTOR, irq_move_cleanup_interrupt);
 	set_bit(IRQ_MOVE_CLEANUP_VECTOR, used_vectors);
+#ifdef CONFIG_IPIPE
+	BUG_ON(IRQ_MOVE_CLEANUP_VECTOR != irq_alloc_descs(IRQ_MOVE_CLEANUP_VECTOR, 0, 1, 0));
 	for_each_possible_cpu(cpu)
 		per_cpu(vector_irq, cpu)[IRQ_MOVE_CLEANUP_VECTOR] =
 			IRQ_MOVE_CLEANUP_VECTOR;
+#endif
 
 	/* IPI used for rebooting/stopping */
 	alloc_intr_gate(REBOOT_VECTOR, reboot_interrupt);
-- 
2.4.6

Re: [Xenomai] [PATCH] x86/ipipe: reserve irq num we use for IRQ_MOVE_CLEANUP_VECTOR

[Jan Kiszka]

On 2015-08-26 19:01, Henning Schild wrote:
> vector_irq must not contain the same irq number for two different
> vectors because ipipe dispatches based on irq numbers.
> For the special case of IRQ_MOVE_CLEANUP_VECTOR, the ipipe patch
> inserts a new entry in vector_irq. To ensure the required uniqueness
> we have to make sure Linux will never make a vector point to irq number
> IRQ_MOVE_CLEANUP_VECTOR.
> 
> Before that patch ipipe did dispatch device interrupts that happened to
> receive number 32 (IRQ_MOVE_CLEANUP_VECTOR) to the cleanup function, the
> device effectively lost its interrupts. Up to 3.18 that was no problem
> since the allocator did not hand out the number ipipe failed to reserve.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  arch/x86/kernel/irqinit.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
> index e850981..5ca0ec7 100644
> --- a/arch/x86/kernel/irqinit.c
> +++ b/arch/x86/kernel/irqinit.c
> @@ -143,9 +143,12 @@ static void __init smp_intr_init(void)
>  	/* Low priority IPI to cleanup after moving an irq */
>  	set_intr_gate(IRQ_MOVE_CLEANUP_VECTOR, irq_move_cleanup_interrupt);
>  	set_bit(IRQ_MOVE_CLEANUP_VECTOR, used_vectors);
> +#ifdef CONFIG_IPIPE
> +	BUG_ON(IRQ_MOVE_CLEANUP_VECTOR != irq_alloc_descs(IRQ_MOVE_CLEANUP_VECTOR, 0, 1, 0));
>  	for_each_possible_cpu(cpu)
>  		per_cpu(vector_irq, cpu)[IRQ_MOVE_CLEANUP_VECTOR] =
>  			IRQ_MOVE_CLEANUP_VECTOR;
> +#endif

Looks like you picked up the instrumentation hunk instead of the actual
fix. ;)

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [PATCH] x86/ipipe: reserve irq num we use for IRQ_MOVE_CLEANUP_VECTOR

[Henning Schild]

On Wed, 26 Aug 2015 20:32:57 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2015-08-26 19:01, Henning Schild wrote:
> > vector_irq must not contain the same irq number for two different
> > vectors because ipipe dispatches based on irq numbers.
> > For the special case of IRQ_MOVE_CLEANUP_VECTOR, the ipipe patch
> > inserts a new entry in vector_irq. To ensure the required uniqueness
> > we have to make sure Linux will never make a vector point to irq
> > number IRQ_MOVE_CLEANUP_VECTOR.
> > 
> > Before that patch ipipe did dispatch device interrupts that
> > happened to receive number 32 (IRQ_MOVE_CLEANUP_VECTOR) to the
> > cleanup function, the device effectively lost its interrupts. Up to
> > 3.18 that was no problem since the allocator did not hand out the
> > number ipipe failed to reserve.
> > 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  arch/x86/kernel/irqinit.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
> > index e850981..5ca0ec7 100644
> > --- a/arch/x86/kernel/irqinit.c
> > +++ b/arch/x86/kernel/irqinit.c
> > @@ -143,9 +143,12 @@ static void __init smp_intr_init(void)
> >  	/* Low priority IPI to cleanup after moving an irq */
> >  	set_intr_gate(IRQ_MOVE_CLEANUP_VECTOR,
> > irq_move_cleanup_interrupt); set_bit(IRQ_MOVE_CLEANUP_VECTOR,
> > used_vectors); +#ifdef CONFIG_IPIPE
> > +	BUG_ON(IRQ_MOVE_CLEANUP_VECTOR !=
> > irq_alloc_descs(IRQ_MOVE_CLEANUP_VECTOR, 0, 1, 0));
> > for_each_possible_cpu(cpu) per_cpu(vector_irq,
> > cpu)[IRQ_MOVE_CLEANUP_VECTOR] = IRQ_MOVE_CLEANUP_VECTOR;
> > +#endif
> 
> Looks like you picked up the instrumentation hunk instead of the
> actual fix. ;)

Does it? No that is the fix. Alloc the irq number before using it and
BUG if that fails.

Henning

Re: [Xenomai] [PATCH] x86/ipipe: reserve irq num we use for IRQ_MOVE_CLEANUP_VECTOR

[Gilles Chanteperdrix]

On Thu, Aug 27, 2015 at 10:42:48AM +0200, Henning Schild wrote:
> On Wed, 26 Aug 2015 20:32:57 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
> > On 2015-08-26 19:01, Henning Schild wrote:
> > > vector_irq must not contain the same irq number for two different
> > > vectors because ipipe dispatches based on irq numbers.
> > > For the special case of IRQ_MOVE_CLEANUP_VECTOR, the ipipe patch
> > > inserts a new entry in vector_irq. To ensure the required uniqueness
> > > we have to make sure Linux will never make a vector point to irq
> > > number IRQ_MOVE_CLEANUP_VECTOR.
> > > 
> > > Before that patch ipipe did dispatch device interrupts that
> > > happened to receive number 32 (IRQ_MOVE_CLEANUP_VECTOR) to the
> > > cleanup function, the device effectively lost its interrupts. Up to
> > > 3.18 that was no problem since the allocator did not hand out the
> > > number ipipe failed to reserve.
> > > 
> > > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > > ---
> > >  arch/x86/kernel/irqinit.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
> > > index e850981..5ca0ec7 100644
> > > --- a/arch/x86/kernel/irqinit.c
> > > +++ b/arch/x86/kernel/irqinit.c
> > > @@ -143,9 +143,12 @@ static void __init smp_intr_init(void)
> > >  	/* Low priority IPI to cleanup after moving an irq */
> > >  	set_intr_gate(IRQ_MOVE_CLEANUP_VECTOR,
> > > irq_move_cleanup_interrupt); set_bit(IRQ_MOVE_CLEANUP_VECTOR,
> > > used_vectors); +#ifdef CONFIG_IPIPE
> > > +	BUG_ON(IRQ_MOVE_CLEANUP_VECTOR !=
> > > irq_alloc_descs(IRQ_MOVE_CLEANUP_VECTOR, 0, 1, 0));
> > > for_each_possible_cpu(cpu) per_cpu(vector_irq,
> > > cpu)[IRQ_MOVE_CLEANUP_VECTOR] = IRQ_MOVE_CLEANUP_VECTOR;
> > > +#endif
> > 
> > Looks like you picked up the instrumentation hunk instead of the
> > actual fix. ;)
> 
> Does it? No that is the fix. Alloc the irq number before using it and
> BUG if that fails.

BUG is like an assert: disabling CONFIG_BUG should not change the
behaviour.

-- 
					    Gilles.
https://click-hack.org

Re: [Xenomai] [PATCH] x86/ipipe: reserve irq num we use for IRQ_MOVE_CLEANUP_VECTOR

[Jan Kiszka]

On 2015-08-27 11:12, Gilles Chanteperdrix wrote:
> On Thu, Aug 27, 2015 at 10:42:48AM +0200, Henning Schild wrote:
>> On Wed, 26 Aug 2015 20:32:57 +0200
>> Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>>> On 2015-08-26 19:01, Henning Schild wrote:
>>>> vector_irq must not contain the same irq number for two different
>>>> vectors because ipipe dispatches based on irq numbers.
>>>> For the special case of IRQ_MOVE_CLEANUP_VECTOR, the ipipe patch
>>>> inserts a new entry in vector_irq. To ensure the required uniqueness
>>>> we have to make sure Linux will never make a vector point to irq
>>>> number IRQ_MOVE_CLEANUP_VECTOR.
>>>>
>>>> Before that patch ipipe did dispatch device interrupts that
>>>> happened to receive number 32 (IRQ_MOVE_CLEANUP_VECTOR) to the
>>>> cleanup function, the device effectively lost its interrupts. Up to
>>>> 3.18 that was no problem since the allocator did not hand out the
>>>> number ipipe failed to reserve.
>>>>
>>>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>>>> ---
>>>>  arch/x86/kernel/irqinit.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
>>>> index e850981..5ca0ec7 100644
>>>> --- a/arch/x86/kernel/irqinit.c
>>>> +++ b/arch/x86/kernel/irqinit.c
>>>> @@ -143,9 +143,12 @@ static void __init smp_intr_init(void)
>>>>  	/* Low priority IPI to cleanup after moving an irq */
>>>>  	set_intr_gate(IRQ_MOVE_CLEANUP_VECTOR,
>>>> irq_move_cleanup_interrupt); set_bit(IRQ_MOVE_CLEANUP_VECTOR,
>>>> used_vectors); +#ifdef CONFIG_IPIPE
>>>> +	BUG_ON(IRQ_MOVE_CLEANUP_VECTOR !=
>>>> irq_alloc_descs(IRQ_MOVE_CLEANUP_VECTOR, 0, 1, 0));
>>>> for_each_possible_cpu(cpu) per_cpu(vector_irq,
>>>> cpu)[IRQ_MOVE_CLEANUP_VECTOR] = IRQ_MOVE_CLEANUP_VECTOR;
>>>> +#endif
>>>
>>> Looks like you picked up the instrumentation hunk instead of the
>>> actual fix. ;)
>>
>> Does it? No that is the fix. Alloc the irq number before using it and
>> BUG if that fails.
> 
> BUG is like an assert: disabling CONFIG_BUG should not change the
> behaviour.

Right. I actually ignored the BUG_ON while looking at the patch and,
thus, missed the allocation. Don't put anything with side effects into
such checks.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[Xenomai] [PATCHv2] x86/ipipe: reserve irq num we use for IRQ_MOVE_CLEANUP_VECTOR

[Henning Schild]

vector_irq must not contain the same irq number for two different
vectors because ipipe dispatches based on irq numbers.
For the special case of IRQ_MOVE_CLEANUP_VECTOR, the ipipe patch
inserts a new entry in vector_irq. To ensure the required uniqueness
we have to make sure Linux will never make a vector point to irq number
IRQ_MOVE_CLEANUP_VECTOR.

Before that patch ipipe did dispatch device interrupts that happened to
receive number 32 (IRQ_MOVE_CLEANUP_VECTOR) to the cleanup function, the
device effectively lost its interrupts. Up to 3.18 that was no problem
since the allocator did not hand out the number ipipe failed to reserve.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 arch/x86/kernel/irqinit.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
index e850981..5ca517f 100644
--- a/arch/x86/kernel/irqinit.c
+++ b/arch/x86/kernel/irqinit.c
@@ -126,6 +126,7 @@ static void __init smp_intr_init(void)
 #ifdef CONFIG_SMP
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
 	unsigned cpu;
+	int ret;
 
 	/*
 	 * The reschedule interrupt is a CPU-to-CPU reschedule-helper
@@ -143,9 +144,13 @@ static void __init smp_intr_init(void)
 	/* Low priority IPI to cleanup after moving an irq */
 	set_intr_gate(IRQ_MOVE_CLEANUP_VECTOR, irq_move_cleanup_interrupt);
 	set_bit(IRQ_MOVE_CLEANUP_VECTOR, used_vectors);
+#ifdef CONFIG_IPIPE
+	ret = irq_alloc_descs(IRQ_MOVE_CLEANUP_VECTOR, 0, 1, 0);
+	BUG_ON(IRQ_MOVE_CLEANUP_VECTOR != ret);
 	for_each_possible_cpu(cpu)
 		per_cpu(vector_irq, cpu)[IRQ_MOVE_CLEANUP_VECTOR] =
 			IRQ_MOVE_CLEANUP_VECTOR;
+#endif
 
 	/* IPI used for rebooting/stopping */
 	alloc_intr_gate(REBOOT_VECTOR, reboot_interrupt);
-- 
2.4.6

Re: [Xenomai] [PATCHv2] x86/ipipe: reserve irq num we use for IRQ_MOVE_CLEANUP_VECTOR

[Jan Kiszka]

On 2015-08-27 11:39, Henning Schild wrote:
> vector_irq must not contain the same irq number for two different
> vectors because ipipe dispatches based on irq numbers.
> For the special case of IRQ_MOVE_CLEANUP_VECTOR, the ipipe patch
> inserts a new entry in vector_irq. To ensure the required uniqueness
> we have to make sure Linux will never make a vector point to irq number
> IRQ_MOVE_CLEANUP_VECTOR.
> 
> Before that patch ipipe did dispatch device interrupts that happened to
> receive number 32 (IRQ_MOVE_CLEANUP_VECTOR) to the cleanup function, the
> device effectively lost its interrupts. Up to 3.18 that was no problem
> since the allocator did not hand out the number ipipe failed to reserve.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  arch/x86/kernel/irqinit.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
> index e850981..5ca517f 100644
> --- a/arch/x86/kernel/irqinit.c
> +++ b/arch/x86/kernel/irqinit.c
> @@ -126,6 +126,7 @@ static void __init smp_intr_init(void)
>  #ifdef CONFIG_SMP
>  #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
>  	unsigned cpu;
> +	int ret;

__maybe_unused, or you'll get warnings when either IPIPE or BUG is disabled.

Jan

>  
>  	/*
>  	 * The reschedule interrupt is a CPU-to-CPU reschedule-helper
> @@ -143,9 +144,13 @@ static void __init smp_intr_init(void)
>  	/* Low priority IPI to cleanup after moving an irq */
>  	set_intr_gate(IRQ_MOVE_CLEANUP_VECTOR, irq_move_cleanup_interrupt);
>  	set_bit(IRQ_MOVE_CLEANUP_VECTOR, used_vectors);
> +#ifdef CONFIG_IPIPE
> +	ret = irq_alloc_descs(IRQ_MOVE_CLEANUP_VECTOR, 0, 1, 0);
> +	BUG_ON(IRQ_MOVE_CLEANUP_VECTOR != ret);
>  	for_each_possible_cpu(cpu)
>  		per_cpu(vector_irq, cpu)[IRQ_MOVE_CLEANUP_VECTOR] =
>  			IRQ_MOVE_CLEANUP_VECTOR;
> +#endif
>  
>  	/* IPI used for rebooting/stopping */
>  	alloc_intr_gate(REBOOT_VECTOR, reboot_interrupt);
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[Xenomai] [PATCHv3] x86/ipipe: reserve irq num we use for IRQ_MOVE_CLEANUP_VECTOR

[Henning Schild]

vector_irq must not contain the same irq number for two different
vectors because ipipe dispatches based on irq numbers.
For the special case of IRQ_MOVE_CLEANUP_VECTOR, the ipipe patch
inserts a new entry in vector_irq. To ensure the required uniqueness
we have to make sure Linux will never make a vector point to irq number
IRQ_MOVE_CLEANUP_VECTOR.

Before that patch ipipe did dispatch device interrupts that happened to
receive number 32 (IRQ_MOVE_CLEANUP_VECTOR) to the cleanup function, the
device effectively lost its interrupts. Up to 3.18 that was no problem
since the allocator did not hand out the number ipipe failed to reserve.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 arch/x86/kernel/irqinit.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
index e850981..f6f3cbe 100644
--- a/arch/x86/kernel/irqinit.c
+++ b/arch/x86/kernel/irqinit.c
@@ -125,7 +125,8 @@ static void __init smp_intr_init(void)
 {
 #ifdef CONFIG_SMP
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_LOCAL_APIC)
-	unsigned cpu;
+	unsigned __maybe_unused cpu;
+	int __maybe_unused ret;
 
 	/*
 	 * The reschedule interrupt is a CPU-to-CPU reschedule-helper
@@ -143,9 +144,13 @@ static void __init smp_intr_init(void)
 	/* Low priority IPI to cleanup after moving an irq */
 	set_intr_gate(IRQ_MOVE_CLEANUP_VECTOR, irq_move_cleanup_interrupt);
 	set_bit(IRQ_MOVE_CLEANUP_VECTOR, used_vectors);
+#ifdef CONFIG_IPIPE
+	ret = irq_alloc_descs(IRQ_MOVE_CLEANUP_VECTOR, 0, 1, 0);
+	BUG_ON(IRQ_MOVE_CLEANUP_VECTOR != ret);
 	for_each_possible_cpu(cpu)
 		per_cpu(vector_irq, cpu)[IRQ_MOVE_CLEANUP_VECTOR] =
 			IRQ_MOVE_CLEANUP_VECTOR;
+#endif
 
 	/* IPI used for rebooting/stopping */
 	alloc_intr_gate(REBOOT_VECTOR, reboot_interrupt);
-- 
2.4.6