[PATCH 0/5] RT: Misc. fixups for ARM/OMAP against 2.6.25.4-rt3

[PATCH 0/5] RT: Misc. fixups for ARM/OMAP against 2.6.25.4-rt3

[Kevin Hilman]

What follows are a series of minor fixes to get -rt3 building/booting on 
  ARM.  The first 3 are ready do be included into the -rt patch, while 
the last 4th one (cmpxchg duplication) needs a little investigating. 
The 5th one needs to go upstream via the OMAP tree.

This was build/boot tested on the TI OMAP3430 SDP platform

Kevin

[PATCH 1/5] RT: ARM: remove non-existant trace_preempt_*_idle calls from idle loop

[Kevin Hilman]

Signed-off-by: Kevin Hilman <khilman@mvista.com>
---
 arch/arm/kernel/process.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 9308346..a0aad84 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -171,12 +171,10 @@ void cpu_idle(void)
 			idle();
 		leds_event(led_idle_end);
 		local_irq_disable();
-		trace_preempt_exit_idle();
 		tick_nohz_restart_sched_tick();
 		__preempt_enable_no_resched();
 		__schedule();
 		preempt_disable();
-		trace_preempt_enter_idle();
 		local_irq_enable();
 	}
 }
-- 
1.5.4.1.166.g6706d

[PATCH 2/5] RT: wrap BUILD_CHECK_IRQ_FLAGS

[Kevin Hilman]

The macros used by BUILD_CHECK_IRQ_FLAGS() are defined in kernel.h,
but this file is sometimes included before kernel.h (or even by
kernel.h)

Signed-off-by: Kevin Hilman <khilman@mvista.com>
---
 include/linux/irqflags.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 6ada2b9..e3bf9d0 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -11,11 +11,15 @@
 #ifndef _LINUX_TRACE_IRQFLAGS_H
 #define _LINUX_TRACE_IRQFLAGS_H
 
+#if defined(BUILD_CHECK_IRQ_FLAGS) && defined(typecheck)
 #define BUILD_CHECK_IRQ_FLAGS(flags)					\
 	do {								\
 		BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long));	\
 		typecheck(unsigned long, flags);			\
 	} while (0)
+#else
+#define BUILD_CHECK_IRQ_FLAGS(flags)
+#endif
 
 #ifdef CONFIG_TRACE_IRQFLAGS
   extern void trace_softirqs_on(unsigned long ip);
-- 
1.5.4.1.166.g6706d

[PATCH 3/5] RT: ARM: remove __attribute_used__ from semaphore code

[Kevin Hilman]

Signed-off-by: Kevin Hilman <khilman@mvista.com>
---
 arch/arm/kernel/semaphore.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/semaphore.c b/arch/arm/kernel/semaphore.c
index 6036f57..11f7d04 100644
--- a/arch/arm/kernel/semaphore.c
+++ b/arch/arm/kernel/semaphore.c
@@ -49,7 +49,7 @@
  *    we cannot lose wakeup events.
  */
 
- void __attribute_used__ __compat_up(struct compat_semaphore *sem)
+ void __used __compat_up(struct compat_semaphore *sem)
 {
 	wake_up(&sem->wait);
 }
@@ -58,7 +58,7 @@ EXPORT_SYMBOL(__compat_up);
 
 static DEFINE_SPINLOCK(semaphore_lock);
 
- void __attribute_used__ __sched __compat_down(struct compat_semaphore * sem)
+ void __used __sched __compat_down(struct compat_semaphore * sem)
 {
 	struct task_struct *tsk = current;
 	DECLARE_WAITQUEUE(wait, tsk);
@@ -93,7 +93,7 @@ static DEFINE_SPINLOCK(semaphore_lock);
 
 EXPORT_SYMBOL(__compat_down);
 
- int __attribute_used__ __sched __compat_down_interruptible(struct compat_semaphore * sem)
+ int __used __sched __compat_down_interruptible(struct compat_semaphore * sem)
 {
 	int retval = 0;
 	struct task_struct *tsk = current;
@@ -154,7 +154,7 @@ EXPORT_SYMBOL(__compat_down_interruptible);
  * single "cmpxchg" without failure cases,
  * but then it wouldn't work on a 386.
  */
- int __attribute_used__ __sched __compat_down_trylock(struct compat_semaphore * sem)
+ int __used __sched __compat_down_trylock(struct compat_semaphore * sem)
 {
 	int sleepers;
 	unsigned long flags;
-- 
1.5.4.1.166.g6706d

[RFC/PATCH 4/5] RT: ARM: disable duplicate cmpxchg

[Kevin Hilman]

FIXME: The ARM cmpxchg macro conflicts with the generic ones from
cmpxchg-local.

Signed-off-by: Kevin Hilman <khilman@mvista.com>
---
 include/asm-arm/atomic.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/asm-arm/atomic.h b/include/asm-arm/atomic.h
index b5ba735..7cdc2fd 100644
--- a/include/asm-arm/atomic.h
+++ b/include/asm-arm/atomic.h
@@ -146,7 +146,7 @@ static inline unsigned long __cmpxchg(volatile void *ptr,
 		return wrong_size_cmpxchg(ptr);
 }
 
-#define cmpxchg(ptr,o,n)						\
+#define cmpxchg__disabled(ptr,o,n)						\
 ({									\
 	__typeof__(*(ptr)) _o_ = (o);					\
 	__typeof__(*(ptr)) _n_ = (n);					\
-- 
1.5.4.1.166.g6706d

[PATCH 4/5] RT: ARM: disable duplicate cmpxchg

[Kevin Hilman]

FIXME: The ARM cmpxchg macro conflicts with the generic ones from
cmpxchg-local.

Signed-off-by: Kevin Hilman <khilman@mvista.com>
---
 include/asm-arm/atomic.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/asm-arm/atomic.h b/include/asm-arm/atomic.h
index b5ba735..7cdc2fd 100644
--- a/include/asm-arm/atomic.h
+++ b/include/asm-arm/atomic.h
@@ -146,7 +146,7 @@ static inline unsigned long __cmpxchg(volatile void *ptr,
 		return wrong_size_cmpxchg(ptr);
 }
 
-#define cmpxchg(ptr,o,n)						\
+#define cmpxchg__disabled(ptr,o,n)						\
 ({									\
 	__typeof__(*(ptr)) _o_ = (o);					\
 	__typeof__(*(ptr)) _n_ = (n);					\
-- 
1.5.4.1.166.g6706d

[PATCH 5/5] ARM: OMAP: remove unnecessary locking in clk_get_rate()

[Kevin Hilman]

The locking in the get_rate() hook is unnecessary, and causes problems
when used with the -rt patch, since it may be called recursively.

Signed-off-by: Kevin Hilman <khilman@mvista.com>
---
 arch/arm/plat-omap/clock.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
index 714dbbf..3842540 100644
--- a/arch/arm/plat-omap/clock.c
+++ b/arch/arm/plat-omap/clock.c
@@ -134,15 +134,12 @@ EXPORT_SYMBOL(clk_get_usecount);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
-	unsigned long flags;
 	unsigned long ret = 0;
 
 	if (clk == NULL || IS_ERR(clk))
 		return 0;
 
-	spin_lock_irqsave(&clockfw_lock, flags);
 	ret = clk->rate;
-	spin_unlock_irqrestore(&clockfw_lock, flags);
 
 	return ret;
 }
-- 
1.5.4.1.166.g6706d

Re: [PATCH 5/5] ARM: OMAP: remove unnecessary locking in clk_get_rate()

[Paul Walmsley]

Hi Kevin,

On Tue, 20 May 2008, Kevin Hilman wrote:

> The locking in the get_rate() hook is unnecessary, and causes problems
> when used with the -rt patch, since it may be called recursively.
> 
> Signed-off-by: Kevin Hilman <khilman@mvista.com>

Acked-by: Paul Walmsley <paul@pwsan.com>

BTW, looks like we could probably get rid of some of the other spinlocking 
in plat-omap/clock.c also -- clk_get_usecount() ?

- Paul

Re: [PATCH 5/5] ARM: OMAP: remove unnecessary locking in clk_get_rate()

[Kevin Hilman]

Paul Walmsley wrote:
> 
> Hi Kevin,
> 
> On Tue, 20 May 2008, Kevin Hilman wrote:
> 
>  > The locking in the get_rate() hook is unnecessary, and causes problems
>  > when used with the -rt patch, since it may be called recursively.
>  >
>  > Signed-off-by: Kevin Hilman <khilman@mvista.com>
> 
> Acked-by: Paul Walmsley <paul@pwsan.com>
> 
> BTW, looks like we could probably get rid of some of the other spinlocking
> in plat-omap/clock.c also -- clk_get_usecount() ?

I agree, most of the locking in there seems unnecessary, but this was 
the only one that could be removed without looking closer at at all the 
platform specific clock code.

Basically, I think there minimal (maybe no) locking in the generic 
layer, and the locking should be done if necessary in the platform 
specific parts.

Kevin

Re: [PATCH 5/5] ARM: OMAP: remove unnecessary locking in clk_get_rate()

[Tony Lindgren]

* Kevin Hilman <khilman@mvista.com> [080604 09:37]:
> Paul Walmsley wrote:
>>
>> Hi Kevin,
>>
>> On Tue, 20 May 2008, Kevin Hilman wrote:
>>
>>  > The locking in the get_rate() hook is unnecessary, and causes problems
>>  > when used with the -rt patch, since it may be called recursively.
>>  >
>>  > Signed-off-by: Kevin Hilman <khilman@mvista.com>
>>
>> Acked-by: Paul Walmsley <paul@pwsan.com>
>>
>> BTW, looks like we could probably get rid of some of the other spinlocking
>> in plat-omap/clock.c also -- clk_get_usecount() ?
>
> I agree, most of the locking in there seems unnecessary, but this was  
> the only one that could be removed without looking closer at at all the  
> platform specific clock code.
>
> Basically, I think there minimal (maybe no) locking in the generic  
> layer, and the locking should be done if necessary in the platform  
> specific parts.

To me it seems locking is needed. What if something is changing a clock's
rate and then something else calls get_rate() while the other thread is
changing the rate?

If get_rate() is not locking, get_rate() may get invalid rate between
something else programming the hardware and changing clk->rate. I guess
just locking set_rate() would still work with local irq disabled,
but would not be SMP safe.

Anyways, ideally the spin_lock would be specific to the clocks accessing
the same clock registers... But that would mean adding yet another field
to struct clk or clock domains. Anybody got better ideas?

And of course anything going through the clocks list needs to be protected
by the mutex.

Tony

Re: [PATCH 5/5] ARM: OMAP: remove unnecessary locking in clk_get_rate()

[Kevin Hilman]

Tony Lindgren wrote:
> * Kevin Hilman <khilman@mvista.com> [080604 09:37]:
>> Paul Walmsley wrote:
>>> Hi Kevin,
>>>
>>> On Tue, 20 May 2008, Kevin Hilman wrote:
>>>
>>>  > The locking in the get_rate() hook is unnecessary, and causes problems
>>>  > when used with the -rt patch, since it may be called recursively.
>>>  >
>>>  > Signed-off-by: Kevin Hilman <khilman@mvista.com>
>>>
>>> Acked-by: Paul Walmsley <paul@pwsan.com>
>>>
>>> BTW, looks like we could probably get rid of some of the other spinlocking
>>> in plat-omap/clock.c also -- clk_get_usecount() ?
>> I agree, most of the locking in there seems unnecessary, but this was  
>> the only one that could be removed without looking closer at at all the  
>> platform specific clock code.
>>
>> Basically, I think there minimal (maybe no) locking in the generic  
>> layer, and the locking should be done if necessary in the platform  
>> specific parts.
> 
> To me it seems locking is needed. What if something is changing a clock's
> rate and then something else calls get_rate() while the other thread is
> changing the rate?

Then the caller of get_rate() has an out of date value.

The same can happen even with the locking.  If someone asks for the 
rate, and then another thread changes the rate right after, the thread 
who asked for the rate has an out-of-date value.

That being said, there may be some locks that need to stay, I haven't 
done a thorough analysis of this.  I just think that there is currently 
to much unnecessary locking.

> If get_rate() is not locking, get_rate() may get invalid rate between
> something else programming the hardware and changing clk->rate. I guess
> just locking set_rate() would still work with local irq disabled,
> but would not be SMP safe.
> 
> Anyways, ideally the spin_lock would be specific to the clocks accessing
> the same clock registers... But that would mean adding yet another field
> to struct clk or clock domains. Anybody got better ideas?
> 
> And of course anything going through the clocks list needs to be protected
> by the mutex.

Agreed, the mutex needs to stay.

Kevin

Re: [PATCH 5/5] ARM: OMAP: remove unnecessary locking in clk_get_rate()

[Igor Stoppa]

Hi,

On Wed, 2008-06-04 at 17:11 -0700, ext Kevin Hilman wrote:

> The same can happen even with the locking.  If someone asks for the 
> rate, and then another thread changes the rate right after, the thread 
> who asked for the rate has an out-of-date value.

I don't understand how this would materialize in practice:

- if the rate is beign changed by 2 threads from the same driver, then
the driver itself needs to take care of its internal state and coherency

- if 2 different drivers are contending the same clock, then 2 children
clocks should be created and the clock fw would take care of failing the
request of the 2nd child, if it is incompatible with the rate set by the
1st one

- if a dvfs change is responsible of changing the rate supplied to a
driver which asked it with clk_get_rate(), the dvfs transition has no
reason to happen in case it would violate the constraints set by clock
users.

Does this make sense?

-- 
Cheers, Igor

---

Igor Stoppa
Nokia Devices R&D - Helsinki

Re: [PATCH 5/5] ARM: OMAP: remove unnecessary locking in clk_get_rate()

[Paul Walmsley]

Hello,

On Thu, 5 Jun 2008, Igor Stoppa wrote:

> - if a dvfs change is responsible of changing the rate supplied to a
> driver which asked it with clk_get_rate(), the dvfs transition has no
> reason to happen in case it would violate the constraints set by clock
> users.

There aren't any such constraints in the current tree.

clk_set_rate() is mostly only useful for code that doesn't expect the 
clock rate to be changed by other code.  Pre- and post-clk-rate-change 
notifiers should fix this for code that does need to know clock rates 
precisely.


- Paul

Re: [PATCH 0/5] RT: Misc. fixups for ARM/OMAP against 2.6.25.4-rt3

[Luís Vitório Cargnini]

well I get the following error in my source code compilation time:
       à partir de kernel/rtmutex.c:13:
include/asm/atomic.h:241:1: attention : « cmpxchg » redéfini
Dans le fichier inclus à partir de include/asm/system.h:378,
          à partir de include/asm/bitops.h:27,
          à partir de include/linux/bitops.h:17,
          à partir de include/linux/thread_info.h:34,
          à partir de include/linux/preempt.h:9,
          à partir de include/linux/spinlock.h:85,
          à partir de kernel/rtmutex.c:13:
include/asm-generic/cmpxchg.h:19:1: attention : ceci est la
localisation d'une précédente définition
kernel/rtmutex.c:124: erreur: redefinition of 'init_lists'
kernel/rtmutex.c:110: erreur: previous definition of 'init_lists' was here
kernel/rtmutex.c: In function 'do_try_to_take_rt_mutex':
kernel/rtmutex.c:425: erreur: too many arguments to function 'try_to_steal_lock'
kernel/rtmutex.c: In function 'rt_spin_lock_slowlock':
kernel/rtmutex.c:787: erreur: 'saved_lock_depth' undeclared (first use
in this function)
kernel/rtmutex.c:787: erreur: (Each undeclared identifier is reported only once
kernel/rtmutex.c:787: erreur: for each function it appears in.)
kernel/rtmutex.c:788: erreur: implicit declaration of function
'rt_reacquire_bkl'
kernel/rtmutex.c: Hors de toute fonction :
kernel/rtmutex.c:1239: erreur: redefinition of 'rt_mutex_lock_killable'
kernel/rtmutex.c:1218: erreur: previous definition of
'rt_mutex_lock_killable' was here
kernel/rtmutex.c:1245: erreur: redefinition of
'__kstrtab_rt_mutex_lock_killable'
kernel/rtmutex.c:1224: erreur: previous definition of
'__kstrtab_rt_mutex_lock_killable' was here
kernel/rtmutex.c:1245: erreur: redefinition of
'__ksymtab_rt_mutex_lock_killable'
kernel/rtmutex.c:1224: erreur: previous definition of
'__ksymtab_rt_mutex_lock_killable' was here
make[1]: *** [kernel/rtmutex.o] Error 1
make: *** [kernel] Error 2


and the following warning is noisy been shown:
include/asm/atomic.h:241:1: attention : « cmpxchg » redéfini
Dans le fichier inclus à partir de include/asm/system.h:378,


On Tue, May 20, 2008 at 7:42 PM, Kevin Hilman <khilman@mvista.com> wrote:
> What follows are a series of minor fixes to get -rt3 building/booting on
>  ARM.  The first 3 are ready do be included into the -rt patch, while the
> last 4th one (cmpxchg duplication) needs a little investigating. The 5th one
> needs to go upstream via the OMAP tree.
>
> This was build/boot tested on the TI OMAP3430 SDP platform
>
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
------------------------------------------------------------------------------
Thanks && Regards
M.Sc. B.Sc. Luís Vitório Cargnini
IEEE Member
Electrical Engineer Faculty @ PUCRS
Ipiranga Avenue, 6681 – Building 30
P.O. Box: 90619-900 – Porto Alegre/RS
Phone: +55 51 3320 3500 extension: 7696
---------------------------------------------------------------------------------