[PATCH 1/2] ath5k: Print out opmode in debugfs.

[PATCH 1/2] ath5k: Print out opmode in debugfs.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Helps debug multi-VIF scenarios.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 c2d549f... a342a9d... M	drivers/net/wireless/ath/ath5k/debug.c
:100644 100644 53e77bd... a9eb787... M	drivers/net/wireless/ath/debug.c
:100644 100644 fd3a020... a3a5a62... M	drivers/net/wireless/ath/debug.h
 drivers/net/wireless/ath/ath5k/debug.c |   10 ++++++++++
 drivers/net/wireless/ath/debug.c       |   29 +++++++++++++++++++++++++++++
 drivers/net/wireless/ath/debug.h       |    3 +++
 3 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/debug.c b/drivers/net/wireless/ath/ath5k/debug.c
index c2d549f..a342a9d 100644
--- a/drivers/net/wireless/ath/ath5k/debug.c
+++ b/drivers/net/wireless/ath/ath5k/debug.c
@@ -60,6 +60,7 @@
 
 #include "base.h"
 #include "debug.h"
+#include "../debug.h"
 
 static unsigned int ath5k_debug;
 module_param_named(debug, ath5k_debug, uint, 0);
@@ -492,6 +493,7 @@ static ssize_t read_file_misc(struct file *file, char __user *user_buf,
 	char buf[700];
 	unsigned int len = 0;
 	u32 filt = ath5k_hw_get_rx_filter(sc->ah);
+	const char *tmp;
 
 	len += snprintf(buf+len, sizeof(buf)-len, "bssid-mask: %pM\n",
 			sc->bssidmask);
@@ -524,6 +526,14 @@ static ssize_t read_file_misc(struct file *file, char __user *user_buf,
 	else
 		len += snprintf(buf+len, sizeof(buf)-len, "\n");
 
+	tmp = ath_opmode_to_string(sc->opmode);
+	if (tmp)
+		len += snprintf(buf+len, sizeof(buf)-len, "opmode: %s\n",
+				tmp);
+	else
+		len += snprintf(buf+len, sizeof(buf)-len,
+				"opmode: UNKNOWN-%i\n", sc->opmode);
+
 	if (len > sizeof(buf))
 		len = sizeof(buf);
 
diff --git a/drivers/net/wireless/ath/debug.c b/drivers/net/wireless/ath/debug.c
index 53e77bd..a9eb787 100644
--- a/drivers/net/wireless/ath/debug.c
+++ b/drivers/net/wireless/ath/debug.c
@@ -30,3 +30,32 @@ void ath_print(struct ath_common *common, int dbg_mask, const char *fmt, ...)
 	va_end(args);
 }
 EXPORT_SYMBOL(ath_print);
+
+const char *ath_opmode_to_string(enum nl80211_iftype opmode)
+{
+	switch (opmode) {
+	case NL80211_IFTYPE_UNSPECIFIED:
+		return "UNSPEC";
+	case NL80211_IFTYPE_ADHOC:
+		return "ADHOC";
+	case NL80211_IFTYPE_STATION:
+		return "STATION";
+	case NL80211_IFTYPE_AP:
+		return "AP";
+	case NL80211_IFTYPE_AP_VLAN:
+		return "AP-VLAN";
+	case NL80211_IFTYPE_WDS:
+		return "WDS";
+	case NL80211_IFTYPE_MONITOR:
+		return "MONITOR";
+	case NL80211_IFTYPE_MESH_POINT:
+		return "MESH";
+	case NL80211_IFTYPE_P2P_CLIENT:
+		return "P2P-CLIENT";
+	case NL80211_IFTYPE_P2P_GO:
+		return "P2P-GO";
+	default:
+		return NULL;
+	}
+}
+EXPORT_SYMBOL(ath_opmode_to_string);
diff --git a/drivers/net/wireless/ath/debug.h b/drivers/net/wireless/ath/debug.h
index fd3a020..a3a5a62 100644
--- a/drivers/net/wireless/ath/debug.h
+++ b/drivers/net/wireless/ath/debug.h
@@ -77,4 +77,7 @@ ath_print(struct ath_common *common, int dbg_mask, const char *fmt, ...)
 }
 #endif /* CONFIG_ATH_DEBUG */
 
+/** Returns string describing opmode, or NULL if unknown mode. */
+const char *ath_opmode_to_string(enum nl80211_iftype opmode);
+
 #endif /* ATH_DEBUG_H */
-- 
1.7.2.2

[PATCH 2/2] ath5k: Adjust opmode when interfaces are removed.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Otherwise, if there is an AP and a STATION, and AP
is removed, the NIC will not revert back to STATION mode.

Reported-by: Eliad Peller <eliad@wizery.com>
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 dad7265... 813425d... M	drivers/net/wireless/ath/ath5k/base.c
 drivers/net/wireless/ath/ath5k/base.c |   40 +++++++++++++++++++++++++-------
 1 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index dad7265..813425d 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -62,6 +62,7 @@
 #include "reg.h"
 #include "debug.h"
 #include "ani.h"
+#include "../debug.h"
 
 static int modparam_nohwcrypt;
 module_param_named(nohwcrypt, modparam_nohwcrypt, bool, S_IRUGO);
@@ -517,12 +518,14 @@ struct ath_vif_iter_data {
 	bool		need_set_hw_addr;
 	bool		found_active;
 	bool		any_assoc;
+	enum nl80211_iftype opmode;
 };
 
 static void ath_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
 {
 	struct ath_vif_iter_data *iter_data = data;
 	int i;
+	struct ath5k_vif *avf = (void *)vif->drv_priv;
 
 	if (iter_data->hw_macaddr)
 		for (i = 0; i < ETH_ALEN; i++)
@@ -539,13 +542,29 @@ static void ath_vif_iter(void *data, u8 *mac, struct ieee80211_vif *vif)
 			iter_data->need_set_hw_addr = false;
 
 	if (!iter_data->any_assoc) {
-		struct ath5k_vif *avf = (void *)vif->drv_priv;
 		if (avf->assoc)
 			iter_data->any_assoc = true;
 	}
+
+	if (avf->opmode == NL80211_IFTYPE_AP)
+		iter_data->opmode = NL80211_IFTYPE_AP;
+	else
+		if (iter_data->opmode == NL80211_IFTYPE_UNSPECIFIED)
+			iter_data->opmode = avf->opmode;
 }
 
-void ath5k_update_bssid_mask(struct ath5k_softc *sc, struct ieee80211_vif *vif)
+static void ath_do_set_opmode(struct ath5k_softc *sc)
+{
+	struct ath5k_hw *ah = sc->ah;
+	ath5k_hw_set_opmode(ah, sc->opmode);
+	ATH5K_DBG(sc, ATH5K_DEBUG_MODE, "mode setup opmode %d (%s)\n",
+		  sc->opmode,
+		  ath_opmode_to_string(sc->opmode) ?
+		  ath_opmode_to_string(sc->opmode) : "UKNOWN");
+}
+
+void ath5k_update_bssid_mask(struct ath5k_softc *sc, struct ieee80211_vif *vif,
+			     bool update_opmode)
 {
 	struct ath_common *common = ath5k_hw_common(sc->ah);
 	struct ath_vif_iter_data iter_data;
@@ -558,6 +577,7 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc, struct ieee80211_vif *vif)
 	memset(&iter_data.mask, 0xff, ETH_ALEN);
 	iter_data.found_active = false;
 	iter_data.need_set_hw_addr = true;
+	iter_data.opmode = NL80211_IFTYPE_UNSPECIFIED;
 
 	if (vif)
 		ath_vif_iter(&iter_data, vif->addr, vif);
@@ -567,6 +587,11 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc, struct ieee80211_vif *vif)
 						   &iter_data);
 	memcpy(sc->bssidmask, iter_data.mask, ETH_ALEN);
 
+	if (update_opmode && sc->opmode != iter_data.opmode) {
+		sc->opmode = iter_data.opmode;
+		ath_do_set_opmode(sc);
+	}
+
 	if (iter_data.need_set_hw_addr && iter_data.found_active)
 		ath5k_hw_set_lladdr(sc->ah, iter_data.active_mac);
 
@@ -584,12 +609,9 @@ ath5k_mode_setup(struct ath5k_softc *sc, struct ieee80211_vif *vif)
 	ath5k_hw_set_rx_filter(ah, rfilt);
 
 	if (ath5k_hw_hasbssidmask(ah))
-		ath5k_update_bssid_mask(sc, vif);
-
-	/* configure operational mode */
-	ath5k_hw_set_opmode(ah, sc->opmode);
+		ath5k_update_bssid_mask(sc, vif, false);
 
-	ATH5K_DBG(sc, ATH5K_DEBUG_MODE, "mode setup opmode %d\n", sc->opmode);
+	ath_do_set_opmode(sc);
 	ATH5K_DBG(sc, ATH5K_DEBUG_MODE, "RX filter 0x%x\n", rfilt);
 }
 
@@ -2688,7 +2710,7 @@ ath5k_attach(struct pci_dev *pdev, struct ieee80211_hw *hw)
 	SET_IEEE80211_PERM_ADDR(hw, mac);
 	memcpy(&sc->lladdr, mac, ETH_ALEN);
 	/* All MAC address bits matter for ACKs */
-	ath5k_update_bssid_mask(sc, NULL);
+	ath5k_update_bssid_mask(sc, NULL, false);
 
 	regulatory->current_rd = ah->ah_capabilities.cap_eeprom.ee_regdomain;
 	ret = ath_regd_init(regulatory, hw->wiphy, ath5k_reg_notifier);
@@ -2905,7 +2927,7 @@ ath5k_remove_interface(struct ieee80211_hw *hw,
 	else if (avf->opmode == NL80211_IFTYPE_ADHOC)
 		sc->num_adhoc_vifs--;
 
-	ath5k_update_bssid_mask(sc, NULL);
+	ath5k_update_bssid_mask(sc, NULL, true);
 	mutex_unlock(&sc->lock);
 }
 
-- 
1.7.2.2

Re: [PATCH 2/2] ath5k: Adjust opmode when interfaces are removed.

[Bob Copeland]

On Fri, Oct 8, 2010 at 12:43 PM,  <greearb@candelatech.com> wrote:
> @@ -558,6 +577,7 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc, struct ieee80211_vif *vif)
>        memset(&iter_data.mask, 0xff, ETH_ALEN);
>        iter_data.found_active = false;
>        iter_data.need_set_hw_addr = true;
> +       iter_data.opmode = NL80211_IFTYPE_UNSPECIFIED;
>
>        if (vif)
>                ath_vif_iter(&iter_data, vif->addr, vif);
> @@ -567,6 +587,11 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc, struct ieee80211_vif *vif)
>                                                   &iter_data);
>        memcpy(sc->bssidmask, iter_data.mask, ETH_ALEN);
>
> +       if (update_opmode && sc->opmode != iter_data.opmode) {
> +               sc->opmode = iter_data.opmode;
> +               ath_do_set_opmode(sc);
> +       }
> +

Should we really couple updating bssid mask and configuring
the opmode?  Generally, I dislike adding boolean flags to
functions because it's hard to figure out from the callsite
what is happening (you have to go back to the prototype), and
it usually indicates that the abstraction is a little broken.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 2/2] ath5k: Adjust opmode when interfaces are removed.

[Ben Greear]

On 10/08/2010 11:03 AM, Bob Copeland wrote:
> On Fri, Oct 8, 2010 at 12:43 PM,<greearb@candelatech.com>  wrote:
>> @@ -558,6 +577,7 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc, struct ieee80211_vif *vif)
>>         memset(&iter_data.mask, 0xff, ETH_ALEN);
>>         iter_data.found_active = false;
>>         iter_data.need_set_hw_addr = true;
>> +       iter_data.opmode = NL80211_IFTYPE_UNSPECIFIED;
>>
>>         if (vif)
>>                 ath_vif_iter(&iter_data, vif->addr, vif);
>> @@ -567,6 +587,11 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc, struct ieee80211_vif *vif)
>>                                                    &iter_data);
>>         memcpy(sc->bssidmask, iter_data.mask, ETH_ALEN);
>>
>> +       if (update_opmode&&  sc->opmode != iter_data.opmode) {
>> +               sc->opmode = iter_data.opmode;
>> +               ath_do_set_opmode(sc);
>> +       }
>> +
>
> Should we really couple updating bssid mask and configuring
> the opmode?  Generally, I dislike adding boolean flags to
> functions because it's hard to figure out from the callsite
> what is happening (you have to go back to the prototype), and
> it usually indicates that the abstraction is a little broken.

Well, we need to do the iteration over all VIFS to figure out what to set this
too.  Seems doing one iteration v/s doing two is worth the
extra flag?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 2/2] ath5k: Adjust opmode when interfaces are removed.

[Bob Copeland]

On Fri, Oct 8, 2010 at 2:06 PM, Ben Greear <greearb@candelatech.com> wrote:
> On 10/08/2010 11:03 AM, Bob Copeland wrote:
>>
>> On Fri, Oct 8, 2010 at 12:43 PM,<greearb@candelatech.com>  wrote:
>>>
>>> @@ -558,6 +577,7 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc,
>>> struct ieee80211_vif *vif)
>>>        memset(&iter_data.mask, 0xff, ETH_ALEN);
>>>        iter_data.found_active = false;
>>>        iter_data.need_set_hw_addr = true;
>>> +       iter_data.opmode = NL80211_IFTYPE_UNSPECIFIED;
>>>
>>>        if (vif)
>>>                ath_vif_iter(&iter_data, vif->addr, vif);
>>> @@ -567,6 +587,11 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc,
>>> struct ieee80211_vif *vif)
>>>                                                   &iter_data);
>>>        memcpy(sc->bssidmask, iter_data.mask, ETH_ALEN);
>>>
>>> +       if (update_opmode&&  sc->opmode != iter_data.opmode) {
>>> +               sc->opmode = iter_data.opmode;
>>> +               ath_do_set_opmode(sc);
>>> +       }
>>> +
>>
>> Should we really couple updating bssid mask and configuring
>> the opmode?  Generally, I dislike adding boolean flags to
>> functions because it's hard to figure out from the callsite
>> what is happening (you have to go back to the prototype), and
>> it usually indicates that the abstraction is a little broken.
>
> Well, we need to do the iteration over all VIFS to figure out what to set
> this
> too.  Seems doing one iteration v/s doing two is worth the
> extra flag?

I admit I haven't looked at the context, so I'm not sure how ugly it
is, but you can often do this kind of thing by inverting the loop:

update_vif_data()
{
   for all vifs:
      compute new bssid & opmode

   update_bssid(new_bssid)
   update_opmode(neW_opmode)
}

and call that instead (even in the single vif case).

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 2/2] ath5k: Adjust opmode when interfaces are removed.

[Ben Greear]

On 10/08/2010 11:13 AM, Bob Copeland wrote:
> On Fri, Oct 8, 2010 at 2:06 PM, Ben Greear<greearb@candelatech.com>  wrote:
>> On 10/08/2010 11:03 AM, Bob Copeland wrote:
>>>
>>> On Fri, Oct 8, 2010 at 12:43 PM,<greearb@candelatech.com>    wrote:
>>>>
>>>> @@ -558,6 +577,7 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc,
>>>> struct ieee80211_vif *vif)
>>>>         memset(&iter_data.mask, 0xff, ETH_ALEN);
>>>>         iter_data.found_active = false;
>>>>         iter_data.need_set_hw_addr = true;
>>>> +       iter_data.opmode = NL80211_IFTYPE_UNSPECIFIED;
>>>>
>>>>         if (vif)
>>>>                 ath_vif_iter(&iter_data, vif->addr, vif);
>>>> @@ -567,6 +587,11 @@ void ath5k_update_bssid_mask(struct ath5k_softc *sc,
>>>> struct ieee80211_vif *vif)
>>>>                                                    &iter_data);
>>>>         memcpy(sc->bssidmask, iter_data.mask, ETH_ALEN);
>>>>
>>>> +       if (update_opmode&&    sc->opmode != iter_data.opmode) {
>>>> +               sc->opmode = iter_data.opmode;
>>>> +               ath_do_set_opmode(sc);
>>>> +       }
>>>> +
>>>
>>> Should we really couple updating bssid mask and configuring
>>> the opmode?  Generally, I dislike adding boolean flags to
>>> functions because it's hard to figure out from the callsite
>>> what is happening (you have to go back to the prototype), and
>>> it usually indicates that the abstraction is a little broken.
>>
>> Well, we need to do the iteration over all VIFS to figure out what to set
>> this
>> too.  Seems doing one iteration v/s doing two is worth the
>> extra flag?
>
> I admit I haven't looked at the context, so I'm not sure how ugly it
> is, but you can often do this kind of thing by inverting the loop:
>
> update_vif_data()
> {
>     for all vifs:
>        compute new bssid&  opmode
>
>     update_bssid(new_bssid)
>     update_opmode(neW_opmode)
> }
>
> and call that instead (even in the single vif case).

I can just always set the opmode if you prefer that.  Maybe
change the name of that method.

I'll work on a patch.

Thanks,
Ben

>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 2/2] ath5k: Adjust opmode when interfaces are removed.

[Bob Copeland]

On Fri, Oct 08, 2010 at 11:18:34AM -0700, Ben Greear wrote:
> I can just always set the opmode if you prefer that.  Maybe
> change the name of that method.
>
> I'll work on a patch.

Yeah I think that would be better from a long-term maintenance standpoint.
Thanks!

-- 
Bob Copeland %% www.bobcopeland.com