The testmode NVS push command is no longer in use. In addition, it has
several implementation issues that prevent it from working correctly:
1. wl1271_tm_cmd_configure relies on wl->chip.id being set. However,
since the device was not necessarily booted by the time the function
is called, wl->chip.id will be initialized to 0.
2. The NVS file is fetched by calling request_firmware() before it is
possible to push an NVS file.
3. The maximum allowed size of nl binary payloads is not sufficient for
pushing NVS files.
4. Pushing 128x NVS files will always fail due to a bug in the
validation code.
5. In case the pushed NVS file is found invalid, the mutex will be kept
locked and the nvs member will become a dangling pointer.
Since this feature is not being used, remove it completely instead of
fixing it.
Signed-off-by: Ido Yariv <[email protected]>
---
drivers/net/wireless/wl12xx/testmode.c | 45 --------------------------------
1 files changed, 0 insertions(+), 45 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/testmode.c b/drivers/net/wireless/wl12xx/testmode.c
index 5d5e1ef..6e6fd56 100644
--- a/drivers/net/wireless/wl12xx/testmode.c
+++ b/drivers/net/wireless/wl12xx/testmode.c
@@ -36,7 +36,6 @@ enum wl1271_tm_commands {
WL1271_TM_CMD_TEST,
WL1271_TM_CMD_INTERROGATE,
WL1271_TM_CMD_CONFIGURE,
- WL1271_TM_CMD_NVS_PUSH,
WL1271_TM_CMD_SET_PLT_MODE,
WL1271_TM_CMD_RECOVER,
@@ -187,48 +186,6 @@ static int wl1271_tm_cmd_configure(struct wl1271 *wl, struct nlattr *tb[])
return 0;
}
-static int wl1271_tm_cmd_nvs_push(struct wl1271 *wl, struct nlattr *tb[])
-{
- int ret = 0;
- size_t len;
- void *buf;
-
- wl1271_debug(DEBUG_TESTMODE, "testmode cmd nvs push");
-
- if (!tb[WL1271_TM_ATTR_DATA])
- return -EINVAL;
-
- buf = nla_data(tb[WL1271_TM_ATTR_DATA]);
- len = nla_len(tb[WL1271_TM_ATTR_DATA]);
-
- mutex_lock(&wl->mutex);
-
- kfree(wl->nvs);
-
- if ((wl->chip.id == CHIP_ID_1283_PG20) &&
- (len != sizeof(struct wl128x_nvs_file)))
- return -EINVAL;
- else if (len != sizeof(struct wl1271_nvs_file))
- return -EINVAL;
-
- wl->nvs = kzalloc(len, GFP_KERNEL);
- if (!wl->nvs) {
- wl1271_error("could not allocate memory for the nvs file");
- ret = -ENOMEM;
- goto out;
- }
-
- memcpy(wl->nvs, buf, len);
- wl->nvs_len = len;
-
- wl1271_debug(DEBUG_TESTMODE, "testmode pushed nvs");
-
-out:
- mutex_unlock(&wl->mutex);
-
- return ret;
-}
-
static int wl1271_tm_cmd_set_plt_mode(struct wl1271 *wl, struct nlattr *tb[])
{
u32 val;
@@ -285,8 +242,6 @@ int wl1271_tm_cmd(struct ieee80211_hw *hw, void *data, int len)
return wl1271_tm_cmd_interrogate(wl, tb);
case WL1271_TM_CMD_CONFIGURE:
return wl1271_tm_cmd_configure(wl, tb);
- case WL1271_TM_CMD_NVS_PUSH:
- return wl1271_tm_cmd_nvs_push(wl, tb);
case WL1271_TM_CMD_SET_PLT_MODE:
return wl1271_tm_cmd_set_plt_mode(wl, tb);
case WL1271_TM_CMD_RECOVER:
--
1.7.4.1
On Tue, 2011-08-02 at 10:24 +0300, Ido Yariv wrote:
> The testmode NVS push command is no longer in use. In addition, it has
> several implementation issues that prevent it from working correctly:
>
> 1. wl1271_tm_cmd_configure relies on wl->chip.id being set. However,
> since the device was not necessarily booted by the time the function
> is called, wl->chip.id will be initialized to 0.
> 2. The NVS file is fetched by calling request_firmware() before it is
> possible to push an NVS file.
> 3. The maximum allowed size of nl binary payloads is not sufficient for
> pushing NVS files.
> 4. Pushing 128x NVS files will always fail due to a bug in the
> validation code.
> 5. In case the pushed NVS file is found invalid, the mutex will be kept
> locked and the nvs member will become a dangling pointer.
>
> Since this feature is not being used, remove it completely instead of
> fixing it.
>
> Signed-off-by: Ido Yariv <[email protected]>
> ---
Acked-by: Luciano Coelho <[email protected]>
John, could you please take these two patches into 3.1?
The first one is removing an obsolete feature that was badly broken
(causing security risks) and the second one is a power management bug
fix, without which suspend doesn't work properly with the wl12xx driver.
Thanks!
--
Cheers,
Luca.
wl1271_sdio_power_on checks if the return value of pm_runtime_get_sync
is non-zero, and if so bails out.
However, pm_runtime_get_sync can return a positive number which does not
suggest an error has occurred. This is problematic for two reasons:
1. The function will needlessly bail out without decrementing back the
runtime PM reference counter.
2. wl1271_power_on only checks if wl1271_power_on return value is
negative. This means that wl1271_power_on will continue even if
wl1271_sdio_power_on bailed out. As a result, sdio transactions will
be initiated without properly enabling the sdio function and claiming
the host. This could even lead to a kernel panic.
Fix this by only checking that the return value of pm_runtime_get_sync
is non-negative.
Signed-off-by: Ido Yariv <[email protected]>
---
drivers/net/wireless/wl12xx/sdio.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/sdio.c b/drivers/net/wireless/wl12xx/sdio.c
index 5cf18c2..fb1fd5a 100644
--- a/drivers/net/wireless/wl12xx/sdio.c
+++ b/drivers/net/wireless/wl12xx/sdio.c
@@ -164,7 +164,7 @@ static int wl1271_sdio_power_on(struct wl1271 *wl)
/* If enabled, tell runtime PM not to power off the card */
if (pm_runtime_enabled(&func->dev)) {
ret = pm_runtime_get_sync(&func->dev);
- if (ret)
+ if (ret < 0)
goto out;
} else {
/* Runtime PM is disabled: power up the card manually */
--
1.7.4.1
On Tue, 2011-08-02 at 10:24 +0300, Ido Yariv wrote:
> wl1271_sdio_power_on checks if the return value of pm_runtime_get_sync
> is non-zero, and if so bails out.
> However, pm_runtime_get_sync can return a positive number which does not
> suggest an error has occurred. This is problematic for two reasons:
>
> 1. The function will needlessly bail out without decrementing back the
> runtime PM reference counter.
> 2. wl1271_power_on only checks if wl1271_power_on return value is
> negative. This means that wl1271_power_on will continue even if
> wl1271_sdio_power_on bailed out. As a result, sdio transactions will
> be initiated without properly enabling the sdio function and claiming
> the host. This could even lead to a kernel panic.
>
> Fix this by only checking that the return value of pm_runtime_get_sync
> is non-negative.
>
> Signed-off-by: Ido Yariv <[email protected]>
> ---
Acked-by: Luciano Coelho <[email protected]>
--
Cheers,
Luca.