[PATCH 0/5] ath6kl: staging fixes

[PATCH 0/5] ath6kl: staging fixes

[Kalle Valo]

Hi Greg,

here are few pending fixes and two new ones which haven't been sent before. The
new patches are:

ath6kl: cache firmware
ath6kl: fix crash when interface is closed but scan is ongoing

Please take a look at them and if they are ok consider applying them to your
staging tree.

Thanks.

---

Kalle Valo (4):
      ath6kl: export firmware version to user space
      ath6kl: testmode support
      ath6kl: cache firmware
      ath6kl: fix crash when interface is closed but scan is ongoing

Vasanthakumar Thiagarajan (1):
      ath6kl: Fix a kernel panic furing suspend/resume


 drivers/staging/ath6kl/os/linux/ar6000_drv.c       |   60 ++++++++++----
 drivers/staging/ath6kl/os/linux/cfg80211.c         |   88 +++++++++++++++++---
 .../staging/ath6kl/os/linux/include/ar6000_drv.h   |    9 ++
 3 files changed, 130 insertions(+), 27 deletions(-)

[PATCH 1/5] ath6kl: export firmware version to user space

[Kalle Valo]

From: Kalle Valo <kalle.valo@atheros.com>

cfg80211 exports wiphy->fw_version to user space via ethtool interface.

The obligatory screenshot:

$ sudo ethtool -i wlan0
driver: ath6kl_hifdev
version: 2.6.39-rc4+
firmware-version: 3:1:1:149
bus-info: mmc0:0001:1
$

Signed-off-by: Kalle Valo <kalle.valo@atheros.com>
---
 drivers/staging/ath6kl/os/linux/ar6000_drv.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/ath6kl/os/linux/ar6000_drv.c b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
index 48dd9e3..30ae63b 100644
--- a/drivers/staging/ath6kl/os/linux/ar6000_drv.c
+++ b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
@@ -4114,6 +4114,13 @@ ar6000_ready_event(void *devt, u8 *datap, u8 phyCap, u32 sw_ver, u32 abi_ver)
     ar->arVersion.wlan_ver = sw_ver;
     ar->arVersion.abi_ver = abi_ver;
 
+    snprintf(ar->wdev->wiphy->fw_version, sizeof(ar->wdev->wiphy->fw_version),
+	     "%u:%u:%u:%u",
+	     (ar->arVersion.wlan_ver & 0xf0000000) >> 28,
+	     (ar->arVersion.wlan_ver & 0x0f000000) >> 24,
+	     (ar->arVersion.wlan_ver & 0x00ff0000) >> 16,
+	     (ar->arVersion.wlan_ver & 0x0000ffff));
+
     /* Indicate to the waiting thread that the ready event was received */
     ar->arWmiReady = true;
     wake_up(&arEvent);

[PATCH 2/5] ath6kl: testmode support

[Kalle Valo]

Add testmode support for running low level hardware tests. The testmode
is enabled by setting testmode module parameter to 1.

For now only a simple command passing is supported. More advanced
support will be implemented later.

Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/staging/ath6kl/os/linux/cfg80211.c |   58 ++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/ath6kl/os/linux/cfg80211.c b/drivers/staging/ath6kl/os/linux/cfg80211.c
index 77dfb40..a0c0819 100644
--- a/drivers/staging/ath6kl/os/linux/cfg80211.c
+++ b/drivers/staging/ath6kl/os/linux/cfg80211.c
@@ -24,6 +24,7 @@
 #include <linux/wireless.h>
 #include <linux/ieee80211.h>
 #include <net/cfg80211.h>
+#include <net/netlink.h>
 
 #include "ar6000_drv.h"
 
@@ -1452,6 +1453,62 @@ ar6k_cfg80211_leave_ibss(struct wiphy *wiphy, struct net_device *dev)
     return 0;
 }
 
+#ifdef CONFIG_NL80211_TESTMODE
+enum ar6k_testmode_attr {
+	__AR6K_TM_ATTR_INVALID	= 0,
+	AR6K_TM_ATTR_CMD	= 1,
+	AR6K_TM_ATTR_DATA	= 2,
+
+	/* keep last */
+	__AR6K_TM_ATTR_AFTER_LAST,
+	AR6K_TM_ATTR_MAX	= __AR6K_TM_ATTR_AFTER_LAST - 1
+};
+
+enum ar6k_testmode_cmd {
+	AR6K_TM_CMD_TCMD		= 0,
+};
+
+#define AR6K_TM_DATA_MAX_LEN 5000
+
+static const struct nla_policy ar6k_testmode_policy[AR6K_TM_ATTR_MAX + 1] = {
+	[AR6K_TM_ATTR_CMD] = { .type = NLA_U32 },
+	[AR6K_TM_ATTR_DATA] = { .type = NLA_BINARY,
+				.len = AR6K_TM_DATA_MAX_LEN },
+};
+
+static int ar6k_testmode_cmd(struct wiphy *wiphy, void *data, int len)
+{
+	struct ar6_softc *ar = wiphy_priv(wiphy);
+	struct nlattr *tb[AR6K_TM_ATTR_MAX + 1];
+	int err, buf_len;
+	void *buf;
+
+	err = nla_parse(tb, AR6K_TM_ATTR_MAX, data, len,
+			ar6k_testmode_policy);
+	if (err)
+		return err;
+
+	if (!tb[AR6K_TM_ATTR_CMD])
+		return -EINVAL;
+
+	switch (nla_get_u32(tb[AR6K_TM_ATTR_CMD])) {
+	case AR6K_TM_CMD_TCMD:
+		if (!tb[AR6K_TM_ATTR_DATA])
+			return -EINVAL;
+
+		buf = nla_data(tb[AR6K_TM_ATTR_DATA]);
+		buf_len = nla_len(tb[AR6K_TM_ATTR_DATA]);
+
+		wmi_test_cmd(ar->arWmi, buf, buf_len);
+
+		return 0;
+
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+#endif
 
 static const
 u32 cipher_suites[] = {
@@ -1627,6 +1684,7 @@ cfg80211_ops ar6k_cfg80211_ops = {
     .join_ibss = ar6k_cfg80211_join_ibss,
     .leave_ibss = ar6k_cfg80211_leave_ibss,
     .get_station = ar6k_get_station,
+    CFG80211_TESTMODE_CMD(ar6k_testmode_cmd)
 };
 
 struct wireless_dev *

Re: [PATCH 2/5] ath6kl: testmode support

[Dan Carpenter]

On Wed, Jun 08, 2011 at 02:54:30PM +0300, Kalle Valo wrote:
> Add testmode support for running low level hardware tests. The testmode
> is enabled by setting testmode module parameter to 1.
> 
> For now only a simple command passing is supported. More advanced
> support will be implemented later.
> 

It's Greg's call, but probably it's late in the release cycle to add
this feature.  Also are we even still developing this driver?  I
thought the real driver was moved to a different tree?

regards,
dan carpenter

Re: [PATCH 2/5] ath6kl: testmode support

[Kalle Valo]

Dan Carpenter <error27@gmail.com> writes:

> On Wed, Jun 08, 2011 at 02:54:30PM +0300, Kalle Valo wrote:
>> Add testmode support for running low level hardware tests. The testmode
>> is enabled by setting testmode module parameter to 1.
>> 
>> For now only a simple command passing is supported. More advanced
>> support will be implemented later.
>> 
>
> It's Greg's call, but probably it's late in the release cycle to add
> this feature.

Do you mean for 3.0? I was thinking of getting this into 3.1, didn't
even consider 3.0.

> Also are we even still developing this driver? I thought the real
> driver was moved to a different tree?

We are maintaining still maintaining the staging driver and fixing the
critical issues until the cleaned up driver is merged.

The testmode support got broke when the private ioctl interfaces were
removed from the driver. I had people asking me how to use testmode
and when I started implementing it through nl80211 testmode interface.
So this was a feature regression.

The cleanup of the driver is happening here:

http://git.kernel.org/?p=linux/kernel/git/kvalo/ath6kl-cleanup.git;a=summary

-- 
Kalle Valo

[PATCH 3/5] ath6kl: cache firmware

[Kalle Valo]

From: Kalle Valo <kalle.valo@atheros.com>

Drivers should not request firmware during resume. Fix ath6kl to
cache the firmware instead.

Signed-off-by: Kalle Valo <kalle.valo@atheros.com>
---
 drivers/staging/ath6kl/os/linux/ar6000_drv.c       |   53 ++++++++++++++------
 .../staging/ath6kl/os/linux/include/ar6000_drv.h   |    9 +++
 2 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/ath6kl/os/linux/ar6000_drv.c b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
index 30ae63b..74aa343 100644
--- a/drivers/staging/ath6kl/os/linux/ar6000_drv.c
+++ b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
@@ -954,9 +954,13 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
     const char *filename;
     const struct firmware *fw_entry;
     u32 fw_entry_size;
+    u8 **buf;
+    size_t *buf_len;
 
     switch (file) {
         case AR6K_OTP_FILE:
+		buf = &ar->fw_otp;
+		buf_len = &ar->fw_otp_len;
             if (ar->arVersion.target_ver == AR6003_REV1_VERSION) {
                 filename = AR6003_REV1_OTP_FILE;
             } else if (ar->arVersion.target_ver == AR6003_REV2_VERSION) {
@@ -970,6 +974,8 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
             break;
 
         case AR6K_FIRMWARE_FILE:
+		buf = &ar->fw;
+		buf_len = &ar->fw_len;
             if (ar->arVersion.target_ver == AR6003_REV1_VERSION) {
                 filename = AR6003_REV1_FIRMWARE_FILE;
             } else if (ar->arVersion.target_ver == AR6003_REV2_VERSION) {
@@ -1028,6 +1034,8 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
             break;
 
         case AR6K_PATCH_FILE:
+		buf = &ar->fw_patch;
+		buf_len = &ar->fw_patch_len;
             if (ar->arVersion.target_ver == AR6003_REV1_VERSION) {
                 filename = AR6003_REV1_PATCH_FILE;
             } else if (ar->arVersion.target_ver == AR6003_REV2_VERSION) {
@@ -1041,6 +1049,8 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
             break;
 
         case AR6K_BOARD_DATA_FILE:
+		buf = &ar->fw_data;
+		buf_len = &ar->fw_data_len;
             if (ar->arVersion.target_ver == AR6003_REV1_VERSION) {
                 filename = AR6003_REV1_BOARD_DATA_FILE;
             } else if (ar->arVersion.target_ver == AR6003_REV2_VERSION) {
@@ -1057,23 +1067,33 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
             AR_DEBUG_PRINTF(ATH_DEBUG_ERR, ("Unknown file type: %d\n", file));
             return A_ERROR;
     }
-    if ((A_REQUEST_FIRMWARE(&fw_entry, filename, ((struct device *)ar->osDevInfo.pOSDevice))) != 0)
-    {
-        AR_DEBUG_PRINTF(ATH_DEBUG_ERR, ("Failed to get %s\n", filename));
-        return A_ENOENT;
+
+    if (WARN_ON((buf == NULL) || (buf_len == NULL)))
+	    return A_ERROR;
+
+    if (*buf == NULL) {
+	    if ((A_REQUEST_FIRMWARE(&fw_entry, filename, ((struct device *)ar->osDevInfo.pOSDevice))) != 0)
+	    {
+		    AR_DEBUG_PRINTF(ATH_DEBUG_ERR, ("Failed to get %s\n", filename));
+		    return A_ENOENT;
+	    }
+
+	    *buf = kmemdup(fw_entry->data, fw_entry->size, GFP_KERNEL);
+	    *buf_len = fw_entry->size;
+	    A_RELEASE_FIRMWARE(fw_entry);
     }
 
 #ifdef SOFTMAC_FILE_USED
-    if (file==AR6K_BOARD_DATA_FILE && fw_entry->data) {
-        ar6000_softmac_update(ar, (u8 *)fw_entry->data, fw_entry->size);
+    if (file==AR6K_BOARD_DATA_FILE && *buf_len) {
+        ar6000_softmac_update(ar, *buf, *buf_len);
     }
 #endif 
 
 
-    fw_entry_size = fw_entry->size;
+    fw_entry_size = *buf_len;
 
     /* Load extended board data for AR6003 */
-    if ((file==AR6K_BOARD_DATA_FILE) && (fw_entry->data)) {
+    if ((file==AR6K_BOARD_DATA_FILE) && *buf) {
         u32 board_ext_address;
         u32 board_ext_data_size;
         u32 board_data_size;
@@ -1089,14 +1109,13 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
         AR_DEBUG_PRINTF(ATH_DEBUG_INFO, ("Board extended Data download address: 0x%x\n", board_ext_address));
 
         /* check whether the target has allocated memory for extended board data and file contains extended board data */
-        if ((board_ext_address) && (fw_entry->size == (board_data_size + board_ext_data_size))) {
+        if ((board_ext_address) && (*buf_len == (board_data_size + board_ext_data_size))) {
             u32 param;
 
-            status = BMIWriteMemory(ar->arHifDevice, board_ext_address, (u8 *)(fw_entry->data + board_data_size), board_ext_data_size);
+            status = BMIWriteMemory(ar->arHifDevice, board_ext_address, (u8 *)(*buf + board_data_size), board_ext_data_size);
 
             if (status) {
                 AR_DEBUG_PRINTF(ATH_DEBUG_ERR, ("BMI operation failed: %d\n", __LINE__));
-                A_RELEASE_FIRMWARE(fw_entry);
                 return A_ERROR;
             }
 
@@ -1110,17 +1129,16 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
     }
 
     if (compressed) {
-        status = BMIFastDownload(ar->arHifDevice, address, (u8 *)fw_entry->data, fw_entry_size);
+        status = BMIFastDownload(ar->arHifDevice, address, *buf, fw_entry_size);
     } else {
-        status = BMIWriteMemory(ar->arHifDevice, address, (u8 *)fw_entry->data, fw_entry_size);
+        status = BMIWriteMemory(ar->arHifDevice, address, *buf, fw_entry_size);
     }
 
     if (status) {
         AR_DEBUG_PRINTF(ATH_DEBUG_ERR, ("BMI operation failed: %d\n", __LINE__));
-        A_RELEASE_FIRMWARE(fw_entry);
         return A_ERROR;
     }
-    A_RELEASE_FIRMWARE(fw_entry);
+
     return 0;
 }
 
@@ -2088,6 +2106,11 @@ ar6000_destroy(struct net_device *dev, unsigned int unregister)
     ar6000_remove_ap_interface();
 #endif /*CONFIG_AP_VIRTUAL_ADAPTER_SUPPORT */
 
+    kfree(ar->fw_otp);
+    kfree(ar->fw);
+    kfree(ar->fw_patch);
+    kfree(ar->fw_data);
+
     AR_DEBUG_PRINTF(ATH_DEBUG_INFO,("-ar6000_destroy \n"));
 }
 
diff --git a/drivers/staging/ath6kl/os/linux/include/ar6000_drv.h b/drivers/staging/ath6kl/os/linux/include/ar6000_drv.h
index 22453b0..2911ea0 100644
--- a/drivers/staging/ath6kl/os/linux/include/ar6000_drv.h
+++ b/drivers/staging/ath6kl/os/linux/include/ar6000_drv.h
@@ -651,6 +651,15 @@ struct ar6_softc {
     void                    *arApDev;
 #endif
     u8 arAutoAuthStage;
+
+	u8 *fw_otp;
+	size_t fw_otp_len;
+	u8 *fw;
+	size_t fw_len;
+	u8 *fw_patch;
+	size_t fw_patch_len;
+	u8 *fw_data;
+	size_t fw_data_len;
 };
 
 #ifdef CONFIG_AP_VIRTUAL_ADAPTER_SUPPORT

Re: [PATCH 3/5] ath6kl: cache firmware

[Dan Carpenter]

On Wed, Jun 08, 2011 at 02:54:40PM +0300, Kalle Valo wrote:
> From: Kalle Valo <kalle.valo@atheros.com>
> 
> Drivers should not request firmware during resume. Fix ath6kl to
> cache the firmware instead.
> 
> Signed-off-by: Kalle Valo <kalle.valo@atheros.com>
> ---
>  drivers/staging/ath6kl/os/linux/ar6000_drv.c       |   53 ++++++++++++++------
>  .../staging/ath6kl/os/linux/include/ar6000_drv.h   |    9 +++
>  2 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/ath6kl/os/linux/ar6000_drv.c b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
> index 30ae63b..74aa343 100644
> --- a/drivers/staging/ath6kl/os/linux/ar6000_drv.c
> +++ b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
> @@ -954,9 +954,13 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
>      const char *filename;
>      const struct firmware *fw_entry;
>      u32 fw_entry_size;
> +    u8 **buf;
> +    size_t *buf_len;

Don't make buf_len a pointer that just makes the code messier.

[snip]

> +
> +    if (WARN_ON((buf == NULL) || (buf_len == NULL)))
> +	    return A_ERROR;

This test is a nonsense.  "buf" and buf_len are always valid
pointers here.  Generally, I'd almost prefer a oops with a stack
trace instead of adding in bogus error handling code for stuff that
can't happen.

Also "buf" is never initialized to NULL, so if there were a
programming bug introduced, it would generate an uninitialized
variable warning even without the superflous test.  So lets remove
it.

> +
> +    if (*buf == NULL) {
> +	    if ((A_REQUEST_FIRMWARE(&fw_entry, filename, ((struct device *)ar->osDevInfo.pOSDevice))) != 0)
> +	    {
            ^

This is on the wrong line.

> +		    AR_DEBUG_PRINTF(ATH_DEBUG_ERR, ("Failed to get %s\n", filename));
> +		    return A_ENOENT;
> +	    }
> +
> +	    *buf = kmemdup(fw_entry->data, fw_entry->size, GFP_KERNEL);
> +	    *buf_len = fw_entry->size;
> +	    A_RELEASE_FIRMWARE(fw_entry);
>      }
>  
>  #ifdef SOFTMAC_FILE_USED
> -    if (file==AR6K_BOARD_DATA_FILE && fw_entry->data) {
> -        ar6000_softmac_update(ar, (u8 *)fw_entry->data, fw_entry->size);
> +    if (file==AR6K_BOARD_DATA_FILE && *buf_len) {
              ^^^^
White space.

> +        ar6000_softmac_update(ar, *buf, *buf_len);
>      }
>  #endif 
>  
>  
> -    fw_entry_size = fw_entry->size;
> +    fw_entry_size = *buf_len;
>  
>      /* Load extended board data for AR6003 */
> -    if ((file==AR6K_BOARD_DATA_FILE) && (fw_entry->data)) {
> +    if ((file==AR6K_BOARD_DATA_FILE) && *buf) {
               ^^^^
White space is wrong here.

>          u32 board_ext_address;
>          u32 board_ext_data_size;
>          u32 board_data_size;
> @@ -1089,14 +1109,13 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
>          AR_DEBUG_PRINTF(ATH_DEBUG_INFO, ("Board extended Data download address: 0x%x\n", board_ext_address));
>  
>          /* check whether the target has allocated memory for extended board data and file contains extended board data */
> -        if ((board_ext_address) && (fw_entry->size == (board_data_size + board_ext_data_size))) {
> +        if ((board_ext_address) && (*buf_len == (board_data_size + board_ext_data_size))) {
               ^                 ^

Those parenthesis are not needed.

>              u32 param;
>  
> -            status = BMIWriteMemory(ar->arHifDevice, board_ext_address, (u8 *)(fw_entry->data + board_data_size), board_ext_data_size);
> +            status = BMIWriteMemory(ar->arHifDevice, board_ext_address, (u8 *)(*buf + board_data_size), board_ext_data_size);
                                                                           ^^^^^^
This cast isn't needed by the way.

regards,
dan carpenter

Re: [PATCH 3/5] ath6kl: cache firmware

[Kalle Valo]

Dan Carpenter <error27@gmail.com> writes:

>> --- a/drivers/staging/ath6kl/os/linux/ar6000_drv.c
>> +++ b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
>> @@ -954,9 +954,13 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
>>      const char *filename;
>>      const struct firmware *fw_entry;
>>      u32 fw_entry_size;
>> +    u8 **buf;
>> +    size_t *buf_len;
>
> Don't make buf_len a pointer that just makes the code messier.

I need the pointer later so that I can store the length:

            *buf_len = fw_entry->size;

>> +    if (WARN_ON((buf == NULL) || (buf_len == NULL)))
>> +	    return A_ERROR;
>
> This test is a nonsense.  "buf" and buf_len are always valid
> pointers here.

Yeah, I tried to be extra careful here, but it looks stupid now.

> Generally, I'd almost prefer a oops with a stack trace instead of
> adding in bogus error handling code for stuff that can't happen.

Depends on the case. In devices where ar6003 is usually is used there
are no serial consoles nor any other way to debug the crash.

> Also "buf" is never initialized to NULL, so if there were a
> programming bug introduced, it would generate an uninitialized
> variable warning even without the superflous test.  So lets remove
> it.

You have a point, I'll remove the test. I'll send v2.

>> +    if (*buf == NULL) {
>> +	    if ((A_REQUEST_FIRMWARE(&fw_entry, filename, ((struct device *)ar->osDevInfo.pOSDevice))) != 0)
>> +	    {
>             ^
>
> This is on the wrong line.

I'll answer this and all the other style comments in one go:

All the style issues are from the original code. I want to separate
functional and cleanup patches so I didn't do any cleanup in this
patch. But as we are doing the cleanup in a different tree anyway I
didn't see the point of sending a separate cleanup patch.

-- 
Kalle Valo

Re: [PATCH 3/5] ath6kl: cache firmware

[Dan Carpenter]

On Thu, Jun 09, 2011 at 04:14:34PM +0300, Kalle Valo wrote:
> Dan Carpenter <error27@gmail.com> writes:
> 
> >> --- a/drivers/staging/ath6kl/os/linux/ar6000_drv.c
> >> +++ b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
> >> @@ -954,9 +954,13 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
> >>      const char *filename;
> >>      const struct firmware *fw_entry;
> >>      u32 fw_entry_size;
> >> +    u8 **buf;
> >> +    size_t *buf_len;
> >
> > Don't make buf_len a pointer that just makes the code messier.
> 
> I need the pointer later so that I can store the length:
> 
>             *buf_len = fw_entry->size;

No.  The normal way to store an int is to do it like this:

		buf_len = fw_entry->size;

[snip]

> >> +    if (*buf == NULL) {
> >> +	    if ((A_REQUEST_FIRMWARE(&fw_entry, filename, ((struct device *)ar->osDevInfo.pOSDevice))) != 0)
> >> +	    {
> >             ^
> >
> > This is on the wrong line.
> 
> I'll answer this and all the other style comments in one go:
> 
> All the style issues are from the original code. I want to separate
> functional and cleanup patches so I didn't do any cleanup in this
> patch. But as we are doing the cleanup in a different tree anyway I
> didn't see the point of sending a separate cleanup patch.
> 

No.  That was in a section of code which was added.  There wasn't
a "buf" variable in the original code.  You obviously know how it's
supposed to look because you did it correctly on the line before.

What I'm saying is don't be lazy and careless.  That's not a
difficult thing.

regards,
dan carpenter

Re: [PATCH 3/5] ath6kl: cache firmware

[Dan Carpenter]

On Thu, Jun 09, 2011 at 06:42:41PM +0300, Dan Carpenter wrote:
> On Thu, Jun 09, 2011 at 04:14:34PM +0300, Kalle Valo wrote:
> > Dan Carpenter <error27@gmail.com> writes:
> > 
> > >> --- a/drivers/staging/ath6kl/os/linux/ar6000_drv.c
> > >> +++ b/drivers/staging/ath6kl/os/linux/ar6000_drv.c
> > >> @@ -954,9 +954,13 @@ ar6000_transfer_bin_file(struct ar6_softc *ar, AR6K_BIN_FILE file, u32 address,
> > >>      const char *filename;
> > >>      const struct firmware *fw_entry;
> > >>      u32 fw_entry_size;
> > >> +    u8 **buf;
> > >> +    size_t *buf_len;
> > >
> > > Don't make buf_len a pointer that just makes the code messier.
> > 
> > I need the pointer later so that I can store the length:
> > 
> >             *buf_len = fw_entry->size;
> 
> No.  The normal way to store an int is to do it like this:
> 
> 		buf_len = fw_entry->size;

I miss read what you were doing here.  You code is correct here.  I
appologize for that.

regards,
dan carpenter

Re: [PATCH 3/5] ath6kl: cache firmware

[Kalle Valo]

Dan Carpenter <error27@gmail.com> writes:

>> >> +    if (*buf == NULL) {
>> >> +	    if ((A_REQUEST_FIRMWARE(&fw_entry, filename, ((struct device *)ar->osDevInfo.pOSDevice))) != 0)
>> >> +	    {
>> >             ^
>> >
>> > This is on the wrong line.
>> 
>> I'll answer this and all the other style comments in one go:
>> 
>> All the style issues are from the original code. I want to separate
>> functional and cleanup patches so I didn't do any cleanup in this
>> patch. But as we are doing the cleanup in a different tree anyway I
>> didn't see the point of sending a separate cleanup patch.
>> 
>
> No.  That was in a section of code which was added.  There wasn't
> a "buf" variable in the original code.  You obviously know how it's
> supposed to look because you did it correctly on the line before.

Oh, you are right. Sorry about that, I'll fix that in v2.

-- 
Kalle Valo

[PATCH 4/5] ath6kl: Fix a kernel panic furing suspend/resume

[Kalle Valo]

From: Vasanthakumar Thiagarajan <vasanth@atheros.com>

The kernel panic happens when we try to complete a pending
scan request while going to suspend state. The cause for this
kernel panic is accessing a freed memory (ar->arWmin). This
is freed before ar6k_cfg80211_scanComplete_event() getting
called where it is dereferenced.

RIP: 0010:[<ffffffffa042e726>]  [<ffffffffa042e726>] wlan_iterate_nodes+0x16/0xc0 [ath6kl]
RSP: 0018:ffff8800719fbce8  EFLAGS: 00010296
RAX: ffff880071bbcc00 RBX: ffff880037b22520 RCX: ffff880077413c80
RDX: ffff880037b221c0 RSI: ffffffffa041ef10 RDI: 0000000000000020
RBP: ffff8800719fbd18 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000000400 R11: 0000000000000000 R12: 0000000000000010
R13: ffff8800719fbdd8 R14: 00007fff83a84b60 R15: 0000000000000001
FS:  00007fdccb8a7700(0000) GS:ffff880077400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000148 CR3: 0000000070604000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
	DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process rmmod (pid: 1998, threadinfo ffff8800719fa000, task ffff880066712d80)
	Stack:
	0000000000000000 ffff880037b22520 0000000000000010 ffff8800719fbdd8
	00007fff83a84b60 0000000000000001 ffff8800719fbd28 ffffffffa0429fe2
	ffff8800719fbd58 ffffffffa041ee5f ffff8800719fbd58 ffff880037b22520
	Call Trace:
	[<ffffffffa0429fe2>] wmi_iterate_nodes+0x12/0x20 [ath6kl]
	[<ffffffffa041ee5f>] ar6k_cfg80211_scanComplete_event+0x3f/0xf0 [ath6kl]
	[<ffffffffa04245f1>] ar6000_close+0x61/0x100 [ath6kl]
	[<ffffffff814d6736>] __dev_close_many+0x96/0x100
	[<ffffffff814d688d>] dev_close_many+0x9d/0x120
	[<ffffffff814d6a48>] rollback_registered_many+0xe8/0x290
	[<ffffffff814d6d16>] unregister_netdevice_queue+0x96/0x100
	[<ffffffff814d6ea0>] unregister_netdev+0x20/0x30
	[<ffffffffa0420259>] ar6000_destroy+0x119/0x180 [ath6kl]
	[<ffffffffa043182a>] ar6k_cleanup_module+0x2a/0x33 [ath6kl]
	[<ffffffff81098fde>] sys_delete_module+0x19e/0x270
	[<ffffffff815d7542>] system_call_fastpath+0x16/0x1b
	Code: c3 0f 1f 40 00 48 89 df e8 68 ff ff ff eb df 66 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 08 0f 1f 44 00 00
	8b af 28 01 00 00 4c 8d 7f 08 49 89 fc 48 89 f3 49 89 d6 41
	RIP  [<ffffffffa042e726>] wlan_iterate_nodes+0x16/0xc0 [ath6kl]
	RSP <ffff8800719fbce8>

Signed-off-by: Vasanthakumar Thiagarajan <vasanth@atheros.com>
---
 drivers/staging/ath6kl/os/linux/cfg80211.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/ath6kl/os/linux/cfg80211.c b/drivers/staging/ath6kl/os/linux/cfg80211.c
index a0c0819..07e6f55 100644
--- a/drivers/staging/ath6kl/os/linux/cfg80211.c
+++ b/drivers/staging/ath6kl/os/linux/cfg80211.c
@@ -871,7 +871,8 @@ ar6k_cfg80211_scanComplete_event(struct ar6_softc *ar, int status)
     if(ar->scan_request)
     {
         /* Translate data to cfg80211 mgmt format */
-        wmi_iterate_nodes(ar->arWmi, ar6k_cfg80211_scan_node, ar->wdev->wiphy);
+	if (ar->arWmi)
+		wmi_iterate_nodes(ar->arWmi, ar6k_cfg80211_scan_node, ar->wdev->wiphy);
 
         cfg80211_scan_done(ar->scan_request,
             ((status & A_ECANCELED) || (status & A_EBUSY)) ? true : false);

[PATCH 5/5] ath6kl: fix crash when interface is closed but scan is ongoing

[Kalle Valo]

When ath6kl module was resumed while a scan was ongoing, for example during
suspend, the driver would crash in ar6k_cfg80211_scanComplete_event():

[26581.586440] Call Trace:
[26581.586440]  [<f99ffeda>] ? ar6k_cfg80211_scanComplete_event+0xaa/0xaa [ath6kl]
[26581.586440]  [<f9a0a020>] wmi_iterate_nodes+0xb/0xd [ath6kl]
[26581.586440]  [<f99ffe78>] ar6k_cfg80211_scanComplete_event+0x48/0xaa [ath6kl]
[26581.586440]  [<f9a038ae>] ar6000_close+0x77/0x7e [ath6kl]
[26581.586440]  [<c139c25d>] __dev_close_many+0x87/0xab
[26581.586440]  [<c139c30a>] dev_close_many+0x54/0xab
[26581.586440]  [<c139c437>] rollback_registered_many+0xa5/0x19e
[26581.586440]  [<c139c595>] rollback_registered+0x23/0x2f
[26581.586440]  [<c139c5ed>] unregister_netdevice_queue+0x4c/0x69
[26581.586440]  [<c139c6b2>] unregister_netdev+0x18/0x1f
[26581.586440]  [<f9a00d4c>] ar6000_destroy+0xf8/0x115 [ath6kl]
[26581.586440]  [<f9a0c765>] ar6k_cleanup_module+0x20/0x29 [ath6kl]
[26581.586440]  [<c1062843>] sys_delete_module+0x181/0x1d9
[26581.586440]  [<c105876b>] ? lock_release_holdtime+0x2b/0xcd
[26581.586440]  [<c10b55dc>] ? sys_munmap+0x3b/0x42
[26581.586440]  [<c14a99dc>] ? restore_all+0xf/0xf
[26581.586440]  [<c14aeb6c>] sysenter_do_call+0x12/0x32
[26581.586440] Code: 89 53 6c 75 07 89 d8 e8 c0 ff ff ff 89 f0 e8 2c f2 a9 c7 5b 5e 5d c3 55 89 e5 57 56 53 89 c3 83 ec 08 89 55 f0 8d 78 04 89 4d ec <8b> b0 b8 00 00 00 46 89 b0 b8 00 00 00 89 f8 e8 ae ed a9 c7 8b

Fix the function not to iterate nodes when the scan is aborted. The nodes
are already freed when the module is being unloaded. Patch "ath6kl: Fix a
kernel panic furing suspend/resume" tried to fix this already but it wasn't
enough.

Also fix a bug where the status was checked as a bitfield with '&' operator.
But it's not a bitfield, just a regular (enum like) value.

Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/staging/ath6kl/os/linux/cfg80211.c |   31 ++++++++++++++++------------
 1 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/ath6kl/os/linux/cfg80211.c b/drivers/staging/ath6kl/os/linux/cfg80211.c
index 07e6f55..b098c22 100644
--- a/drivers/staging/ath6kl/os/linux/cfg80211.c
+++ b/drivers/staging/ath6kl/os/linux/cfg80211.c
@@ -868,26 +868,31 @@ ar6k_cfg80211_scanComplete_event(struct ar6_softc *ar, int status)
 
     AR_DEBUG_PRINTF(ATH_DEBUG_INFO, ("%s: status %d\n", __func__, status));
 
-    if(ar->scan_request)
-    {
-        /* Translate data to cfg80211 mgmt format */
-	if (ar->arWmi)
-		wmi_iterate_nodes(ar->arWmi, ar6k_cfg80211_scan_node, ar->wdev->wiphy);
+    if (!ar->scan_request)
+	    return;
+
+    if ((status == A_ECANCELED) || (status == A_EBUSY)) {
+	    cfg80211_scan_done(ar->scan_request, true);
+	    goto out;
+    }
+
+    /* Translate data to cfg80211 mgmt format */
+    wmi_iterate_nodes(ar->arWmi, ar6k_cfg80211_scan_node, ar->wdev->wiphy);
 
-        cfg80211_scan_done(ar->scan_request,
-            ((status & A_ECANCELED) || (status & A_EBUSY)) ? true : false);
+    cfg80211_scan_done(ar->scan_request, false);
 
-        if(ar->scan_request->n_ssids &&
-           ar->scan_request->ssids[0].ssid_len) {
+    if(ar->scan_request->n_ssids &&
+       ar->scan_request->ssids[0].ssid_len) {
             u8 i;
 
             for (i = 0; i < ar->scan_request->n_ssids; i++) {
-                wmi_probedSsid_cmd(ar->arWmi, i+1, DISABLE_SSID_FLAG,
-                                   0, NULL);
+		    wmi_probedSsid_cmd(ar->arWmi, i+1, DISABLE_SSID_FLAG,
+				       0, NULL);
             }
-        }
-        ar->scan_request = NULL;
     }
+
+out:
+    ar->scan_request = NULL;
 }
 
 static int

Re: [PATCH 5/5] ath6kl: fix crash when interface is closed but scan is ongoing

[Dan Carpenter]

On Wed, Jun 08, 2011 at 02:55:01PM +0300, Kalle Valo wrote:
> -        /* Translate data to cfg80211 mgmt format */
> -	if (ar->arWmi)
            ^^^^^^^^^
	You removed this check on purpose?  Is this only NULL when
	the scan is canceled?

> -		wmi_iterate_nodes(ar->arWmi, ar6k_cfg80211_scan_node, ar->wdev->wiphy);
> +    if (!ar->scan_request)
> +	    return;
> +
> +    if ((status == A_ECANCELED) || (status == A_EBUSY)) {
> +	    cfg80211_scan_done(ar->scan_request, true);
> +	    goto out;
> +    }
> +
> +    /* Translate data to cfg80211 mgmt format */
> +    wmi_iterate_nodes(ar->arWmi, ar6k_cfg80211_scan_node, ar->wdev->wiphy);

regards,
dan carpenter

Re: [PATCH 5/5] ath6kl: fix crash when interface is closed but scan is ongoing

[Dan Carpenter]

On Wed, Jun 08, 2011 at 11:03:32PM +0300, Dan Carpenter wrote:
> On Wed, Jun 08, 2011 at 02:55:01PM +0300, Kalle Valo wrote:
> > -        /* Translate data to cfg80211 mgmt format */
> > -	if (ar->arWmi)
>             ^^^^^^^^^
> 	You removed this check on purpose?  Is this only NULL when
> 	the scan is canceled?

Grr...  That's the check that was just added in [PATCH 4/4] of this
exact same series!  Seriously?  Why are we submitting known wrong
patches???

regards,
dan carpenter

Re: [PATCH 5/5] ath6kl: fix crash when interface is closed but scan is ongoing

[Kalle Valo]

Dan Carpenter <error27@gmail.com> writes:

> On Wed, Jun 08, 2011 at 11:03:32PM +0300, Dan Carpenter wrote:
>> On Wed, Jun 08, 2011 at 02:55:01PM +0300, Kalle Valo wrote:
>> > -        /* Translate data to cfg80211 mgmt format */
>> > -	if (ar->arWmi)
>>             ^^^^^^^^^
>> 	You removed this check on purpose?  Is this only NULL when
>> 	the scan is canceled?
>
> Grr...  That's the check that was just added in [PATCH 4/4] of this
> exact same series!  Seriously?  Why are we submitting known wrong
> patches???

I only combined the pending ath6kl patches, I did not edit them.
Vasanth sent patch 4 few days earlier and that's why you see the
change here. But if greg wants, I can drop patch 4 and resend the
series.

But now that you mention this, I realise that my sob line is missing
from patch 4 as well.

-- 
Kalle Valo

Re: [PATCH 5/5] ath6kl: fix crash when interface is closed but scan is ongoing

[Dan Carpenter]

On Thu, Jun 09, 2011 at 04:23:36PM +0300, Kalle Valo wrote:
> I only combined the pending ath6kl patches, I did not edit them.
> Vasanth sent patch 4 few days earlier and that's why you see the
> change here. But if greg wants, I can drop patch 4 and resend the
> series.

Part of the problem is that patch doesn't add the check for NULL
consistently throughout the function.  There is a place where a NULL
deref could still be triggered later on.  Let's just drop it instead
of fixing it.

regards,
dan carpenter

Re: [PATCH 5/5] ath6kl: fix crash when interface is closed but scan is ongoing

[Kalle Valo]

Dan Carpenter <error27@gmail.com> writes:

> On Thu, Jun 09, 2011 at 04:23:36PM +0300, Kalle Valo wrote:
>> I only combined the pending ath6kl patches, I did not edit them.
>> Vasanth sent patch 4 few days earlier and that's why you see the
>> change here. But if greg wants, I can drop patch 4 and resend the
>> series.
>
> Part of the problem is that patch doesn't add the check for NULL
> consistently throughout the function.  There is a place where a NULL
> deref could still be triggered later on.  Let's just drop it instead
> of fixing it.

Agreed.

But I see that Greg applied the patch 4 already. So I just drop patch
4 from my patchset and resend patch 5 as is (ie. removing the
unnecessary null check).

-- 
Kalle Valo

Re: [PATCH 5/5] ath6kl: fix crash when interface is closed but scan is ongoing

[Dan Carpenter]

On Mon, Jun 13, 2011 at 10:52:33AM +0300, Kalle Valo wrote:
> Dan Carpenter <error27@gmail.com> writes:
> 
> > On Thu, Jun 09, 2011 at 04:23:36PM +0300, Kalle Valo wrote:
> >> I only combined the pending ath6kl patches, I did not edit them.
> >> Vasanth sent patch 4 few days earlier and that's why you see the
> >> change here. But if greg wants, I can drop patch 4 and resend the
> >> series.
> >
> > Part of the problem is that patch doesn't add the check for NULL
> > consistently throughout the function.  There is a place where a NULL
> > deref could still be triggered later on.  Let's just drop it instead
> > of fixing it.
> 
> Agreed.
> 
> But I see that Greg applied the patch 4 already. So I just drop patch
> 4 from my patchset and resend patch 5 as is (ie. removing the
> unnecessary null check).

Yep.  It really should go the 3.0 kernel along with [PATCH 3/5]
ath6kl: cache firmware.

regards,
dan carpenter

Re: [PATCH 5/5] ath6kl: fix crash when interface is closed but scan is ongoing

[Kalle Valo]

Dan Carpenter <error27@gmail.com> writes:

> On Mon, Jun 13, 2011 at 10:52:33AM +0300, Kalle Valo wrote:
>> Dan Carpenter <error27@gmail.com> writes:
>> 
>> > On Thu, Jun 09, 2011 at 04:23:36PM +0300, Kalle Valo wrote:
>> >> I only combined the pending ath6kl patches, I did not edit them.
>> >> Vasanth sent patch 4 few days earlier and that's why you see the
>> >> change here. But if greg wants, I can drop patch 4 and resend the
>> >> series.
>> >
>> > Part of the problem is that patch doesn't add the check for NULL
>> > consistently throughout the function.  There is a place where a NULL
>> > deref could still be triggered later on.  Let's just drop it instead
>> > of fixing it.
>> 
>> Agreed.
>> 
>> But I see that Greg applied the patch 4 already. So I just drop patch
>> 4 from my patchset and resend patch 5 as is (ie. removing the
>> unnecessary null check).
>
> Yep.  It really should go the 3.0 kernel along with [PATCH 3/5]
> ath6kl: cache firmware.

Thanks, I'll mention this in the cover letter. I'll also change the
order of patches so that the most important patches are first.

Thanks for all the review and help.

-- 
Kalle Valo

Re: [PATCH 5/5] ath6kl: fix crash when interface is closed but scan is ongoing

[Dan Carpenter]

On Mon, Jun 13, 2011 at 11:03:37AM +0300, Kalle Valo wrote:
> Thanks, I'll mention this in the cover letter. I'll also change the
> order of patches so that the most important patches are first.
> 

Greg could say for sure, but sometimes it helps to break it up into
two separate patch sets.  When you're applying them, you just tag
all the messages in the thread and apply them with one command.

[PATCH 0/2] fix crashes for 3.0
[PATCH 0/2] new stuff for 3.1  (applies on top of the fixes)

regards,
dan carpenter

Re: [PATCH 5/5] ath6kl: fix crash when interface is closed but scan is ongoing

[Kalle Valo]

Dan Carpenter <error27@gmail.com> writes:

> On Mon, Jun 13, 2011 at 11:03:37AM +0300, Kalle Valo wrote:
>> Thanks, I'll mention this in the cover letter. I'll also change the
>> order of patches so that the most important patches are first.
>> 
>
> Greg could say for sure, but sometimes it helps to break it up into
> two separate patch sets.  When you're applying them, you just tag
> all the messages in the thread and apply them with one command.
>
> [PATCH 0/2] fix crashes for 3.0
> [PATCH 0/2] new stuff for 3.1  (applies on top of the fixes)

I'll do that, thanks.

-- 
Kalle Valo

Re: [PATCH 5/5] ath6kl: fix crash when interface is closed but scan is ongoing

[Kalle Valo]

Dan Carpenter <error27@gmail.com> writes:

> On Wed, Jun 08, 2011 at 02:55:01PM +0300, Kalle Valo wrote:
>> -        /* Translate data to cfg80211 mgmt format */
>> -	if (ar->arWmi)
>             ^^^^^^^^^
> 	You removed this check on purpose?  Is this only NULL when
> 	the scan is canceled?

It's null only when the module is being destroyed. And I changed it so
that when ar6000_destroy() aborts the scan we do not use ar->arWmi at
all and avoid the crash.

-- 
Kalle Valo