[PATCH] Real-Time Preemption V0.7.52-07: rt_init_MUTEX_LOCKED declaration

[PATCH] Real-Time Preemption V0.7.52-07: rt_init_MUTEX_LOCKED declaration

[Luca Falavigna]

This patch fixes broken rt_init_MUTEX_LOCKED declaration using rt_sema_init()
macro. This way we fix a potential compile bug: rt_init_MUTEX_LOCKED calls
there_is_no_init_MUTEX_LOCKED_for_RT_semaphores, which is not referenced.
(e.g. drivers/char/watchdog/cpu5wdt.c: "cpu5wdt: Unknown symbol
there_is_no_init_MUTEX_LOCKED_for_RT_semaphores")



Signed-off-by: Luca Falavigna <dktrkranz@gmail.com>

--- realtime-preempt-2.6.13-rc4-RT-V0.7.52-07.orig	2005-07-31 23:35:18.000000000
+0000
+++ realtime-preempt-2.6.13-rc4-RT-V0.7.52-07		2005-08-01 15:59:31.000000000 +0000
@@ -8342,16 +8342,6 @@ Index: linux/drivers/cpufreq/cpufreq.c
 ===================================================================
 --- linux.orig/drivers/cpufreq/cpufreq.c
 +++ linux/drivers/cpufreq/cpufreq.c
-@@ -605,7 +605,8 @@ static int cpufreq_add_dev (struct sys_d
- 	policy->cpu = cpu;
- 	policy->cpus = cpumask_of_cpu(cpu);
-
--	init_MUTEX_LOCKED(&policy->lock);
-+	init_MUTEX(&policy->lock);
-+	down(&policy->lock);
- 	init_completion(&policy->kobj_unregister);
- 	INIT_WORK(&policy->update, handle_update, (void *)(long)cpu);
-
 @@ -614,6 +615,7 @@ static int cpufreq_add_dev (struct sys_d
  	 */
  	ret = cpufreq_driver->init(policy);
@@ -14350,7 +14340,7 @@ Index: linux/include/linux/rt_lock.h
 ===================================================================
 --- /dev/null
 +++ linux/include/linux/rt_lock.h
-@@ -0,0 +1,391 @@
+@@ -0,0 +1,385 @@
 +#ifndef __LINUX_RT_LOCK_H
 +#define __LINUX_RT_LOCK_H
 +
@@ -14589,14 +14579,8 @@ Index: linux/include/linux/rt_lock.h
 +extern void FASTCALL(__init_MUTEX(struct semaphore *sem, char *name, char
*file, int line));
 +#define rt_init_MUTEX(sem) \
 +		__init_MUTEX(sem, #sem, __FILE__, __LINE__)
-+
-+extern void there_is_no_init_MUTEX_LOCKED_for_RT_semaphores(void);
-+
-+/*
-+ * No locked initialization for RT semaphores
-+ */
 +#define rt_init_MUTEX_LOCKED(sem) \
-+		there_is_no_init_MUTEX_LOCKED_for_RT_semaphores()
++		rt_sema_init(sem, 0)
 +extern void FASTCALL(rt_down(struct semaphore *sem));
 +extern int FASTCALL(rt_down_interruptible(struct semaphore *sem));
 +extern int FASTCALL(rt_down_trylock(struct semaphore *sem));




Regards,
-- 
					Luca

Re: [PATCH] Real-Time Preemption V0.7.52-07: rt_init_MUTEX_LOCKED declaration

[Steven Rostedt]

On Mon, 2005-08-01 at 16:26 +0000, Luca Falavigna wrote:
> This patch fixes broken rt_init_MUTEX_LOCKED declaration using rt_sema_init()
> macro. This way we fix a potential compile bug: rt_init_MUTEX_LOCKED calls
> there_is_no_init_MUTEX_LOCKED_for_RT_semaphores, which is not referenced.
> (e.g. drivers/char/watchdog/cpu5wdt.c: "cpu5wdt: Unknown symbol
> there_is_no_init_MUTEX_LOCKED_for_RT_semaphores")
> 
> 

Ingo,

When did you solve the problem of ownership of locked semaphores? ;-)

Luca,
Unless Ingo did solve the problem of semaphores that can be locked by
one task and unlocked by another task, I wouldn't use your patch.
There's a problem with priority inheritance when it comes to these
semaphores.  That is who owns a locked semaphore that will later be
unlocked by someone else?  When a RT process blocks on this semaphore,
who does it boost to release it?  Ingo purposely put this in to crash
the compile so that we know where this can be a problem right away.

The patch you wanted to send was:

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Index: linux_realtime_ernie/drivers/char/watchdog/cpu5wdt.c
===================================================================
--- linux_realtime_ernie/drivers/char/watchdog/cpu5wdt.c	(revision 265)
+++ linux_realtime_ernie/drivers/char/watchdog/cpu5wdt.c	(working copy)
@@ -56,7 +56,7 @@
 /* some device data */
 
 static struct {
-	struct semaphore stop;
+	struct compat_semaphore stop;
 	volatile int running;
 	struct timer_list timer;
 	volatile int queue;

Re: [PATCH] Real-Time Preemption V0.7.52-07: rt_init_MUTEX_LOCKED declaration

[Luca Falavigna]

> Ingo purposely put this in to crash
> the compile so that we know where this can be a problem right away.
And it works nice ;)

> The patch you wanted to send was:
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Index: linux_realtime_ernie/drivers/char/watchdog/cpu5wdt.c
> ===================================================================
> --- linux_realtime_ernie/drivers/char/watchdog/cpu5wdt.c        (revision 265)
> +++ linux_realtime_ernie/drivers/char/watchdog/cpu5wdt.c        (working copy)
> @@ -56,7 +56,7 @@
>  /* some device data */
> 
>  static struct {
> -       struct semaphore stop;
> +       struct compat_semaphore stop;
>         volatile int running;
>         struct timer_list timer;
>         volatile int queue;

Another solution could be this (as shown in drivers/cpufreq/cpufreq.c):
-	init_MUTEX_LOCKED(&policy->lock);
+	init_MUTEX(&policy->lock);
+	down(&policy->lock);

Re: [PATCH] Real-Time Preemption V0.7.52-07: rt_init_MUTEX_LOCKED declaration

[Sven-Thorsten Dietrich]

On Mon, 2005-08-01 at 19:05 +0000, Luca Falavigna wrote:
> > The patch you wanted to send was:
> > 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > Index: linux_realtime_ernie/drivers/char/watchdog/cpu5wdt.c
> > ===================================================================
> > --- linux_realtime_ernie/drivers/char/watchdog/cpu5wdt.c        (revision 265)
> > +++ linux_realtime_ernie/drivers/char/watchdog/cpu5wdt.c        (working copy)
> > @@ -56,7 +56,7 @@
> >  /* some device data */
> > 
> >  static struct {
> > -       struct semaphore stop;
> > +       struct compat_semaphore stop;
> >         volatile int running;
> >         struct timer_list timer;
> >         volatile int queue;
> 
> Another solution could be this (as shown in drivers/cpufreq/cpufreq.c):
> -	init_MUTEX_LOCKED(&policy->lock);
> +	init_MUTEX(&policy->lock);
> +	down(&policy->lock);
> -

If the semaphore is being used as a mutex, 
then it could be converted to an RT-mutex.

That would nicely side-step the problem.

Note this won't work if you are counting with the sema.

Sven

Re: [PATCH] Real-Time Preemption V0.7.52-07: rt_init_MUTEX_LOCKED declaration

[Steven Rostedt]

On Mon, 2005-08-01 at 12:13 -0700, Sven-Thorsten Dietrich wrote:
> On Mon, 2005-08-01 at 19:05 +0000, Luca Falavigna wrote:
> >
> > 
> > Another solution could be this (as shown in drivers/cpufreq/cpufreq.c):
> > -	init_MUTEX_LOCKED(&policy->lock);
> > +	init_MUTEX(&policy->lock);
> > +	down(&policy->lock);
> > -
> 
> If the semaphore is being used as a mutex, 
> then it could be converted to an RT-mutex.
> 
> That would nicely side-step the problem.
> 
> Note this won't work if you are counting with the sema.

Well it's not really a counter.  It's one of these situations where the
semaphore is locked on init, later someone else ups it on a trigger, and
then the most horrible, it gets down again on module exit! I'm not
really sure what it is doing. The only place that an up is called is on
the watchdog trigger.  I guess it's being used to not let you unload the
module if the watchdog hasn't gone off yet, or something to that effect.

-- Steve

Re: [PATCH] Real-Time Preemption V0.7.52-07: rt_init_MUTEX_LOCKED declaration

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> -	struct semaphore stop;
> +	struct compat_semaphore stop;

i think it's policy->lock that is the issue here?

	Ingo

Re: [PATCH] Real-Time Preemption V0.7.52-07: rt_init_MUTEX_LOCKED declaration

[Steven Rostedt]

On Mon, 2005-08-01 at 23:03 +0200, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > -	struct semaphore stop;
> > +	struct compat_semaphore stop;
> 
> i think it's policy->lock that is the issue here?
> 

I was looking at Luca's original message where he showed the bug of 
-- drivers/char/watchdog/cpu5wdt.c: "cpu5wdt: Unknown symbol
there_is_no_init_MUTEX_LOCKED_for_RT_semaphores") --
Looking into this file the only init_MUTEX_LOCKED that I found was 

static int __devinit cpu5wdt_init(void)
{
	unsigned int val;
	int err;

[...]

	/* watchdog reboot? */
	val = inb(port + CPU5WDT_STATUS_REG);
	val = (val >> 2) & 1;
	if ( !val )
		printk(KERN_INFO PFX "sorry, was my fault\n");

	init_MUTEX_LOCKED(&cpu5wdt_device.stop);
	cpu5wdt_device.queue = 0;

	clear_bit(0, &cpu5wdt_device.inuse);



Here I see that cpu5wdt_device.stop is being initialized with
init_MUTEX_LOCKED, so that is what I went to fix.  I even added the
driver to my config and compiled it before sending it in.  I don't have
the device, but the driver compiled :-)

-- Steve

Re: [PATCH] Real-Time Preemption V0.7.52-07: rt_init_MUTEX_LOCKED declaration

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 2005-08-01 at 23:03 +0200, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > -	struct semaphore stop;
> > > +	struct compat_semaphore stop;
> > 
> > i think it's policy->lock that is the issue here?
> > 
> 
> I was looking at Luca's original message where he showed the bug of 
> -- drivers/char/watchdog/cpu5wdt.c: "cpu5wdt: Unknown symbol
> there_is_no_init_MUTEX_LOCKED_for_RT_semaphores") --
> Looking into this file the only init_MUTEX_LOCKED that I found was 
> 
> static int __devinit cpu5wdt_init(void)
> {
> 	unsigned int val;
> 	int err;
> 
> [...]
> 
> 	/* watchdog reboot? */
> 	val = inb(port + CPU5WDT_STATUS_REG);
> 	val = (val >> 2) & 1;
> 	if ( !val )
> 		printk(KERN_INFO PFX "sorry, was my fault\n");
> 
> 	init_MUTEX_LOCKED(&cpu5wdt_device.stop);
> 	cpu5wdt_device.queue = 0;
> 
> 	clear_bit(0, &cpu5wdt_device.inuse);
> 
> 
> 
> Here I see that cpu5wdt_device.stop is being initialized with 
> init_MUTEX_LOCKED, so that is what I went to fix.  I even added the 
> driver to my config and compiled it before sending it in.  I don't 
> have the device, but the driver compiled :-)

ok - the fix is in -52-10.

	Ingo

Re: [PATCH] Real-Time Preemption V0.7.52-07: rt_init_MUTEX_LOCKED declaration

[Ingo Molnar]

* Luca Falavigna <dktrkranz@gmail.com> wrote:

> This patch fixes broken rt_init_MUTEX_LOCKED declaration using 
> rt_sema_init() macro. This way we fix a potential compile bug: 
> rt_init_MUTEX_LOCKED calls 
> there_is_no_init_MUTEX_LOCKED_for_RT_semaphores, which is not 
> referenced. (e.g. drivers/char/watchdog/cpu5wdt.c: "cpu5wdt: Unknown 
> symbol there_is_no_init_MUTEX_LOCKED_for_RT_semaphores")

the right solution would be to mark policy->lock as a compat_semaphore.  
That will revert things back to the stock semantics. (at the price of 
not having PI, which isnt a big issue in this case.)

> -+extern void there_is_no_init_MUTEX_LOCKED_for_RT_semaphores(void);

the reason for not allowing init_MUTEX_LOCKED() is that in basically 
every case that does it, the semaphore is not used as a true mutex in 
the strict sense. So the affected semaphore should be changed to 
compat_semaphore, to gain full semantics.

	Ingo