Hi,
With some drivers (at least wl1271 and wl1251), we need to know the BSSID that we are interested in when changing the channel. They don't have the concept of listening to a channel without listening to a specific BSSID. This is needed for BT-coext, some power saving mechanisms and filtering.
To support these chips correctly, I've introduced the concept of operating BSSID. We can set this bssid before we are associated with it. It's almost like the operating channel. The hardware needs to know which bssid we are interested in, in the same way as it needs to know which channel we are listening to.
I've spent some time analysing different options and other potential solutions to this problem but this this solution is the least intrusive that I could come up with. Any other ideas on how to implement this are very welcome.
Please take a look at my proposal and tell me what you think. If everyone is satisfied, I'll send them for inclusion.
In the same patchset, there is an example implementation in the driver side. It shows how this feature is used and why it is needed.
Cheers,
Luca.
Luciano Coelho (3):
mac80211: WIP - add operating BSSID to device configuration struct
wl1271: WIP - use operating BSSID for joining and disconnecting
wl1271: WIP - remove workaround for disconnection
drivers/net/wireless/wl12xx/wl1271.h | 3 --
drivers/net/wireless/wl12xx/wl1271_cmd.c | 15 -----------
drivers/net/wireless/wl12xx/wl1271_main.c | 40 ++++++++++++++++++++++++++---
include/net/mac80211.h | 8 ++++++
net/mac80211/mlme.c | 27 ++++++++++++++++++-
5 files changed, 69 insertions(+), 24 deletions(-)
ext Kalle Valo wrote:
> Luciano Coelho <[email protected]> writes:
>
>>>> As we discussed on IRC, it is really needed. We must always provide a
>>>> BSSID when changing channels in preparation for an association.
>>> I'm not sure about the must part here. Maybe we can workaround it by
>>> using ff:ff:ff:ff:ff bssid during authentication and association and
>>> send a new join command after association. But that's not a clean
>>> solution.
>> During one of our camps with TI, they told us that we must send the
>> correct BSSID, otherwise we are going to have some side-effects. At
>> least BT coext will be affected. And we have also seen the firmware
>> send probe_reqs to ff:ff:ff:ff:ff:ff, which was causing problems.
>> The firmware is simply not designed to do this.
>
> So wl1271 sends a probe request in every join command?
Not for every join command, but we observed some time ago, that the firmware was
sending broadcast probe_reqs on its own and we figured out that the issue was
related to the broadcast join commands. Again, we don't really know how the
firmware works, because we don't have access to the source, but according to TI
we can experience weird side-effects if we try to do it as you propose (and as
we were doing earlier).
>> You probably also remember that we have recently removed extra joins
>> from the wl1251 code as well, because they were causing some
>> problems.
>
> I think in wl1251 we would be able to workaround this with a careful
> placement of join and disconnect commands, but it would be really
> complicated. Having proper support in mac80211 is much better choice.
In wl1251 it *may* work. But not because the firmware supports it, but simply
because you may be lucky and not trigger any unexpected side-effect. In any
case, it would really be a workaround and I think it would be better to have
support for this on the stack, so we can do things as specified in the WiLink
firmware APIs. We probably have a slight chance of convincing TI to fix this in
future firmwares, but I'm almost sure it won't happen for the wl1251 and it is
very unlikely to happen for wl1271.
--
Cheers,
Luca.
On Fri, 2009-10-16 at 15:27 +0300, Luciano Coelho wrote:
> @@ -2405,7 +2411,15 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
> * to sleep and then change channel etc.
> */
> sdata->local->oper_channel = req->bss->channel;
> - ieee80211_hw_config(sdata->local, 0);
> +
> + if (memcmp(sdata->local->hw.conf.oper_bssid,
> + req->bss->bssid, ETH_ALEN)) {
> + config_changed |= IEEE80211_CONF_CHANGE_OPER_BSSID;
> + memcpy(sdata->local->hw.conf.oper_bssid,
> + req->bss->bssid, ETH_ALEN);
> + }
> +
> + ieee80211_hw_config(sdata->local, config_changed);
>
Is this really necessary before you can auth?
If it is, you're going to have to do a lot more work and go all the way
up to userspace so wpa_supplicant with -Dnl80211 knows about this too,
and doesn't try to authenticate twice or do FT-OTA.
johannes
The wl1271 firmware requires the BSSID on which we are interested
(ie. associating or associated with) to be passed in the join command, which
is also used to switch to different channels. Unfortunately the wl1271 chip
doesn't have the concept of being listening to one channel without a specific
BSSID to listen to.
This patch uses the new IEEE80211_CONF_CHANGE_OPER_BSSID flag to find out
which BSSID we are associating with. When the operating BSSID changes to
zero, we know that we have to disconnect.
Signed-off-by: Luciano Coelho <[email protected]>
---
drivers/net/wireless/wl12xx/wl1271_main.c | 38 +++++++++++++++++++++++++++-
1 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
index 86132bb..3077509 100644
--- a/drivers/net/wireless/wl12xx/wl1271_main.c
+++ b/drivers/net/wireless/wl12xx/wl1271_main.c
@@ -1114,10 +1114,12 @@ static int wl1271_op_config(struct ieee80211_hw *hw, u32 changed)
channel = ieee80211_frequency_to_channel(conf->channel->center_freq);
- wl1271_debug(DEBUG_MAC80211, "mac80211 config ch %d psm %s power %d",
+ wl1271_debug(DEBUG_MAC80211, "mac80211 config ch %d psm %s power %d "
+ "bssid %pM",
channel,
conf->flags & IEEE80211_CONF_PS ? "on" : "off",
- conf->power_level);
+ conf->power_level,
+ conf->oper_bssid);
mutex_lock(&wl->mutex);
@@ -1141,6 +1143,27 @@ static int wl1271_op_config(struct ieee80211_hw *hw, u32 changed)
if (ret < 0)
goto out_sleep;
+ if (changed & IEEE80211_CONF_CHANGE_OPER_BSSID) {
+ if (is_zero_ether_addr(conf->oper_bssid)) {
+ wl1271_debug(DEBUG_MAC80211, "gone to idle");
+
+ ret = wl1271_cmd_disconnect(wl);
+ if (ret < 0) {
+ wl1271_error("failed to disconnect");
+ goto out_sleep;
+ }
+ memset(wl->bssid, 0, ETH_ALEN);
+ } else if (memcmp(wl->bssid, conf->oper_bssid, ETH_ALEN)) {
+ wl1271_debug(DEBUG_MAC80211, "oper_bssid changed");
+
+ memcpy(wl->bssid, conf->oper_bssid, ETH_ALEN);
+
+ ret = wl1271_cmd_join(wl);
+ if (ret < 0)
+ goto out_sleep;
+ }
+ }
+
if (conf->flags & IEEE80211_CONF_PS && !wl->psm_requested) {
wl1271_info("psm enabled");
@@ -1474,6 +1497,17 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
if (ret < 0)
goto out;
+ if ((changed & BSS_CHANGED_BSSID) &&
+ memcmp(wl->bssid, bss_conf->bssid, ETH_ALEN)) {
+ wl1271_debug(DEBUG_MAC80211, "bssid changed");
+
+ memcpy(wl->bssid, bss_conf->bssid, ETH_ALEN);
+
+ ret = wl1271_cmd_join(wl);
+ if (ret < 0)
+ goto out_sleep;
+ }
+
if (changed & BSS_CHANGED_ASSOC) {
if (bss_conf->assoc) {
wl->aid = bss_conf->aid;
--
1.5.6.5
Luciano Coelho <[email protected]> writes:
> Some chips need to know the BSSID that we are interested in before we start
> the association process. This patch adds an operating BSSID to the device
> configuration struct ieee80211_conf so that it can be passed to the driver
> when op_config is called.
>
> With this solution we also solve the problem of some chips that require
> an explicit disconnect command when disassociating.
You could explain here more how disconnection is possible. It's not
obvious from the commit log.
> + * @oper_bssid: BSSID on which we are operating; some chips need to know
> + * which BSSID we are "tuned" to in order to improve power management,
> + * coexistence with other technologies (such as Bluetooth) and other
> + * filtering issues.
"Tuned" is confusing here. Also I think it's enough to specify the
reasons in the commit log, and here just explain what the parameters
is used for. Maybe something like this:
* @oper_bssid: BSSID on which we are operating; some chips need to
* know the BSSID before association. If zero, there is no
* association established nor any requests of it.
--
Kalle Valo
Luciano Coelho <[email protected]> writes:
> ext Johannes Berg wrote:
>> On Fri, 2009-10-16 at 15:27 +0300, Luciano Coelho wrote:
>>
>>> + if (memcmp(sdata->local->hw.conf.oper_bssid,
>>> + req->bss->bssid, ETH_ALEN)) {
>>> + config_changed |= IEEE80211_CONF_CHANGE_OPER_BSSID;
>>> + memcpy(sdata->local->hw.conf.oper_bssid,
>>> + req->bss->bssid, ETH_ALEN);
>>> + }
>>> +
>>> + ieee80211_hw_config(sdata->local, config_changed);
>>>
>>
>> Is this really necessary before you can auth?
>
> As we discussed on IRC, it is really needed. We must always provide a
> BSSID when changing channels in preparation for an association.
I'm not sure about the must part here. Maybe we can workaround it by
using ff:ff:ff:ff:ff bssid during authentication and association and
send a new join command after association. But that's not a clean
solution.
> Also, I'm not 100% sure, but IIRC Kalle has mentioned that the
> at76c50x-usb card also needs this feature.
Yeah, at76c50x-usb also has a similar requirement. wl1251, wl1271 and
at76c50x-usb have these weird join/start commands which need bssid and
must be executed before any data transmission. I tried to workaround
the bssid requirement in at76c50x-usb a long time ago but failed. I
need to try it again.
--
Kalle Valo
Luciano Coelho <[email protected]> writes:
>>> As we discussed on IRC, it is really needed. We must always provide a
>>> BSSID when changing channels in preparation for an association.
>>
>> I'm not sure about the must part here. Maybe we can workaround it by
>> using ff:ff:ff:ff:ff bssid during authentication and association and
>> send a new join command after association. But that's not a clean
>> solution.
>
> During one of our camps with TI, they told us that we must send the
> correct BSSID, otherwise we are going to have some side-effects. At
> least BT coext will be affected. And we have also seen the firmware
> send probe_reqs to ff:ff:ff:ff:ff:ff, which was causing problems.
> The firmware is simply not designed to do this.
So wl1271 sends a probe request in every join command?
> You probably also remember that we have recently removed extra joins
> from the wl1251 code as well, because they were causing some
> problems.
I think in wl1251 we would be able to workaround this with a careful
placement of join and disconnect commands, but it would be really
complicated. Having proper support in mac80211 is much better choice.
--
Kalle Valo
ext Kalle Valo wrote:
> Luciano Coelho <[email protected]> writes:
>
>> Some chips need to know the BSSID that we are interested in before we start
>> the association process. This patch adds an operating BSSID to the device
>> configuration struct ieee80211_conf so that it can be passed to the driver
>> when op_config is called.
>>
>> With this solution we also solve the problem of some chips that require
>> an explicit disconnect command when disassociating.
>
> You could explain here more how disconnection is possible. It's not
> obvious from the commit log.
>
>> + * @oper_bssid: BSSID on which we are operating; some chips need to know
>> + * which BSSID we are "tuned" to in order to improve power management,
>> + * coexistence with other technologies (such as Bluetooth) and other
>> + * filtering issues.
>
> "Tuned" is confusing here. Also I think it's enough to specify the
> reasons in the commit log, and here just explain what the parameters
> is used for. Maybe something like this:
>
> * @oper_bssid: BSSID on which we are operating; some chips need to
> * know the BSSID before association. If zero, there is no
> * association established nor any requests of it.
>
Very good points. I'll fix this as part of my rework on these patches. Thanks
for your comments!
--
Cheers,
Luca.
ext Kalle Valo wrote:
> Luciano Coelho <[email protected]> writes:
>
>> ext Johannes Berg wrote:
>>> On Fri, 2009-10-16 at 15:27 +0300, Luciano Coelho wrote:
>>>
>>>> + if (memcmp(sdata->local->hw.conf.oper_bssid,
>>>> + req->bss->bssid, ETH_ALEN)) {
>>>> + config_changed |= IEEE80211_CONF_CHANGE_OPER_BSSID;
>>>> + memcpy(sdata->local->hw.conf.oper_bssid,
>>>> + req->bss->bssid, ETH_ALEN);
>>>> + }
>>>> +
>>>> + ieee80211_hw_config(sdata->local, config_changed);
>>>>
>>> Is this really necessary before you can auth?
>> As we discussed on IRC, it is really needed. We must always provide a
>> BSSID when changing channels in preparation for an association.
>
> I'm not sure about the must part here. Maybe we can workaround it by
> using ff:ff:ff:ff:ff bssid during authentication and association and
> send a new join command after association. But that's not a clean
> solution.
During one of our camps with TI, they told us that we must send the correct
BSSID, otherwise we are going to have some side-effects. At least BT coext will
be affected. And we have also seen the firmware send probe_reqs to
ff:ff:ff:ff:ff:ff, which was causing problems. The firmware is simply not
designed to do this. You probably also remember that we have recently removed
extra joins from the wl1251 code as well, because they were causing some
problems. So, from my point-of-view, this is unfortunately a must for wl1271. :(
--
Cheers,
Luca.
Some chips need to know the BSSID that we are interested in before we start
the association process. This patch adds an operating BSSID to the device
configuration struct ieee80211_conf so that it can be passed to the driver
when op_config is called.
With this solution we also solve the problem of some chips that require
an explicit disconnect command when disassociating.
Signed-off-by: Luciano Coelho <[email protected]>
---
include/net/mac80211.h | 8 ++++++++
net/mac80211/mlme.c | 27 +++++++++++++++++++++++++--
2 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 466859b..654d753 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -599,6 +599,7 @@ enum ieee80211_conf_changed {
IEEE80211_CONF_CHANGE_CHANNEL = BIT(6),
IEEE80211_CONF_CHANGE_RETRY_LIMITS = BIT(7),
IEEE80211_CONF_CHANGE_IDLE = BIT(8),
+ IEEE80211_CONF_CHANGE_OPER_BSSID = BIT(9),
};
/**
@@ -629,6 +630,11 @@ enum ieee80211_conf_changed {
* @short_frame_max_tx_count: Maximum number of transmissions for a "short"
* frame, called "dot11ShortRetryLimit" in 802.11, but actually means the
* number of transmissions not the number of retries
+ *
+ * @oper_bssid: BSSID on which we are operating; some chips need to know
+ * which BSSID we are "tuned" to in order to improve power management,
+ * coexistence with other technologies (such as Bluetooth) and other
+ * filtering issues.
*/
struct ieee80211_conf {
u32 flags;
@@ -641,6 +647,8 @@ struct ieee80211_conf {
struct ieee80211_channel *channel;
enum nl80211_channel_type channel_type;
+
+ u8 oper_bssid[ETH_ALEN];
};
/**
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 8d26e9b..cac542d 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1077,6 +1077,11 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
/* channel(_type) changes are handled by ieee80211_hw_config */
local->oper_channel_type = NL80211_CHAN_NO_HT;
+ if (!is_zero_ether_addr(sdata->local->hw.conf.oper_bssid)) {
+ config_changed |= IEEE80211_CONF_CHANGE_OPER_BSSID;
+ memset(sdata->local->hw.conf.oper_bssid, 0, ETH_ALEN);
+ }
+
/* on the next assoc, re-program HT parameters */
sdata->ht_opmode_valid = false;
@@ -2356,6 +2361,7 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
const u8 *ssid;
struct ieee80211_mgd_work *wk;
+ u32 config_changed = 0;
u16 auth_alg;
switch (req->auth_type) {
@@ -2405,7 +2411,15 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
* to sleep and then change channel etc.
*/
sdata->local->oper_channel = req->bss->channel;
- ieee80211_hw_config(sdata->local, 0);
+
+ if (memcmp(sdata->local->hw.conf.oper_bssid,
+ req->bss->bssid, ETH_ALEN)) {
+ config_changed |= IEEE80211_CONF_CHANGE_OPER_BSSID;
+ memcpy(sdata->local->hw.conf.oper_bssid,
+ req->bss->bssid, ETH_ALEN);
+ }
+
+ ieee80211_hw_config(sdata->local, config_changed);
mutex_lock(&ifmgd->mtx);
list_add(&wk->list, &sdata->u.mgd.work_list);
@@ -2420,6 +2434,7 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
{
struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
struct ieee80211_mgd_work *wk, *found = NULL;
+ u32 config_changed = 0;
int i, err;
mutex_lock(&ifmgd->mtx);
@@ -2457,7 +2472,15 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
ifmgd->flags |= IEEE80211_STA_DISABLE_11N;
sdata->local->oper_channel = req->bss->channel;
- ieee80211_hw_config(sdata->local, 0);
+
+ if (memcmp(sdata->local->hw.conf.oper_bssid,
+ req->bss->bssid, ETH_ALEN)) {
+ config_changed |= IEEE80211_CONF_CHANGE_OPER_BSSID;
+ memcpy(sdata->local->hw.conf.oper_bssid,
+ req->bss->bssid, ETH_ALEN);
+ }
+
+ ieee80211_hw_config(sdata->local, config_changed);
if (req->ie && req->ie_len) {
memcpy(wk->ie, req->ie, req->ie_len);
--
1.5.6.5
ext Johannes Berg wrote:
> On Fri, 2009-10-16 at 15:27 +0300, Luciano Coelho wrote:
>
>> @@ -2405,7 +2411,15 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
>> * to sleep and then change channel etc.
>> */
>> sdata->local->oper_channel = req->bss->channel;
>> - ieee80211_hw_config(sdata->local, 0);
>> +
>> + if (memcmp(sdata->local->hw.conf.oper_bssid,
>> + req->bss->bssid, ETH_ALEN)) {
>> + config_changed |= IEEE80211_CONF_CHANGE_OPER_BSSID;
>> + memcpy(sdata->local->hw.conf.oper_bssid,
>> + req->bss->bssid, ETH_ALEN);
>> + }
>> +
>> + ieee80211_hw_config(sdata->local, config_changed);
>>
>
> Is this really necessary before you can auth?
As we discussed on IRC, it is really needed. We must always provide a BSSID
when changing channels in preparation for an association. For this reason (and
others) the wl1271 with the current firmware does not support promiscuous mode
or frame injections.
I'll ask TI next time we meet them, if there is any possibility to remove the
need for the BSSID at this point, so things will be greatly simplified. There
is a very slight chance that this could be possible for wl1271, but for wl1251 I
think there's no hope to change it anymore. So in any case, we will need to
support this on the stack if we want to have wl1251 to work correctly. :(
Also, I'm not 100% sure, but IIRC Kalle has mentioned that the at76c50x-usb card
also needs this feature.
> If it is, you're going to have to do a lot more work and go all the way
> up to userspace so wpa_supplicant with -Dnl80211 knows about this too,
> and doesn't try to authenticate twice or do FT-OTA.
Thanks a lot for pointing this out. I think I understand it better after our
discussion on IRC. I'll have to study more the authentication mechanism and
figure out how to deal with multiple authentications, as you pointed out.
These are definitely details that I'll need to sort out. But in principle, do
you agree with the idea of passing the "operating" BSSID down to the driver in
the hw_config op? And saving it as a hardware configuration parameter in
local->hw.conf?
Once again, I really appreciate your help and the time you spend discussing and
explaining these things in detail to me! :)
--
Cheers,
Luca.
With the new mac80211 IEEE80211_CONF_CHANGE_OPER_BSSID we don't need to save
the "joined" state to disconnect while trying to rejoin. Now the oper_bssid
value can be used in order to disconnect.
This patch removes this workaround.
Signed-off-by: Luciano Coelho <[email protected]>
---
drivers/net/wireless/wl12xx/wl1271.h | 3 ---
drivers/net/wireless/wl12xx/wl1271_cmd.c | 15 ---------------
drivers/net/wireless/wl12xx/wl1271_main.c | 2 --
3 files changed, 0 insertions(+), 20 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/wl1271.h b/drivers/net/wireless/wl12xx/wl1271.h
index 566f152..bb06301 100644
--- a/drivers/net/wireless/wl12xx/wl1271.h
+++ b/drivers/net/wireless/wl12xx/wl1271.h
@@ -432,9 +432,6 @@ struct wl1271 {
struct ieee80211_vif *vif;
- /* Used for a workaround to send disconnect before rejoining */
- bool joined;
-
/* Current chipset configuration */
struct conf_drv_settings conf;
diff --git a/drivers/net/wireless/wl12xx/wl1271_cmd.c b/drivers/net/wireless/wl12xx/wl1271_cmd.c
index 0666328..52c4e9c 100644
--- a/drivers/net/wireless/wl12xx/wl1271_cmd.c
+++ b/drivers/net/wireless/wl12xx/wl1271_cmd.c
@@ -191,19 +191,6 @@ int wl1271_cmd_join(struct wl1271 *wl)
do_cal = false;
}
- /* FIXME: This is a workaround, because with the current stack, we
- * cannot know when we have disassociated. So, if we have already
- * joined, we disconnect before joining again. */
- if (wl->joined) {
- ret = wl1271_cmd_disconnect(wl);
- if (ret < 0) {
- wl1271_error("failed to disconnect before rejoining");
- goto out;
- }
-
- wl->joined = false;
- }
-
join = kzalloc(sizeof(*join), GFP_KERNEL);
if (!join) {
ret = -ENOMEM;
@@ -268,8 +255,6 @@ int wl1271_cmd_join(struct wl1271 *wl)
goto out_free;
}
- wl->joined = true;
-
/*
* ugly hack: we should wait for JOIN_EVENT_COMPLETE_ID but to
* simplify locking we just sleep instead, for now
diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
index 3077509..ba85c8a 100644
--- a/drivers/net/wireless/wl12xx/wl1271_main.c
+++ b/drivers/net/wireless/wl12xx/wl1271_main.c
@@ -983,7 +983,6 @@ static void wl1271_op_stop(struct ieee80211_hw *hw)
wl->tx_security_seq_32 = 0;
wl->time_offset = 0;
wl->session_counter = 0;
- wl->joined = false;
for (i = 0; i < NUM_TX_QUEUES; i++)
wl->tx_blocks_freed[i] = 0;
@@ -1865,7 +1864,6 @@ static int __devinit wl1271_probe(struct spi_device *spi)
wl->basic_rate_set = WL1271_DEFAULT_BASIC_RATE_SET;
wl->band = IEEE80211_BAND_2GHZ;
wl->vif = NULL;
- wl->joined = false;
for (i = 0; i < ACX_TX_DESCRIPTORS; i++)
wl->tx_frames[i] = NULL;
--
1.5.6.5