arm: Fix build error with context tracking calls

Commit Message

Frederic Weisbecker Sept. 13, 2013, 5:03 p.m. UTC

On Tue, Sep 10, 2013 at 08:07:45AM -0700, Kevin Hilman wrote:
> Nicolas Pitre <[e-mail]> writes:
> 
> > Adding Kevin Hilman to the CC as he might be interested as well.
> 
> Thanks, I'd already noticed this but was distracted getting arm-soc
> changes ready for the merge window.  Looking into it now.

Kevin, can you please try this patch? It built tested fine on ARM
and live tested correctly on x86. There should be no problem but
just in case.

Thanks!

---
From 9bb601401baea0a2b2eead3909a9f186d894d617 Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker <[e-mail]>
Date: Tue, 10 Sep 2013 00:54:17 +0200
Subject: [PATCH] arm: Fix build error with context tracking calls

ad65782fba50 (context_tracking: Optimize main APIs off case
with static key) converted context tracking main APIs to inline
functions and left ARM asm callers behind.

This can be easily fixed by making ARM calling the post static
keys context tracking functions. We just need to replicate the
static key checks there. We'll remove these later when ARM will
support the context tracking static keys.

Reported-by: Guenter Roeck <[e-mail]>
Reported-by: Russell King <[e-mail]>
Signed-off-by: Frederic Weisbecker <[e-mail]>
Cc: Kevin Hilman <[e-mail]>
Cc: Guenter Roeck <[e-mail]>
Cc: Nicolas Pitre <[e-mail]>
Cc: Anil Kumar <[e-mail]>
Cc: Tony Lindgren <[e-mail]>
Cc: Benoit Cousson <[e-mail]>
---
 arch/arm/kernel/entry-header.S |    8 ++++----
 kernel/context_tracking.c      |   12 ++++++++++++
 2 files changed, 16 insertions(+), 4 deletions(-)

Comments

Kevin Hilman Sept. 13, 2013, 10:50 p.m. UTC | #1

Frederic Weisbecker <[e-mail]> writes:

> On Tue, Sep 10, 2013 at 08:07:45AM -0700, Kevin Hilman wrote:
>> Nicolas Pitre <[e-mail]> writes:
>> 
>> > Adding Kevin Hilman to the CC as he might be interested as well.
>> 
>> Thanks, I'd already noticed this but was distracted getting arm-soc
>> changes ready for the merge window.  Looking into it now.
>
> Kevin, can you please try this patch? It built tested fine on ARM
> and live tested correctly on x86. There should be no problem but
> just in case.

Tested-by: Kevin Hilman <[e-mail]>

Things are back to working again on ARM.  Thanks for the fix, and for
build testing on ARM.  

As you noticed to test on ARM, we still have to carry that patch to
drop that 'depends on 64BIT' in order to build/test this... any more
thoughts on what the remaining obstacles there are?

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Frederic Weisbecker Sept. 14, 2013, 2:19 p.m. UTC | #2

On Fri, Sep 13, 2013 at 03:50:07PM -0700, Kevin Hilman wrote:
> Frederic Weisbecker <[e-mail]> writes:
> 
> > On Tue, Sep 10, 2013 at 08:07:45AM -0700, Kevin Hilman wrote:
> >> Nicolas Pitre <[e-mail]> writes:
> >> 
> >> > Adding Kevin Hilman to the CC as he might be interested as well.
> >> 
> >> Thanks, I'd already noticed this but was distracted getting arm-soc
> >> changes ready for the merge window.  Looking into it now.
> >
> > Kevin, can you please try this patch? It built tested fine on ARM
> > and live tested correctly on x86. There should be no problem but
> > just in case.
> 
> Tested-by: Kevin Hilman <[e-mail]>
> 
> Things are back to working again on ARM.  Thanks for the fix, and for
> build testing on ARM.  
> 
> As you noticed to test on ARM, we still have to carry that patch to
> drop that 'depends on 64BIT' in order to build/test this... any more
> thoughts on what the remaining obstacles there are?

I was mostly fine with your last patchset that enabled that +
a few stuff to make it working on ARM. There were just a few details
to polish. But I remember I sent detailed reviews, may be you missed them
somewhow?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Russell King - ARM Linux Sept. 14, 2013, 2:32 p.m. UTC | #3

On Fri, Sep 13, 2013 at 07:03:34PM +0200, Frederic Weisbecker wrote:
> diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
> index 247091b..859c8df 100644
> --- a/kernel/context_tracking.c
> +++ b/kernel/context_tracking.c
> @@ -51,6 +51,15 @@ void context_tracking_user_enter(void)
>  	unsigned long flags;
>  
>  	/*
> +	 * Repeat the user_enter() check here because some archs may be calling
> +	 * this from asm and if no CPU needs context tracking, they shouldn't
> +	 * go further. We can remove that check here once these archs support the static key
> +	 * check.

The only (minor) comment on this is that the above would fail checkpatch,
but that's not a good enough reason to respin it if you've already applied
it IMHO.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman Sept. 16, 2013, 5:11 p.m. UTC | #4

On Sat, Sep 14, 2013 at 7:19 AM, Frederic Weisbecker <[e-mail]> wrote:
> On Fri, Sep 13, 2013 at 03:50:07PM -0700, Kevin Hilman wrote:
>> Frederic Weisbecker <[e-mail]> writes:
>>
>> > On Tue, Sep 10, 2013 at 08:07:45AM -0700, Kevin Hilman wrote:
>> >> Nicolas Pitre <[e-mail]> writes:
>> >>
>> >> > Adding Kevin Hilman to the CC as he might be interested as well.
>> >>
>> >> Thanks, I'd already noticed this but was distracted getting arm-soc
>> >> changes ready for the merge window.  Looking into it now.
>> >
>> > Kevin, can you please try this patch? It built tested fine on ARM
>> > and live tested correctly on x86. There should be no problem but
>> > just in case.
>>
>> Tested-by: Kevin Hilman <[e-mail]>
>>
>> Things are back to working again on ARM.  Thanks for the fix, and for
>> build testing on ARM.
>>
>> As you noticed to test on ARM, we still have to carry that patch to
>> drop that 'depends on 64BIT' in order to build/test this... any more
>> thoughts on what the remaining obstacles there are?
>
> I was mostly fine with your last patchset that enabled that +
> a few stuff to make it working on ARM. There were just a few details
> to polish. But I remember I sent detailed reviews, may be you missed them
> somewhow?

Hmm, looks like I missed those, but see them now.  I'll go back
through those and respin/resend.

Thanks,

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Frederic Weisbecker Sept. 22, 2013, 4:59 p.m. UTC | #5

2013/9/14, Russell King - ARM Linux <[e-mail]>:
> On Fri, Sep 13, 2013 at 07:03:34PM +0200, Frederic Weisbecker wrote:
>> diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
>> index 247091b..859c8df 100644
>> --- a/kernel/context_tracking.c
>> +++ b/kernel/context_tracking.c
>> @@ -51,6 +51,15 @@ void context_tracking_user_enter(void)
>>  	unsigned long flags;
>>
>>  	/*
>> +	 * Repeat the user_enter() check here because some archs may be calling
>> +	 * this from asm and if no CPU needs context tracking, they shouldn't
>> +	 * go further. We can remove that check here once these archs support
>> the static key
>> +	 * check.
>
> The only (minor) comment on this is that the above would fail checkpatch,
> but that's not a good enough reason to respin it if you've already applied
> it IMHO.
>

Ah I haven't yet pushed that patch anywhere so I can definetly fix
that. Thanks for the review. BTW, I initially first thought this fix
could go through your tree but if you prefer I can push it through
Ingo to the context tracking bits.

Just tell me either way.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman Sept. 25, 2013, 5:38 p.m. UTC | #6

Frederic Weisbecker <[e-mail]> writes:

> 2013/9/14, Russell King - ARM Linux <[e-mail]>:
>> On Fri, Sep 13, 2013 at 07:03:34PM +0200, Frederic Weisbecker wrote:
>>> diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
>>> index 247091b..859c8df 100644
>>> --- a/kernel/context_tracking.c
>>> +++ b/kernel/context_tracking.c
>>> @@ -51,6 +51,15 @@ void context_tracking_user_enter(void)
>>>  	unsigned long flags;
>>>
>>>  	/*
>>> +	 * Repeat the user_enter() check here because some archs may be calling
>>> +	 * this from asm and if no CPU needs context tracking, they shouldn't
>>> +	 * go further. We can remove that check here once these archs support
>>> the static key
>>> +	 * check.
>>
>> The only (minor) comment on this is that the above would fail checkpatch,
>> but that's not a good enough reason to respin it if you've already applied
>> it IMHO.
>>
>
> Ah I haven't yet pushed that patch anywhere so I can definetly fix
> that. Thanks for the review. BTW, I initially first thought this fix
> could go through your tree but if you prefer I can push it through
> Ingo to the context tracking bits.
>
> Just tell me either way.

Frederic, I suggest you queue it directly for v3.12-rc.  Thanks,

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Frederic Weisbecker Sept. 27, 2013, 1:30 p.m. UTC | #7

On Wed, Sep 25, 2013 at 10:38:58AM -0700, Kevin Hilman wrote:
> Frederic Weisbecker <[e-mail]> writes:
> 
> > 2013/9/14, Russell King - ARM Linux <[e-mail]>:
> >> On Fri, Sep 13, 2013 at 07:03:34PM +0200, Frederic Weisbecker wrote:
> >>> diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
> >>> index 247091b..859c8df 100644
> >>> --- a/kernel/context_tracking.c
> >>> +++ b/kernel/context_tracking.c
> >>> @@ -51,6 +51,15 @@ void context_tracking_user_enter(void)
> >>>  	unsigned long flags;
> >>>
> >>>  	/*
> >>> +	 * Repeat the user_enter() check here because some archs may be calling
> >>> +	 * this from asm and if no CPU needs context tracking, they shouldn't
> >>> +	 * go further. We can remove that check here once these archs support
> >>> the static key
> >>> +	 * check.
> >>
> >> The only (minor) comment on this is that the above would fail checkpatch,
> >> but that's not a good enough reason to respin it if you've already applied
> >> it IMHO.
> >>
> >
> > Ah I haven't yet pushed that patch anywhere so I can definetly fix
> > that. Thanks for the review. BTW, I initially first thought this fix
> > could go through your tree but if you prefer I can push it through
> > Ingo to the context tracking bits.
> >
> > Just tell me either way.
> 
> Frederic, I suggest you queue it directly for v3.12-rc.  Thanks,

Ok, I'm doing that then, thanks you!
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/kernel/entry-header.S b/arch/arm/kernel/entry-header.S
index de23a9b..39f89fb 100644
--- a/arch/arm/kernel/entry-header.S
+++ b/arch/arm/kernel/entry-header.S
@@ -329,10 +329,10 @@ 
 #ifdef CONFIG_CONTEXT_TRACKING
 	.if	\save
 	stmdb   sp!, {r0-r3, ip, lr}
-	bl	user_exit
+	bl	context_tracking_user_exit
 	ldmia	sp!, {r0-r3, ip, lr}
 	.else
-	bl	user_exit
+	bl	context_tracking_user_exit
 	.endif
 #endif
 	.endm
@@ -341,10 +341,10 @@ 
 #ifdef CONFIG_CONTEXT_TRACKING
 	.if	\save
 	stmdb   sp!, {r0-r3, ip, lr}
-	bl	user_enter
+	bl	context_tracking_user_enter
 	ldmia	sp!, {r0-r3, ip, lr}
 	.else
-	bl	user_enter
+	bl	context_tracking_user_enter
 	.endif
 #endif
 	.endm
diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index 247091b..859c8df 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -51,6 +51,15 @@  void context_tracking_user_enter(void)
 	unsigned long flags;
 
 	/*
+	 * Repeat the user_enter() check here because some archs may be calling
+	 * this from asm and if no CPU needs context tracking, they shouldn't
+	 * go further. We can remove that check here once these archs support the static key
+	 * check.
+	 */
+	if (!static_key_false(&context_tracking_enabled))
+		return;
+
+	/*
 	 * Some contexts may involve an exception occuring in an irq,
 	 * leading to that nesting:
 	 * rcu_irq_enter() rcu_user_exit() rcu_user_exit() rcu_irq_exit()
@@ -151,6 +160,9 @@  void context_tracking_user_exit(void)
 {
 	unsigned long flags;
 
+	if (!static_key_false(&context_tracking_enabled))
+		return;
+
 	if (in_interrupt())
 		return;