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

Groups > linux.kernel > #1332404 > unrolled thread

[PATCH] arm64: make irq_stack_ptr more robust

Back to article view | Back to linux.kernel

Contents

  [PATCH] arm64: make irq_stack_ptr more robust Yang Shi <yang.shi@linaro.org> - 2016-02-11 23:20 +0100
    Re: [PATCH] arm64: make irq_stack_ptr more robust James Morse <james.morse@arm.com> - 2016-02-12 14:50 +0100
      Re: [PATCH] arm64: make irq_stack_ptr more robust "Shi, Yang" <yang.shi@linaro.org> - 2016-02-12 18:40 +0100
        Re: [PATCH] arm64: make irq_stack_ptr more robust Will Deacon <will.deacon@arm.com> - 2016-02-12 18:50 +0100
          Re: [PATCH] arm64: make irq_stack_ptr more robust "Shi, Yang" <yang.shi@linaro.org> - 2016-02-12 18:50 +0100
            Reinitiazling an arm machine without restarting the kernel or  rebooting the machine Rudici Cazeao <rudici.cazeao@litepoint.com> - 2016-02-12 19:00 +0100
        Re: [PATCH] arm64: make irq_stack_ptr more robust "Shi, Yang" <yang.shi@linaro.org> - 2016-02-12 19:00 +0100

#1332404 — [PATCH] arm64: make irq_stack_ptr more robust

Switching between stacks is only valid if we are tracing ourselves while on the
irq_stack, so it is only valid when in current and non-preemptible context,
otherwise is is just zeroed off.

Signed-off-by: Yang Shi <yang.shi@linaro.org>
---
 arch/arm64/kernel/stacktrace.c | 13 ++++++-------
 arch/arm64/kernel/traps.c      | 11 ++++++++++-
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 12a18cb..d9751a4 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -44,14 +44,13 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	unsigned long irq_stack_ptr;
 
 	/*
-	 * Use raw_smp_processor_id() to avoid false-positives from
-	 * CONFIG_DEBUG_PREEMPT. get_wchan() calls unwind_frame() on sleeping
-	 * task stacks, we can be pre-empted in this case, so
-	 * {raw_,}smp_processor_id() may give us the wrong value. Sleeping
-	 * tasks can't ever be on an interrupt stack, so regardless of cpu,
-	 * the checks will always fail.
+	 * Switching between stacks is valid when tracing current and in
+	 * non-preemptible context.
 	 */
-	irq_stack_ptr = IRQ_STACK_PTR(raw_smp_processor_id());
+	if (tsk == current && !preemptible())
+		irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
+	else
+		irq_stack_ptr = 0;
 
 	low  = frame->sp;
 	/* irq stacks are not THREAD_SIZE aligned */
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index cbedd72..7d8db3a 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -146,9 +146,18 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
 static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 {
 	struct stackframe frame;
-	unsigned long irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
+	unsigned long irq_stack_ptr;
 	int skip;
 
+	/*
+	 * Switching between  stacks is valid when tracing current and in
+	 * non-preemptible context.
+	 */
+	if (tsk == current && !preemptible())
+		irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
+	else
+		irq_stack_ptr = 0;
+
 	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
 
 	if (!tsk)
-- 
2.0.2

[toc] | [next] | [standalone]

#1332747

Hi!

On 11/02/16 21:53, Yang Shi wrote:
> Switching between stacks is only valid if we are tracing ourselves while on the
> irq_stack, so it is only valid when in current and non-preemptible context,
> otherwise is is just zeroed off.

Given it was picked up with CONFIG_DEBUG_PREEMPT:

Fixes: 132cd887b5c5 ("arm64: Modify stack trace and dump for use with irq_stack")


> Signed-off-by: Yang Shi <yang.shi@linaro.org>
> ---
>  arch/arm64/kernel/stacktrace.c | 13 ++++++-------
>  arch/arm64/kernel/traps.c      | 11 ++++++++++-
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 12a18cb..d9751a4 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -44,14 +44,13 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  	unsigned long irq_stack_ptr;
>  
>  	/*
> -	 * Use raw_smp_processor_id() to avoid false-positives from
> -	 * CONFIG_DEBUG_PREEMPT. get_wchan() calls unwind_frame() on sleeping
> -	 * task stacks, we can be pre-empted in this case, so
> -	 * {raw_,}smp_processor_id() may give us the wrong value. Sleeping
> -	 * tasks can't ever be on an interrupt stack, so regardless of cpu,
> -	 * the checks will always fail.
> +	 * Switching between stacks is valid when tracing current and in
> +	 * non-preemptible context.
>  	 */
> -	irq_stack_ptr = IRQ_STACK_PTR(raw_smp_processor_id());
> +	if (tsk == current && !preemptible())
> +		irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
> +	else
> +		irq_stack_ptr = 0;
>  
>  	low  = frame->sp;
>  	/* irq stacks are not THREAD_SIZE aligned */
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index cbedd72..7d8db3a 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -146,9 +146,18 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>  static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>  {
>  	struct stackframe frame;
> -	unsigned long irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
> +	unsigned long irq_stack_ptr;
>  	int skip;
>  
> +	/*
> +	 * Switching between  stacks is valid when tracing current and in

Nit: Two spaces: "between[ ][ ]stacks"


> +	 * non-preemptible context.
> +	 */
> +	if (tsk == current && !preemptible())
> +		irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
> +	else
> +		irq_stack_ptr = 0;
> +
>  	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
>  
>  	if (!tsk)
> 

Neither file includes 'linux/preempt.h' for the definition of preemptible().
(I can't talk: I should have included smp.h for smp_processor_id())


Acked-by: James Morse <james.morse@arm.com>
Tested-by: James Morse <james.morse@arm.com>


Thanks!

James

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

#1332966

On 2/12/2016 5:47 AM, James Morse wrote:
> Hi!
>
> On 11/02/16 21:53, Yang Shi wrote:
>> Switching between stacks is only valid if we are tracing ourselves while on the
>> irq_stack, so it is only valid when in current and non-preemptible context,
>> otherwise is is just zeroed off.
>
> Given it was picked up with CONFIG_DEBUG_PREEMPT:
>
> Fixes: 132cd887b5c5 ("arm64: Modify stack trace and dump for use with irq_stack")

Wii add in v2.

>
>
>> Signed-off-by: Yang Shi <yang.shi@linaro.org>
>> ---
>>   arch/arm64/kernel/stacktrace.c | 13 ++++++-------
>>   arch/arm64/kernel/traps.c      | 11 ++++++++++-
>>   2 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 12a18cb..d9751a4 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -44,14 +44,13 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>>   	unsigned long irq_stack_ptr;
>>
>>   	/*
>> -	 * Use raw_smp_processor_id() to avoid false-positives from
>> -	 * CONFIG_DEBUG_PREEMPT. get_wchan() calls unwind_frame() on sleeping
>> -	 * task stacks, we can be pre-empted in this case, so
>> -	 * {raw_,}smp_processor_id() may give us the wrong value. Sleeping
>> -	 * tasks can't ever be on an interrupt stack, so regardless of cpu,
>> -	 * the checks will always fail.
>> +	 * Switching between stacks is valid when tracing current and in
>> +	 * non-preemptible context.
>>   	 */
>> -	irq_stack_ptr = IRQ_STACK_PTR(raw_smp_processor_id());
>> +	if (tsk == current && !preemptible())
>> +		irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
>> +	else
>> +		irq_stack_ptr = 0;
>>
>>   	low  = frame->sp;
>>   	/* irq stacks are not THREAD_SIZE aligned */
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index cbedd72..7d8db3a 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -146,9 +146,18 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>>   static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>>   {
>>   	struct stackframe frame;
>> -	unsigned long irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
>> +	unsigned long irq_stack_ptr;
>>   	int skip;
>>
>> +	/*
>> +	 * Switching between  stacks is valid when tracing current and in
>
> Nit: Two spaces: "between[ ][ ]stacks"

Will fix in v2.

>
>
>> +	 * non-preemptible context.
>> +	 */
>> +	if (tsk == current && !preemptible())
>> +		irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
>> +	else
>> +		irq_stack_ptr = 0;
>> +
>>   	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
>>
>>   	if (!tsk)
>>
>
> Neither file includes 'linux/preempt.h' for the definition of preemptible().
> (I can't talk: I should have included smp.h for smp_processor_id())

I tried to build the kernel with preempt and without preempt, both 
works. And, I saw arch/arm64/include/asm/Kbuild has:

generic-y += preempt.h

So, it sounds preempt.h has been included by default.

Thanks,
Yang

>
>
> Acked-by: James Morse <james.morse@arm.com>
> Tested-by: James Morse <james.morse@arm.com>
>
>
> Thanks!
>
> James
>
>

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

#1332971

On Fri, Feb 12, 2016 at 09:38:51AM -0800, Shi, Yang wrote:
> On 2/12/2016 5:47 AM, James Morse wrote:
> >On 11/02/16 21:53, Yang Shi wrote:
> >>Switching between stacks is only valid if we are tracing ourselves while on the
> >>irq_stack, so it is only valid when in current and non-preemptible context,
> >>otherwise is is just zeroed off.
> >
> >Given it was picked up with CONFIG_DEBUG_PREEMPT:
> >
> >Fixes: 132cd887b5c5 ("arm64: Modify stack trace and dump for use with irq_stack")
> 
> Wii add in v2.

I already queued the patch with this and the whitespace fix, so no need
for a v2.

Will

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

#1332977

On 2/12/2016 9:41 AM, Will Deacon wrote:
> On Fri, Feb 12, 2016 at 09:38:51AM -0800, Shi, Yang wrote:
>> On 2/12/2016 5:47 AM, James Morse wrote:
>>> On 11/02/16 21:53, Yang Shi wrote:
>>>> Switching between stacks is only valid if we are tracing ourselves while on the
>>>> irq_stack, so it is only valid when in current and non-preemptible context,
>>>> otherwise is is just zeroed off.
>>>
>>> Given it was picked up with CONFIG_DEBUG_PREEMPT:
>>>
>>> Fixes: 132cd887b5c5 ("arm64: Modify stack trace and dump for use with irq_stack")
>>
>> Wii add in v2.
>
> I already queued the patch with this and the whitespace fix, so no need
> for a v2.

Thanks so much.

Yang

>
> Will
>

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

#1332982 — Reinitiazling an arm machine without restarting the kernel or rebooting the machine

All,
I have the following arch machine defined as follows:
MACHINE_START(TRANSCEDE, "Transcede 2200/3300")
      /* Maintainer: Intel Corporation */
      .boot_params  = PHYS_OFFSET + 0x00000100,
      .map_io       = transcede_map_io,
      .init_irq     = transcede_init_irq,
      .timer        = &transcede_timer,
      .init_machine = transcede_init,
      .reserve      = transcede_reserve,
MACHINE_END

I would like to be reable to reinitialize the machine in the state that it was in when it had firt booted up whithout actually rebooting or restarting the kernel.

How can I go about this?


I was thinking about restarting from the following statement from init/main.c

setup_arch(&command_line);


Is this the right way to go?


Thanks,

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

#1332984

On 2/12/2016 9:38 AM, Shi, Yang wrote:
> On 2/12/2016 5:47 AM, James Morse wrote:
>> Hi!
>>
>> On 11/02/16 21:53, Yang Shi wrote:
>>> Switching between stacks is only valid if we are tracing ourselves
>>> while on the
>>> irq_stack, so it is only valid when in current and non-preemptible
>>> context,
>>> otherwise is is just zeroed off.
>>
>> Given it was picked up with CONFIG_DEBUG_PREEMPT:
>>
>> Fixes: 132cd887b5c5 ("arm64: Modify stack trace and dump for use with
>> irq_stack")
>
> Wii add in v2.
>
>>
>>
>>> Signed-off-by: Yang Shi <yang.shi@linaro.org>
>>> ---
>>>   arch/arm64/kernel/stacktrace.c | 13 ++++++-------
>>>   arch/arm64/kernel/traps.c      | 11 ++++++++++-
>>>   2 files changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/stacktrace.c
>>> b/arch/arm64/kernel/stacktrace.c
>>> index 12a18cb..d9751a4 100644
>>> --- a/arch/arm64/kernel/stacktrace.c
>>> +++ b/arch/arm64/kernel/stacktrace.c
>>> @@ -44,14 +44,13 @@ int notrace unwind_frame(struct task_struct *tsk,
>>> struct stackframe *frame)
>>>       unsigned long irq_stack_ptr;
>>>
>>>       /*
>>> -     * Use raw_smp_processor_id() to avoid false-positives from
>>> -     * CONFIG_DEBUG_PREEMPT. get_wchan() calls unwind_frame() on
>>> sleeping
>>> -     * task stacks, we can be pre-empted in this case, so
>>> -     * {raw_,}smp_processor_id() may give us the wrong value. Sleeping
>>> -     * tasks can't ever be on an interrupt stack, so regardless of cpu,
>>> -     * the checks will always fail.
>>> +     * Switching between stacks is valid when tracing current and in
>>> +     * non-preemptible context.
>>>        */
>>> -    irq_stack_ptr = IRQ_STACK_PTR(raw_smp_processor_id());
>>> +    if (tsk == current && !preemptible())
>>> +        irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
>>> +    else
>>> +        irq_stack_ptr = 0;
>>>
>>>       low  = frame->sp;
>>>       /* irq stacks are not THREAD_SIZE aligned */
>>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>>> index cbedd72..7d8db3a 100644
>>> --- a/arch/arm64/kernel/traps.c
>>> +++ b/arch/arm64/kernel/traps.c
>>> @@ -146,9 +146,18 @@ static void dump_instr(const char *lvl, struct
>>> pt_regs *regs)
>>>   static void dump_backtrace(struct pt_regs *regs, struct task_struct
>>> *tsk)
>>>   {
>>>       struct stackframe frame;
>>> -    unsigned long irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
>>> +    unsigned long irq_stack_ptr;
>>>       int skip;
>>>
>>> +    /*
>>> +     * Switching between  stacks is valid when tracing current and in
>>
>> Nit: Two spaces: "between[ ][ ]stacks"
>
> Will fix in v2.
>
>>
>>
>>> +     * non-preemptible context.
>>> +     */
>>> +    if (tsk == current && !preemptible())
>>> +        irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
>>> +    else
>>> +        irq_stack_ptr = 0;
>>> +
>>>       pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
>>>
>>>       if (!tsk)
>>>
>>
>> Neither file includes 'linux/preempt.h' for the definition of
>> preemptible().
>> (I can't talk: I should have included smp.h for smp_processor_id())
>
> I tried to build the kernel with preempt and without preempt, both
> works. And, I saw arch/arm64/include/asm/Kbuild has:
>
> generic-y += preempt.h
>
> So, it sounds preempt.h has been included by default.

In addition, linux/sched.h, which is included by both traps.c and 
stacktrace.c, includes preempt.h already.

Yang

>
> Thanks,
> Yang
>
>>
>>
>> Acked-by: James Morse <james.morse@arm.com>
>> Tested-by: James Morse <james.morse@arm.com>
>>
>>
>> Thanks!
>>
>> James
>>
>>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web