* annotating semaphores.
@ 2007-12-12 20:00 Dave Jones
2007-12-13 22:00 ` Christopher Li
0 siblings, 1 reply; 3+ messages in thread
From: Dave Jones @ 2007-12-12 20:00 UTC (permalink / raw)
To: linux-sparse
Today I came across a bug in the kernel cpufreq code where
we were missing a bunch of up_write() calls in error paths
of a function.
I've been trying to get sparse's context checking to pick up
on the errors and failing. The kernel patch below is what I have
so far, but it seems to report no output whatsoever.
What am I missing ?
Dave
diff --git a/include/asm-x86/rwsem.h b/include/asm-x86/rwsem.h
index 041906f..3111853 100644
--- a/include/asm-x86/rwsem.h
+++ b/include/asm-x86/rwsem.h
@@ -106,6 +106,7 @@ LOCK_PREFIX " incl (%%eax)\n\t" /* adds 0x00000001, returns the old value
: "+m" (sem->count)
: "a" (sem)
: "memory", "cc");
+ __acquire(sem);
}
/*
@@ -128,6 +129,7 @@ LOCK_PREFIX " cmpxchgl %2,%0\n\t"
: "+m" (sem->count), "=&a" (result), "=&r" (tmp)
: "i" (RWSEM_ACTIVE_READ_BIAS)
: "memory", "cc");
+ __acquire(sem);
return result>=0 ? 1 : 0;
}
@@ -167,6 +169,7 @@ static inline int __down_write_trylock(struct rw_semaphore *sem)
RWSEM_ACTIVE_WRITE_BIAS);
if (ret == RWSEM_UNLOCKED_VALUE)
return 1;
+ __acquire(sem);
return 0;
}
@@ -186,6 +189,7 @@ LOCK_PREFIX " xadd %%edx,(%%eax)\n\t" /* subtracts 1, returns the old valu
: "+m" (sem->count), "=d" (tmp)
: "a" (sem), "1" (tmp)
: "memory", "cc");
+ __release(sem);
}
/*
@@ -204,6 +208,7 @@ LOCK_PREFIX " xaddl %%edx,(%%eax)\n\t" /* tries to transition 0xffff0001 ->
: "+m" (sem->count)
: "a" (sem), "i" (-RWSEM_ACTIVE_WRITE_BIAS)
: "memory", "cc", "edx");
+ __release(sem);
}
/*
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index c4e0016..cf110b0 100644
diff --git a/include/linux/rwsem-spinlock.h b/include/linux/rwsem-spinlock.h
index 813cee1..90a4b2b 100644
--- a/include/linux/rwsem-spinlock.h
+++ b/include/linux/rwsem-spinlock.h
@@ -60,13 +60,13 @@ do { \
__init_rwsem((sem), #sem, &__key); \
} while (0)
-extern void FASTCALL(__down_read(struct rw_semaphore *sem));
-extern int FASTCALL(__down_read_trylock(struct rw_semaphore *sem));
-extern void FASTCALL(__down_write(struct rw_semaphore *sem));
+extern void FASTCALL(__down_read(struct rw_semaphore *sem)) __acquires(sem);
+extern int FASTCALL(__down_read_trylock(struct rw_semaphore *sem)) __acquires(sem);
+extern void FASTCALL(__down_write(struct rw_semaphore *sem)) __acquires(sem);
extern void FASTCALL(__down_write_nested(struct rw_semaphore *sem, int subclass));
-extern int FASTCALL(__down_write_trylock(struct rw_semaphore *sem));
-extern void FASTCALL(__up_read(struct rw_semaphore *sem));
-extern void FASTCALL(__up_write(struct rw_semaphore *sem));
+extern int FASTCALL(__down_write_trylock(struct rw_semaphore *sem) __acquires(sem));
+extern void FASTCALL(__up_read(struct rw_semaphore *sem)) __releases(sem);
+extern void FASTCALL(__up_write(struct rw_semaphore *sem)) __releases(sem);
extern void FASTCALL(__downgrade_write(struct rw_semaphore *sem));
static inline int rwsem_is_locked(struct rw_semaphore *sem)
diff --git a/lib/rwsem-spinlock.c b/lib/rwsem-spinlock.c
index c4cfd6c..c13d112 100644
--- a/lib/rwsem-spinlock.c
+++ b/lib/rwsem-spinlock.c
@@ -162,6 +162,7 @@ void fastcall __sched __down_read(struct rw_semaphore *sem)
tsk->state = TASK_RUNNING;
out:
+ __acquire(sem);
;
}
@@ -184,6 +185,7 @@ int fastcall __down_read_trylock(struct rw_semaphore *sem)
spin_unlock_irqrestore(&sem->wait_lock, flags);
+ __acquire(sem);
return ret;
}
@@ -228,6 +230,7 @@ void fastcall __sched __down_write_nested(struct rw_semaphore *sem, int subclass
tsk->state = TASK_RUNNING;
out:
+ __acquire(sem);
;
}
@@ -253,6 +256,7 @@ int fastcall __down_write_trylock(struct rw_semaphore *sem)
}
spin_unlock_irqrestore(&sem->wait_lock, flags);
+ __acquire(sem);
return ret;
}
@@ -270,6 +274,7 @@ void fastcall __up_read(struct rw_semaphore *sem)
sem = __rwsem_wake_one_writer(sem);
spin_unlock_irqrestore(&sem->wait_lock, flags);
+ __release(sem);
}
/*
@@ -286,6 +291,7 @@ void fastcall __up_write(struct rw_semaphore *sem)
sem = __rwsem_do_wake(sem, 1);
spin_unlock_irqrestore(&sem->wait_lock, flags);
+ __release(sem);
}
/*
--
http://www.codemonkey.org.uk
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: annotating semaphores.
2007-12-12 20:00 annotating semaphores Dave Jones
@ 2007-12-13 22:00 ` Christopher Li
2007-12-13 22:05 ` Dave Jones
0 siblings, 1 reply; 3+ messages in thread
From: Christopher Li @ 2007-12-13 22:00 UTC (permalink / raw)
To: Dave Jones; +Cc: linux-sparse
On Dec 12, 2007 12:00 PM, Dave Jones <davej@redhat.com> wrote:
> Today I came across a bug in the kernel cpufreq code where
> we were missing a bunch of up_write() calls in error paths
> of a function.
>
> I've been trying to get sparse's context checking to pick up
> on the errors and failing. The kernel patch below is what I have
> so far, but it seems to report no output whatsoever.
> What am I missing ?
Can you share the example buggy cpufreq code that miss the up_write() calls?
> --- a/lib/rwsem-spinlock.c
> +++ b/lib/rwsem-spinlock.c
> @@ -162,6 +162,7 @@ void fastcall __sched __down_read(struct rw_semaphore *sem)
>
> tsk->state = TASK_RUNNING;
> out:
> + __acquire(sem);
> ;
> }
That is not needed. Because the __down_read is not an inline function. The
extern void FASTCALL(__down_read(struct rw_semaphore *sem)) __acquires(sem);
declare should be good enough.
Chris
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: annotating semaphores.
2007-12-13 22:00 ` Christopher Li
@ 2007-12-13 22:05 ` Dave Jones
0 siblings, 0 replies; 3+ messages in thread
From: Dave Jones @ 2007-12-13 22:05 UTC (permalink / raw)
To: Christopher Li; +Cc: linux-sparse
On Thu, Dec 13, 2007 at 02:00:09PM -0800, Christopher Li wrote:
> On Dec 12, 2007 12:00 PM, Dave Jones <davej@redhat.com> wrote:
> > Today I came across a bug in the kernel cpufreq code where
> > we were missing a bunch of up_write() calls in error paths
> > of a function.
> >
> > I've been trying to get sparse's context checking to pick up
> > on the errors and failing. The kernel patch below is what I have
> > so far, but it seems to report no output whatsoever.
> > What am I missing ?
>
> Can you share the example buggy cpufreq code that miss the up_write() calls?
Sure. In drivers/cpufreq/cpufreq.c cpufreq_add_dev() is missing several
calls to unlock_policy_rwsem_write() in the error paths.
The patch below should make it more obvious..
Dave
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5e626b1..79581fa 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -841,19 +841,25 @@ static int cpufreq_add_dev (struct sys_device * sys_dev)
drv_attr = cpufreq_driver->attr;
while ((drv_attr) && (*drv_attr)) {
ret = sysfs_create_file(&policy->kobj, &((*drv_attr)->attr));
- if (ret)
+ if (ret) {
+ unlock_policy_rwsem_write(cpu);
goto err_out_driver_exit;
+ }
drv_attr++;
}
if (cpufreq_driver->get){
ret = sysfs_create_file(&policy->kobj, &cpuinfo_cur_freq.attr);
- if (ret)
+ if (ret) {
+ unlock_policy_rwsem_write(cpu);
goto err_out_driver_exit;
+ }
}
if (cpufreq_driver->target){
ret = sysfs_create_file(&policy->kobj, &scaling_cur_freq.attr);
- if (ret)
+ if (ret) {
+ unlock_policy_rwsem_write(cpu);
goto err_out_driver_exit;
+ }
}
spin_lock_irqsave(&cpufreq_driver_lock, flags);
--
http://www.codemonkey.org.uk
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2007-12-13 22:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-12-12 20:00 annotating semaphores Dave Jones
2007-12-13 22:00 ` Christopher Li
2007-12-13 22:05 ` Dave Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).