Add omitted mutex_unlock to one of wl12xx_op_start fail paths (when
wl12xx_chip_wakeup fails).
Not sure if the device should be powered off?
Signed-off-by: Jiri Slaby <[email protected]>
---
drivers/net/wireless/wl12xx/main.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 603d611..d241e4a 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -336,7 +336,7 @@ static int wl12xx_op_start(struct ieee80211_hw *hw)
ret = wl12xx_chip_wakeup(wl);
if (ret < 0)
- return ret;
+ goto unlock;
ret = wl->chip.op_boot(wl);
if (ret < 0)
@@ -357,7 +357,7 @@ static int wl12xx_op_start(struct ieee80211_hw *hw)
out:
if (ret < 0)
wl12xx_power_off(wl);
-
+unlock:
mutex_unlock(&wl->mutex);
return ret;
--
1.6.3.2
On Mon, 2009-07-13 at 23:24 +0200, Jiri Slaby wrote:
> Add omitted mutex_unlock to one of wl12xx_op_start fail paths (when
> wl12xx_chip_wakeup fails).
By the way, are you using some tool to find these? I've had local hacks
many times to make sparse aware of mutexes, is there a reason they are
not annotated with __acquire(s)/__release(s) like spinlocks etc.?
johannes
On 07/13/2009 11:40 PM, Johannes Berg wrote:
> On Mon, 2009-07-13 at 23:24 +0200, Jiri Slaby wrote:
>> Add omitted mutex_unlock to one of wl12xx_op_start fail paths (when
>> wl12xx_chip_wakeup fails).
>
> By the way, are you using some tool to find these?
Yup, it's called stanse[1], but we still work on that to make it stable.
> I've had local hacks
> many times to make sparse aware of mutexes, is there a reason they are
> not annotated with __acquire(s)/__release(s) like spinlocks etc.?
Mutexes are often locked/unlocked interprocedural which I think sparse
can't do much about.
[1] http://iti.fi.muni.cz/stanse/
On 07/13/2009 11:44 PM, Jiri Slaby wrote:
>> I've had local hacks
>> many times to make sparse aware of mutexes, is there a reason they are
>> not annotated with __acquire(s)/__release(s) like spinlocks etc.?
>
> Mutexes are often locked/unlocked interprocedural which I think sparse
> can't do much about.
(which means it has high false positive rate in those cases)
On Mon, 2009-07-13 at 23:44 +0200, Jiri Slaby wrote:
> > I've had local hacks
> > many times to make sparse aware of mutexes, is there a reason they are
> > not annotated with __acquire(s)/__release(s) like spinlocks etc.?
>
> Mutexes are often locked/unlocked interprocedural which I think sparse
> can't do much about.
Well, you annotate those functions too, of course.
johannes
On 07/13/2009 11:49 PM, Johannes Berg wrote:
> On Mon, 2009-07-13 at 23:44 +0200, Jiri Slaby wrote:
>
>>> I've had local hacks
>>> many times to make sparse aware of mutexes, is there a reason they are
>>> not annotated with __acquire(s)/__release(s) like spinlocks etc.?
>>
>> Mutexes are often locked/unlocked interprocedural which I think sparse
>> can't do much about.
>
> Well, you annotate those functions too, of course.
Sorry, I don't understand. What functions I annotate?
On Mon, 2009-07-13 at 23:51 +0200, Jiri Slaby wrote:
> On 07/13/2009 11:49 PM, Johannes Berg wrote:
> > On Mon, 2009-07-13 at 23:44 +0200, Jiri Slaby wrote:
> >
> >>> I've had local hacks
> >>> many times to make sparse aware of mutexes, is there a reason they are
> >>> not annotated with __acquire(s)/__release(s) like spinlocks etc.?
> >>
> >> Mutexes are often locked/unlocked interprocedural which I think sparse
> >> can't do much about.
> >
> > Well, you annotate those functions too, of course.
>
> Sorry, I don't understand. What functions I annotate?
Well those that take the mutex, e.g.
void acquire_foo(struct foo *f)
{
mutex_lock(&f->mtx);
}
turns to
void acquire_foo(struct foo *f)
__acquires(f->mtx)
{
mutex_lock(&f->mtx);
}
johannes
ext Jiri Slaby wrote:
> Add omitted mutex_unlock to one of wl12xx_op_start fail paths (when
> wl12xx_chip_wakeup fails).
>
Cool, very nice catch. We actually just fixed this bug in our wl1271
code (which I will hopefully send upstream this week), but we hadn't
fixed it in the wl1251-specific code yet.
> Not sure if the device should be powered off?
>
You should. If the chip cannot be booted, why should it remain powered
on? In some rare cases, the chip might fail to initialize, but can
recover if powered off and on again, so turning it off at this point is
the right thing to do.
> Signed-off-by: Jiri Slaby <[email protected]>
> ---
>
> drivers/net/wireless/wl12xx/main.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
> index 603d611..d241e4a 100644
> --- a/drivers/net/wireless/wl12xx/main.c
> +++ b/drivers/net/wireless/wl12xx/main.c
>
> @@ -336,7 +336,7 @@ static int wl12xx_op_start(struct ieee80211_hw *hw)
>
> ret = wl12xx_chip_wakeup(wl);
> if (ret < 0)
> - return ret;
> + goto unlock;
>
Here you can just "goto out;" so that the chip is powered off before we
return.
>
> ret = wl->chip.op_boot(wl);
> if (ret < 0)
> @@ -357,7 +357,7 @@ static int wl12xx_op_start(struct ieee80211_hw *hw)
> out:
> if (ret < 0)
> wl12xx_power_off(wl);
> -
> +unlock:
> mutex_unlock(&wl->mutex);
>
> return ret;
>
Thanks a lot for your patch!
--
Cheers,
Luca.
* Johannes Berg <[email protected]> wrote:
> On Mon, 2009-07-13 at 23:51 +0200, Jiri Slaby wrote:
> > On 07/13/2009 11:49 PM, Johannes Berg wrote:
> > > On Mon, 2009-07-13 at 23:44 +0200, Jiri Slaby wrote:
> > >
> > >>> I've had local hacks
> > >>> many times to make sparse aware of mutexes, is there a reason they are
> > >>> not annotated with __acquire(s)/__release(s) like spinlocks etc.?
> > >>
> > >> Mutexes are often locked/unlocked interprocedural which I think sparse
> > >> can't do much about.
> > >
> > > Well, you annotate those functions too, of course.
> >
> > Sorry, I don't understand. What functions I annotate?
>
> Well those that take the mutex, e.g.
>
> void acquire_foo(struct foo *f)
> {
> mutex_lock(&f->mtx);
> }
>
>
> turns to
>
> void acquire_foo(struct foo *f)
> __acquires(f->mtx)
> {
> mutex_lock(&f->mtx);
> }
>
> johannes
Yes. And in fact 'nice' code wants to be either annotated explicitly
as 'I am taking locks', or should be balanced.
I was thinking about also using lockdep plus the function-graph
tracer for that (in the dynamic lock debugging department).
It would work like this: __acquires()/__releases() would also emit
section markers like __lockfunc, and lockdep would warn about
functions that return with unbalanced locks, irqs or preempt counts
and do not declare themselves as locking related functions.
This would help catch imbalances at their source.
Plus static tools like Jiri is working on are very useful as well. I
think Coverty does that too and it's a pity we dont have free tools
for that. In fact Covery will sweep clean the kernel of such bugs,
giving OSS tools like 'stanse' the false impression that there are
no such bugs. There are such bugs - there's a constant influx of
them. So please work on this, it looks very useful.
Ingo
On Sat, 2009-07-18 at 13:19 +0200, Ingo Molnar wrote:
> Yes. And in fact 'nice' code wants to be either annotated explicitly
> as 'I am taking locks', or should be balanced.
I agree.
> I was thinking about also using lockdep plus the function-graph
> tracer for that (in the dynamic lock debugging department).
Yeah, but that's dynamic again -- all the error paths are never caught.
> It would work like this: __acquires()/__releases() would also emit
> section markers like __lockfunc, and lockdep would warn about
> functions that return with unbalanced locks, irqs or preempt counts
> and do not declare themselves as locking related functions.
>
> This would help catch imbalances at their source.
I don't see a need to do it dynamically since sparse warns about things
like this. It's quirky in some ways and I've tried to fix it up before
(and failed) but it's not something that can't be fixed, it just needs
more than a night of hacking.
> Plus static tools like Jiri is working on are very useful as well. I
> think Coverty does that too and it's a pity we dont have free tools
> for that. In fact Covery will sweep clean the kernel of such bugs,
> giving OSS tools like 'stanse' the false impression that there are
> no such bugs. There are such bugs - there's a constant influx of
> them. So please work on this, it looks very useful.
What's "this" in this context?
johannes
* Johannes Berg <[email protected]> wrote:
> > It would work like this: __acquires()/__releases() would also
> > emit section markers like __lockfunc, and lockdep would warn
> > about functions that return with unbalanced locks, irqs or
> > preempt counts and do not declare themselves as locking related
> > functions.
> >
> > This would help catch imbalances at their source.
>
> I don't see a need to do it dynamically since sparse warns about
> things like this. It's quirky in some ways and I've tried to fix
> it up before (and failed) but it's not something that can't be
> fixed, it just needs more than a night of hacking.
Yeah - but Sparse warns about this if it can analyze the code path.
If it cannot see through it then it cannot warn. Static analysis
will go only that far - dynamic analysis will catch the cases that
do happen.
So it's best to have both: static analysis is good at finding
imbalances even if they have a very low likelyhood of occuring in
practice, while dynamic analysis will catch everything that does
trigger in practice, regardless of code flow complexity.
( The only white area on the map is rarely executed code that has a
complex code flow. Such code is being frowned upon in general at
the review stage. )
> > Plus static tools like Jiri is working on are very useful as
> > well. I think Coverty does that too and it's a pity we dont have
> > free tools for that. In fact Covery will sweep clean the kernel
> > of such bugs, giving OSS tools like 'stanse' the false
> > impression that there are no such bugs. There are such bugs -
> > there's a constant influx of them. So please work on this, it
> > looks very useful.
>
> What's "this" in this context?
this == stanse, the static code analyzing thing Jiri mentioned he is
working on. The webpage says it will be under the GPL - that's good.
Jiri, any release date for the source code?
Ingo
On 07/18/2009 06:10 PM, Ingo Molnar wrote:
> this == stanse, the static code analyzing thing Jiri mentioned he is
> working on. The webpage says it will be under the GPL - that's good.
> Jiri, any release date for the source code?
Sorry for the delay. We are currently rewriting the core: we added
interprocedural analysis and some false-positives killers. We also
merged thread-checker (it can catch (not only) circular lock deps like
in [1]). We are finishing, but it needs some testing, so I would say in
about a month.
[1] http://lkml.indiana.edu/hypermail/linux/kernel/0904.1/03221.html
On 07/18/2009 06:10 PM, Ingo Molnar wrote:
>>> Plus static tools like Jiri is working on are very useful as
>>> well. I think Coverty does that too and it's a pity we dont have
>>> free tools for that. In fact Covery will sweep clean the kernel
>>> of such bugs, giving OSS tools like 'stanse' the false
>>> impression that there are no such bugs. There are such bugs -
>>> there's a constant influx of them. So please work on this, it
>>> looks very useful.
>>
>> What's "this" in this context?
>
> this == stanse, the static code analyzing thing Jiri mentioned he is
> working on. The webpage says it will be under the GPL - that's good.
> Jiri, any release date for the source code?
Hi all. I'm pleased to announce the first official Stanse release.
For those who are interested, the tool (with other information and
documentation) is available at
http://stanse.fi.muni.cz/
There are also prebuilt rpm packages for openSUSE and Fedora 11. (Some
of them still building, will be available soon. Sorry for that, build
service is slow as hell these days.)
Also I pre-ran the stanse on 2.6.32-rc3 with results available online.
The webpage concerning this is at:
http://decibel.fi.muni.cz/~xslaby/stanse/?db=32-rc
So for those who are not interested in the toolkit itself but only in
results from more-or-less latest kernel may try it that way.
There are many janitor-like bugs. E.g. not testing return values from
k*alloc might be seamlessly taken care of by janitors, I think.
As it's only static analysis not based on symbolic execution (we will
play with that later), there are some/many (depending on a checker) many
false positives. There is a mechanism for marking them. I ask everybody
who finds a FP or error to mark the entry as such.
I'm still trying to lower FP rate. If somebody finds out another method
to do so, please share with us.
Any input appreciated. Hope it helps.
Thanks to those who support us,
--js
* Jiri Slaby <[email protected]> wrote:
> On 07/18/2009 06:10 PM, Ingo Molnar wrote:
> >>> Plus static tools like Jiri is working on are very useful as
> >>> well. I think Coverty does that too and it's a pity we dont have
> >>> free tools for that. In fact Covery will sweep clean the kernel
> >>> of such bugs, giving OSS tools like 'stanse' the false
> >>> impression that there are no such bugs. There are such bugs -
> >>> there's a constant influx of them. So please work on this, it
> >>> looks very useful.
> >>
> >> What's "this" in this context?
> >
> > this == stanse, the static code analyzing thing Jiri mentioned he is
> > working on. The webpage says it will be under the GPL - that's good.
> > Jiri, any release date for the source code?
>
> Hi all. I'm pleased to announce the first official Stanse release.
>
> For those who are interested, the tool (with other information and
> documentation) is available at
> http://stanse.fi.muni.cz/
the list of bugs found:
http://stanse.fi.muni.cz/bugs.html
is already impressive!
Ingo