2009-07-13 21:33:07

by Jiri Slaby

Subject: [PATCH] wireless: wl12xx, fix lock imbalance

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

2009-07-14 05:44:21

by Luciano Coelho

Subject: Re: [PATCH] wireless: wl12xx, fix lock imbalance

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.